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

[Simon Glass]

Hi Ilias,

On Thu, 26 Sept 2024 at 13:18, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Thu, 26 Sept 2024 at 14:04, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Patrick,
> >
> > On Thu, 26 Sept 2024 at 10:01, Patrick Rudolph
> > <patrick.rudolph at 9elements.com> wrote:
> > >
> > > Hi Simon,
> > > On Fri, Sep 20, 2024 at 6:01 PM 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.
> > > >
> > > > >
> > > > > >
> > > > > > 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 - 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'
> > > >
> > >
> > > So what's the consent and the next step here?
> > > Is the current code OK as is, as it works with both BLOBLIST and without?
> > > Should I drop support for one or the other?
> >
> > Just BLOBLIST. The EFI allocation is done using efi_acpi.c which you
> > can check to make sure it is working.
> >
> > Using efi_allocate_pages() before the app starts is not good.
>
> Simon, please stop trying to enforce decisions on subsystems you don't
> maintain. I am pretty sure both Heinrich and I said no to this.

I am the ACPI maintainer, and x86 maintainer long before ACPI came in
for ARM (which I originally hacked together, as you know). I also
wrote bloblist, including the header file which says:

 * 6. Bloblist is designed to be passed to Linux as reserved memory. While linux
 * doesn't understand the bloblist header, it can be passed the indivdual blobs.
 * For example, ACPI tables can reside in a blob and the address of those is
 * passed to Linux, without Linux ever being away of the existence of a
 * bloblist. Having all the blobs contiguous in memory simplifies the
 * reserved-memory space.

This decision has serious impacts on memory management in U-Boot. It
also bears on the complexity of memory, how bootstd works, board
scripts and the like. We should discuss this and figure out a path
forward.

Regards,
Simon