[PATCH v2] smbios: arm64: Allow table to be written at a fixed addr
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Update to search for a suitable area automatically, if enabled
lib/Kconfig | 12 +++++++ lib/efi_loader/efi_smbios.c | 63 ++++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/lib/Kconfig b/lib/Kconfig index f6ca559897e7..a1eec98b392f 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -994,6 +994,18 @@ config GENERATE_SMBIOS_TABLE See also SMBIOS_SYSINFO which allows SMBIOS values to be provided in the devicetree.
+config SMBIOS_TABLE_FIXED + bool "Place the SMBIOS table at a special address" + depends on GENERATE_SMBIOS_TABLE && ARM64 && SMBIOS && EFI_LOADER + default y + help + Use this option to place the SMBIOS table at a special address. + + U-Boot typically sets up its malloc() pool near the top of memory. On + ARM64 systems this can result in an SMBIOS table above 4GB which is + not supported by SMBIOSv2. This option works around this problem by + chosing an address just below 4GB, if needed. + endmenu
config LIB_RATIONAL diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 48446f654d9b..bdbce4c4d785 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -47,6 +47,60 @@ efi_status_t efi_smbios_register(void) map_sysmem(addr, 0)); }
+/** + * find_addr_below() - Find a usable region below the given max_addr + * + * Check if *addrp is suitable to define a memory region which finishes below + * @max_addr + @req_size. If so, do nothing and return 0 + * + * As a backup, if CONFIG_SMBIOS_TABLE_FIXED is enabled, search for a + * 4KB-aligned DRAM region which is large enough. Make sure it is below U-Boot's + * stack space, assumed to be 64KB. + * + * @max_addr: Maximum address that can be used (region must finish before here) + * @req:size: Required size of region + * @addrp: On entry: Current proposed address; on exit, holds the new address, + * on success + * Return 0 if OK (existing region was OK, or a new one was found), -ENOSPC if + * nothing suitable was found + */ +static int find_addr_below(ulong max_addr, ulong req_size, ulong *addrp) +{ + struct bd_info *bd = gd->bd; + ulong max_base; + int i; + + max_base = max_addr - req_size; + if (*addrp <= max_base) + return 0; + + if (!IS_ENABLED(CONFIG_SMBIOS_TABLE_FIXED)) + return -ENOSPC; + + /* Make sure that the base is at least 64KB below the stack */ + max_base = min(max_base, + ALIGN(gd->start_addr_sp - SZ_64K - req_size, SZ_4K)); + + for (i = CONFIG_NR_DRAM_BANKS - 1; i >= 0; i--) { + ulong start = bd->bi_dram[i].start; + ulong size = bd->bi_dram[i].size; + ulong addr; + + /* chose an address at most req_size bytes before the end */ + addr = min(max_base, start - req_size + size); + + /* check this is in the range */ + if (addr >= start && addr + req_size < start + size) { + *addrp = addr; + log_warning("Forcing SMBIOS table to address %lx\n", + addr); + return 0; + } + } + + return -ENOSPC; +} + static int install_smbios_table(void) { ulong addr; @@ -61,7 +115,14 @@ static int install_smbios_table(void) return log_msg_ret("mem", -ENOMEM);
addr = map_to_sysmem(buf); - if (!write_smbios_table(addr)) { + + /* + * Deal with a fixed address if needed. For simplicity we assume that + * the SMBIOS-table size is <64KB. If a suitable address cannot be + * found, then write_smbios_table() returns an error. + */ + if (find_addr_below(SZ_4G, SZ_64K, &addr) || + !write_smbios_table(addr)) { log_err("Failed to write SMBIOS table\n"); return log_msg_ret("smbios", -EINVAL); }
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
Best regards
Heinrich
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Update to search for a suitable area automatically, if enabled
lib/Kconfig | 12 +++++++ lib/efi_loader/efi_smbios.c | 63 ++++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/lib/Kconfig b/lib/Kconfig index f6ca559897e7..a1eec98b392f 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -994,6 +994,18 @@ config GENERATE_SMBIOS_TABLE See also SMBIOS_SYSINFO which allows SMBIOS values to be provided in the devicetree.
+config SMBIOS_TABLE_FIXED
- bool "Place the SMBIOS table at a special address"
- depends on GENERATE_SMBIOS_TABLE && ARM64 && SMBIOS && EFI_LOADER
- default y
- help
Use this option to place the SMBIOS table at a special address.U-Boot typically sets up its malloc() pool near the top of memory. OnARM64 systems this can result in an SMBIOS table above 4GB which isnot supported by SMBIOSv2. This option works around this problem bychosing an address just below 4GB, if needed.endmenu
config LIB_RATIONAL diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 48446f654d9b..bdbce4c4d785 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -47,6 +47,60 @@ efi_status_t efi_smbios_register(void) map_sysmem(addr, 0)); }
+/**
- find_addr_below() - Find a usable region below the given max_addr
- Check if *addrp is suitable to define a memory region which finishes below
- @max_addr + @req_size. If so, do nothing and return 0
- As a backup, if CONFIG_SMBIOS_TABLE_FIXED is enabled, search for a
- 4KB-aligned DRAM region which is large enough. Make sure it is below U-Boot's
- stack space, assumed to be 64KB.
- @max_addr: Maximum address that can be used (region must finish before here)
- @req:size: Required size of region
- @addrp: On entry: Current proposed address; on exit, holds the new address,
- on success
- Return 0 if OK (existing region was OK, or a new one was found), -ENOSPC if
- nothing suitable was found
- */
+static int find_addr_below(ulong max_addr, ulong req_size, ulong *addrp) +{
- struct bd_info *bd = gd->bd;
- ulong max_base;
- int i;
- max_base = max_addr - req_size;
- if (*addrp <= max_base)
return 0;- if (!IS_ENABLED(CONFIG_SMBIOS_TABLE_FIXED))
return -ENOSPC;- /* Make sure that the base is at least 64KB below the stack */
- max_base = min(max_base,
ALIGN(gd->start_addr_sp - SZ_64K - req_size, SZ_4K));- for (i = CONFIG_NR_DRAM_BANKS - 1; i >= 0; i--) {
ulong start = bd->bi_dram[i].start;ulong size = bd->bi_dram[i].size;ulong addr;/* chose an address at most req_size bytes before the end */addr = min(max_base, start - req_size + size);/* check this is in the range */if (addr >= start && addr + req_size < start + size) {*addrp = addr;log_warning("Forcing SMBIOS table to address %lx\n",addr);return 0;}- }
- return -ENOSPC;
+}
static int install_smbios_table(void) { ulong addr; @@ -61,7 +115,14 @@ static int install_smbios_table(void) return log_msg_ret("mem", -ENOMEM);
addr = map_to_sysmem(buf);
- if (!write_smbios_table(addr)) {
- /*
* Deal with a fixed address if needed. For simplicity we assume that* the SMBIOS-table size is <64KB. If a suitable address cannot be* found, then write_smbios_table() returns an error.*/- if (find_addr_below(SZ_4G, SZ_64K, &addr) ||
!write_smbios_table(addr)) {
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
Regards, Simon
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Best regards
Heinrich
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=377650
On Sun, Oct 29, 2023 at 07:41:27AM +1300, Simon Glass wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
No one "needs" it, but it can be helpful. And yes, a future discussion should be about what we should put in an SMBIOS 3 table, and when. But for today, being part of EFI (outside of X86) is fine.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader
I don't think the X86 approach is right, certainly for ARM. We'll be populating the table with what we're also getting from the device tree that we're passing to the OS. On x86 I don't know if we'd be loading anything that might correct/change what goes in the table but on ARM we have a bunch of places that set the serial# so some function, so some function in the actual OS boot flow that creates and populates the table makes sense. I don't know on x86 how we can examine the table.
So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=377650
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
Best regards
Heinrich
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=377650
Hi Heinrich,
On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org: > U-Boot typically sets up its malloc() pool near the top of memory. On > ARM64 systems this can result in an SMBIOS table above 4GB which is > not supported by SMBIOSv2. > > Work around this problem by providing a new option to choose an address > below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
That is fine when EFI is used, but what about when it is not? That is the piece I don't yet understand.
Regards, Simon
On Fri, Nov 03, 2023 at 12:14:46PM -0600, Simon Glass wrote:
Hi Heinrich,
On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org: >> U-Boot typically sets up its malloc() pool near the top of memory. On >> ARM64 systems this can result in an SMBIOS table above 4GB which is >> not supported by SMBIOSv2. >> >> Work around this problem by providing a new option to choose an address >> below 4GB (but as high as possible), if needed. > > You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
That is fine when EFI is used, but what about when it is not? That is the piece I don't yet understand.
We also don't have the use case for when EFI is not used defined and understood, so it can wait until then?
Hi Tom,
On Fri, 3 Nov 2023 at 13:26, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 03, 2023 at 12:14:46PM -0600, Simon Glass wrote:
Hi Heinrich,
On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote: > Hi Heinrich, > > On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote: >> >> >> >> Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org: >>> U-Boot typically sets up its malloc() pool near the top of memory. On >>> ARM64 systems this can result in an SMBIOS table above 4GB which is >>> not supported by SMBIOSv2. >>> >>> Work around this problem by providing a new option to choose an address >>> below 4GB (but as high as possible), if needed. >> >> You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools. > > That is not my intention and I don't believe this code does that. EFI > is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
> > The bit I am confused about is that we don't support SMBIOS3 in > U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
That is fine when EFI is used, but what about when it is not? That is the piece I don't yet understand.
We also don't have the use case for when EFI is not used defined and understood, so it can wait until then?
There is/was an argument going on where (for ARM) the SMBIOS table requires ACPI and EFI. So I am nervous about anything like that. EFI has become a huge impediment to booting with open source components. If there has been an agreement on how ARM can boot without EFI (but still provide SMBIOS), please can someone link to it?
The fact is that U-Boot makes lots of allocations while it runs. None of them go through the EFI allocator, so far as I know. Certainly they shouldn't. Then at the end, EFI just adds allocations for them retrospectively. What change that?
EFI is not in control of memory allocation in U-Boot. In fact, the efi_memory_init() call should probably be dropped, just done when bootefi starts. It seems to be creating a lot of confusion.
My patch here does work, so far as I can tell. It adds an allocation correctly, in the same manner as is done with ACPI - see efi_acpi_register(). Given all the uncertainty I think we should consider taking this patch as it is for now. I see it is marked as 'changes requested' in patchwork so I have changed it to 'New', hoping that it will get another look.
Regards, Simon
On Fri, Nov 03, 2023 at 01:38:59PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 3 Nov 2023 at 13:26, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 03, 2023 at 12:14:46PM -0600, Simon Glass wrote:
Hi Heinrich,
On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > On 10/25/23 04:49, Simon Glass wrote: > > Hi Heinrich, > > > > On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > >> > >> > >> > >> Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org: > >>> U-Boot typically sets up its malloc() pool near the top of memory. On > >>> ARM64 systems this can result in an SMBIOS table above 4GB which is > >>> not supported by SMBIOSv2. > >>> > >>> Work around this problem by providing a new option to choose an address > >>> below 4GB (but as high as possible), if needed. > >> > >> You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools. > > > > That is not my intention and I don't believe this code does that. EFI > > is not running at this point, is it? > > The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
> > We have: > EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); > This is invoked after efi_memory_init(). > > The EFI specification requires that the memory area occupied by the > SMBIOS table uses one of a specific set of memory types where > EfiRuntimeServicesData is recommended. So you must call > > u64 addr = UINT_MAX; > ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, > EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr); > > to allocate the memory. If the return code is not EFI_SUCCESS, no memory > below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
> > > > > The bit I am confused about is that we don't support SMBIOS3 in > > U-Boot. I am trying to fix an introduced bug... > > I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
That is fine when EFI is used, but what about when it is not? That is the piece I don't yet understand.
We also don't have the use case for when EFI is not used defined and understood, so it can wait until then?
There is/was an argument going on where (for ARM) the SMBIOS table requires ACPI and EFI. So I am nervous about anything like that. EFI has become a huge impediment to booting with open source components. If there has been an agreement on how ARM can boot without EFI (but still provide SMBIOS), please can someone link to it?
I think you're missing things right now. Are you sure you need ACPI for SMBIOS on 64bit ARM platforms to be seen and used? That's not what was hearing just yesterday but I don't have something I can double check that on real quick. What is unclear is that ARM and SMBIOS without EFI is something that's usable today as I suspect that nothing says where the SMBIOS table will be found, as there's not some legacy case like x86 of a known hard-coded location.
The fact is that U-Boot makes lots of allocations while it runs. None of them go through the EFI allocator, so far as I know. Certainly they shouldn't. Then at the end, EFI just adds allocations for them retrospectively. What change that?
It depends on what allocations you're talking about? If you want to allocate something that will be preserved and untouched and passed along to the kernel then yes, you're either doing what EFI does or you're doing whatever else we do for special cases like initrd/dtb. Saying "and smbios too" means that yes, it's either EFI (because that will in turn do what needs to be done so the OS knows it exists and where it exists) or it's a special case EFI isn't involved and however the OS knows to not trample it is how it's not trampled.
EFI is not in control of memory allocation in U-Boot. In fact, the efi_memory_init() call should probably be dropped, just done when bootefi starts. It seems to be creating a lot of confusion.
My patch here does work, so far as I can tell. It adds an allocation correctly, in the same manner as is done with ACPI - see efi_acpi_register(). Given all the uncertainty I think we should consider taking this patch as it is for now. I see it is marked as 'changes requested' in patchwork so I have changed it to 'New', hoping that it will get another look.
The problem will then be that the table isn't preserved and used on ARM with EFI? What cases have you tested here with all of this.
Hi Tom,
On Fri, 3 Nov 2023 at 14:13, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 03, 2023 at 01:38:59PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 3 Nov 2023 at 13:26, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 03, 2023 at 12:14:46PM -0600, Simon Glass wrote:
Hi Heinrich,
On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote: > > [unfortunately I am not receiving email from the list at present] > > Hi Heinrich, > > On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > On 10/25/23 04:49, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > >> > > >> > > >> > > >> Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org: > > >>> U-Boot typically sets up its malloc() pool near the top of memory. On > > >>> ARM64 systems this can result in an SMBIOS table above 4GB which is > > >>> not supported by SMBIOSv2. > > >>> > > >>> Work around this problem by providing a new option to choose an address > > >>> below 4GB (but as high as possible), if needed. > > >> > > >> You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools. > > > > > > That is not my intention and I don't believe this code does that. EFI > > > is not running at this point, is it? > > > > The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y. > > That is because ARM devices don't normally need it, right? Anyway, > that option isn't related to this patch. If ARM devices started using > SMBIOS and had another way to pass it to Linux (other than EFI) then > we would want to install it. > > > > > We have: > > EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); > > This is invoked after efi_memory_init(). > > > > The EFI specification requires that the memory area occupied by the > > SMBIOS table uses one of a specific set of memory types where > > EfiRuntimeServicesData is recommended. So you must call > > > > u64 addr = UINT_MAX; > > ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, > > EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr); > > > > to allocate the memory. If the return code is not EFI_SUCCESS, no memory > > below 4 GiB is available. > > The root problem here is that x86 and ARM used to work differently. > When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS > table as part of the 'bootefi' command. On x86, the tables were > written on startup, so you can examine them within U-Boot. Clearly the > x86 approach is correct. For one thing, a previous-stage bootloader > may set up the tables, so it simply isn't valid to write them in that > case. So we need to separate writing the tables from telling EFI about > them. > > So I have fixed that, so ARM now writes the tables at the start. But > using an EFI allocation function is clearly not right. This is generic > code, nothing to do with EFI, really. In fact, the SMBIOS writing > should move out of efi_loader. The install_smbios_table() function > should be somewhere in lib, i suppose, with just efi_smbios_register() > sitting in lib/efi_loader > > Also, why is efi_memory_init() called early in init? Is there anything > that needs that in the init sequence? Could we move it to the end, or > perhaps skip it completely until the 'bootefi' command is used? > > Another point I should make is that it should be fine for U-Boot to > put something in memory and then call efi_add_memory_map() to tell EFI > about it. What problems does that cause? It isn't as if EFI allocates > things in the 'conventional' memory (is that the name for memory below > 4GB?) This is how efi_acpi_register() works. > > (Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in > drivers/video/rockchip_rk_vop.c and other such files) > > > > > > > > > The bit I am confused about is that we don't support SMBIOS3 in > > > U-Boot. I am trying to fix an introduced bug... > > > > I would not know why we should not use SMBIOS 3. > > Neither do I. Perhaps there are compatibility concerns? If it is OK to > do that then we could go back to my previous series [1]. What do you > think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
That is fine when EFI is used, but what about when it is not? That is the piece I don't yet understand.
We also don't have the use case for when EFI is not used defined and understood, so it can wait until then?
There is/was an argument going on where (for ARM) the SMBIOS table requires ACPI and EFI. So I am nervous about anything like that. EFI has become a huge impediment to booting with open source components. If there has been an agreement on how ARM can boot without EFI (but still provide SMBIOS), please can someone link to it?
I think you're missing things right now. Are you sure you need ACPI for SMBIOS on 64bit ARM platforms to be seen and used? That's not what was hearing just yesterday but I don't have something I can double check that on real quick. What is unclear is that ARM and SMBIOS without EFI is something that's usable today as I suspect that nothing says where the SMBIOS table will be found, as there's not some legacy case like x86 of a known hard-coded location.
OK, I'm not sure about that. But we cannot tie SMBIOS to EFI...it is a table that is produced on startup, just like any other.
BTW at some point we should refactor table generation to use a context struct, a bit like how ACPI is implemented.
The fact is that U-Boot makes lots of allocations while it runs. None of them go through the EFI allocator, so far as I know. Certainly they shouldn't. Then at the end, EFI just adds allocations for them retrospectively. What change that?
It depends on what allocations you're talking about? If you want to allocate something that will be preserved and untouched and passed along to the kernel then yes, you're either doing what EFI does or you're doing whatever else we do for special cases like initrd/dtb. Saying "and smbios too" means that yes, it's either EFI (because that will in turn do what needs to be done so the OS knows it exists and where it exists) or it's a special case EFI isn't involved and however the OS knows to not trample it is how it's not trampled.
As I said above, I know there is an argument going on in the kernel about how to pass this info without SMBIOS. I don't want to get into that.
It is simple to me...we need to allocate memory for an SMBIOS table before U-Boot reaches the cmdline. EFI may or may not be used to boot...we cannot know that until we actually boot. At that point we can 'carve out' the memory for the table in EFI's allocations, just like we do for ACPI and FDT and everything else in add_u_boot_and_runtime() etc.
EFI is not in control of memory allocation in U-Boot. In fact, the efi_memory_init() call should probably be dropped, just done when bootefi starts. It seems to be creating a lot of confusion.
My patch here does work, so far as I can tell. It adds an allocation correctly, in the same manner as is done with ACPI - see efi_acpi_register(). Given all the uncertainty I think we should consider taking this patch as it is for now. I see it is marked as 'changes requested' in patchwork so I have changed it to 'New', hoping that it will get another look.
The problem will then be that the table isn't preserved and used on ARM with EFI? What cases have you tested here with all of this.
This is the bug report of trying to boot into U-Boot on arm64 with more than 3GB of memory.
I feel that too many discussions are muddied by EFI. Yes, the implementation in U-Boot needs rework to integrate it better. But I am a bit tired of 'what about the special case for EFI' comments.
Again, this patch fixes a bug and works fine with EFI booting and without. If the upstream kernel people do something with [1] then it will still work; we'll just need to add something to the FDT fixup.
Regards, Simon
[1] https://lore.kernel.org/lkml/ZKgLKvBoWKSxzm6r@sunil-laptop/
Splitting things in two again..
On Sat, Nov 18, 2023 at 10:10:17AM -0700, Simon Glass wrote:
[snip]
This is the bug report of trying to boot into U-Boot on arm64 with more than 3GB of memory.
OK, so can we just please fix this, for v2024.01, in a way that won't make things harder? Fixed memory addresses aren't the right choice. We need to have the discussion about memory allocation/reservation elsewhere, outside of this patch, to have something that works for everyones use-cases. And possibly a similar discussion about when data structures get populated/updated.
Hi Tom,
On Sat, 18 Nov 2023 at 12:15, Tom Rini trini@konsulko.com wrote:
Splitting things in two again..
On Sat, Nov 18, 2023 at 10:10:17AM -0700, Simon Glass wrote:
[snip]
This is the bug report of trying to boot into U-Boot on arm64 with more than 3GB of memory.
OK, so can we just please fix this, for v2024.01, in a way that won't make things harder? Fixed memory addresses aren't the right choice.
It actually doesn't use fixed addresses. That was the previous version. I believe this is the right fix, until...
We need to have the discussion about memory allocation/reservation elsewhere, outside of this patch, to have something that works for everyones use-cases. And possibly a similar discussion about when data structures get populated/updated.
This is the thread that needs to be started.
But just for the tables (SMBIOS and ACPI) I would like to use bloblist, as is done on x86. It keeps all the tables in one place and provides allocation, etc.
Regards, Simon
Am 3. November 2023 20:14:46 OEZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass sjg@chromium.org:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org: >> U-Boot typically sets up its malloc() pool near the top of memory. On >> ARM64 systems this can result in an SMBIOS table above 4GB which is >> not supported by SMBIOSv2. >> >> Work around this problem by providing a new option to choose an address >> below 4GB (but as high as possible), if needed. > > You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Regards, Simon
All memory below the stack is controlled by the EFI subsystem. I notified you of the function you need to call. I can't see what information you are lacking.
That is fine when EFI is used, but what about when it is not? That is the piece I don't yet understand.
If CONFIG_EFI_LOADER=y and booting via booti, it does not harm to have added the SMBIOS table to the EFI memory map which will be ignored. When booting via bootefi you will have a correct memory map.
If CONFIG_EFI_LOADER=n, you are free with the placement of SMBIOS.
Best regards
Heinrich
Regards, Simon
On Fri, Nov 03, 2023 at 11:12:40AM -0600, Simon Glass wrote:
Hi,
On Sat, 28 Oct 2023 at 12:41, Simon Glass sjg@chromium.org wrote:
[unfortunately I am not receiving email from the list at present]
Hi Heinrich,
On Wed, 25 Oct 2023 at 21:39, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/25/23 04:49, Simon Glass wrote:
Hi Heinrich,
On Tue, 24 Oct 2023 at 18:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass sjg@chromium.org:
U-Boot typically sets up its malloc() pool near the top of memory. On ARM64 systems this can result in an SMBIOS table above 4GB which is not supported by SMBIOSv2.
Work around this problem by providing a new option to choose an address below 4GB (but as high as possible), if needed.
You must not overwrite memory controlled by the EFI subsystem without calling its allocator. We should provide SMBIOS 3. SMBIOS 2 is only a fallback for outdated tools.
That is not my intention and I don't believe this code does that. EFI is not running at this point, is it?
The function install_smbios_table() only exists if CONFIG_EFI_LOADER=y.
That is because ARM devices don't normally need it, right? Anyway, that option isn't related to this patch. If ARM devices started using SMBIOS and had another way to pass it to Linux (other than EFI) then we would want to install it.
We have: EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_smbios_table); This is invoked after efi_memory_init().
The EFI specification requires that the memory area occupied by the SMBIOS table uses one of a specific set of memory types where EfiRuntimeServicesData is recommended. So you must call
u64 addr = UINT_MAX; ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(size), *addr);
to allocate the memory. If the return code is not EFI_SUCCESS, no memory below 4 GiB is available.
The root problem here is that x86 and ARM used to work differently. When the ARM SMBIOS stuff was done, it worked by writing the SMBIOS table as part of the 'bootefi' command. On x86, the tables were written on startup, so you can examine them within U-Boot. Clearly the x86 approach is correct. For one thing, a previous-stage bootloader may set up the tables, so it simply isn't valid to write them in that case. So we need to separate writing the tables from telling EFI about them.
So I have fixed that, so ARM now writes the tables at the start. But using an EFI allocation function is clearly not right. This is generic code, nothing to do with EFI, really. In fact, the SMBIOS writing should move out of efi_loader. The install_smbios_table() function should be somewhere in lib, i suppose, with just efi_smbios_register() sitting in lib/efi_loader
Also, why is efi_memory_init() called early in init? Is there anything that needs that in the init sequence? Could we move it to the end, or perhaps skip it completely until the 'bootefi' command is used?
Another point I should make is that it should be fine for U-Boot to put something in memory and then call efi_add_memory_map() to tell EFI about it. What problems does that cause? It isn't as if EFI allocates things in the 'conventional' memory (is that the name for memory below 4GB?) This is how efi_acpi_register() works.
(Aside: it is bizarre to me that CONFIG_EFI_LOADER appears in drivers/video/rockchip_rk_vop.c and other such files)
The bit I am confused about is that we don't support SMBIOS3 in U-Boot. I am trying to fix an introduced bug...
I would not know why we should not use SMBIOS 3.
Neither do I. Perhaps there are compatibility concerns? If it is OK to do that then we could go back to my previous series [1]. What do you think?
Tom responded but I missed it. In part it says:
"So, can we please start by just doing the minimal changes to get the SMBIOS table done correctly for memory above 4G, via EFI, and then start the next steps?"
I am OK to do an EFI hack for ARM so long as we agree that after the release we will revert it and generate the table using generic memory allocation, not dependent on EFI. Does that sound reasonable?
I don't seem to have received any response from Heinrich to the various points I made above. I cannot see any response on patchwork either.
Drop it right after the release? No. We will replace it with what's more appropriate once we figure out what that is. And please don't call it a hack, unless I'm missing something I don't see yet how to make use of the SMBIOS table on ARM without EFI. I see something that implies MIPS can (and loongarch and ia64).
participants (3)
-
Heinrich Schuchardt -
Simon Glass -
Tom Rini