[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20170525172711.GB3935@red-moon>
Date: Thu, 25 May 2017 18:27:11 +0100
From: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To: Marc Zyngier <marc.zyngier@....com>
Cc: Julien Grall <julien.grall@....com>,
linux-arm-kernel@...ts.infradead.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
jason@...edaemon.net, tglx@...utronix.de, ahs3@...hat.com
Subject: Re: irqchip/irq-gic: BAD_MADT_GICC_ENTRY may fail when booting with
ACPI 5.1
On Wed, May 24, 2017 at 02:00:02PM +0100, Marc Zyngier wrote:
> On 24/05/17 12:18, Julien Grall wrote:
> > Hi Lorenzo,
> >
> > On 05/23/2017 06:06 PM, Lorenzo Pieralisi wrote:
> >> [+Al]
> >>
> >> On Tue, May 23, 2017 at 05:40:28PM +0100, Julien Grall wrote:
> >>> Hi all,
> >>>
> >>> I am currently looking at adding support of ACPI 5.1 in Xen.
> >>> When trying to boot DOM00 I get a panic in Linux (for the full
> >>> log see [1]):
> >>>
> >>> (XEN) DOM0: [ 0.000000] No valid GICC entries exist
> >>>
> >>> The error message is coming from gic_v2_acpi_init.
> >>> Digging down in the code, it is failing because of
> >>> BAD_MADT_GICC_ENTRY is returning false in
> >>> gic_acpi_parse_madt_cpu:
> >>>
> >>> /* Macros for consistency checks of the GICC subtable of MADT */
> >>> #define ACPI_MADT_GICC_LENGTH \
> >>> (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> >>>
> >>> #define BAD_MADT_GICC_ENTRY(entry, end) \
> >>> (!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) || \
> >>> (entry)->header.length != ACPI_MADT_GICC_LENGTH)
> >>>
> >>> The 'end' parameter corresponds to the end of the MADT table.
> >>> In the case of ACPI 5.1, the size of GICC is smaller compare
> >>> to 6.0+ (76 vs 80 bytes) but the parameter 'entry' is type
> >>> of acpi_madt_generic_interrupt (sizeof(...) = 80).
> >>
> >> #define BAD_MADT_GICC_ENTRY(entry, end) \
> >> (!(entry) || (entry)->header.length != ACPI_MADT_GICC_LENGTH || \
> >> ((unsigned long)(entry) + ACPI_MADT_GICC_LENGTH) > (end))
> >>
> >> Would this solve it ?
> >
> > Yes, I am now able to boot DOM0 up to the prompt. My concern with this
> > solution is the code will still use the acpi_madt_generic_interrupt
> > code. If someone tries to access field not existing in 5.1 (such as
> > efficiency_class), it may return wrong value or even worst crash.
> >
> > Although, I don't see any user of efficiency_class in Linux so far.
>
> Such code would have to check whether the ACPI version before doing so.
> To be honest, it is quite surprising we don't have one structure per
> version of the GICC subtable. This would at least make the user aware of
> the potential gotcha...
We will have to, as soon as a) someone starts using the GICC
efficiency_class field or b) we start using the three bytes left as
reserved (which I suspect it may happen sooner than we think), whatever
comes first.
In the meantime to fix the regression Julien reported, is the kludge
above ok for everyone ?
Thanks,
Lorenzo
Powered by blists - more mailing lists