[PATCH v2 02/11] efi_selftest: add ACPI configuration table test

[Simon Glass]

Hi Heinrich,

On Thu, 25 Jun 2026 at 12:26, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 6/25/26 10:52, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On 2026-06-21T08:19:04, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> 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 <heinrich.schuchardt at canonical.com>
> >>
> >> lib/efi_selftest/Makefile            |   1 +
> >>   lib/efi_selftest/efi_selftest_acpi.c | 138 +++++++++++++++++++++++++++++++++++
> >>   2 files changed, 139 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> >> 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.

>
> 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?

Regards,
Simon