[PATCH v2 14/28] efi_loader: Install ACPI tables

[Heinrich Schuchardt]

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 at 9elements.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Tom Rini <trini at konsulko.com>

Please, cc maintainers as reported by scripts/get_maintainers in future.

> ---
>   lib/efi_loader/efi_acpi.c        | 57 ++++++++++++++++++++++++++++++--
>   test/py/tests/test_event_dump.py |  1 +
>   2 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> index 67bd7f8ca2..9262f21ea6 100644
> --- a/lib/efi_loader/efi_acpi.c
> +++ b/lib/efi_loader/efi_acpi.c
> @@ -6,15 +6,23 @@
>    */
>
>   #include <efi_loader.h>
> -#include <log.h>
> -#include <mapmem.h>
>   #include <acpi/acpi_table.h>
>   #include <asm/global_data.h>
> +#include <asm/io.h>
> +#include <bloblist.h>
> +#include <linux/sizes.h>
> +#include <linux/log2.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <mapmem.h>
>
>   DECLARE_GLOBAL_DATA_PTR;
>
>   static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
>
> +enum {
> +	TABLE_SIZE	= SZ_64K,
> +};
>   /*
>    * Install the ACPI table as a configuration table.
>    *
> @@ -47,3 +55,48 @@ efi_status_t efi_acpi_register(void)
>   	return efi_install_configuration_table(&acpi_guid,
>   					       (void *)(ulong)addr);
>   }
> +

Please, provide a function description.

> +static int install_acpi_table(void)
> +{
> +	u64 rom_addr, rom_table_end;

There is no ROM involved here. Please, use meaningful variable names.

> +	void *addr;
> +
> +	if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE) ||
> +	    IS_ENABLED(CONFIG_X86) ||
> +	    IS_ENABLED(CONFIG_QFW_ACPI))

Why would you want to call write_acpi_tables() twice on the sandbox? See

board/sandbox/sandbox.c:180:
             end = write_acpi_tables(addr);

We should remove the redundant code from board/sandbox/sandbox.c with
this patch.

> +		return 0;
> +
> +	/* Align the table to a 4KB boundary to keep EFI happy */
> +	if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> +		addr = bloblist_add(BLOBLISTT_ACPI_TABLES, TABLE_SIZE,
> +				    ilog2(SZ_4K));
> +	else
> +		addr = memalign(SZ_4K, TABLE_SIZE);

The UEFI specification wants ACPI tables to reside in
EfiACPIReclaimMemory. Neither bloblist_add() nor memalign() should be
used here.

> +
> +	if (!addr)
> +		return log_msg_ret("mem", -ENOBUFS);
> +
> +	rom_addr = virt_to_phys(addr);
> +
> +	gd->arch.table_start_high = rom_addr;
> +
> +	rom_table_end = write_acpi_tables(rom_addr);
> +	if (!rom_table_end) {
> +		log_err("Can't create ACPI configuration table\n");
> +		return -EINTR;
> +	}
> +
> +	debug("- wrote 'acpi' to %llx, end %llx\n", rom_addr, rom_table_end);

Please, use log_debug().

> +	if (rom_table_end - rom_addr > TABLE_SIZE) {
> +		log_err("Out of space for configuration tables: need %llx, have %x\n",

This check is too late. We must ensure that write_acpi_table() does not
overwrite unallocated memory.

> +			rom_table_end - rom_addr, TABLE_SIZE);
> +		return log_msg_ret("acpi", -ENOSPC);
> +	}
> +	gd->arch.table_end_high = rom_table_end;
> +
> +	debug("- done writing tables\n");

ditto

Best regards

Heinrich

> +
> +	return 0;
> +}
> +
> +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, install_acpi_table);
> diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py
> index e282c67335..88bbf34f71 100644
> --- a/test/py/tests/test_event_dump.py
> +++ b/test/py/tests/test_event_dump.py
> @@ -18,6 +18,7 @@ def test_event_dump(u_boot_console):
>   --------------------  ------------------------------  ------------------------------
>   EVT_FT_FIXUP          bootmeth_vbe_ft_fixup           .*boot/vbe_request.c:.*
>   EVT_FT_FIXUP          bootmeth_vbe_simple_ft_fixup    .*boot/vbe_simple_os.c:.*
> +EVT_LAST_STAGE_INIT   install_acpi_table              .*lib/efi_loader/efi_acpi.c:.*
>   EVT_LAST_STAGE_INIT   install_smbios_table            .*lib/efi_loader/efi_smbios.c:.*
>   EVT_MISC_INIT_F       sandbox_early_getopt_check      .*arch/sandbox/cpu/start.c:.*
>   EVT_TEST              h_adder_simple                  .*test/common/event.c:'''