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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/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.
> 
> 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 <yann.dirson@xxxxxxxxxx>
> Co-developed-by: Yann Sionneau <yann.sionneau@xxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> 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.)

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

Jan