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

[Simon Glass]

Hi Heinrich,

On Thu, 25 Jun 2026 at 16:23, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 6/25/26 16:55, Simon Glass wrote:
> > 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.
>
> 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.

Well in v1.0 we would just have used sizeof() of the whole struct.
Creating v2 created the length 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.

Yes, sounds good. Either offsetof() or 20 is fine by me.

Regards,
Simon