Re: irqchip/irq-gic: BAD_MADT_GICC_ENTRY may fail when booting with ACPI 5.1

[Marc Zyngier]

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

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...