[PATCH v4 14/35] efi_loader: Allocate and write ACPI tables

[Ilias Apalodimas]

Hi Simon,

On Fri, 20 Sept 2024 at 19:01, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Fri, 20 Sept 2024 at 08:36, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Thu, 19 Sept 2024 at 18:36, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, 19 Sept 2024 at 17:20, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Thu, 19 Sept 2024 at 18:00, Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > > > > +
> > > > > > > > +               if (!addr)
> > > > > > > > +                       return log_msg_ret("mem", -ENOMEM);
> > > > > > > > +       } else {
> > > > > > > > +               pages = efi_size_in_pages(TABLE_SIZE);
> > > > > > > > +
> > > > > > > > +               ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> > > > > > > > +                                        EFI_ACPI_RECLAIM_MEMORY,
> > > > > > > > +                                        pages, &new_acpi_addr);
> > > > > > > > +               if (ret != EFI_SUCCESS)
> > > > > > > > +                       return log_msg_ret("mem", -ENOMEM);
> > > > > > > > +
> > > > > > > > +               addr = (void *)(uintptr_t)new_acpi_addr;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > >
> > > > > > > The tables should be written regardless of whether EFI_LOADER is enabled.
> > > > > >
> > > > > > *Why*? How do you expect to hand them over to the OS?
> > > > >
> > > > > Why - because boards which need ACPI tables to boot should generate
> > > > > them;
> > > >
> > > > Noone argued that.
> > > >
> > > > > also this happens when U-Boot starts up, in last_stage_init()
> > > > > How - it isn't possible, but eventually I suppose it will be, once we
> > > > > have a use case for booting with ACPI but without EFI
> > > >
> > > > Are you aware of such an OS? If not, we can accept the patches when we
> > > > have a reason.
> > >
> > > Which patches? This is how it works today. We set up the tables in
> > > last_stage_init(), so they can be examined while in U-Boot.
> >
> > What I mean, is that until we have a valid use case to store the ACPI
> > in a bloblist, I prefer them being allocated with proper EFI memory
> > backing
>
> I'm going to assert prior art here. If ARM is to have ACPI in U-Boot,
> it should follow the most recent x86 approach and store it in the
> bloblist. That is what the blloblist is for. It also avoids using
> memory below the U-Boot area, which is not allowed.

By prior art you mean [0], which was none of the EFI maintainers got involved?

>
> >
> > >
> > > It is the patches to change this which I object to.
> > >
> > > > I remember patches being hard NAK'ed on using DTs to pass the ACPI
> > > > table address in the past.
> > >
> > > Yes, I believe Bytedance carries a patch locally for that :-)
> >
> > Exactly
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Can we drop this else clause? We should always use the bloblist.
> > > > > >
> > > > > > Both Heinrich and I said the exact opposite. Unless there's a
> > > > > > perfectly good reason why we should keep them in bloblist memory, I'd
> > > > > > like us to do that
> > > > >
> > > > > The main reason is that we should not be putting things in memory
> > > > > between the start of RAM and the bottom of the U-Boot area. That
> > > > > region of memory is supposed to be for loading images. Most boards
> > > > > hard-code the image-load addresses in environment variables. But even
> > > > > if they didn't, we should not be using that memory for U-Boot data.
> > > >
> > > > EFI allows you to request a specific address to use. We can choose one
> > > > that fits the requirements
> > >
> > > So long as it is in U-Boot's space, that is fine. I just sent a patch
> > > about relocation which covers this. It occured to me that this feature
> > > of U-Boot, which I have always assumed was common knowledge, may have
> > > not been noticed.
> > >
> > > > >
> > > > > Another reason is that the bloblist is designed for this, for keeping
> > > > > tables in a small, contiguous area of memory, so that when passed to
> > > > > Linux they are not spread all over the place.
> > > >
> > > > Linux does not and will not read tables from a bloblist though
> > >
> > > Indeed. There really isn't any point, anyway, since there are other
> > > ways of passing the tables along...except for ACPI with FDT but that
> > > isn't a valid use case for Linux so far.
> >
> > What we are trying to do with EFI is to stay as close to the spec and
> > the current OS implementations as possible. So again, please allocate
> > EFI memory for the ACPI tables. The patches to convert it to bloblist
> > allocating them are minimal, so we can do it, once there's an actuall
> > need for it.
>
> As it stands today, efi_acpi_register() does not do an EFI allocation,
> it just adds the existing allocation to the EFI tables. This works
> fine for x86 - see arch/x86/lib/tables.c -

Yes and I've already mentioned that in my original response.

> so I would like ARM to do
> the same. The need for it is (as above) that we should ensure that
> memory usage is within the U-Boot area. This is something which
> bloblist handles automatically. It is also nice to see all the tables
> in one place with 'bloblist list'

Unless bloblist is not compiled.... in which case you still need to
malloc and align.
I am just putting this out there, because your next email is going to
be "oh look bloblist is mandatory now".
For the last time, bloblist make zero sense to allocate data that
arent going to be handed across loaders.

Patrick, my suggestion is to either use efi_allocate_pages() as you do
or malloc a properly aligned buffer since we set the memory type in
efi_acpi_register().
IIRC the reason we set that in efi_acpi_register() is for QEMU which
hands us over ACPI tables, so we can't do much. I would personally
prefer efi allocated memory.

Thanks
/Ilias


>
> Regards,
> Simon

[0] https://lore.kernel.org/u-boot/20201104165743.632571-11-sjg@chromium.org/