On 09.01.2026 17:23, Oleksii Kurochko wrote: > > On 1/9/26 11:03 AM, Jan Beulich wrote: >> On 09.01.2026 10:27, Oleksii Kurochko wrote: >>> Newer ACPI revisions define the MADT GICC entry with Length = 82 bytes [1]. >>> The current BAD_MADT_GICC_ENTRY() check rejects entries whose length does >>> not >>> match the known values, which leads to: >>> GICv3: No valid GICC entries exist. >>> as observed on the AmpereOne platform. >>> >>> To fix this, import the logic from Linux commit 9eb1c92b47c7: >>> The BAD_MADT_GICC_ENTRY check is a little too strict because >>> it rejects MADT entries that don't match the currently known >>> lengths. We should remove this restriction to avoid problems >>> if the table length changes. Future code which might depend on >>> additional fields should be written to validate those fields >>> before using them, rather than trying to globally check >>> known MADT version lengths. >>> >>> >>> Link:https://lkml.kernel.org/r/[email protected] >>> Signed-off-by: Jeremy Linton<[email protected]> >>> [[email protected]: added MADT macro comments] >>> Signed-off-by: Lorenzo Pieralisi<[email protected]> >>> Acked-by: Sudeep Holla<[email protected]> >>> Cc: Will Deacon<[email protected]> >>> Cc: Catalin Marinas<[email protected]> >>> Cc: Al Stone<[email protected]> >>> Cc: "Rafael J. Wysocki"<[email protected]> >>> Signed-off-by: Will Deacon<[email protected]> >>> >>> As ACPI_MADT_GICC_LENGTH is dropped, update the functions where it is >>> used. As we rewrite the MADT for hwdom, reuse the host GICC header length >>> instead of ACPI_MADT_GICC_LENGTH. >>> >>> Mark gic_get_hwdom_madt_size() as __init since its only caller is also >>> __init. >>> >>> [1]https://uefi.org/specs/ACPI/6.6/05_ACPI_Software_Programming_Model.html#gic-cpu-interface-gicc-structure >>> >>> Reported-By: Yann Dirson<[email protected]> >>> Co-developed-by: Yann Sionneau<[email protected]> >>> Signed-off-by: Oleksii Kurochko<[email protected]> >>> --- >>> I ran CI tests where it made sense for this patch, as I don’t see any CI job >>> that builds Xen with CONFIG_ACPI=y: >>> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2252409762 >>> >>> I also built Xen manually with CONFIG_ACPI=y enabled and tested it on the >>> AmpereOne platform. >>> --- >>> xen/arch/arm/acpi/domain_build.c | 6 ++++++ >>> xen/arch/arm/gic-v2.c | 3 ++- >>> xen/arch/arm/gic-v3.c | 3 ++- >>> xen/arch/arm/gic.c | 13 +++++++++++-- >>> xen/arch/arm/include/asm/acpi.h | 21 +++++++++++++++------ >>> 5 files changed, 36 insertions(+), 10 deletions(-) >>> >>> diff --git a/xen/arch/arm/acpi/domain_build.c >>> b/xen/arch/arm/acpi/domain_build.c >>> index 1c3555d814cc..959698d13ac3 100644 >>> --- a/xen/arch/arm/acpi/domain_build.c >>> +++ b/xen/arch/arm/acpi/domain_build.c >>> @@ -458,6 +458,12 @@ static int __init estimate_acpi_efi_size(struct domain >>> *d, >>> acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8); >>> >>> madt_size = gic_get_hwdom_madt_size(d); >>> + if ( !madt_size ) >>> + { >>> + printk("Unable to get hwdom MADT size\n"); >>> + return -EINVAL; >>> + } >>> + >>> acpi_size += ROUNDUP(madt_size, 8); >>> >>> addr = acpi_os_get_root_pointer(); >>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >>> index b23e72a3d05d..aae6a7bf3076 100644 >>> --- a/xen/arch/arm/gic-v2.c >>> +++ b/xen/arch/arm/gic-v2.c >>> @@ -1121,7 +1121,8 @@ static int gicv2_make_hwdom_madt(const struct domain >>> *d, u32 offset) >>> host_gicc = container_of(header, struct acpi_madt_generic_interrupt, >>> header); >>> >>> - size = ACPI_MADT_GICC_LENGTH; >>> + size = host_gicc->header.length; >>> + >>> /* Add Generic Interrupt */ >>> for ( i = 0; i < d->max_vcpus; i++ ) >>> { >>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >>> index bc07f97c16ab..75b89efad462 100644 >>> --- a/xen/arch/arm/gic-v3.c >>> +++ b/xen/arch/arm/gic-v3.c >>> @@ -1672,7 +1672,8 @@ static int gicv3_make_hwdom_madt(const struct domain >>> *d, u32 offset) >>> >>> host_gicc = container_of(header, struct acpi_madt_generic_interrupt, >>> header); >>> - size = ACPI_MADT_GICC_LENGTH; >>> + size = host_gicc->header.length; >>> + >>> for ( i = 0; i < d->max_vcpus; i++ ) >>> { >>> gicc = (struct acpi_madt_generic_interrupt *)(base_ptr + >>> table_len); >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>> index ee75258fc3c3..e4fcfd60205d 100644 >>> --- a/xen/arch/arm/gic.c >>> +++ b/xen/arch/arm/gic.c >>> @@ -414,12 +414,21 @@ int gic_make_hwdom_madt(const struct domain *d, u32 >>> offset) >>> return gic_hw_ops->make_hwdom_madt(d, offset); >>> } >>> >>> -unsigned long gic_get_hwdom_madt_size(const struct domain *d) >>> +unsigned long __init gic_get_hwdom_madt_size(const struct domain *d) >>> { >>> unsigned long madt_size; >>> + const struct acpi_subtable_header *header; >>> + const struct acpi_madt_generic_interrupt *host_gicc; >>> + >>> + header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>> 0); >>> + if ( !header ) >>> + return 0; >>> + >>> + host_gicc = container_of(header, const struct >>> acpi_madt_generic_interrupt, >>> + header); >>> >>> madt_size = sizeof(struct acpi_table_madt) >>> - + ACPI_MADT_GICC_LENGTH * d->max_vcpus >>> + + host_gicc->header.length * d->max_vcpus >> Just to double-check: All entries are strictly required to be of the same >> length? (Related question further down.) > > If I understood the ACPI spec correctly, then yes, it should be the same > length, > as|GICC->length| is defined as a well defined constant value (82 in ACPI 6.6): > > https://uefi.org/specs/ACPI/6.6/05_ACPI_Software_Programming_Model.html#gic-cpu-interface-gicc-structure
It's not entirely explicit there, but my reading would concur. Then ... >>> --- a/xen/arch/arm/include/asm/acpi.h >>> +++ b/xen/arch/arm/include/asm/acpi.h >>> @@ -53,13 +53,22 @@ void acpi_smp_init_cpus(void); >>> */ >>> paddr_t acpi_get_table_offset(struct membank tbl_add[], EFI_MEM_RES >>> index); >>> >>> -/* Macros for consistency checks of the GICC subtable of MADT */ >>> -#define ACPI_MADT_GICC_LENGTH \ >>> - (acpi_gbl_FADT.header.revision < 6 ? 76 : 80) >> Given this, ... >> >>> +/* >>> + * MADT GICC minimum length refers to the MADT GICC structure table length >>> as >>> + * defined in the earliest ACPI version supported on arm64, ie ACPI 5.1. >>> + * >>> + * The efficiency_class member was added to the >>> + * struct acpi_madt_generic_interrupt to represent the MADT GICC structure >>> + * "Processor Power Efficiency Class" field, added in ACPI 6.0 whose offset >>> + * is therefore used to delimit the MADT GICC structure minimum length >>> + * appropriately. >>> + */ >>> +#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \ >>> + struct acpi_madt_generic_interrupt, efficiency_class) >>> >>> -#define BAD_MADT_GICC_ENTRY(entry, end) >>> \ >>> - (!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) || >>> \ >>> - (entry)->header.length != ACPI_MADT_GICC_LENGTH) >>> +#define BAD_MADT_GICC_ENTRY(entry, end) \ >>> + (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \ >>> + (unsigned long)(entry) + (entry)->header.length > (end)) >> ... is 76 a valid length when the FADT revision is 6 or higher? And 80 is a >> valid length for 6.5 or higher? > > I'm not ACPI expert but my understanding that it isn't "very valid" values as > I mentioned > above GICC->length is defined as a constant value. But the idea here is to > provide > forward compatibility so only minumum MADT GICC length is checked and as > mentioned > here [1] by one of ACPI for Arm64 maintainer: > > - (acpi_gbl_FADT.header.revision < 6 ? 76 : 80) > +#define > ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \ > + struct > acpi_madt_generic_interrupt, efficiency_class) > > > This makes it 76 always which is fine, just that the first user of > > efficiency_class should check for the length before accessing it. > > No user of efficiency_class yet, so I am fine with this change. > > And I think the same is true for ACPI 6.3 which adds spe_interrupt and ACPI > 6.5 > trbe_interrupt. ... imo precise checks for the version dependent length should be done. Jan > [1]https://lore.kernel.org/all/20181015092919.GA1778@e107155-lin/ > > ~ Oleksii >
