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

[Simon Glass]

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.

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 :-)

>
> >
> > >
> > > >
> > > > 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.

Regards,
Simon

[..]