[PATCH v2 11/11] efi_loader: add ACPI table dump EFI app

[Simon Glass]

Hi Heinrich,

On 2026-06-21T08:19:04, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
> efi_loader: add ACPI table dump EFI app
>
> Provide an EFI binary that can dump all ACPI tables to files
> <table_name>.dat. These can then be further analyzed by decompiling
> with iasl.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>
> lib/efi_loader/Makefile   |   3 +-
>  lib/efi_loader/acpidump.c | 563 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 565 insertions(+), 1 deletion(-)

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,563 @@
> +static efi_status_t add_table(struct acpi_dump_table *tables, u32 *table_count,
> +                           uintptr_t addr, u32 len, const char *sig)
> +{
> +     struct acpi_dump_table *table = &tables[*table_count];
> +
> +     memset(table, 0, sizeof(*table));
> +     memcpy(table->signature, sig, 4);
> +     table->addr = addr;
> +     table->len = len;
> +     ++*table_count;
> +
> +     return EFI_SUCCESS;
> +}

add_table() can only ever return EFI_SUCCESS, so every 'if (ret !=
EFI_SUCCESS) goto err_out' that follows is dead. Either make it void,
or maybe have it bounds-check *table_count against max_tables and
return EFI_BUFFER_TOO_SMALL on overflow; otherwise a malformed
RSDT/XSDT that lies about its length could walk past the allocation.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,563 @@
> +             ret = bs->allocate_pool(EFI_LOADER_DATA, tables[i].len,
> +                                    (void **)&buf);
> +             if (ret != EFI_SUCCESS) {
> +                     error(u"Out of memory\r\n");
> +                     break;
> +             }
> +
> +             memcpy(buf, (void *)tables[i].addr, tables[i].len);
> +             ret = save_file(filename, buf, tables[i].len);

The intermediate buffer looks superfluous - save_file() only reads
from buf and passes it to file->write(). You can drop the
allocate_pool/memcpy/free_pool and pass (void *)tables[i].addr
directly. That also removes the only failure path between collecting
and writing.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,563 @@
> +static void u32_to_utf16_dec(u16 *dst, efi_uintn_t dst_size, u32 val)
> +{
> +     append_uint_dec(dst, 0, dst_size, val);
> +}

Single-call wrapper around append_uint_dec() with pos=0; the only
caller is do_check(). Please just call append_uint_dec() directly and
drop the helper.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,563 @@
> +static void build_table_filename(const char *sig, u32 instance, u16 *name,
> +                              efi_uintn_t name_size)
> +{
> +     efi_uintn_t pos = 0;
> +     int i;
> +
> +     if (!name_size)
> +             return;
> +
> +     for (i = 0; i < 4 && pos + 1 < name_size; ++i)
> +             name[pos++] = is_filename_char(sig[i]) ? (u16)sig[i] : u'_';
> +
> +     if (instance)
> +             pos = append_uint_dec(name, pos, name_size, instance);
> +
> +     if (pos + 4 < name_size) {
> +             name[pos++] = u'.';
> +             name[pos++] = u'd';
> +             name[pos++] = u'a';
> +             name[pos++] = u't';
> +     }
> +     name[pos] = 0;
> +}

If name_size is too small for the .dat suffix, the function silently
produces a filename with no extension. In the current tree that can't
happen...but better to return an error/bool, or guarantee correctness
by sizing name_size appropriately and dropping the conditional.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,563 @@
> +    efi_loader: add ACPI table dump EFI app
> +
> +    Provide an EFI binary that can dump all ACPI tables to files
> +    <table_name>.dat. These can then be further analyzed by decompiling
> +    with iasl.

The commit message could usefully mention the two interactive commands
(check and write), the duplicate-signature numbering scheme, and that
DSDT/FACS are pulled in via FADT so the output matches 'acpidump -b'.
Right now the design choices are visible only in the code.

Regards,
Simon