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

Simon Glass sjg at chromium.org
Fri Mar 28 13:01:18 CET 2025


Hi Heinrich,

On Fri, 21 Mar 2025 at 06:24, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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 at 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.

I found that one table was wrong so I wanted a positive ack that each
table is either OK or BAD. I made the check optional though.

If you like, we could always check checksums but only print the OK/BAD
if -c is provided?

BTW I see you send a cleanup series for ACPI checksums. Is that based
on top of this series?

Regards,
Simon


More information about the U-Boot mailing list