| Message ID | 1435880916-2153-3-git-send-email-al.stone@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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)
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.
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
On Sat, Jul 04, 2015 at 01:54:15AM +0200, Rafael J. Wysocki wrote: > On Friday, July 03, 2015 01:51:36 PM Al Stone wrote: > > 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. That's fine by me (as long as there is a plan to fix it properly, ideally in 4.3).
On 07/03/2015 05:54 PM, Rafael J. Wysocki wrote: > 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. ACK. > 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 Yup, that's what I was thinking.
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))) + /* Basic configuration for ACPI */ #ifdef CONFIG_ACPI /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
The BAD_MADT_ENTRY() macro is designed to work for all of the subtables of the MADT. In the ACPI 5.1 version of the spec, the struct for the GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in ACPI 6.0, the struct is 80 bytes long. But, there is only one definition in ACPICA for this struct -- and that is the 6.0 version. Hence, when BAD_MADT_ENTRY() compares the struct size to the length in the GICC subtable, it fails if 5.1 structs are in use, and there are systems in the wild that have them. This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable only, accounting for the difference in specification versions that are possible. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables. This code is being added to an arm64 header file since that is currently the only architecture using the GICC subtable of the MADT. As a GIC is specific to ARM, it is also unlikely the subtable will be used elsewhere. Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) Signed-off-by: Al Stone <al.stone@linaro.org> --- arch/arm64/include/asm/acpi.h | 11 +++++++++++ 1 file changed, 11 insertions(+)