Hi Heinrich,
On Fri, 6 Sept 2024 at 04:56, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 9/6/24 09:22, Patrick Rudolph wrote:
Install ACPI tables inside the efi_loader similar to SMBIOS tables. When ACPI is enabled and wasn't installed in other places, install the ACPI table in EFI. Since EFI is necessary to pass the ACPI table location when FDT isn't used, there's no need to install it separately.
When CONFIG_BLOBLIST_TABLES is set the tables will be stored in a bloblist.
The UEFI specification clearly describes how to hand over ACPI tables. I can't see why bloblists should be of any relevance in the UEFI context.
TEST: Booted QEMU SBSA using EFI and ACPI only.
With this description it is unclear if you used QFW to install ACPI tables which would not test the code below.
Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
Please, cc maintainers as reported by scripts/get_maintainers in future.
lib/efi_loader/efi_acpi.c | 57 ++++++++++++++++++++++++++++++-- test/py/tests/test_event_dump.py | 1 + 2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c index 67bd7f8ca2..9262f21ea6 100644 --- a/lib/efi_loader/efi_acpi.c +++ b/lib/efi_loader/efi_acpi.c @@ -6,15 +6,23 @@ */
#include <efi_loader.h> -#include <log.h> -#include <mapmem.h> #include <acpi/acpi_table.h> #include <asm/global_data.h> +#include <asm/io.h> +#include <bloblist.h> +#include <linux/sizes.h> +#include <linux/log2.h> +#include <log.h> +#include <malloc.h> +#include <mapmem.h>
DECLARE_GLOBAL_DATA_PTR;
static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
+enum {
TABLE_SIZE = SZ_64K,+}; /*
- Install the ACPI table as a configuration table.
@@ -47,3 +55,48 @@ efi_status_t efi_acpi_register(void) return efi_install_configuration_table(&acpi_guid, (void *)(ulong)addr); }
Please, provide a function description.
+static int install_acpi_table(void) +{
u64 rom_addr, rom_table_end;There is no ROM involved here. Please, use meaningful variable names.
void *addr;if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) ||IS_ENABLED(CONFIG_X86) ||IS_ENABLED(CONFIG_QFW_ACPI))Why would you want to call write_acpi_tables() twice on the sandbox? See
board/sandbox/sandbox.c:180: end = write_acpi_tables(addr);
We should remove the redundant code from board/sandbox/sandbox.c with this patch.
return 0;/* Align the table to a 4KB boundary to keep EFI happy */if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))addr = bloblist_add(BLOBLISTT_ACPI_TABLES, TABLE_SIZE,ilog2(SZ_4K));elseaddr = memalign(SZ_4K, TABLE_SIZE);The UEFI specification wants ACPI tables to reside in EfiACPIReclaimMemory. Neither bloblist_add() nor memalign() should be used here.
I just want to address this and something below.
We do want to put tables in a bloblist. This is how x86 works (although I haven't gone back and cleaned up all the boards) and I think we should do it for all archs.
The memalign() here is fine too, since the actual setting of EFI memory type is done later, when the table is registered.
if (!addr)return log_msg_ret("mem", -ENOBUFS);rom_addr = virt_to_phys(addr);gd->arch.table_start_high = rom_addr;rom_table_end = write_acpi_tables(rom_addr);if (!rom_table_end) {log_err("Can't create ACPI configuration table\n");return -EINTR;}debug("- wrote 'acpi' to %llx, end %llx\n", rom_addr, rom_table_end);Please, use log_debug().
if (rom_table_end - rom_addr > TABLE_SIZE) {log_err("Out of space for configuration tables: need %llx, have %x\n",This check is too late. We must ensure that write_acpi_table() does not overwrite unallocated memory.
That is not supported at present, unfortunately. I agree we should fix it, but not in this series. For now we can set a size of 64KB which should be plenty. The same bug exists in x86.
rom_table_end - rom_addr, TABLE_SIZE);return log_msg_ret("acpi", -ENOSPC);}gd->arch.table_end_high = rom_table_end;debug("- done writing tables\n");ditto
Best regards
Heinrich
return 0;+}
+EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_acpi_table); diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py index e282c67335..88bbf34f71 100644 --- a/test/py/tests/test_event_dump.py +++ b/test/py/tests/test_event_dump.py @@ -18,6 +18,7 @@ def test_event_dump(u_boot_console):
EVT_FT_FIXUP bootmeth_vbe_ft_fixup .*boot/vbe_request.c:.* EVT_FT_FIXUP bootmeth_vbe_simple_ft_fixup .*boot/vbe_simple_os.c:.* +EVT_LAST_STAGE_INIT install_acpi_table .*lib/efi_loader/efi_acpi.c:.* EVT_LAST_STAGE_INIT install_smbios_table .*lib/efi_loader/efi_smbios.c:.* EVT_MISC_INIT_F sandbox_early_getopt_check .*arch/sandbox/cpu/start.c:.* EVT_TEST h_adder_simple .*test/common/event.c:'''
Regards, Simon