[PATCH 2/3] cmd: acpi: fix acpi list command

[Simon Glass]

Hi Heinrich,

On Sat, 11 Nov 2023 at 06:42, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> ACPI tables may comprise either RSDT, XSDT, or both. The current code fails
> to check the presence of the RSDT table before accessing it. This leads to
> an exception if the RSDT table is not provided.
>
> The XSDT table takes precedence over the RSDT table.
>
> Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
>
> The ACPI specification does not require that the sequence of tables are the
> same in XSDT and RSDT. Comparing entries with the same index makes no
> sense.

Does it make any sense to have different sequences? Have you seen that
in the wild?

>
> The FACS table header does not provide revision information. Correct the
> description of dump_hdr().
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
>  cmd/acpi.c | 48 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/cmd/acpi.c b/cmd/acpi.c
> index 7e397d1a74..d02108d821 100644
> --- a/cmd/acpi.c
> +++ b/cmd/acpi.c
> @@ -17,7 +17,8 @@ DECLARE_GLOBAL_DATA_PTR;
>  /**
>   * dump_hdr() - Dump an ACPI header
>   *
> - * If the header is for FACS then it shows the revision information as well
> + * Except for the Firmware ACPI Control Structure (FACS)
> + * additionally show the revision information.
>   *
>   * @hdr: ACPI header to dump
>   */
> @@ -25,7 +26,7 @@ static void dump_hdr(struct acpi_table_header *hdr)
>  {
>         bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
>
> -       printf("%.*s  %08lx  %5x", ACPI_NAME_LEN, hdr->signature,
> +       printf("%.*s  %016lx  %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,
> @@ -43,7 +44,7 @@ static int dump_table_name(const char *sig)
>         hdr = acpi_find_table(sig);
>         if (!hdr)
>                 return -ENOENT;
> -       printf("%.*s @ %08lx\n", ACPI_NAME_LEN, hdr->signature,
> +       printf("%.*s @ %016lx\n", ACPI_NAME_LEN, hdr->signature,

We really don't want 16 hex digits of output. It just isn't readable.

Should we add a way of writing a hex number with an underscore between
digits 8 and 9?

12345678_3846263d

Better I think (at least for now) would be to use %10lx and live with
the column misalignment in the unlikely event that very large
addresses are used.

>                (ulong)map_to_sysmem(hdr));
>         print_buffer(0, hdr, 1, hdr->length, 0);
>
> @@ -62,26 +63,34 @@ static int list_rsdt(struct acpi_rsdt *rsdt, struct acpi_xsdt *xsdt)
>  {
>         int len, i, count;
>
> -       dump_hdr(&rsdt->header);
> -       if (xsdt)
> +       if (rsdt)
> +               dump_hdr(&rsdt->header);
> +       if (xsdt) {
>                 dump_hdr(&xsdt->header);
> -       len = rsdt->header.length - sizeof(rsdt->header);
> -       count = len / sizeof(u32);
> +               len = xsdt->header.length - sizeof(xsdt->header);
> +               count = len / sizeof(u64);
> +       } else if (rsdt) {
> +               len = rsdt->header.length - sizeof(rsdt->header);
> +               count = len / sizeof(u32);
> +       } else {
> +               return 0;
> +       }
> +
>         for (i = 0; i < count; i++) {
>                 struct acpi_table_header *hdr;
>
> -               if (!rsdt->entry[i])
> -                       break;
> -               hdr = map_sysmem(rsdt->entry[i], 0);
> +               if (xsdt) {
> +                       if (!xsdt->entry[i])
> +                               break;
> +                       hdr = map_sysmem(xsdt->entry[i], 0);
> +               } else {
> +                       if (!rsdt->entry[i])
> +                               break;
> +                       hdr = map_sysmem(rsdt->entry[i], 0);
> +               }
>                 dump_hdr(hdr);
>                 if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN))
>                         list_fadt((struct acpi_fadt *)hdr);
> -               if (xsdt) {
> -                       if (xsdt->entry[i] != rsdt->entry[i]) {
> -                               printf("   (xsdt mismatch %llx)\n",
> -                                      xsdt->entry[i]);
> -                       }
> -               }
>         }
>
>         return 0;
> @@ -92,10 +101,11 @@ static int list_rsdp(struct acpi_rsdp *rsdp)
>         struct acpi_rsdt *rsdt;
>         struct acpi_xsdt *xsdt;
>
> -       printf("RSDP  %08lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
> +       printf("RSDP  %016lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
>                rsdp->length, rsdp->revision, rsdp->oem_id);
>         rsdt = map_sysmem(rsdp->rsdt_address, 0);
>         xsdt = map_sysmem(rsdp->xsdt_address, 0);
> +
>         list_rsdt(rsdt, xsdt);
>
>         return 0;
> @@ -111,8 +121,8 @@ static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
>                 printf("No ACPI tables present\n");
>                 return 0;
>         }
> -       printf("Name      Base   Size  Detail\n");
> -       printf("----  --------  -----  ------\n");
> +       printf("Name  Base               Size  Detail\n"
> +              "----  ----------------  -----  ----------------------------\n");
>         list_rsdp(rsdp);
>
>         return 0;
> --
> 2.40.1
>

Regards,
Simon