Re: [PATCH v6 14/37] efi_loader: Allocate and write ACPI tables

Simon Glass
2024-10-02T22:57:01Z

Hi Patrick,

On Wed, 2 Oct 2024 at 03:52, 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
>
> Changelog v6:
> - Update header order
>
> ---
> lib/efi_loader/efi_acpi.c | 76 ++++++++++++++++++++++++++++++++
> test/py/tests/test_event_dump.py | 1 +
> 2 files changed, 77 insertions(+)
>
> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> index 67bd7f8ca2..c11abe17b6 100644
> --- a/lib/efi_loader/efi_acpi.c
> +++ b/lib/efi_loader/efi_acpi.c
> @@ -6,15 +6,23 @@
> */
>
> #include <efi_loader.h>
> +#include <bloblist.h>
> #include <log.h>
> +#include <malloc.h>
> #include <mapmem.h>
> #include <acpi/acpi_table.h>
> #include <asm/global_data.h>
> +#include <asm/io.h>
> +#include <linux/sizes.h>
> +#include <linux/log2.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;

ulong addr since an address in U-Boot is a ulong, not a pointer

> +
> + 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,

void *ptr =

(this is because sandbox doesn't support simple casts between
addresses and pointers)

> + ilog2(SZ_4K));
> +
> + if (!addr)
> + return log_msg_ret("mem", -ENOMEM);

addr = map_to_sysmem(ptr);

> + } 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 really don't want this allocation. At least for now, can you get rid
of the else{} part here and always use bloblist_add()?

> + 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.2
>

Regards,
Simon