On Fri, Sep 6, 2024 at 12:44 PM Heinrich Schuchardt <[email protected]> wrote: > > On 9/6/24 09:22, Patrick Rudolph wrote: > > Install ACPI tables inside the efi_loader 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 UEFI specification clearly describes how to hand over ACPI tables. I > can't see why bloblists should be of any relevance in the UEFI context. > It's where tables are stored, not how to pass them to the OS.
> > > > TEST: Booted QEMU SBSA using EFI and ACPI only. > > With this description it is unclear if you used QFW to install ACPI > tables which would not test the code below. > QEMU SBSA implies that no QFW was used. Will update the commit message. > > > > Signed-off-by: Patrick Rudolph <[email protected]> > > Cc: Simon Glass <[email protected]> > > Cc: Tom Rini <[email protected]> > > Please, cc maintainers as reported by scripts/get_maintainers in future. > Thanks, will do. > > --- > > lib/efi_loader/efi_acpi.c | 57 ++++++++++++++++++++++++++++++-- > > test/py/tests/test_event_dump.py | 1 + > > 2 files changed, 56 insertions(+), 2 deletions(-) > > > > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c > > index 67bd7f8ca2..9262f21ea6 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,48 @@ efi_status_t efi_acpi_register(void) > > return efi_install_configuration_table(&acpi_guid, > > (void *)(ulong)addr); > > } > > + > > Please, provide a function description. > > > +static int install_acpi_table(void) > > +{ > > + u64 rom_addr, rom_table_end; > > There is no ROM involved here. Please, use meaningful variable names. > > > + void *addr; > > + > > + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) || > > + IS_ENABLED(CONFIG_X86) || > > + IS_ENABLED(CONFIG_QFW_ACPI)) > > Why would you want to call write_acpi_tables() twice on the sandbox? See > > board/sandbox/sandbox.c:180: > end = write_acpi_tables(addr); > > We should remove the redundant code from board/sandbox/sandbox.c with > this patch. > Missed that, thanks! > > + 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)); > > + else > > + addr = memalign(SZ_4K, TABLE_SIZE); > > The UEFI specification wants ACPI tables to reside in > EfiACPIReclaimMemory. Neither bloblist_add() nor memalign() should be > used here. > > > + > > + if (!addr) > > + return log_msg_ret("mem", -ENOBUFS); > > + > > + rom_addr = virt_to_phys(addr); > > + > > + gd->arch.table_start_high = rom_addr; > > + > > + rom_table_end = write_acpi_tables(rom_addr); > > + if (!rom_table_end) { > > + log_err("Can't create ACPI configuration table\n"); > > + return -EINTR; > > + } > > + > > + debug("- wrote 'acpi' to %llx, end %llx\n", rom_addr, rom_table_end); > > Please, use log_debug(). > > > + if (rom_table_end - rom_addr > TABLE_SIZE) { > > + log_err("Out of space for configuration tables: need %llx, > > have %x\n", > > This check is too late. We must ensure that write_acpi_table() does not > overwrite unallocated memory. > At least there's a check. > > + rom_table_end - rom_addr, TABLE_SIZE); > > + return log_msg_ret("acpi", -ENOSPC); > > + } > > + gd->arch.table_end_high = rom_table_end; > > + > > + debug("- done writing tables\n"); > > ditto > > Best regards > > Heinrich > > > + > > + return 0; > > +} > > + > > +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_acpi_table); > > diff --git a/test/py/tests/test_event_dump.py > > b/test/py/tests/test_event_dump.py > > index e282c67335..88bbf34f71 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 install_acpi_table > > .*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:''' >
