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

[Heinrich Schuchardt]

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.

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