[PATCH v4 14/35] efi_loader: Allocate and write ACPI tables
Simon Glass
sjg at chromium.org
Fri Sep 27 14:35:06 CEST 2024
Hi Ilias,
On Fri, 27 Sept 2024 at 05:30, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Fri, 27 Sept 2024 at 13:53, Simon Glass <sjg at chromium.org> wrote:
> >
> > 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:
>
> That's not the EFI subsystem though. And last time I checked where the
> ACPI tables are allocated makes no difference as long as they are
> installed on a config table.
With my maintainer hat on, ACPI tables go in a bloblist.
The difference it makes is that the tables end up in the U-Boot memory-area.
>
> >
> > * 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.
>
> And as I already said, *when* bloblist is consumed by any OS and you
> have a valid use case other than "I can do bloblist list and I like
> it", you can change 3 lines of code and allocate the ACPI table there.
Bloblist is not intended to be consumed by an OS. It is a firmware
construct, a way for U-Boot to arrange things in such a way that they
can be located and updated at runtime. What objection do you actually
have to using bloblist for tables?
>
> > * 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.
>
> No, it doesn't because all of the other tables are currently allocated
> by the EFI subsystem. So it actually fragments it. Apart from that You
> dont really know what the OS is going to do with that memory. It
> depends on the EFI memory type and OS decisions.
The ACPI table is not allocated by the EFI subsystem. Neither is
SMBIOS, which currently uses malloc() but should move into a bloblist
too. The system table is in U-Boot's data region. Much of EFI's data
and all the driver model stuff is in the malloc() region.
>
> >
> > 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.
>
> It has close to zero impact because *all* of the EFI memory lives in
> top memory. So apart from the EFI_BOUNCE_BUFFER perhaps, I don't see
> why the impact is """serious""".
> EFI allocates memory which does not affect U-Boot of file loading
> apart from the max size allowed. Now with LMB it is marked as
> reserved. So why is this any different from any other LMB reservation?
> It's memory used by someone that you aren't allowed to use, just like
> we reserve U-Boot memory. If there's a bug in reservations, by all
> means, let's fix it.
I think you are saying that because only a small amount of memory is
affected it doesn't matter much. Well, I suppose so. But if it doesn't
matter, why is Sughosh spending all this time solving the problem?
Also with [1] I definitely saw it allocating stuff where it should
not. If it has so little impact, why are we still discussing it? Also,
my efforts seem to have uncovered latent bugs, where EFI tests fail if
there is not a large margin after each allocation. That should be
cause for concern. We would not put up with that elsewhere in U-Boot.
I am not trying to mess with EFI in U-Boot, if that is what you are
worried about. Actually I haven't been paying it enough attention as
it is in good hands...but I need to put more time into it. As a
subsystem it is here for the long haul and it needs to interoperate
properly. I was at a conference last week where there was a talk about
U-Boot as an EFI app with a pass-through (i.e. not calling
exit-boot-services) to Linux's EFI stub. There is a lot of activity in
this area. EFI is an important piece of technology in the industry.
But this is U-Boot, and we just need to design things properly. I have
quietly commented on some of my concerns (e.g. efi_set_bootdev()) but
have not put time into fixing them.
There is no need for the 6-8 tidy allocations, which are done before
the app starts, to be allocated in the wrong place, with a small
impact. We can just do it in malloc(). Let's just fix it.
There is no need for the tables to be allocated in an EFI-specific
way, or using lmb. We have bloblist for that purpose. What's the
concern?
Regards,
Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20240725135629.3505072-7-sjg@chromium.org/
More information about the U-Boot
mailing list