[PATCH 0/3] Correct for ACPI 5.1->6.0 spec changes in MADT GICC entries

[PATCH 0/3] Correct for ACPI 5.1->6.0 spec changes in MADT GICC entries

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

In the ACPI 5.1 version of the spec, the struct for the GICC subtable (struct
acpi_madt_generic_interrupt) of the MADT 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.

Note that this was found in linux-next and these patches apply against that
tree and the arm64 kernel tree; 4.1-rc7 does not appear to have this problem
since it has the 5.1 struct definition.

This patch set first adds macros for easily using the ACPI spec version, and
then adds the BAD_MADT_GICC_ENTRY() macro that uses them to check the GICC
subtable only, accounting for the difference in specification versions that
are possible.  The final patch adds in usage of the BAD_MADT_GICC_ENTRY
macro.  The BAD_MADT_ENTRY() will continue to work as is for all other
MADT subtables.

If these patches are acceptable, a cleanup effort will follow to simplify
the use of ACPI spec version numbers elsewhere.

These were tested and known to work on an APM Mustang system, where the
problem was originally uncovered.


Al Stone (3):
  ACPI: introduce macros for using the ACPI specification version
  ACPI: add BAD_MADT_GICC_ENTRY() macro
  ACPI / ARM64: use the new BAD_MADT_GICC_ENTRY macro

 arch/arm64/kernel/smp.c   |  2 +-
 drivers/irqchip/irq-gic.c |  2 +-
 include/linux/acpi.h      | 12 ++++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.1.0

[PATCH 1/3] ACPI: introduce macros for using the ACPI specification version

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

Add the ACPI_SPEC_VERSION() macro to build a proper version number from
a major and minor revision number.  Add also the ACPI_FADT_SPEC_VERSION
that constructs a proper version number from the entries in the current
FADT.

These macros are added in order to simplify retrieving and comparing ACPI
specification version numbers, since this is becoming a more frequent need.
In particular, there are some architectures that require at least a certain
version of the spec, and there are differences in some structure sizes that
have changed with recent versions but can only be tracked by spec version
number.

Signed-off-by: Al Stone <al.stone@linaro.org>
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>

CC: Rafael J. Wysocki <rjw@rjwysocki.net>
CC: Len Brown <lenb@kernel.org>

---
 include/linux/acpi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a4acb55..33ed313 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -48,6 +48,11 @@
 #include <acpi/acpi_io.h>
 #include <asm/acpi.h>
 
+#define ACPI_SPEC_VERSION(major, minor)	((major<<8)|minor)
+#define ACPI_FADT_SPEC_VERSION	\
+	ACPI_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
+			  acpi_gbl_FADT.minor_revision)
+
 static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
 {
 	return adev ? adev->handle : NULL;
-- 
2.1.0

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

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

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.

Signed-off-by: Al Stone <al.stone@linaro.org>
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>

CC: Rafael J. Wysocki <rjw@rjwysocki.net>
CC: Len Brown <lenb@kernel.org>

---
 include/linux/acpi.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 33ed313..8a83f91 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -127,6 +127,13 @@ static inline void acpi_initrd_override(void *data, size_t size)
 		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
 		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
 
+#define BAD_MADT_GICC_ENTRY(entry, end) (				    \
+		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
+		((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(5,1)) &&	    \
+		 (entry->header.length != 76))				||  \
+		((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6,0)) &&	    \
+		 (entry->header.length != 80)))
+
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);
-- 
2.1.0

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

[Lorenzo Pieralisi]

On Thu, Jun 11, 2015 at 08:45:10PM +0100, al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> 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.

Unfortunately that's nothing new, it seems. ia64 put in place a quite
nifty solution to that (I *guess* owing to ACPI 3.0 updates to Local
sapic specs), have a look at:

arch/ia64/kernel/acpi.c acpi_parse_lsacpi()

/*Skip BAD_MADT_ENTRY check, as lsapic size could vary */

We remove the check, job done ;-)

> Signed-off-by: Al Stone <al.stone@linaro.org>
> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
> Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>
> 
> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
> CC: Len Brown <lenb@kernel.org>
> 
> ---
>  include/linux/acpi.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 33ed313..8a83f91 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -127,6 +127,13 @@ static inline void acpi_initrd_override(void *data, size_t size)
>  		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>  		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>  
> +#define BAD_MADT_GICC_ENTRY(entry, end) (				    \
> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> +		((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(5,1)) &&	    \
> +		 (entry->header.length != 76))				||  \
> +		((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6,0)) &&	    \
> +		 (entry->header.length != 80)))

I would make those length magic numbers ACPICA defines at least.

It is not a GICC only issue, that's true for all MADT subtables that change
size with versions so, maybe we can replace the sizeof(*entry) in
BAD_MADT_ENTRY with a macro compound statement returning the subtable
length (where you can add a switch case on entry->type and return
sizeof(*entry) in the default case) ?

Overkill ? Certainly ugly, but at least you do not need to patch anything
else.

I am inclined to relegate these checks to ACPICA tools (statically)
altogether.

It is better to check Len and Rafael opinion on this first before coding it.

Lorenzo

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

[Al Stone]

On 06/12/2015 08:52 AM, Lorenzo Pieralisi wrote:
> On Thu, Jun 11, 2015 at 08:45:10PM +0100, al.stone at linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> 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.
> 
> Unfortunately that's nothing new, it seems. ia64 put in place a quite
> nifty solution to that (I *guess* owing to ACPI 3.0 updates to Local
> sapic specs), have a look at:
> 
> arch/ia64/kernel/acpi.c acpi_parse_lsacpi()
> 
> /*Skip BAD_MADT_ENTRY check, as lsapic size could vary */
> 
> We remove the check, job done ;-)

Heh.  Yes, that's one way to do it :).

>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>
>>
>> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
>> CC: Len Brown <lenb@kernel.org>
>>
>> ---
>>  include/linux/acpi.h | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 33ed313..8a83f91 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -127,6 +127,13 @@ static inline void acpi_initrd_override(void *data, size_t size)
>>  		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>>  		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>>  
>> +#define BAD_MADT_GICC_ENTRY(entry, end) (				    \
>> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>> +		((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(5,1)) &&	    \
>> +		 (entry->header.length != 76))				||  \
>> +		((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6,0)) &&	    \
>> +		 (entry->header.length != 80)))
> 
> I would make those length magic numbers ACPICA defines at least.

Yup, you're right.  I'll fix that.

> It is not a GICC only issue, that's true for all MADT subtables that change
> size with versions so, maybe we can replace the sizeof(*entry) in
> BAD_MADT_ENTRY with a macro compound statement returning the subtable
> length (where you can add a switch case on entry->type and return
> sizeof(*entry) in the default case) ?
> 
> Overkill ? Certainly ugly, but at least you do not need to patch anything
> else.

There is precedent for this; there's a function to print MADT subtable
entries with exactly this structure.  And yeah, it's a bit ugly, but it
does the job.

> I am inclined to relegate these checks to ACPICA tools (statically)
> altogether.

Interestingly enough, the acpi_table_print_madt_entry() function I was
referring to is in the drivers/acpi code, not ACPICA.  I wonder what the
history is on that and why it ended up there...

> It is better to check Len and Rafael opinion on this first before coding it.
> 
> Lorenzo
> 

Yup.  Agreed.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------

[PATCH 3/3] ACPI / ARM64: use the new BAD_MADT_GICC_ENTRY macro

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

For those parts of the arm64 ACPI code that need to check GICC subtables
in the MADT, use the new BAD_MADT_GICC_ENTRY macro instead of the previous
BAD_MADT_ENTRY.  The new macro takes into account differences in the size
of the GICC subtable that the old macro did not; this caused failures even
though the subtable entries are valid.

Signed-off-by: Al Stone <al.stone@linaro.org>
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>

CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Jason Cooper <jason@lakedaemon.net>

---
 arch/arm64/kernel/smp.c   | 2 +-
 drivers/irqchip/irq-gic.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 4b2121b..80d5984 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -438,7 +438,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
 	struct acpi_madt_generic_interrupt *processor;
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
-	if (BAD_MADT_ENTRY(processor, end))
+	if (BAD_MADT_GICC_ENTRY(processor, end))
 		return -EINVAL;
 
 	acpi_table_print_madt_entry(header);
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8d7e1c8..4dd8826 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1055,7 +1055,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
+	if (BAD_MADT_GICC_ENTRY(processor, end))
 		return -EINVAL;
 
 	/*
-- 
2.1.0