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

[Heinrich Schuchardt]

Am 3. November 2023 20:14:46 OEZ schrieb Simon Glass <sjg at chromium.org>:
>Hi Heinrich,
>
>On Fri, 3 Nov 2023 at 11:52, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>>
>>
>> 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.
>
>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