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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
I have tested these patches on an APM Mustang with version 1.15 firmware, where the problem was found, and they fix the problem.
Changes for v2: -- Replace magic constants with proper defines (Lorenzo) -- Minor syntax clean-up noted by checkpatch -- Send out CCs properly this time -- Minor clean-up of the paragraphs in this cover letter
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 | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-)
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 06/19/2015 06:36 AM, Al Stone wrote:
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)
and minor comments for code style
((major<<8)|minor) - > ((major << 8) | minor)
other than that:
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
+#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 06/19/2015 04:49 AM, Hanjun Guo wrote:
On 06/19/2015 06:36 AM, Al Stone wrote:
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)
and minor comments for code style
((major<<8)|minor) - > ((major << 8) | minor)
Ah. Yeah, good idea. I can roll that into next version.
other than that:
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks.
Thanks Hanjun
+#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;
-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
Hi Al,
On Fri, Jun 19, 2015 at 12:36 AM, Al Stone al.stone@linaro.org wrote:
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)
One nit here.
acpi_gbl_FADT.header.revision is of type u8 originally, so shifting it by 8 bit positions only works due to some implicit type casting I suppose.
Moreover, it is not entirely clear why the macro is specific to the computation of the ACPI spec version.
So I'd drop ACPI_SPEC_VERSION and only define ACPI_FADT_SPEC_VERSION as something like
#define ACPI_FADT_SPEC_VERSION (((unsigned int)acpi_gbl_FADT.header.revision << 8) | (unsigned int)acpi_gbl_FADT.minor_revision)
Thanks, Rafael
On 06/30/2015 02:12 PM, Rafael J. Wysocki wrote:
Hi Al,
On Fri, Jun 19, 2015 at 12:36 AM, Al Stone al.stone@linaro.org wrote:
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)
One nit here.
acpi_gbl_FADT.header.revision is of type u8 originally, so shifting it by 8 bit positions only works due to some implicit type casting I suppose.
Bah. That was being sloppy on my part. Sorry about that. Will fix.
Moreover, it is not entirely clear why the macro is specific to the computation of the ACPI spec version.
As far as I know, that's the only way to extract the spec version from tables; I don't recall there being any other table with that info. Since I will likely use this again, it seemed to make sense at the time.
So I'd drop ACPI_SPEC_VERSION and only define ACPI_FADT_SPEC_VERSION as something like
#define ACPI_FADT_SPEC_VERSION (((unsigned int)acpi_gbl_FADT.header.revision << 8) | (unsigned int)acpi_gbl_FADT.minor_revision)
Thanks, Rafael
Sure. That makes sense. It makes it clearer that this is the version just from the FADT. I'll do that.
On 2015/7/1 5:15, Al Stone wrote:
On 06/30/2015 02:12 PM, Rafael J. Wysocki wrote:
Hi Al,
On Fri, Jun 19, 2015 at 12:36 AM, Al Stone al.stone@linaro.org wrote:
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)
One nit here.
acpi_gbl_FADT.header.revision is of type u8 originally, so shifting it by 8 bit positions only works due to some implicit type casting I suppose.
Bah. That was being sloppy on my part. Sorry about that. Will fix.
Moreover, it is not entirely clear why the macro is specific to the computation of the ACPI spec version.
As far as I know, that's the only way to extract the spec version from tables; I don't recall there being any other table with that info. Since I will likely use this again, it seemed to make sense at the time.
That's true from ACPI 5.1, as we discussed in ACPI spec working group, FADT Major Version and FADT Minor Version are recognized as ACPI Major/Minor version. yes, the spec itself didn't state that explicitly, maybe a ECR to make it explicit will be good, I can prepare one if that makes sense.
Thanks Hanjun
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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 33ed313..d3a1758 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -127,6 +127,16 @@ 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 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_SPEC_VERSION(5, 1)) && \ + (entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \ + ((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6, 0)) && \ + (entry->header.length != ACPI_MADT_GICC_60_LENGTH))) + 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);
On 06/19/2015 06:36 AM, Al Stone wrote:
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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 33ed313..d3a1758 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -127,6 +127,16 @@ 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 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_SPEC_VERSION(5, 1)) && \(entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6, 0)) && \(entry->header.length != ACPI_MADT_GICC_60_LENGTH)))- 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);
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
On 06/19/2015 04:49 AM, Hanjun Guo wrote:
On 06/19/2015 06:36 AM, Al Stone wrote:
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 | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 33ed313..d3a1758 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -127,6 +127,16 @@ 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 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_SPEC_VERSION(5, 1)) && \(entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6, 0)) && \(entry->header.length != ACPI_MADT_GICC_60_LENGTH)))- 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);
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
Thanks for the review.
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 Thu, Jun 18, 2015 at 11:36:08PM +0100, Al Stone wrote:
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(-)
Acked-by: Will Deacon will.deacon@arm.com
Good to see this stuff is holding up so well...
Will
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.4.0
On 06/19/2015 03:46 AM, Will Deacon wrote:
On Thu, Jun 18, 2015 at 11:36:08PM +0100, Al Stone wrote:
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(-)
Acked-by: Will Deacon will.deacon@arm.com
Good to see this stuff is holding up so well...
Will
Thanks, Will.
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.4.0
-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/19/2015 06:36 AM, Al Stone wrote:
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;
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
On 06/19/2015 06:36 AM, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
I have tested these patches on an APM Mustang with version 1.15 firmware, where the problem was found, and they fix the problem.
I also tested on QEMU, it fixed the problem when I was using ACPICA 6.0 updates for MADT table,
Tested-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Changes for v2: -- Replace magic constants with proper defines (Lorenzo) -- Minor syntax clean-up noted by checkpatch -- Send out CCs properly this time -- Minor clean-up of the paragraphs in this cover letter
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 | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-)
On 06/19/2015 04:54 AM, Hanjun Guo wrote:
On 06/19/2015 06:36 AM, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
I have tested these patches on an APM Mustang with version 1.15 firmware, where the problem was found, and they fix the problem.
I also tested on QEMU, it fixed the problem when I was using ACPICA 6.0 updates for MADT table,
Tested-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Cool. The patches did what they should. Looks like they work on AMD Seattle with older firmware, also. Thanks.
Changes for v2: -- Replace magic constants with proper defines (Lorenzo) -- Minor syntax clean-up noted by checkpatch -- Send out CCs properly this time -- Minor clean-up of the paragraphs in this cover letter
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 | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-)
-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping? Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
Rafael
On Tue, Jun 30, 2015 at 8:25 PM, Rafael J. Wysocki rafael@kernel.org wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
Like what about defining it in linux/irqchip/arm-gic-acpi.h for example?
Rafael
On 2015/7/1 2:35, Rafael J. Wysocki wrote:
On Tue, Jun 30, 2015 at 8:25 PM, Rafael J. Wysocki rafael@kernel.org wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
Like what about defining it in linux/irqchip/arm-gic-acpi.h for example?
This BAD_MADT_GICC_ENTRY is both used by SMP init and GIC irqchip init for ARM64, would it be good to put BAD_MADT_GICC_ENTRY in arch/arm64/include/asm/acpi.h?
Thanks Hanjun
On 06/30/2015 08:06 PM, Hanjun Guo wrote:
On 2015/7/1 2:35, Rafael J. Wysocki wrote:
On Tue, Jun 30, 2015 at 8:25 PM, Rafael J. Wysocki rafael@kernel.org wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
Like what about defining it in linux/irqchip/arm-gic-acpi.h for example?
This BAD_MADT_GICC_ENTRY is both used by SMP init and GIC irqchip init for ARM64, would it be good to put BAD_MADT_GICC_ENTRY in arch/arm64/include/asm/acpi.h?
Thanks Hanjun
Ah, right. Good point. Let me try it in that file, then. It is -- for the time being -- arm64 specific.
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
I only placed it there since it seemed to make sense, and the issue is generic to ACPI, not just ARM. Granted ARM is the only arch using the GICC subtable in MADT, but this is fixing how ACPICA implemented the spec, which in turn was ambiguous (and an errata is forthcoming to fix that).
That being said, though, I'm definitely open to other possibilities.
Hi Al,
On Tue, Jun 30, 2015 at 8:39 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
I only placed it there since it seemed to make sense, and the issue is generic to ACPI, not just ARM. Granted ARM is the only arch using the GICC subtable in MADT,
Precisely.
but this is fixing how ACPICA implemented the spec,
So that should be fixed in ACPICA eventually and linux/acpi.h is not an ACPICA file even.
It is possible to apply an ACPICA fix to Linux before it goes to upstream ACPICA if it fixes a real problem in Linux. We've done things like that.
which in turn was ambiguous (and an errata is forthcoming to fix that).
That being said, though, I'm definitely open to other possibilities.
So I'd prefer an ACPICA fix and if that's not viable, an ARM-specific fix to fill the gap while ACPICA is being updated.
Thanks, Rafael
On 06/30/2015 01:05 PM, Rafael J. Wysocki wrote:
Hi Al,
On Tue, Jun 30, 2015 at 8:39 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Do we need these to go through your tree or the arm64 tree? Without this series (or an ia64-like solution), we have ACPI systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go into include/linux/acpi.h. Why is it necessary in there?
I only placed it there since it seemed to make sense, and the issue is generic to ACPI, not just ARM. Granted ARM is the only arch using the GICC subtable in MADT,
Precisely.
but this is fixing how ACPICA implemented the spec,
So that should be fixed in ACPICA eventually and linux/acpi.h is not an ACPICA file even.
It is possible to apply an ACPICA fix to Linux before it goes to upstream ACPICA if it fixes a real problem in Linux. We've done things like that.
Fair enough. I've been reluctant to add further divergence, personally.
which in turn was ambiguous (and an errata is forthcoming to fix that).
That being said, though, I'm definitely open to other possibilities.
So I'd prefer an ACPICA fix and if that's not viable, an ARM-specific fix to fill the gap while ACPICA is being updated.
Thanks, Rafael
Hrm. I'll look into the ACPICA fix. I'm sure it's possible, but it may be messy. I will talk to Bob Moore and Lv Zheng about that, too. This sort of thing has surely happened before, though.
In the meantime, I'll put together a new version of this patch that is ARM-specific to fill the gap. Using linux/irqchip/arm-gic-acpi.h does make sense.
Thanks for all the feedback, Rafael.
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Ah, right. Sorry about missing the tag. On the other hand, we're not really fixing anything so much as working around a problem in the ACPI specification. IA64 has seen the same problem, but the choice there was to just remove the use of BAD_MADT_ENTRY(); my preference was to keep the safety check the macro represents, but do it properly for the MADT subtable involved.
So, the commit that I see as the trigger is actually correct:
commit aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
That commit implements a change to the GICC subtable that is new for ACPI 6.0, and this is the correct change. However, this commit changes the length of the struct for the subtable. The problem is that both the old ACPI 5.1 length field value *and* the new ACPI 6.0 length field are now valid, but ACPICA 20150515 only has the ACPI 6.0 definition.
The right long term change is for the spec to disambiguate the different definitions of the GICC subtable so that ACPICA knows what to implement -- and that spec change is in progress and should be noted in the next errata. ACPICA will then pick up the errata change, I presume.
In the meantime, however, BAD_MADT_ENTRY() compares the length field of the GICC subtable, which is now allowed to have multiple different values, with the length of the struct holding that data, which is only the proper length for ACPI 6.0. The macro makes no distinction between spec versions or even MADT versions, and hence fails when it compares an ACPI 5.1 length field with an ACPI 6.0 sized struct.
So I guess that's why the Fixes: tag did not immediately pop to mind. ACPICA is not really broken, and the commit that triggers the problem is actually correct. But, because of the BAD_MADT_ENTRY() macro, Linux assumes that all MADT subtables with a length field will have that length value be the same as the current ACPICA data structure size, which is no longer true for the GICC subtable.
Is there a Deal-with-Spec-Weirdness tag I can use??
Hi Al,
On Tue, Jun 30, 2015 at 9:45 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone ahs3@redhat.com wrote:
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
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-rc8 does not appear to have this problem since it still has the 5.1 struct definition.
Even though there is precendent in ia64 code for ignoring the changes in size, this patch set instead tries to verify correctness. The first patch in the set adds macros for easily using the ACPI spec version. The second patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to check the GICC subtable only, accounting for the difference in specification versions that are possible. The final patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the only architecture affected. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
We need to get this series or a patch to remove the check(similar to ia64) based on what Rafael prefers. Without that, platforms using ACPI on ARM64 fails to boot with latest mainline. This blocks any testing on ARM64/ACPI systems.
Regards, Sudeep
I have not received any other feedback than some Reviewed-bys from Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression, sorry about that.
Actually, if your patch fixes an error introduced by a specific commit, it is good to use the Fixes: tag to indicate that. Which I still would like to do, so which commit is fixed by this?
Ah, right. Sorry about missing the tag. On the other hand, we're not really fixing anything so much as working around a problem in the ACPI specification. IA64 has seen the same problem, but the choice there was to just remove the use of BAD_MADT_ENTRY(); my preference was to keep the safety check the macro represents, but do it properly for the MADT subtable involved.
So, the commit that I see as the trigger is actually correct:
commit aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
That commit implements a change to the GICC subtable that is new for ACPI 6.0, and this is the correct change. However, this commit changes the length of the struct for the subtable. The problem is that both the old ACPI 5.1 length field value *and* the new ACPI 6.0 length field are now valid, but ACPICA 20150515 only has the ACPI 6.0 definition.
The right long term change is for the spec to disambiguate the different definitions of the GICC subtable so that ACPICA knows what to implement -- and that spec change is in progress and should be noted in the next errata. ACPICA will then pick up the errata change, I presume.
In the meantime, however, BAD_MADT_ENTRY() compares the length field of the GICC subtable, which is now allowed to have multiple different values, with the length of the struct holding that data, which is only the proper length for ACPI 6.0. The macro makes no distinction between spec versions or even MADT versions, and hence fails when it compares an ACPI 5.1 length field with an ACPI 6.0 sized struct.
So I guess that's why the Fixes: tag did not immediately pop to mind. ACPICA is not really broken, and the commit that triggers the problem is actually correct.
Well, Linux is technically broken if it doesn't boot on systems where it worked previously and the "Fixes:" tag refers to Linux commits (which might have come from ACPICA, but they are Linux commits nevertheless).
So if a given commit breaks Linux boot on any systems, putting it in the "Fixes:" tag is definitely justified even though the commit might be regarded as valid in a different context.
That said I still don't think that include/linux/acpi.h is the right place for the new macro.
You're working around the problem specifically for ARM64, so the workaround should be contained withing the ARM64 code (and I'm not talking about patch [1/3] which adds macros that make sense in general).
Thanks, Rafael
-
Al Stone -
Al Stone -
Hanjun Guo -
Hanjun Guo -
Rafael J. Wysocki -
Sudeep Holla -
Will Deacon