Re: [PATCH v1] acpi/arm: relax MADT GICC entry length check to support newer ACPI revisions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 18 Dec 2025, 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 import from Linux commit 9eb1c92:
>   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/20181012192937.3819951-1-jeremy.linton@xxxxxxx
>   Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
>   [lorenzo.pieralisi@xxxxxxx: added MADT macro comments]
>   Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>   Acked-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>   Cc: Will Deacon <will.deacon@xxxxxxx>
>   Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>   Cc: Al Stone <ahs3@xxxxxxxxxx>
>   Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
>   Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> 
> 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.
> 
> [1] 
> https://uefi.org/specs/ACPI/6.6/05_ACPI_Software_Programming_Model.html#gic-cpu-interface-gicc-structure
> 
> Reported-By: Yann Dirson <yann.dirson@xxxxxxxxxx>
> Co-developed-by: Yann Sionneau <yann.sionneau@xxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> 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/2222160666
> 
> I also built Xen manually with CONFIG_ACPI=y enabled and tested it on the
> AmpereOne platform.
> ---
>  xen/arch/arm/gic-v2.c           |  3 ++-
>  xen/arch/arm/gic-v3.c           |  3 ++-
>  xen/arch/arm/gic.c              | 12 +++++++++++-
>  xen/arch/arm/include/asm/acpi.h | 21 +++++++++++++++------
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index b23e72a3d0..aae6a7bf30 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 bc07f97c16..75b89efad4 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 ee75258fc3..a0ccda14bf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -418,8 +418,18 @@ unsigned long gic_get_hwdom_madt_size(const struct 
> domain *d)
>  {
>      unsigned long madt_size;
>  
> +    struct acpi_subtable_header *header;
> +    struct acpi_madt_generic_interrupt *host_gicc;
> +
> +    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +    if ( !header )
> +        panic("Can't get GICC entry");
> +
> +    host_gicc = container_of(header, 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
>                  + sizeof(struct acpi_madt_generic_distributor)
>                  + gic_hw_ops->get_hwdom_extra_madt_size(d);
>  
> diff --git a/xen/arch/arm/include/asm/acpi.h b/xen/arch/arm/include/asm/acpi.h
> index 13756dd341..30bc446d1f 100644
> --- 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)
> +/*
> + * 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))
>  
>  #ifdef CONFIG_ACPI
>  extern bool acpi_disabled;
> -- 
> 2.52.0
>