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.
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.
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(-)
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 --- 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;
On 10 June 2015 at 23:28, al.stone@linaro.org wrote:
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
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)
Would it be better to make this a function to avoid using this macro before the acpi_gbl* stuff is initialized?
Regards, Ashwin.
On 06/11/2015 11:07 AM, Ashwin Chaugule wrote:
On 10 June 2015 at 23:28, al.stone@linaro.org wrote:
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
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)Would it be better to make this a function to avoid using this macro before the acpi_gbl* stuff is initialized?
Perhaps. Couldn't that happen even as a function, though? I think this would have to be an __init function which is why I'm not sure it would make a difference. Or am I misunderstanding something? It's happened at least once before :)...
Regards, Ashwin.
On 11 June 2015 at 13:47, Al Stone al.stone@linaro.org wrote:
On 06/11/2015 11:07 AM, Ashwin Chaugule wrote:
On 10 June 2015 at 23:28, al.stone@linaro.org wrote:
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
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)Would it be better to make this a function to avoid using this macro before the acpi_gbl* stuff is initialized?
Perhaps. Couldn't that happen even as a function, though? I think this would have to be an __init function which is why I'm not sure it would make a difference. Or am I misunderstanding something? It's happened at least once before :)...
Right. Sorry I should've been clear. It could be a multi line macro too I guess (could get messy though). I was suggesting checking if acpi_gbl* is initialized and returning -EINVAL or something, would probably be better than assuming it is always sane. I dont know if this macro would/can ever be used very early on before acpi_gbl* is populated so I'll let you guys take a call whether stricter checking is necessary or not. :)
On 06/11/2015 12:10 PM, Ashwin Chaugule wrote:
On 11 June 2015 at 13:47, Al Stone al.stone@linaro.org wrote:
On 06/11/2015 11:07 AM, Ashwin Chaugule wrote:
On 10 June 2015 at 23:28, al.stone@linaro.org wrote:
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
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)Would it be better to make this a function to avoid using this macro before the acpi_gbl* stuff is initialized?
Perhaps. Couldn't that happen even as a function, though? I think this would have to be an __init function which is why I'm not sure it would make a difference. Or am I misunderstanding something? It's happened at least once before :)...
Right. Sorry I should've been clear. It could be a multi line macro too I guess (could get messy though). I was suggesting checking if acpi_gbl* is initialized and returning -EINVAL or something, would probably be better than assuming it is always sane. I dont know if this macro would/can ever be used very early on before acpi_gbl* is populated so I'll let you guys take a call whether stricter checking is necessary or not. :)
No worries. I understand now. Yeah, I can never tell how paranoid to be; I'm willing to let upstream provide direction there, I guess.
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 --- 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);
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 --- 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;
/*
On 06/10/2015 09:28 PM, al.stone@linaro.org wrote:
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.
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.
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(-)
BTW, these patches are against linux-next. They do need some run-time testing to make sure I haven't broken anything but I'll have to do that tomorrow.
On Wed, Jun 10, 2015 at 09:28:09PM -0600, al.stone@linaro.org wrote:
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.
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.
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(-)
Hi Al, series looks good to me, much better than v1.
Feel free to add my review!
Graeme
On 06/11/2015 06:07 PM, Graeme Gregory wrote:
On Wed, Jun 10, 2015 at 09:28:09PM -0600, al.stone@linaro.org wrote:
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.
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.
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(-)
Hi Al, series looks good to me, much better than v1.
Feel free to add my review!
+1,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
On 06/11/2015 05:05 AM, Hanjun Guo wrote:
On 06/11/2015 06:07 PM, Graeme Gregory wrote:
On Wed, Jun 10, 2015 at 09:28:09PM -0600, al.stone@linaro.org wrote:
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.
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.
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(-)
Hi Al, series looks good to me, much better than v1.
Feel free to add my review!
+1,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, guys. I'll let it brew just a bit more while testing and send it up.
-
Al Stone -
al.stone＠linaro.org
-
Ashwin Chaugule -
Graeme Gregory -
Hanjun Guo