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

[Heinrich Schuchardt]

Am 3. November 2023 19:12:40 OEZ schrieb Simon Glass <sjg at chromium.org>:
>Hi,
>
>On Sat, 28 Oct 2023 at 12:41, Simon Glass <sjg at 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 at 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 at gmx.de> wrote:
>> > >>
>> > >>
>> > >>
>> > >> Am 25. Oktober 2023 01:31:19 MESZ schrieb Simon Glass <sjg at 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