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

[Simon Glass]

Hi Tom,

On Fri, 3 Nov 2023 at 13:26, Tom Rini <trini at 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 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.
>
> 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