On Sat, 11 Nov 2023 at 06:42, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

The size of the ACPI table header is not a multiple of 8. We have to mark struct acpi_xsdt as packed to correctly read the entries.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/acpi/acpi_table.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass sjg@chromium.org

It is really unfortunate.

Would it be worth adding a test for this, which actually checks the member offset?

diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h index a3b67259e6..20ac3b51ba 100644 --- a/include/acpi/acpi_table.h +++ b/include/acpi/acpi_table.h @@ -80,7 +80,7 @@ struct acpi_rsdt { };

/* XSDT (Extended System Description Table) */ -struct acpi_xsdt { +struct __packed acpi_xsdt { struct acpi_table_header header; u64 entry[MAX_ACPI_TABLES]; }; -- 2.40.1

Regards, Simon

Hi Heinrich,

On Sat, 11 Nov 2023 at 06:42, Heinrich Schuchardt heinrich.schuchardt@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@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