| Message ID | 20240906072231.2531491-15-patrick.rudolph@9elements.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Implement ACPI on aarch64 | expand |
Hi Patrick, On Fri, Sep 06, 2024 at 09:22:16AM +0200, 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. > > TEST: Booted QEMU SBSA using EFI and ACPI only. > Why none of the EFI maintainers were CC'ed into that? > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > 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); > } > + > +static int install_acpi_table(void) This function doesn't install anything. efi_acpi_register() nstalls the config tables for ACPI. This is just preparing and setting up correctly table_start_high/table_end_high AFAICT > +{ > + u64 rom_addr, rom_table_end; > + void *addr; > + > + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) || > + IS_ENABLED(CONFIG_X86) || > + IS_ENABLED(CONFIG_QFW_ACPI)) > + 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 alignment is already taken care of in efi_acpi_register(). Also an > + > + 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); > + if (rom_table_end - rom_addr > TABLE_SIZE) { > + log_err("Out of space for configuration tables: need %llx, have %x\n", > + 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"); > + > + return 0; > +} > + > +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_acpi_table); Why is this an event and how do you make sure it runs before efi_acpi_register()?. This should just run explicitly before that to prepare the ACPI tables and if it fails we shouldn't even try to install the config table /Ilias > 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:''' > -- > 2.45.2 >
Apologies for the noise +cc Heinrich On Fri, 6 Sept 2024 at 13:15, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Patrick, > > On Fri, Sep 06, 2024 at 09:22:16AM +0200, 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. > > > > TEST: Booted QEMU SBSA using EFI and ACPI only. > > > > Why none of the EFI maintainers were CC'ed into that? > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > --- > > 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); > > } > > + > > +static int install_acpi_table(void) > > This function doesn't install anything. efi_acpi_register() > nstalls the config tables for ACPI. This is just preparing and setting up > correctly table_start_high/table_end_high AFAICT > > > +{ > > + u64 rom_addr, rom_table_end; > > + void *addr; > > + > > + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) || > > + IS_ENABLED(CONFIG_X86) || > > + IS_ENABLED(CONFIG_QFW_ACPI)) > > + 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 alignment is already taken care of in efi_acpi_register(). Also an > > > + > > + 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); > > + if (rom_table_end - rom_addr > TABLE_SIZE) { > > + log_err("Out of space for configuration tables: need %llx, have %x\n", > > + 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"); > > + > > + return 0; > > +} > > + > > +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_acpi_table); > > Why is this an event and how do you make sure it runs before > efi_acpi_register()?. This should just run explicitly before that to prepare > the ACPI tables and if it fails we shouldn't even try to install the config > table > > /Ilias > > 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:''' > > -- > > 2.45.2 > >
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. > > 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. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> Please, cc maintainers as reported by scripts/get_maintainers in future. > --- > 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. > + 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. > + 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:'''
Hi Heinrich, On Fri, 6 Sept 2024 at 04:56, Heinrich Schuchardt <xypron.glpk@gmx.de> 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. > > > > > 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. > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > Please, cc maintainers as reported by scripts/get_maintainers in future. > > > --- > > 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. > > > + 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. I just want to address this and something below. We do want to put tables in a bloblist. This is how x86 works (although I haven't gone back and cleaned up all the boards) and I think we should do it for all archs. The memalign() here is fine too, since the actual setting of EFI memory type is done later, when the table is registered. > > > + > > + 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. That is not supported at present, unfortunately. I agree we should fix it, but not in this series. For now we can set a size of 64KB which should be plenty. The same bug exists in x86. > > > + 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:''' > Regards, Simon
On Fri, Sep 6, 2024 at 12:15 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Patrick, > > On Fri, Sep 06, 2024 at 09:22:16AM +0200, 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. > > > > TEST: Booted QEMU SBSA using EFI and ACPI only. > > > > Why none of the EFI maintainers were CC'ed into that? > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > --- > > 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); > > } > > + > > +static int install_acpi_table(void) > > This function doesn't install anything. efi_acpi_register() > nstalls the config tables for ACPI. This is just preparing and setting up > correctly table_start_high/table_end_high AFAICT > It was copied from efi_smbios.c. Please fix that file as well if you think it has the wrong name. > > +{ > > + u64 rom_addr, rom_table_end; > > + void *addr; > > + > > + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) || > > + IS_ENABLED(CONFIG_X86) || > > + IS_ENABLED(CONFIG_QFW_ACPI)) > > + 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 alignment is already taken care of in efi_acpi_register(). Also an > When the caller does not align the memory it could happen that efi_acpi_register() changes the page attribute of an existing mapping, which might break on the OS side. Please fix efi_acpi_register() to not assume anything and do not silently modify mem-ranges. An assert should be used instead to enforce proper alignment on page boundary. > > + > > + 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); > > + if (rom_table_end - rom_addr > TABLE_SIZE) { > > + log_err("Out of space for configuration tables: need %llx, have %x\n", > > + 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"); > > + > > + return 0; > > +} > > + > > +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_acpi_table); > > Why is this an event and how do you make sure it runs before > efi_acpi_register()?. This should just run explicitly before that to prepare > the ACPI tables and if it fails we shouldn't even try to install the config > table > It's reusing the same mechanism as existing code to make sure it runs before efi_acpi_register(). Existing code uses EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT,...); to make sure it runs before the tables are registered. > /Ilias > > 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:''' > > -- > > 2.45.2 > >
On Fri, Sep 6, 2024 at 12:44 PM Heinrich Schuchardt <xypron.glpk@gmx.de> 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 <patrick.rudolph@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > 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:''' >
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); } + +static int install_acpi_table(void) +{ + u64 rom_addr, rom_table_end; + void *addr; + + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) || + IS_ENABLED(CONFIG_X86) || + IS_ENABLED(CONFIG_QFW_ACPI)) + 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); + + 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); + if (rom_table_end - rom_addr > TABLE_SIZE) { + log_err("Out of space for configuration tables: need %llx, have %x\n", + 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"); + + 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:'''
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. TEST: Booted QEMU SBSA using EFI and ACPI only. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@konsulko.com> --- lib/efi_loader/efi_acpi.c | 57 ++++++++++++++++++++++++++++++-- test/py/tests/test_event_dump.py | 1 + 2 files changed, 56 insertions(+), 2 deletions(-)