On 6/25/26 16:55, Simon Glass wrote:
Hi Heinrich,
On Thu, 25 Jun 2026 at 12:26, Heinrich Schuchardt
<[email protected]> wrote:
On 6/25/26 10:52, Simon Glass wrote:
Hi Heinrich,
On 2026-06-21T08:19:04, Heinrich Schuchardt
<[email protected]> wrote:
efi_selftest: add ACPI configuration table test
The new unit test checks if ACPI tables are available.
If yes, it checks if the RSDP correctly points to the RSDT or XSDT.
Signed-off-by: Heinrich Schuchardt <[email protected]>
lib/efi_selftest/Makefile | 1 +
lib/efi_selftest/efi_selftest_acpi.c | 138
+++++++++++++++++++++++++++++++++++
2 files changed, 139 insertions(+)
Reviewed-by: Simon Glass <[email protected]>
diff --git a/lib/efi_selftest/efi_selftest_acpi.c
b/lib/efi_selftest/efi_selftest_acpi.c
@@ -0,0 +1,138 @@
+ if (!checksum_ok(rsdp, 20)) {
+ efi_st_error("Invalid RSDP checksum\n");
+ return EFI_ST_FAILURE;
+ }
Please avoid the magic 20 - how about offsetof(struct acpi_rsdp,
length) or a named constant?
Regards,
Simon
Thank you for reviewing.
20 is not an offset. It is the length of the ACPI 1.0 RSDP header.
...which is the offset of the length field. See the struct:
struct __packed acpi_rsdp {
char signature[8]; /* RSDP signature */
u8 checksum; /* Checksum of the first 20 bytes */
char oem_id[6]; /* OEM ID */
u8 revision; /* 0 for ACPI 1.0, others 2 */
u32 rsdt_address; /* Physical address of RSDT (32 bits) */
u32 length; /* Total RSDP length (incl. extended part) */
u64 xsdt_address; /* Physical address of XSDT (64 bits) */
u8 ext_checksum; /* Checksum of the whole table */
u8 reserved[3];
};
The original struct ended with rsdt_address as the final field. Then
they added a length and some more information. So the offset of
'length' is actually what you want.
My failure to use offsetof() was probably just that I didn't know it
existed, or perhaps because some of the code came from coreboot.
The field length does not exist in ACPI 1.0. So it does not make any
sense to relate to it for calculating the checksum field.
Below is the precedent that Bin and you have been setting:
arch/x86/lib/acpi.c:18: if (table_compute_checksum((void *)rsdp, 20) != 0)
drivers/misc/qfw_acpi.c:310: rsdp->checksum =
table_compute_checksum(rsdp, 20);
lib/acpi/base.c:36: rsdp->checksum = table_compute_checksum(rsdp, 20);
test/dm/acpi.c:371: ut_assertok(table_compute_checksum(rsdp, 20));
Yes, this is quite common. How about declaring a constant for it?
That is beyond the scope of this series. It has enough patches.
Let's do it separately.
Best regards
Heinrich
