Hi Heinrich, On Thu, 25 Jun 2026 at 16:23, Heinrich Schuchardt <[email protected]> wrote: > > 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.
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
