Hi Heinrich,
On Fri, 21 Mar 2025 at 06:24, Heinrich Schuchardt <xypron.g...@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 <s...@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
