[PATCH] smbios: arm64: Allow table to be written at a fixed addr

[Caleb Connolly]

On 23/10/2023 08:04, Simon Glass wrote:
> Hi Caleb,
> 
> On Sat, 21 Oct 2023 at 01:43, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On 21/10/2023 01:45, Simon Glass wrote:
>>> 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
>>> just below 4GB, if needed.
>> I may well be missing something here, but I don't quite understand the
>> justification behind the original patch [1] which caused this regression.
>>
>> I'm currently preparing support for a few different Qualcomm boards
>> which have different memory layouts, so far it's been possible to avoid
>> having any fixed addresses, so the same u-boot image can be used on all
>> of them (with just a different DTB).
> 
> What sort of memory layout do you have? I could enhance the patch to
> find the top of a memory bank below 4GB, perhaps? E.g. see the
> 'bdinfo' command.

I think that would probably be sensible (afaict this is more of less
what the EFI function did).

One of my boards has RAM starting at 0x40000000 and ending at
0xC0000000, frustratingly not quite managing to use the entire 4G
address space. There is also a hole in RAM, so u-boot relocates itself
to the end of the first memory bank

=> bdinfo
boot_params = 0x0000000000000000
DRAM bank   = 0x0000000000000000
-> start    = 0x0000000040000000
-> size     = 0x000000003eb00000
DRAM bank   = 0x0000000000000001
-> start    = 0x0000000080000000
-> size     = 0x0000000040000000
flashstart  = 0x0000000000000000
flashsize   = 0x0000000000000000
flashoffset = 0x0000000000000000
baudrate    = 115200 bps
relocaddr   = 0x000000007ea12000
reloc off   = 0x000000007ea12000
Build       = 64-bit
fdt_blob    = 0x000000007e5ec5c0
new_fdt     = 0x000000007e5ec5c0
fdt_size    = 0x0000000000006780
lmb_dump_all:
 memory.cnt = 0x2 / max = 0x40
 memory[0]	[0x40000000-0x7eafffff], 0x3eb00000 bytes flags: 0
 memory[1]	[0x80000000-0xbfffffff], 0x40000000 bytes flags: 0
 reserved.cnt = 0x8 / max = 0x40
 reserved[0]	[0x45700000-0x45cfffff], 0x00600000 bytes flags: 4
 reserved[1]	[0x45e00000-0x45f3ffff], 0x00140000 bytes flags: 4
 reserved[2]	[0x45fff000-0x461fffff], 0x00201000 bytes flags: 4
 reserved[3]	[0x4ab00000-0x53616fff], 0x08b17000 bytes flags: 4
 reserved[4]	[0x5c000000-0x5cffffff], 0x01000000 bytes flags: 4
 reserved[5]	[0x60000000-0x638fffff], 0x03900000 bytes flags: 4
 reserved[6]	[0x7d5e8000-0x7eafffff], 0x01518000 bytes flags: 0
 reserved[7]	[0x89b01000-0x89d00fff], 0x00200000 bytes flags: 4
> 
>>
>> As I mentioned before, efi_allocate_pages() seems to work fine from
>> install_smbios_table(), I haven't delved into the code too much but I
>> wonder if this could be an acceptable solution?
> 
> Unfortunately that function is EFI-specific. The function can only be
> called once EFI is inited. We only want to do that if booting with
> EFI.
> 
> The tables are written at the start of U-Boot, partly because that is
> how it is done on x86 and partly so the 'acpi' command actually works.
> 
> The EFI code ended up writing a separate set of tables, which is of
> course not correct.
> 
> I did look at creating an API in U-Boot to alloc memory below 4GB but
> then decided that for just this one use case it might not be worth it.

The LMB allocator can already do this with lmb_alloc_base(), would this
be an ok solution?

I have tested it and it seems to work as intended, allocating the first
free 32-bit address, and avoiding reserved regions.

It seems like the LMB allocator doesn't have a global pool, so I'm not
sure how it avoids conflicts, although maybe that just isn't really an
issue in reality.
> 
>>
>> Barring that, could we use an environment variable to pass this address
>> in? Although I think that might not be a very desirable solution.
>>
>> I appreciate you taking the time to prepare this patch and CC me. If
>> nobody else objects to this patch then I'd prefer to avoid blocking it.
>> In either case, I'd greatly appreciate any input on the above
>> suggestions so that I can implement a solution for my boards.
> 
> I would like it to be automatic. I assumed on ARM64 that there is
> memory available at 3.99GB if U-Boot has relocated to above 4GB, but
> as above I could make this more clever.
> 
> Regards,
> Simon
> 
> 
>>
>> Thanks and regards,
>>
>> [1]:
>> https://lore.kernel.org/u-boot/20230920030027.1385833-16-sjg@chromium.org/
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>   lib/Kconfig                 | 17 +++++++++++++++++
>>>   lib/efi_loader/efi_smbios.c | 12 ++++++++++++
>>>   2 files changed, 29 insertions(+)
>>>
>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>> index f6ca559897e7..a77f2f3e9089 100644
>>> --- a/lib/Kconfig
>>> +++ b/lib/Kconfig
>>> @@ -994,6 +994,23 @@ 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 fixed 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.
>>> +
>>> +config SMBIOS_TABLE_FIXED_ADDR
>>> +     hex "Fixed address for use for SMBIOS table"
>>> +     depends on SMBIOS_TABLE_FIXED
>>> +     default 0xffff0000
>>> +
>>>   endmenu
>>>
>>>   config LIB_RATIONAL
>>> diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
>>> index 48446f654d9b..84ea027ea48c 100644
>>> --- a/lib/efi_loader/efi_smbios.c
>>> +++ b/lib/efi_loader/efi_smbios.c
>>> @@ -61,6 +61,18 @@ static int install_smbios_table(void)
>>>               return log_msg_ret("mem", -ENOMEM);
>>>
>>>       addr = map_to_sysmem(buf);
>>> +
>>> +     /*
>>> +      * Deal with a fixed address if needed. For simplicity we assume that
>>> +      * the SMBIOS-table size is <64KB and that the malloc region does not
>>> +      * straddle the 4GB boundary.
>>> +      */
>>> +     if (IS_ENABLED(CONFIG_SMBIOS_TABLE_FIXED) && addr >= SZ_4G - SZ_64K) {
>>> +             addr = IF_ENABLED_INT(CONFIG_SMBIOS_TABLE_FIXED,
>>> +                                   CONFIG_SMBIOS_TABLE_FIXED_ADDR);
>>> +             log_warning("Forcing SMBIOS table to address %lx\n", addr);
>>> +     }
>>> +
>>>       if (!write_smbios_table(addr)) {
>>>               log_err("Failed to write SMBIOS table\n");
>>>               return log_msg_ret("smbios", -EINVAL);
>>
>> --
>> // Caleb (they/them)

-- 
// Caleb (they/them)