| Message ID | 20240911062511.494855-15-patrick.rudolph@9elements.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Implement ACPI on aarch64 | expand |
Hi Patrick, On Wed, 11 Sept 2024 at 00:25, Patrick Rudolph <patrick.rudolph@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@9elements.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@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); > + } else { > + pages = efi_size_in_pages(TABLE_SIZE); > + > + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, > + EFI_ACPI_RECLAIM_MEMORY, > + pages, &new_acpi_addr); I know we are going to have a problem here since EFI memory-allocation is a bit of a mess, and you got conflicting advice on the previous version.... But we cannot call efi_allocate_pages() during U-Boot init. EFI may not even be enabled. The allocation ends up being in the area between the bottom of memory at the bottom of the stack, which is not supposed to be used, at least until images are loaded. I think in fact we should make this ACPI stuff depend on bloblist and just put the tables there. So set up bloblist size to something small, like 0x100 and set BLOBLIST_SIZE_RELOC to larger than you need (e.g. 68KB). Everything should work OK. I will do a bit of a tidy-up after this series goes in. > + if (ret != EFI_SUCCESS) > + return log_msg_ret("mem", -ENOMEM); > + > + addr = (void *)(uintptr_t)new_acpi_addr; > + } > + > + 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 > Regards, Simon
Hi Patrick On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph <patrick.rudolph@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@9elements.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@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. 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. 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 >
Oh and can you please cc me on the entire series, it's overall very interesting to me Thanks /Ilias On Thu, 12 Sept 2024 at 09:54, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Patrick > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > <patrick.rudolph@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@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@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. > > 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. > > 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 > >
On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Patrick > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > <patrick.rudolph@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@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@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. > > 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. > 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 > >
On Thu, 19 Sept 2024 at 09:41, Patrick Rudolph <patrick.rudolph@9elements.com> wrote: > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Patrick > > > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > > <patrick.rudolph@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@9elements.com> > > > Cc: Simon Glass <sjg@chromium.org> > > > Cc: Tom Rini <trini@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. > > > > > 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. 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
On 11.09.24 08:24, Patrick Rudolph 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@9elements.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> Could you, please, describe how QEMU SBSA is passing the ACPI table to U-Boot and where you convert this to a bloblist. I could not find this in patches 1-13. Please, provide a test that we can run on our CI. Best regards Heinrich > --- > 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); > + } 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; > + } > + > + 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:'''
On Thu, Sep 19, 2024 at 2:39 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11.09.24 08:24, Patrick Rudolph 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@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > Could you, please, describe how QEMU SBSA is passing the ACPI table to > U-Boot and where you convert this to a bloblist. I could not find this > in patches 1-13. I doesn't. All ACPI tables are generated within U-Boot. > > Please, provide a test that we can run on our CI. How would such a test look like? > > Best regards > > Heinrich > > > --- > > 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); > > + } 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; > > + } > > + > > + 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:''' >
On 12.09.24 02:58, Simon Glass wrote: > Hi Patrick, > > On Wed, 11 Sept 2024 at 00:25, Patrick Rudolph > <patrick.rudolph@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@9elements.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Tom Rini <trini@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); >> + } else { >> + pages = efi_size_in_pages(TABLE_SIZE); >> + >> + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, >> + EFI_ACPI_RECLAIM_MEMORY, >> + pages, &new_acpi_addr); > > I know we are going to have a problem here since EFI memory-allocation > is a bit of a mess, and you got conflicting advice on the previous > version.... > > But we cannot call efi_allocate_pages() during U-Boot init. EFI may > not even be enabled. The allocation ends up being in the area between > the bottom of memory at the bottom of the stack, which is not supposed > to be used, at least until images are loaded. Please, have a look at init_sequence_r[]. The memory allocate for the EFI sub-system is initialized in efi_memory_init directly after initr_lmb. So for sure we can and should use efi_allocate_pages() here. We must not use bloblist_add() as the memory allocation would not be reported as AcpiReclaimMemory. Best regards Heinrich > > I think in fact we should make this ACPI stuff depend on bloblist and > just put the tables there. So set up bloblist size to something small, > like 0x100 and set BLOBLIST_SIZE_RELOC to larger than you need (e.g. > 68KB). Everything should work OK. I will do a bit of a tidy-up after > this series goes in. > >> + if (ret != EFI_SUCCESS) >> + return log_msg_ret("mem", -ENOMEM); >> + >> + addr = (void *)(uintptr_t)new_acpi_addr; >> + } >> + >> + 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 >> > > Regards, > Simon
Hi Ilias, On Thu, 19 Sept 2024 at 09:30, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Thu, 19 Sept 2024 at 09:41, Patrick Rudolph > <patrick.rudolph@9elements.com> wrote: > > > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Patrick > > > > > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > > > <patrick.rudolph@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@9elements.com> > > > > Cc: Simon Glass <sjg@chromium.org> > > > > Cc: Tom Rini <trini@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. > > > > > > > > > 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
Hi Simon, On Thu, 19 Sept 2024 at 16:01, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Thu, 19 Sept 2024 at 09:30, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > On Thu, 19 Sept 2024 at 09:41, Patrick Rudolph > > <patrick.rudolph@9elements.com> wrote: > > > > > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Patrick > > > > > > > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > > > > <patrick.rudolph@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@9elements.com> > > > > > Cc: Simon Glass <sjg@chromium.org> > > > > > Cc: Tom Rini <trini@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
On Thu, 19 Sept 2024 at 15:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12.09.24 02:58, Simon Glass wrote: > > Hi Patrick, > > > > On Wed, 11 Sept 2024 at 00:25, Patrick Rudolph > > <patrick.rudolph@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@9elements.com> > >> Cc: Simon Glass <sjg@chromium.org> > >> Cc: Tom Rini <trini@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 +++++++++++++++++++++++++++++++- [...] > > Please, have a look at init_sequence_r[]. > > The memory allocate for the EFI sub-system is initialized in > efi_memory_init directly after initr_lmb. > > So for sure we can and should use efi_allocate_pages() here. Ah thanks Heinrich. Yes we should, I wrongly remembered we init the memory subsystem when we init the EFI one. Thanks /Ilias > > We must not use bloblist_add() as the memory allocation would not be > reported as AcpiReclaimMemory. > > Best regards > > Heinrich > > > > > I think in fact we should make this ACPI stuff depend on bloblist and > > just put the tables there. So set up bloblist size to something small, > > like 0x100 and set BLOBLIST_SIZE_RELOC to larger than you need (e.g. > > 68KB). Everything should work OK. I will do a bit of a tidy-up after > > this series goes in. > > > >> + if (ret != EFI_SUCCESS) > >> + return log_msg_ret("mem", -ENOMEM); > >> + > >> + addr = (void *)(uintptr_t)new_acpi_addr; > >> + } > >> + > >> + 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 > >> > > > > Regards, > > Simon >
Hi Heinrich, On Thu, 19 Sept 2024 at 14:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12.09.24 02:58, Simon Glass wrote: > > Hi Patrick, > > > > On Wed, 11 Sept 2024 at 00:25, Patrick Rudolph > > <patrick.rudolph@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@9elements.com> > >> Cc: Simon Glass <sjg@chromium.org> > >> Cc: Tom Rini <trini@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); > >> + } else { > >> + pages = efi_size_in_pages(TABLE_SIZE); > >> + > >> + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, > >> + EFI_ACPI_RECLAIM_MEMORY, > >> + pages, &new_acpi_addr); > > > > I know we are going to have a problem here since EFI memory-allocation > > is a bit of a mess, and you got conflicting advice on the previous > > version.... > > > > But we cannot call efi_allocate_pages() during U-Boot init. EFI may > > not even be enabled. The allocation ends up being in the area between > > the bottom of memory at the bottom of the stack, which is not supposed > > to be used, at least until images are loaded. > > Please, have a look at init_sequence_r[]. > > The memory allocate for the EFI sub-system is initialized in > efi_memory_init directly after initr_lmb. > > So for sure we can and should use efi_allocate_pages() here. No, no. We need to keep the memory free for loading images. I am working to drop all EFI allocations until an EFI app is actually booted. > > We must not use bloblist_add() as the memory allocation would not be > reported as AcpiReclaimMemory. That is the purpose of the efi_add_memory_map() call in lib/efi_loader/efi_acpi.c Regards, Simon > > Best regards > > Heinrich > > > > > I think in fact we should make this ACPI stuff depend on bloblist and > > just put the tables there. So set up bloblist size to something small, > > like 0x100 and set BLOBLIST_SIZE_RELOC to larger than you need (e.g. > > 68KB). Everything should work OK. I will do a bit of a tidy-up after > > this series goes in. > > > >> + if (ret != EFI_SUCCESS) > >> + return log_msg_ret("mem", -ENOMEM); > >> + > >> + addr = (void *)(uintptr_t)new_acpi_addr; > >> + } > >> + > >> + 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 > >> > > > > Regards, > > Simon >
Hi Ilias, On Thu, 19 Sept 2024 at 15:18, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Thu, 19 Sept 2024 at 16:01, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Thu, 19 Sept 2024 at 09:30, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > On Thu, 19 Sept 2024 at 09:41, Patrick Rudolph > > > <patrick.rudolph@9elements.com> wrote: > > > > > > > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Patrick > > > > > > > > > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > > > > > <patrick.rudolph@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@9elements.com> > > > > > > Cc: Simon Glass <sjg@chromium.org> > > > > > > Cc: Tom Rini <trini@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. Please take a look at the top of bloblist.h - this is how newer x86 platforms work and it has always been intended that way. It is not a transfer list, but a bloblist, at least in U-Boot parlance. Regards, Simon > > 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
Hi Patrick, On Thu, 19 Sept 2024 at 08:41, Patrick Rudolph <patrick.rudolph@9elements.com> wrote: > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Patrick > > > > On Wed, 11 Sept 2024 at 09:25, Patrick Rudolph > > <patrick.rudolph@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@9elements.com> > > > Cc: Simon Glass <sjg@chromium.org> > > > Cc: Tom Rini <trini@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. Yes. > > > > > 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. Right, there should be no need to copy. EFI can point to the tables in the bloblist. > > > 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 > > > Regards, Simon
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); + } 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; + } + + 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:'''
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@9elements.com> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@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(-)