Re: [PATCH v5 45/46] acpi: Support checking checksums

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 21.03.25 11:49, Heinrich Schuchardt wrote:
> On 15.03.25 15:26, Simon Glass wrote:
>> When the ACPI tables come from an earlier bootloader it is helpful to
>> see whether the checksums are correct or not. Add a -c flag to the
>> 'acpi list' command to support that.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> (no changes since v3)
>>
>> Changes in v3:
>> - Add new patch to support checking checksums with ACPI command
>>
>>   cmd/acpi.c             | 57 +++++++++++++++++++++++++++---------------
>>   doc/usage/cmd/acpi.rst | 20 ++++++++++++++-
>>   test/dm/acpi.c         | 46 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 102 insertions(+), 21 deletions(-)
>>
>> diff --git a/cmd/acpi.c b/cmd/acpi.c
>> index 2273176f106..bb243202009 100644
>> --- a/cmd/acpi.c
>> +++ b/cmd/acpi.c
>> @@ -7,6 +7,7 @@
>>   #include <display_options.h>
>>   #include <log.h>
>>   #include <mapmem.h>
>> +#include <tables_csum.h>
>>   #include <acpi/acpi_table.h>
>>   #include <asm/acpi_table.h>
>>   #include <asm/global_data.h>
>> @@ -15,6 +16,17 @@
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> +static const char *show_checksum(void *ptr, uint size, bool chksums)
>> +{
>> +    uint checksum;
>> +
>> +    if (!chksums)
>> +        return "";
>> +    checksum = table_compute_checksum(ptr, size);
>> +
>> +    return checksum ? "  bad" : "  OK";
> 
> Please, move the leading spaces to the printf statements below.
> 
>> +}
>> +
>>   /**
>>    * dump_hdr() - Dump an ACPI header
>>    *
>> @@ -23,16 +35,17 @@ DECLARE_GLOBAL_DATA_PTR;
>>    *
>>    * @hdr: ACPI header to dump
>>    */
>> -static void dump_hdr(struct acpi_table_header *hdr)
>> +static void dump_hdr(struct acpi_table_header *hdr, bool chksums)
>>   {
>>       bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
>>
>>       printf("%.*s  %16lx  %5x", ACPI_NAME_LEN, hdr->signature,
>>              (ulong)map_to_sysmem(hdr), hdr->length);
>>       if (has_hdr) {
>> -        printf("  v%02d %.6s %.8s %x %.4s %x\n", hdr->revision,
>> +        printf("  v%02d %.6s %.8s %x %.4s %x%s\n", hdr->revision,
> 
> %x results in variable length output. To avoid ragged layout, please,
> define the output length.
> 
> Best regards
> 
> Heinrich
> 
>>                  hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
>> -               hdr->creator_id, hdr->creator_revision);
>> +               hdr->creator_id, hdr->creator_revision,
>> +               show_checksum(hdr, hdr->length, chksums));
>>       } else {
>>           printf("\n");
>>       }
>> @@ -52,22 +65,22 @@ static int dump_table_name(const char *sig)
>>       return 0;
>>   }
>>
>> -static void list_fadt(struct acpi_fadt *fadt)
>> +static void list_fadt(struct acpi_fadt *fadt, bool chksums)
>>   {
>>       if (fadt->header.revision >= 3 && fadt->x_dsdt)
>> -        dump_hdr(nomap_sysmem(fadt->x_dsdt, 0));
>> +        dump_hdr(nomap_sysmem(fadt->x_dsdt, 0), chksums);
>>       else if (fadt->dsdt)
>> -        dump_hdr(nomap_sysmem(fadt->dsdt, 0));
>> +        dump_hdr(nomap_sysmem(fadt->dsdt, 0), chksums);
>>       if (!IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_SANDBOX) &&
>>           !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
>>           log_err("FADT not ACPI-hardware-reduced-compliant\n");
>>       if (fadt->header.revision >= 3 && fadt->x_firmware_ctrl)
>> -        dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0));
>> +        dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0), chksums);
>>       else if (fadt->firmware_ctrl)
>> -        dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0));
>> +        dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0), chksums);
>>   }
>>
>> -static void list_rsdt(struct acpi_rsdp *rsdp)
>> +static void list_rsdt(struct acpi_rsdp *rsdp, bool chksums)
>>   {
>>       int len, i, count;
>>       struct acpi_rsdt *rsdt;
>> @@ -75,11 +88,11 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
>>
>>       if (rsdp->rsdt_address) {
>>           rsdt = nomap_sysmem(rsdp->rsdt_address, 0);
>> -        dump_hdr(&rsdt->header);
>> +        dump_hdr(&rsdt->header, chksums);
>>       }
>>       if (rsdp->xsdt_address) {
>>           xsdt = nomap_sysmem(rsdp->xsdt_address, 0);
>> -        dump_hdr(&xsdt->header);
>> +        dump_hdr(&xsdt->header, chksums);
>>           len = xsdt->header.length - sizeof(xsdt->header);
>>           count = len / sizeof(u64);
>>       } else if (rsdp->rsdt_address) {
>> @@ -100,24 +113,28 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
>>           if (!entry)
>>               break;
>>           hdr = nomap_sysmem(entry, 0);
>> -        dump_hdr(hdr);
>> +        dump_hdr(hdr, chksums);
>>           if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN))
>> -            list_fadt((struct acpi_fadt *)hdr);
>> +            list_fadt((struct acpi_fadt *)hdr, chksums);
>>       }
>>   }
>>
>> -static void list_rsdp(struct acpi_rsdp *rsdp)
>> +static void list_rsdp(struct acpi_rsdp *rsdp, bool chksums)
>>   {
>> -    printf("RSDP  %16lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
>> -           rsdp->length, rsdp->revision, rsdp->oem_id);
>> -    list_rsdt(rsdp);
>> +    printf("RSDP  %16lx  %5x  v%02d %.6s%s%s\n",
>> +           (ulong)map_to_sysmem(rsdp), rsdp->length, rsdp->revision,
>> +           rsdp->oem_id, show_checksum(rsdp, 0x14, chksums),
>> +           show_checksum(rsdp, rsdp->length, chksums));
>> +    list_rsdt(rsdp, chksums);
>>   }
>>
>>   static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
>>               char *const argv[])
>>   {
>>       struct acpi_rsdp *rsdp;
>> +    bool chksums;
>>
>> +    chksums = argc >= 2 && !strcmp("-c", argv[1]);
>>       rsdp = map_sysmem(gd_acpi_start(), 0);
>>       if (!rsdp) {
>>           printf("No ACPI tables present\n");
>> @@ -125,7 +142,7 @@ static int do_acpi_list(struct cmd_tbl *cmdtp, int 
>> flag, int argc,
>>       }
>>       printf("Name              Base   Size  Detail\n"
>>              "----  ----------------  -----  
>> ----------------------------\n");
>> -    list_rsdp(rsdp);
>> +    list_rsdp(rsdp, chksums);
>>
>>       return 0;
>>   }
>> @@ -187,13 +204,13 @@ static int do_acpi_dump(struct cmd_tbl *cmdtp, 
>> int flag, int argc,
>>   }
>>
>>   U_BOOT_LONGHELP(acpi,
>> -    "list  - list ACPI tables\n"
>> +    "list [-c] - list ACPI tables [check checksums]\n"

Let's keep things simple for the user:

Always check the checksums.
Only print a notice if a checksum is wrong.

Best regards

Heinrich

>>       "acpi items [-d]   - List/dump each piece of ACPI data from 
>> devices\n"
>>       "acpi set [<addr>] - Set or show address of ACPI tables\n"
>>       "acpi dump <name>  - Dump ACPI table");
>>
>>   U_BOOT_CMD_WITH_SUBCMDS(acpi, "ACPI tables", acpi_help_text,
>> -    U_BOOT_SUBCMD_MKENT(list, 1, 1, do_acpi_list),
>> +    U_BOOT_SUBCMD_MKENT(list, 2, 1, do_acpi_list),
>>       U_BOOT_SUBCMD_MKENT(items, 2, 1, do_acpi_items),
>>       U_BOOT_SUBCMD_MKENT(set, 2, 1, do_acpi_set),
>>       U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_acpi_dump));
>> diff --git a/doc/usage/cmd/acpi.rst b/doc/usage/cmd/acpi.rst
>> index 9f30972fe53..e652968d584 100644
>> --- a/doc/usage/cmd/acpi.rst
>> +++ b/doc/usage/cmd/acpi.rst
>> @@ -11,7 +11,7 @@ Synopsis
>>
>>   ::
>>
>> -    acpi list
>> +    acpi list [-c]
>>       acpi items [-d]
>>       acpi dump <name>
>>       acpi set <address>
>> @@ -38,6 +38,9 @@ List the ACPI tables that have been generated. Each 
>> table has a 4-character
>>   table name (e.g. SSDT, FACS) and has a format defined by the
>>   `ACPI specification`_.
>>
>> +The `-c` flag tells U-Boot to verify the checksums and print 'OK' or 
>> 'BAD' next
>> +to each table.
>> +
>>   U-Boot does not currently support decoding the tables. Unlike 
>> devicetree, ACPI
>>   tables have no regular schema and also some include bytecode, so 
>> decoding the
>>   tables requires a lot of code.
>> @@ -259,5 +262,20 @@ pointer::
>>       WAET  bff76a3b     28  v01 BOCHS  BXPC     1 BXPC 1
>>       SSDT  bff95040     c5  v02 COREv4 COREBOOT 2a CORE 20221020
>>
>> +This shows checking that the checksums are correct for each table::
>> +
>> +    => acpi list -c
>> +    Name              Base   Size  Detail
>> +    ----  ----------------  -----  ----------------------------
>> +    RSDP          bec9a000     24  v00 BOCHS   OK OK
>> +    RSDT          bec9bd4a     38  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    FACP          bec9bb46     74  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    DSDT          bec9a080   1ac6  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    FACS          bec9a040     40
>> +    APIC          bec9bbba     78  v03 BOCHS  BXPC     1 BXPC 1  OK
>> +    HPET          bec9bc32     38  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    SRAT          bec9bc6a     b8  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    WAET          bec9bd22     28  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +
>>
>>   .. _`ACPI specification`: https://uefi.org/sites/default/files/ 
>> resources/ACPI_6_3_final_Jan30.pdf
>> diff --git a/test/dm/acpi.c b/test/dm/acpi.c
>> index f98f9d1e74b..db012b6d2f1 100644
>> --- a/test/dm/acpi.c
>> +++ b/test/dm/acpi.c
>> @@ -439,6 +439,52 @@ static int dm_test_acpi_cmd_list(struct 
>> unit_test_state *uts)
>>   }
>>   DM_TEST(dm_test_acpi_cmd_list, UTF_SCAN_PDATA | UTF_SCAN_FDT | 
>> UTF_CONSOLE);
>>
>> +/* Test 'acpi list -c' command */
>> +static int dm_test_acpi_cmd_list_chksum(struct unit_test_state *uts)
>> +{
>> +    struct acpi_ctx ctx;
>> +    ulong addr;
>> +    void *buf;
>> +
>> +    buf = memalign(16, BUF_SIZE);
>> +    ut_assertnonnull(buf);
>> +    addr = map_to_sysmem(buf);
>> +    ut_assertok(setup_ctx_and_base_tables(uts, &ctx, addr));
>> +
>> +    ut_assertok(acpi_write_dev_tables(&ctx));
>> +
>> +    run_command("acpi list -c", 0);
>> +    ut_assert_nextline("Name              Base   Size  Detail");
>> +    ut_assert_nextline("----  ----------------  -----  
>> ----------------------------");
>> +    ut_assert_nextline("RSDP  %16lx  %5zx  v02 U-BOOT  OK  OK", addr,
>> +               sizeof(struct acpi_rsdp));
>> +    addr = ALIGN(addr + sizeof(struct acpi_rsdp), 16);
>> +    ut_assert_nextline("RSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_table_header) +
>> +               3 * sizeof(u32), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_rsdt), 16);
>> +    ut_assert_nextline("XSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_table_header) +
>> +               3 * sizeof(u64), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_xsdt), 64);
>> +    ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_dmar), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
>> +    ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_dmar), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
>> +    ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_dmar), OEM_REVISION);
>> +    ut_assert_console_end();
>> +    ut_assert_console_end();
>> +    unmap_sysmem(buf);
>> +    free(buf);
>> +
>> +    return 0;
>> +}
>> +DM_TEST(dm_test_acpi_cmd_list_chksum,
>> +    UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
>> +
>>   /* Test 'acpi dump' command */
>>   static int dm_test_acpi_cmd_dump(struct unit_test_state *uts)
>>   {
>