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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 18/12/2025 14:22, 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:
Shouldn't this be s/from import//?

Also 7 characters commit ID is too short for Linux and known to clash. 
You want to use 12 characters (which is also the default for Xen). I 
usually have the following in my global .gitconfig:
        [core]
                abbrev = 12

>    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>
> ---
> 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;
For both variables, you don't seem to modify the content. So I think 
they should be const.
> +
> +    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +    if ( !header )
> +        panic("Can't get GICC entry");
I would feel happier to use panic() in this function if 
gic_get_hwdom_madt_size() is __init (its only caller is during boot)).
An alternative is to stash the GICC size in a global variable.

> +
> +    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;
Cheers,

--
Julien Grall