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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/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.)
> 
> 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
>