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
