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

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

[Heinrich Schuchardt]

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.

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.

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                | 48 +++++++++++++++++++++++----------------
 include/acpi/acpi_table.h |  2 +-
 test/dm/acpi.c            | 28 +++++++++++++++--------
 3 files changed, 49 insertions(+), 29 deletions(-)

-- 
2.40.1

[PATCH v2 1/2] acpi: fix struct acpi_xsdt

[Heinrich Schuchardt]

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>
---
v2:
	add unit test
---
 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 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];
 };
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);
-- 
2.40.1

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

[Heinrich Schuchardt]

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.

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>
---
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     | 48 +++++++++++++++++++++++++++++-------------------
 test/dm/acpi.c | 18 +++++++++---------
 2 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/cmd/acpi.c b/cmd/acpi.c
index 7e397d1a74..bf3fd08857 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);
 
@@ -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  %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;
@@ -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;
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();
 
-- 
2.40.1

Re: [PATCH v2 2/2] cmd: acpi: fix acpi list command

[Simon Glass]

On Sun, 12 Nov 2023 at 00:03, 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.
>
> 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>
> ---
> 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     | 48 +++++++++++++++++++++++++++++-------------------
>  test/dm/acpi.c | 18 +++++++++---------
>  2 files changed, 38 insertions(+), 28 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 1/2] acpi: fix struct acpi_xsdt

[Simon Glass]

On Sun, 12 Nov 2023 at 00:03, 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 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>
> ---
> v2:
>         add unit test
> ---
>  include/acpi/acpi_table.h |  2 +-
>  test/dm/acpi.c            | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 2/2] cmd: acpi: fix acpi list command

[Heinrich Schuchardt]

On 11/12/23 21:01, Simon Glass wrote:
> On Sun, 12 Nov 2023 at 00:03, 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.
>>
>> 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>
>> ---
>> 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     | 48 +++++++++++++++++++++++++++++-------------------
>>   test/dm/acpi.c | 18 +++++++++---------
>>   2 files changed, 38 insertions(+), 28 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

The patch does not consider that

     map_sysmem(0, 0) != NULL

on the sandbox.

list_rsdp() needs to be changed to pass rsdt or xsdt as NULL if 
rsdt_address or xsdt_address is zero when invoking list_rsdt().

So this needs a respin.

Best regards

Heinrich