[PATCH v3 14/30] efi_loader: Allocate and write ACPI tables
Ilias Apalodimas
ilias.apalodimas at linaro.org
Thu Sep 19 15:17:25 CEST 2024
Hi Simon,
On Thu, 19 Sept 2024 at 16:01, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 19 Sept 2024 at 09:30, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > On Thu, 19 Sept 2024 at 09:41, Patrick Rudolph
> > <patrick.rudolph at 9elements.com> wrote:
> > >
> > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > Hi Patrick
> > > >
> > > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph
> > > > <patrick.rudolph at 9elements.com> wrote:
> > > > >
> > > > > Allocate memory for ACPI tables inside the efi_loader and write out
> > > > > the tables similar to SMBIOS tables. When ACPI is enabled and wasn't
> > > > > installed in other places, install the ACPI table in EFI. Since EFI
> > > > > is necessary to pass the ACPI table location when FDT isn't used,
> > > > > there's no need to install it separately.
> > > > >
> > > > > When CONFIG_BLOBLIST_TABLES is set the tables will be stored in a
> > > > > bloblist. The tables are still passed to the OS using EFI.
> > > > >
> > > > > This allows non x86 platforms to boot using ACPI only in case the
> > > > > EFI loader is being used.
> > > > >
> > > > > TEST: Booted QEMU SBSA (no QFW) using EFI and ACPI only.
> > > > >
> > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph at 9elements.com>
> > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > ---
> > > > > Changelog v3:
> > > > > - Drop memalign and use efi_allocate_pages
> > > > > - Use log_debug instead of debug
> > > > > - Clarify commit message
> > > > > - Skip writing ACPI tables on sandbox
> > > > > - Rename function
> > > > > - Add function comment
> > > > > ---
> > > > > lib/efi_loader/efi_acpi.c | 80 +++++++++++++++++++++++++++++++-
> > > > > test/py/tests/test_event_dump.py | 1 +
> > > > > 2 files changed, 79 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> > > > > index 67bd7f8ca2..9d38d0060c 100644
> > > > > --- a/lib/efi_loader/efi_acpi.c
> > > > > +++ b/lib/efi_loader/efi_acpi.c
> > > > > @@ -6,15 +6,23 @@
> > > > > */
> > > > >
> > > > > #include <efi_loader.h>
> > > > > -#include <log.h>
> > > > > -#include <mapmem.h>
> > > > > #include <acpi/acpi_table.h>
> > > > > #include <asm/global_data.h>
> > > > > +#include <asm/io.h>
> > > > > +#include <bloblist.h>
> > > > > +#include <linux/sizes.h>
> > > > > +#include <linux/log2.h>
> > > > > +#include <log.h>
> > > > > +#include <malloc.h>
> > > > > +#include <mapmem.h>
> > > > >
> > > > > DECLARE_GLOBAL_DATA_PTR;
> > > > >
> > > > > static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
> > > > >
> > > > > +enum {
> > > > > + TABLE_SIZE = SZ_64K,
> > > > > +};
> > > > > /*
> > > > > * Install the ACPI table as a configuration table.
> > > > > *
> > > > > @@ -47,3 +55,71 @@ efi_status_t efi_acpi_register(void)
> > > > > return efi_install_configuration_table(&acpi_guid,
> > > > > (void *)(ulong)addr);
> > > > > }
> > > > > +
> > > > > +/*
> > > > > + * Allocate memory for ACPI tables and write ACPI tables to the
> > > > > + * allocated buffer.
> > > > > + *
> > > > > + * Return: status code
> > > > > + */
> > > > > +static int alloc_write_acpi_tables(void)
> > > > > +{
> > > > > + u64 table_addr, table_end;
> > > > > + u64 new_acpi_addr = 0;
> > > > > + efi_uintn_t pages;
> > > > > + efi_status_t ret;
> > > > > + void *addr;
> > > > > +
> > > > > + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE))
> > > > > + return 0;
> > > > > +
> > > > > + if (IS_ENABLED(CONFIG_X86) ||
> > > > > + IS_ENABLED(CONFIG_QFW_ACPI) ||
> > > > > + IS_ENABLED(CONFIG_SANDBOX)) {
> > > > > + log_debug("Skipping writing ACPI tables as already done\n");
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + /* Align the table to a 4KB boundary to keep EFI happy */
> > > > > + if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) {
> > > > > + addr = bloblist_add(BLOBLISTT_ACPI_TABLES, TABLE_SIZE,
> > > > > + ilog2(SZ_4K));
> > > > > +
> > > > > + if (!addr)
> > > > > + return log_msg_ret("mem", -ENOMEM);
> > > >
> > > > This addr is not in ACPI_RECLAIM_MEMORY now. It might work since
> > > > efi_acpi_register() adads it on the EFI memory map.
> > > > In the last version, Simon replied something along the lines of "This
> > > > is how x86 works", but I don't understand why. U-Boot at this point is
> > > > the last bootloader you'll run and then jump to your EFI payload. Can
> > > > someone clearly explain what's going on here?
> > > >
> > > > > + } 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;
> > > >
> > > > So this would be fine if we initialized the EFI subsystem early, but
> > > > we don't. On top of that the efi_acpi_register() call is trying to
> > > > add the allocated memory to the efi memmap, so we don't need to do
> > > > that twice.
> > >
> > > Sounds like always using BLOBLIST would fix this issue. It would decouple
> > > memory allocation and installation.
> >
> > bloblist is supposed to be used by early-stage bootloaders to pass
> > information around and is an implementation of [0]. Why you would
> > U-Boot need to add the item in a bloblist? There's no loader after
> > U-Boot that can consume it.
>
> That is one use of bloblist.
>
> Within U-Boot proper, bloblist is used to hold all the tables
> generated by U-Boot, so they are all in one place.
What do you mean by 'tables'. That makes little sense to me,
especially with the caveats it comes along. ACPI is an EFI construct
and we should limit it there. I can understand this if a next stage
loader needs the ACPI in a transfer list, but that use case does not
exist.
Thanks
/Ilias
>
> >
> > >
> > > >
> > > > I think there's a cleaner approach overall to this which will allow us
> > > > to decouple the installation etc from EFI.
> > > > You could allocate a piece of memory, assign it to
> > > > arch.table_start/end and then tweak efi_acpi_register(). Instead of
> > > > marking the table_end - table_start as ACPI_RECLAIM_MEMORY, allocate
> > > > the pages there, where you know the EFI subsystem is up and copy it.
> > > > It would then be the callers responsibilty to free that memory.
> > > >
> > > We cannot memcpy ACPI tables to a new location. For example the RSDT and XSDT
> > > contain physical addresses to memory within the ACPI reclaim window,
> > > which must be tweaked
> > > when you copy tables. On x86 there's also the GNVS which is patched
> > > into the DSDT, which points
> > > to memory within the ACPI reclaim memory window.
> > > There might be more places I'm not aware of as of now.
> >
> > Ok, that's fine though, we can just mark the memory as
> > ACPI_RECLAIM_MEMORY, like we currently do, without allocating and
> > copying over stuff.
>
> Regards,
> Simon
>
>
> >
> > Thanks
> > /Ilias
> > >
> > > > Heinrich, do you see a problem with that?
> > > >
> > > > Thanks
> > > > /Ilias
> > > >
> > > > > + }
> > > > > +
> > > > > + table_addr = virt_to_phys(addr);
> > > > > +
> > > > > + gd->arch.table_start_high = table_addr;
> > > > > +
> > > > > + table_end = write_acpi_tables(table_addr);
> > > > > + if (!table_end) {
> > > > > + log_err("Can't create ACPI configuration table\n");
> > > > > + return -EINTR;
> > > > > + }
> > > > > +
> > > > > + log_debug("- wrote 'acpi' to %llx, end %llx\n", table_addr, table_end);
> > > > > + if (table_end - table_addr > TABLE_SIZE) {
> > > > > + log_err("Out of space for configuration tables: need %llx, have %x\n",
> > > > > + table_end - table_addr, TABLE_SIZE);
> > > > > + return log_msg_ret("acpi", -ENOSPC);
> > > > > + }
> > > > > + gd->arch.table_end_high = table_end;
> > > > > +
> > > > > + log_debug("- done writing tables\n");
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, alloc_write_acpi_tables);
> > > > > diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py
> > > > > index e282c67335..459bfa26bb 100644
> > > > > --- a/test/py/tests/test_event_dump.py
> > > > > +++ b/test/py/tests/test_event_dump.py
> > > > > @@ -18,6 +18,7 @@ def test_event_dump(u_boot_console):
> > > > > -------------------- ------------------------------ ------------------------------
> > > > > EVT_FT_FIXUP bootmeth_vbe_ft_fixup .*boot/vbe_request.c:.*
> > > > > EVT_FT_FIXUP bootmeth_vbe_simple_ft_fixup .*boot/vbe_simple_os.c:.*
> > > > > +EVT_LAST_STAGE_INIT alloc_write_acpi_tables .*lib/efi_loader/efi_acpi.c:.*
> > > > > EVT_LAST_STAGE_INIT install_smbios_table .*lib/efi_loader/efi_smbios.c:.*
> > > > > EVT_MISC_INIT_F sandbox_early_getopt_check .*arch/sandbox/cpu/start.c:.*
> > > > > EVT_TEST h_adder_simple .*test/common/event.c:'''
> > > > > --
> > > > > 2.46.0
> > > > >
> >
> > [0] https://github.com/FirmwareHandoff/firmware_handoff
More information about the U-Boot
mailing list