Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Rafael J. Wysocki]

On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
> On 07/03/2015 08:06 AM, Catalin Marinas wrote:
> > On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index 39248d3..a3c26a4 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -19,6 +19,17 @@
> >>  #include <asm/psci.h>
> >>  #include <asm/smp_plat.h>
> >>  
> >> +/* Macros for consistency checks of the GICC subtable of MADT */
> >> +#define ACPI_MADT_GICC_51_LENGTH       76
> >> +#define ACPI_MADT_GICC_60_LENGTH       80
> >> +
> >> +#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
> >> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> >> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
> >> +		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
> >> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
> >> +		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
> > 
> > This looks ugly but, well, we could live with this.
> 
> Nod.  It's right at the hairy edge of becoming a function, I think.
> 
> > However, I'd like to avoid having to extend this macro every time we get
> > a new spec released, like 6.1 defining another 80 or 84 etc. So, how
> > about we only update this when there is an actual change in the length?
> > Something like:
> > 
> > #define ACPI_MADT_GICC_LENGTH	({					\
> > 	u8 length;							\
> > 	if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0)	\
> > 		length = 76;						\
> > 	else								\
> > 		length = 80;						\
> > 	length;								\
> > })
> > 
> > or just:
> > 
> > #define ACPI_MADT_GICC_LENGTH	\
> > 	(ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
> > 
> > (the latter is simpler but may not look nice if we change it again in
> > 6.1; though we could re-write this macro when needed, not a problem)
> > 
> 
> Perhaps the sanity checking for the MADT subtables needs to be revisited
> and a more general solution provided -- this is not the only MADT subtable
> with this problem and it may occur again.
> 
> Even the versions above are not technically compliant with the spec.  If
> we implement what the spec currently says, it might look something like
> this:
> 
> #define ACPI_MADT_GICC_LENGTH ({					\
> 	u8 length;							\
> 	switch (ACPI_FADT_SPEC_VERSION) {				\
> 	case ACPI_FADT_SPEC_VERSION_5_0:				\
> 		length = 40;						\
> 		break;							\
> 	case ACPI_FADT_SPEC_VERSION_5_1:				\
> 		length = 76;						\
> 		break;							\
> 	default:	/* use 6.0 size */				\
> 		length = 80;						\
> 	}								\
> 	length;								\
> })
> 
> So it's just messy and there will be a need for change.  Let me think about
> making this a function instead of a macro; it may make sense to really fix
> BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable,
> but it could also be overkill.

So here's my suggestion.

First, make ARM64 boot with 4.2+ in the simplest way possible.

Second, set out to fix BAD_MADT_ENTRY() etc.  Start with fixing ACPICA to
distinguish between the different formats depending on the spec version and
follow up from there.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/