[PATCH v3 0/2] cmd: acpi: fix acpi list command

The size of the ACPI table header is not a multiple of 8. We have to mark struct acpi_xsdt as packed to correctly access field Entry.
Add a unit test for the offsets of field Entry in the RSDT and XSDT tables.
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.
The return values of list_rsdt() and list_rsdp() are always zero and not checked. Remove the return values.
Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
As the RSDT table has to be ignored if the XSDT command is present there is no need to compare the tables in a display command. Anyway the specification does not require that the sequence of addresses in the RSDT and XSDT table are the same.
The FACS table header does not provide revision information. Correct the description of dump_hdr().
Adjust the ACPI test to match the changed output format of the 'acpi list' command.
v3: consider map_sysmem(0, 0) != NULL on the sandbox adjust signature of list_rsdt() and list_rsdp() v2: merge patch 2 and 3 remove leading zeros from address output
Heinrich Schuchardt (2): acpi: fix struct acpi_xsdt cmd: acpi: fix acpi list command
cmd/acpi.c | 67 +++++++++++++++++++++------------------ include/acpi/acpi_table.h | 2 +- test/dm/acpi.c | 28 ++++++++++------ 3 files changed, 56 insertions(+), 41 deletions(-)

The size of the ACPI table header is not a multiple of 8. We have to mark struct acpi_xsdt as packed to correctly access field Entry.
Add a unit test for the offsets of field Entry in the RSDT and XSDT tables.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Simon Glass sjg@chromium.org --- v3: no change v2: add unit test for offset of field Entry in RSDT, XSDT --- include/acpi/acpi_table.h | 2 +- test/dm/acpi.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h index 1f85de091d..59ab79ed17 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]; }; diff --git a/test/dm/acpi.c b/test/dm/acpi.c index 5997bda649..559cf1693b 100644 --- a/test/dm/acpi.c +++ b/test/dm/acpi.c @@ -651,3 +651,13 @@ static int dm_test_acpi_cmd_set(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_acpi_cmd_set, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +/* Test offsets in RSDT, XSDT */ +static int dm_test_acpi_offsets(struct unit_test_state *uts) +{ + ut_asserteq(36, offsetof(struct acpi_rsdt, entry)); + ut_asserteq(36, offsetof(struct acpi_xsdt, entry)); + + return 0; +} +DM_TEST(dm_test_acpi_offsets, 0);

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.
The return values of list_rsdt() and list_rsdp() are always zero and not checked. Remove the return values.
Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
As the RSDT table has to be ignored if the XSDT command is present there is no need to compare the tables in a display command. Anyway the specification does not require that the sequence of addresses in the RSDT and XSDT table are the same.
The FACS table header does not provide revision information. Correct the description of dump_hdr().
Adjust the ACPI test to match the changed output format of the 'acpi list' command.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v3: consider map_sysmem(0, 0) != NULL on the sandbox adjust signature of list_rsdt() and list_rsdp() v2: add unit test for offset of field Entry in RSDT, XSDT merge patch 2 and 3 remove leading zeros from address output --- cmd/acpi.c | 67 +++++++++++++++++++++++++++----------------------- test/dm/acpi.c | 18 +++++++------- 2 files changed, 45 insertions(+), 40 deletions(-)
diff --git a/cmd/acpi.c b/cmd/acpi.c index 7e397d1a74..fe14656ebb 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 %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, @@ -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 @ %16lx\n", ACPI_NAME_LEN, hdr->signature, (ulong)map_to_sysmem(hdr)); print_buffer(0, hdr, 1, hdr->length, 0);
@@ -58,47 +59,51 @@ static void list_fadt(struct acpi_fadt *fadt) dump_hdr(map_sysmem(fadt->firmware_ctrl, 0)); }
-static int list_rsdt(struct acpi_rsdt *rsdt, struct acpi_xsdt *xsdt) +static void list_rsdt(struct acpi_rsdp *rsdp) { int len, i, count; + struct acpi_rsdt *rsdt; + struct acpi_xsdt *xsdt;
- dump_hdr(&rsdt->header); - if (xsdt) + if (rsdp->rsdt_address) { + rsdt = map_sysmem(rsdp->rsdt_address, 0); + dump_hdr(&rsdt->header); + } + if (rsdp->xsdt_address) { + xsdt = map_sysmem(rsdp->xsdt_address, 0); 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 (rsdp->rsdt_address) { + len = rsdt->header.length - sizeof(rsdt->header); + count = len / sizeof(u32); + } else { + return; + } + for (i = 0; i < count; i++) { struct acpi_table_header *hdr;
- if (!rsdt->entry[i]) - break; - hdr = map_sysmem(rsdt->entry[i], 0); + if (rsdp->xsdt_address) { + 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; }
-static int list_rsdp(struct acpi_rsdp *rsdp) +static void 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 %16lx %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; + list_rsdt(rsdp); }
static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc, @@ -111,8 +116,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; diff --git a/test/dm/acpi.c b/test/dm/acpi.c index 559cf1693b..89b8ef513a 100644 --- a/test/dm/acpi.c +++ b/test/dm/acpi.c @@ -395,26 +395,26 @@ static int dm_test_acpi_cmd_list(struct unit_test_state *uts)
console_record_reset(); run_command("acpi list", 0); - ut_assert_nextline("Name Base Size Detail"); - ut_assert_nextline("---- -------- ----- ------"); - ut_assert_nextline("RSDP %08lx %5zx v02 U-BOOT", addr, + ut_assert_nextline("Name Base Size Detail"); + ut_assert_nextline("---- ---------------- ----- ----------------------------"); + ut_assert_nextline("RSDP %16lx %5zx v02 U-BOOT", addr, sizeof(struct acpi_rsdp)); addr = ALIGN(addr + sizeof(struct acpi_rsdp), 16); - ut_assert_nextline("RSDT %08lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", + ut_assert_nextline("RSDT %16lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", addr, sizeof(struct acpi_table_header) + 3 * sizeof(u32), OEM_REVISION); addr = ALIGN(addr + sizeof(struct acpi_rsdt), 16); - ut_assert_nextline("XSDT %08lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", + ut_assert_nextline("XSDT %16lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", addr, sizeof(struct acpi_table_header) + 3 * sizeof(u64), OEM_REVISION); addr = ALIGN(addr + sizeof(struct acpi_xsdt), 64); - ut_assert_nextline("DMAR %08lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", + ut_assert_nextline("DMAR %16lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", addr, sizeof(struct acpi_dmar), OEM_REVISION); addr = ALIGN(addr + sizeof(struct acpi_dmar), 16); - ut_assert_nextline("DMAR %08lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", + ut_assert_nextline("DMAR %16lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", addr, sizeof(struct acpi_dmar), OEM_REVISION); addr = ALIGN(addr + sizeof(struct acpi_dmar), 16); - ut_assert_nextline("DMAR %08lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", + ut_assert_nextline("DMAR %16lx %5zx v01 U-BOOT U-BOOTBL %x INTL 0", addr, sizeof(struct acpi_dmar), OEM_REVISION); ut_assert_console_end();
@@ -446,7 +446,7 @@ static int dm_test_acpi_cmd_dump(struct unit_test_state *uts) console_record_reset(); run_command("acpi dump dmar", 0); addr = ALIGN(map_to_sysmem(ctx.xsdt) + sizeof(struct acpi_xsdt), 64); - ut_assert_nextline("DMAR @ %08lx", addr); + ut_assert_nextline("DMAR @ %16lx", addr); ut_assert_nextlines_are_dump(0x30); ut_assert_console_end();

On Sat, 18 Nov 2023 at 15:54, 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.
The return values of list_rsdt() and list_rsdp() are always zero and not checked. Remove the return values.
Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
As the RSDT table has to be ignored if the XSDT command is present there is no need to compare the tables in a display command. Anyway the specification does not require that the sequence of addresses in the RSDT and XSDT table are the same.
The FACS table header does not provide revision information. Correct the description of dump_hdr().
Adjust the ACPI test to match the changed output format of the 'acpi list' command.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v3: consider map_sysmem(0, 0) != NULL on the sandbox adjust signature of list_rsdt() and list_rsdp() v2: add unit test for offset of field Entry in RSDT, XSDT merge patch 2 and 3 remove leading zeros from address output
cmd/acpi.c | 67 +++++++++++++++++++++++++++----------------------- test/dm/acpi.c | 18 +++++++------- 2 files changed, 45 insertions(+), 40 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

On Sat, Nov 18, 2023 at 11:52:48PM +0100, Heinrich Schuchardt 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.
The return values of list_rsdt() and list_rsdp() are always zero and not checked. Remove the return values.
Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
As the RSDT table has to be ignored if the XSDT command is present there is no need to compare the tables in a display command. Anyway the specification does not require that the sequence of addresses in the RSDT and XSDT table are the same.
The FACS table header does not provide revision information. Correct the description of dump_hdr().
Adjust the ACPI test to match the changed output format of the 'acpi list' command.
(Side question: Do you use --histogram when preparing patches? if no, try it.)
...
if (rsdp->xsdt_address) {
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);
}
With a help of temporary variable this can be rewritten as
tmp = 0; // or NULL, I haven't checked the type.
if (rsdp->xsdt_address) tmp = xsdt->entry[i]; else tmp = rsdt->entry[i];
if (!tmp) break;
hdr = map_sysmem(tmp, 0);

On 11/20/23 12:28, Andy Shevchenko wrote:
On Sat, Nov 18, 2023 at 11:52:48PM +0100, Heinrich Schuchardt 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.
The return values of list_rsdt() and list_rsdp() are always zero and not checked. Remove the return values.
Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
As the RSDT table has to be ignored if the XSDT command is present there is no need to compare the tables in a display command. Anyway the specification does not require that the sequence of addresses in the RSDT and XSDT table are the same.
The FACS table header does not provide revision information. Correct the description of dump_hdr().
Adjust the ACPI test to match the changed output format of the 'acpi list' command.
(Side question: Do you use --histogram when preparing patches? if no, try it.)
Thanks for reviewing.
No I don't use --histogram.
Without --histogram: 2 files changed, 45 insertions(+), 40 deletions(-)
With --histogram: 2 files changed, 54 insertions(+), 49 deletions(-)
The histogram algorithms finds less commonalities. Why should I prefer this?
...
if (rsdp->xsdt_address) {
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);
}
With a help of temporary variable this can be rewritten as
tmp = 0; // or NULL, I haven't checked the type.
The type would have to be u64. An assignment would not be needed here as none of the code paths below uses it.
Best regards
Heinrich
if (rsdp->xsdt_address) tmp = xsdt->entry[i]; else tmp = rsdt->entry[i]; if (!tmp) break; hdr = map_sysmem(tmp, 0);

On Mon, Nov 20, 2023 at 01:34:22PM +0100, Heinrich Schuchardt wrote:
On 11/20/23 12:28, Andy Shevchenko wrote:
On Sat, Nov 18, 2023 at 11:52:48PM +0100, Heinrich Schuchardt wrote:
...
(Side question: Do you use --histogram when preparing patches? if no, try it.)
Thanks for reviewing.
No I don't use --histogram.
Without --histogram: 2 files changed, 45 insertions(+), 40 deletions(-)
With --histogram: 2 files changed, 54 insertions(+), 49 deletions(-)
The histogram algorithms finds less commonalities. Why should I prefer this?
Readability of the patch.
participants (3)
-
Andy Shevchenko
-
Heinrich Schuchardt
-
Simon Glass