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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/8/26 9:41 AM, Jan Beulich wrote:
> On 07.01.2026 17:34, 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.
> >
> > [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/2229673951
> >
> > I also built Xen manually with CONFIG_ACPI=y enabled and tested it on the
> > AmpereOne platform.
> > ---
> > Changes in v2:
> >   - Update the commit message:
> >     - Use more characters for commit ID.
> >     - Drop 'import from'.
> >   - Add Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>.
> Was this a legitimate thing to do, considering ...
>
> >   - Make the local variables host_gicc const in  gic_get_hwdom_madt_size().
> >     (header variable isn't const as container_of() will discard 'const' 
> > qualifier
> >     and so compilation error will occur).
> >   - Return 0 instead of panic() in gic_get_hwdom_madt_size().
> ... all of these (plus leaving partly unaddressed a comment from Julien)?
Probably you are right, it was to early. I will drop it and ask Stefano to 
re-review.

> > --- 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;
> Why is there a blank line left between declarations? In unusual situations (very
> many variables, for example) that may be okay, but otherwise the first blank 
> line
> generally wants to separate decls from statements.
It was visually better for me to have header and host_gicc variable in a 
"separate
block". I can drop a blank line.

> Also Julien asked for this to be const. You claimed a compile error would occur
> if you do, but afaict that's only because ...
>
> > +    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, struct acpi_madt_generic_interrupt,
> > +                             header);
> ... you don't use const properly here as well.
Oh, right, I missed to add const for the second argument of container_of().

> Finally (possibly not for this patch, but mentioning since originally it was
> pointed out as an option) the function imo wants to become __init anyway, for
> (as said by Julien) its only caller being so.
I didn't put __init here as you mentioned it was pointed out as an option. 
Anyway
I think I will put __init in the next patch version.

Thanks.

~ Oleksii