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 v3: -- Modified the macros for using spec version numbers in order to make them clearer (Rafael, Hanjun) -- Moved the definition of the BAD_MADT_GICC_ENTRY macro to an arm64-specific header file since only this architecture uses the GICC subtable (Rafael) -- Added Reviewed-by (Hanjun) and Acked-by (Will) tags to 3/3, the only unchanged patch; other tags could be applied but the patches have changed. -- Added Fixes: tag to patches
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 / ARM64: add BAD_MADT_GICC_ENTRY() macro ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro
arch/arm64/include/asm/acpi.h | 11 +++++++++++ arch/arm64/kernel/smp.c | 2 +- drivers/irqchip/irq-gic.c | 2 +- include/linux/acpi.h | 10 ++++++++++ 4 files changed, 23 insertions(+), 2 deletions(-)
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h>
+#define __ACPI_FADT_SPEC_VERSION(major, minor) \ + ((unsigned int)major << 8 | (unsigned int)minor) + +#define ACPI_FADT_SPEC_VERSION \ + __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \ + acpi_gbl_FADT.minor_revision) + +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0) + static inline acpi_handle acpi_device_handle(struct acpi_device *adev) { return adev ? adev->handle : NULL;
On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
- ((unsigned int)major << 8 | (unsigned int)minor)
+#define ACPI_FADT_SPEC_VERSION \
- __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
acpi_gbl_FADT.minor_revision)+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
And what if there is 5.2 or even 5.3?
static inline acpi_handle acpi_device_handle(struct acpi_device *adev) { return adev ? adev->handle : NULL;
Hi Rafael,
On 2015/7/3 8:21, Rafael J. Wysocki wrote:
On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
- ((unsigned int)major << 8 | (unsigned int)minor)
+#define ACPI_FADT_SPEC_VERSION \
- __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
acpi_gbl_FADT.minor_revision)+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
Agreed.
And what if there is 5.2 or even 5.3?
Hmm, do you mean in the future or just now? for both two cases, only 5.1 and 5.1 errata (still has the same ACPI version with 5.1) will be available, then jump to 6.0 and going forward if new versions in the future.
I'm not sure if I understand your question correctly, if not, please correct me :)
Thanks Hanjun
On 07/02/2015 11:23 PM, Hanjun Guo wrote:
Hi Rafael,
On 2015/7/3 8:21, Rafael J. Wysocki wrote:
On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
- ((unsigned int)major << 8 | (unsigned int)minor)
+#define ACPI_FADT_SPEC_VERSION \
- __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
acpi_gbl_FADT.minor_revision)+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
Agreed.
Will do. This was a flip of the coin, on my part.
And what if there is 5.2 or even 5.3?
Hmm, do you mean in the future or just now? for both two cases, only 5.1 and 5.1 errata (still has the same ACPI version with 5.1) will be available, then jump to 6.0 and going forward if new versions in the future.
I'm not sure if I understand your question correctly, if not, please correct me :)
Thanks Hanjun
I'm not sure I understand the question, either. Traditionally, the spec versioning has been exclusively linear -- i.e., now that 6.0 has replaced 5.1, there will be no more 5.x. There may be errata published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but the errata are not encoded in tables anywhere since they are meant only as corrections to the base version. This is unlikely to change, but not impossible, of course :).
The only reason for putting in macros for 5.1 and 6.0 is that those are the versions that I'm concerned with for this particular fix and I know I will use them. If others are needed, I'd have those that need them add them.
On Friday, July 03, 2015 01:22:13 PM Al Stone wrote:
On 07/02/2015 11:23 PM, Hanjun Guo wrote:
Hi Rafael,
On 2015/7/3 8:21, Rafael J. Wysocki wrote:
On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
- ((unsigned int)major << 8 | (unsigned int)minor)
+#define ACPI_FADT_SPEC_VERSION \
- __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
acpi_gbl_FADT.minor_revision)+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
Agreed.
Will do. This was a flip of the coin, on my part.
And what if there is 5.2 or even 5.3?
Hmm, do you mean in the future or just now? for both two cases, only 5.1 and 5.1 errata (still has the same ACPI version with 5.1) will be available, then jump to 6.0 and going forward if new versions in the future.
I'm not sure if I understand your question correctly, if not, please correct me :)
Thanks Hanjun
I'm not sure I understand the question, either. Traditionally, the spec versioning has been exclusively linear -- i.e., now that 6.0 has replaced 5.1, there will be no more 5.x. There may be errata published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but the errata are not encoded in tables anywhere since they are meant only as corrections to the base version. This is unlikely to change, but not impossible, of course :).
The only reason for putting in macros for 5.1 and 6.0 is that those are the versions that I'm concerned with for this particular fix and I know I will use them. If others are needed, I'd have those that need them add them.
It seems to me that you only need to compare acpi_gbl_FADT.header.revision with 6 for this fix, though (if less than 6, use the old way, or use the new way otherwise).
Isn't that the case?
Rafael
On 07/03/2015 05:50 PM, Rafael J. Wysocki wrote:
On Friday, July 03, 2015 01:22:13 PM Al Stone wrote:
On 07/02/2015 11:23 PM, Hanjun Guo wrote:
Hi Rafael,
On 2015/7/3 8:21, Rafael J. Wysocki wrote:
On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
- ((unsigned int)major << 8 | (unsigned int)minor)
+#define ACPI_FADT_SPEC_VERSION \
- __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
acpi_gbl_FADT.minor_revision)+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
Agreed.
Will do. This was a flip of the coin, on my part.
And what if there is 5.2 or even 5.3?
Hmm, do you mean in the future or just now? for both two cases, only 5.1 and 5.1 errata (still has the same ACPI version with 5.1) will be available, then jump to 6.0 and going forward if new versions in the future.
I'm not sure if I understand your question correctly, if not, please correct me :)
Thanks Hanjun
I'm not sure I understand the question, either. Traditionally, the spec versioning has been exclusively linear -- i.e., now that 6.0 has replaced 5.1, there will be no more 5.x. There may be errata published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but the errata are not encoded in tables anywhere since they are meant only as corrections to the base version. This is unlikely to change, but not impossible, of course :).
The only reason for putting in macros for 5.1 and 6.0 is that those are the versions that I'm concerned with for this particular fix and I know I will use them. If others are needed, I'd have those that need them add them.
It seems to me that you only need to compare acpi_gbl_FADT.header.revision with 6 for this fix, though (if less than 6, use the old way, or use the new way otherwise).
Isn't that the case?
Rafael
Ah, sorry for being a bit dense. Yes, on arm64 we only support 5.1 or later so for now, less than 6 is a sufficient test. The 5.0 spec defines yet a third size for the GICC subtable with the same MADT version number (40 bytes) but we won't initialize ACPI if we have anything before 5.1.
So, I could take this patch out of the set and reduce patch 2/3 somewhat by just comparing against acpi_gbl_FADT.header.revision. I think that's what I'll do unless you think these macros have intrinsic value for other reasons.
On Monday, July 06, 2015 03:53:14 PM Al Stone wrote:
On 07/03/2015 05:50 PM, Rafael J. Wysocki wrote:
On Friday, July 03, 2015 01:22:13 PM Al Stone wrote:
On 07/02/2015 11:23 PM, Hanjun Guo wrote:
Hi Rafael,
On 2015/7/3 8:21, Rafael J. Wysocki wrote:
On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) 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 c471dfc..0e525e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -48,6 +48,16 @@ #include <acpi/acpi_io.h> #include <asm/acpi.h> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
- ((unsigned int)major << 8 | (unsigned int)minor)
+#define ACPI_FADT_SPEC_VERSION \
- __ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
acpi_gbl_FADT.minor_revision)+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1) +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
Agreed.
Will do. This was a flip of the coin, on my part.
And what if there is 5.2 or even 5.3?
Hmm, do you mean in the future or just now? for both two cases, only 5.1 and 5.1 errata (still has the same ACPI version with 5.1) will be available, then jump to 6.0 and going forward if new versions in the future.
I'm not sure if I understand your question correctly, if not, please correct me :)
Thanks Hanjun
I'm not sure I understand the question, either. Traditionally, the spec versioning has been exclusively linear -- i.e., now that 6.0 has replaced 5.1, there will be no more 5.x. There may be errata published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but the errata are not encoded in tables anywhere since they are meant only as corrections to the base version. This is unlikely to change, but not impossible, of course :).
The only reason for putting in macros for 5.1 and 6.0 is that those are the versions that I'm concerned with for this particular fix and I know I will use them. If others are needed, I'd have those that need them add them.
It seems to me that you only need to compare acpi_gbl_FADT.header.revision with 6 for this fix, though (if less than 6, use the old way, or use the new way otherwise).
Isn't that the case?
Rafael
Ah, sorry for being a bit dense. Yes, on arm64 we only support 5.1 or later so for now, less than 6 is a sufficient test. The 5.0 spec defines yet a third size for the GICC subtable with the same MADT version number (40 bytes) but we won't initialize ACPI if we have anything before 5.1.
OK
So, I could take this patch out of the set and reduce patch 2/3 somewhat by just comparing against acpi_gbl_FADT.header.revision. I think that's what I'll do unless you think these macros have intrinsic value for other reasons.
That would be fine by me, so please do it.
Thanks, Rafael
Hi Al,
On 2015/7/3 7:48, Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
If I understand correctly, will it be (and for next two patches as well):
0cff8dc0099f (ACPICA: ACPI 6.0: Add changes for MADT table.)?
FADT table update was not introduce any regressions for ARM64 I think.
Thanks Hanjun
On 07/02/2015 11:16 PM, Hanjun Guo wrote:
Hi Al,
On 2015/7/3 7:48, Al Stone wrote:
Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version number from a major and minor revision number. Add also macros that use the helper to construct the current version from the values in the FADT (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
If I understand correctly, will it be (and for next two patches as well):
0cff8dc0099f (ACPICA: ACPI 6.0: Add changes for MADT table.)?
FADT table update was not introduce any regressions for ARM64 I think.
Thanks Hanjun
Right. As I noted in the discussion over v2 of these patches, the FADT change is correctly implementing a change for ACPI 6.0. However, it is this change which adds eight bytes to the GICC subtable so that the struct length and the length field in the GICC subtable no longer match, which is what causes BAD_MADT_ENTRY to fail.
So, while this commit is correct, the patches in this set are needed to work around the ambiguity this causes in the spec and the ACPICA implementation of it.
The BAD_MADT_ENTRY() macro is designed to work for all of the subtables of the MADT. In the ACPI 5.1 version of the spec, the struct for the GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in ACPI 6.0, the struct is 80 bytes long. But, there is only one definition in ACPICA for this struct -- and that is the 6.0 version. Hence, when BAD_MADT_ENTRY() compares the struct size to the length in the GICC subtable, it fails if 5.1 structs are in use, and there are systems in the wild that have them.
This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable only, accounting for the difference in specification versions that are possible. The BAD_MADT_ENTRY() will continue to work as is for all other MADT subtables.
This code is being added to an arm64 header file since that is currently the only architecture using the GICC subtable of the MADT. As a GIC is specific to ARM, it is also unlikely the subtable will be used elsewhere.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) Signed-off-by: Al Stone al.stone@linaro.org --- arch/arm64/include/asm/acpi.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 39248d3..a3c26a4 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,17 @@ #include <asm/psci.h> #include <asm/smp_plat.h>
+/* Macros for consistency checks of the GICC subtable of MADT */ +#define ACPI_MADT_GICC_51_LENGTH 76 +#define ACPI_MADT_GICC_60_LENGTH 80 + +#define BAD_MADT_GICC_ENTRY(entry, end) ( \ + (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ + ((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) && \ + (entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \ + ((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) && \ + (entry->header.length != ACPI_MADT_GICC_60_LENGTH))) + /* Basic configuration for ACPI */ #ifdef CONFIG_ACPI /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 39248d3..a3c26a4 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,17 @@ #include <asm/psci.h> #include <asm/smp_plat.h> +/* Macros for consistency checks of the GICC subtable of MADT */ +#define ACPI_MADT_GICC_51_LENGTH 76 +#define ACPI_MADT_GICC_60_LENGTH 80
+#define BAD_MADT_GICC_ENTRY(entry, end) ( \
(!entry) || (unsigned long)entry + sizeof(*entry) > end || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) && \(entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) && \(entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
This looks ugly but, well, we could live with this.
However, I'd like to avoid having to extend this macro every time we get a new spec released, like 6.1 defining another 80 or 84 etc. So, how about we only update this when there is an actual change in the length? Something like:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0) \ length = 76; \ else \ length = 80; \ length; \ })
or just:
#define ACPI_MADT_GICC_LENGTH \ (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
(the latter is simpler but may not look nice if we change it again in 6.1; though we could re-write this macro when needed, not a problem)
On 07/03/2015 08:06 AM, Catalin Marinas wrote:
On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 39248d3..a3c26a4 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,17 @@ #include <asm/psci.h> #include <asm/smp_plat.h> +/* Macros for consistency checks of the GICC subtable of MADT */ +#define ACPI_MADT_GICC_51_LENGTH 76 +#define ACPI_MADT_GICC_60_LENGTH 80
+#define BAD_MADT_GICC_ENTRY(entry, end) ( \
(!entry) || (unsigned long)entry + sizeof(*entry) > end || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) && \(entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) && \(entry->header.length != ACPI_MADT_GICC_60_LENGTH)))This looks ugly but, well, we could live with this.
Nod. It's right at the hairy edge of becoming a function, I think.
However, I'd like to avoid having to extend this macro every time we get a new spec released, like 6.1 defining another 80 or 84 etc. So, how about we only update this when there is an actual change in the length? Something like:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0) \ length = 76; \ else \ length = 80; \ length; \ })
or just:
#define ACPI_MADT_GICC_LENGTH \ (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
(the latter is simpler but may not look nice if we change it again in 6.1; though we could re-write this macro when needed, not a problem)
Perhaps the sanity checking for the MADT subtables needs to be revisited and a more general solution provided -- this is not the only MADT subtable with this problem and it may occur again.
Even the versions above are not technically compliant with the spec. If we implement what the spec currently says, it might look something like this:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ switch (ACPI_FADT_SPEC_VERSION) { \ case ACPI_FADT_SPEC_VERSION_5_0: \ length = 40; \ break; \ case ACPI_FADT_SPEC_VERSION_5_1: \ length = 76; \ break; \ default: /* use 6.0 size */ \ length = 80; \ } \ length; \ })
So it's just messy and there will be a need for change. Let me think about making this a function instead of a macro; it may make sense to really fix BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable, but it could also be overkill.
On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
On 07/03/2015 08:06 AM, Catalin Marinas wrote:
On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 39248d3..a3c26a4 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,17 @@ #include <asm/psci.h> #include <asm/smp_plat.h> +/* Macros for consistency checks of the GICC subtable of MADT */ +#define ACPI_MADT_GICC_51_LENGTH 76 +#define ACPI_MADT_GICC_60_LENGTH 80
+#define BAD_MADT_GICC_ENTRY(entry, end) ( \
(!entry) || (unsigned long)entry + sizeof(*entry) > end || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) && \(entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) && \(entry->header.length != ACPI_MADT_GICC_60_LENGTH)))This looks ugly but, well, we could live with this.
Nod. It's right at the hairy edge of becoming a function, I think.
However, I'd like to avoid having to extend this macro every time we get a new spec released, like 6.1 defining another 80 or 84 etc. So, how about we only update this when there is an actual change in the length? Something like:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0) \ length = 76; \ else \ length = 80; \ length; \ })
or just:
#define ACPI_MADT_GICC_LENGTH \ (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
(the latter is simpler but may not look nice if we change it again in 6.1; though we could re-write this macro when needed, not a problem)
Perhaps the sanity checking for the MADT subtables needs to be revisited and a more general solution provided -- this is not the only MADT subtable with this problem and it may occur again.
Even the versions above are not technically compliant with the spec. If we implement what the spec currently says, it might look something like this:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ switch (ACPI_FADT_SPEC_VERSION) { \ case ACPI_FADT_SPEC_VERSION_5_0: \ length = 40; \ break; \ case ACPI_FADT_SPEC_VERSION_5_1: \ length = 76; \ break; \ default: /* use 6.0 size */ \ length = 80; \ } \ length; \ })
So it's just messy and there will be a need for change. Let me think about making this a function instead of a macro; it may make sense to really fix BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable, but it could also be overkill.
So here's my suggestion.
First, make ARM64 boot with 4.2+ in the simplest way possible.
Second, set out to fix BAD_MADT_ENTRY() etc. Start with fixing ACPICA to distinguish between the different formats depending on the spec version and follow up from there.
Thanks, Rafael
On Sat, Jul 04, 2015 at 01:54:15AM +0200, Rafael J. Wysocki wrote:
On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
Perhaps the sanity checking for the MADT subtables needs to be revisited and a more general solution provided -- this is not the only MADT subtable with this problem and it may occur again.
Even the versions above are not technically compliant with the spec. If we implement what the spec currently says, it might look something like this:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ switch (ACPI_FADT_SPEC_VERSION) { \ case ACPI_FADT_SPEC_VERSION_5_0: \ length = 40; \ break; \ case ACPI_FADT_SPEC_VERSION_5_1: \ length = 76; \ break; \ default: /* use 6.0 size */ \ length = 80; \ } \ length; \ })
So it's just messy and there will be a need for change. Let me think about making this a function instead of a macro; it may make sense to really fix BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable, but it could also be overkill.
So here's my suggestion.
First, make ARM64 boot with 4.2+ in the simplest way possible.
Second, set out to fix BAD_MADT_ENTRY() etc. Start with fixing ACPICA to distinguish between the different formats depending on the spec version and follow up from there.
That's fine by me (as long as there is a plan to fix it properly, ideally in 4.3).
On 07/03/2015 05:54 PM, Rafael J. Wysocki wrote:
On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
On 07/03/2015 08:06 AM, Catalin Marinas wrote:
On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 39248d3..a3c26a4 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,17 @@ #include <asm/psci.h> #include <asm/smp_plat.h> +/* Macros for consistency checks of the GICC subtable of MADT */ +#define ACPI_MADT_GICC_51_LENGTH 76 +#define ACPI_MADT_GICC_60_LENGTH 80
+#define BAD_MADT_GICC_ENTRY(entry, end) ( \
(!entry) || (unsigned long)entry + sizeof(*entry) > end || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) && \(entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) && \(entry->header.length != ACPI_MADT_GICC_60_LENGTH)))This looks ugly but, well, we could live with this.
Nod. It's right at the hairy edge of becoming a function, I think.
However, I'd like to avoid having to extend this macro every time we get a new spec released, like 6.1 defining another 80 or 84 etc. So, how about we only update this when there is an actual change in the length? Something like:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0) \ length = 76; \ else \ length = 80; \ length; \ })
or just:
#define ACPI_MADT_GICC_LENGTH \ (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
(the latter is simpler but may not look nice if we change it again in 6.1; though we could re-write this macro when needed, not a problem)
Perhaps the sanity checking for the MADT subtables needs to be revisited and a more general solution provided -- this is not the only MADT subtable with this problem and it may occur again.
Even the versions above are not technically compliant with the spec. If we implement what the spec currently says, it might look something like this:
#define ACPI_MADT_GICC_LENGTH ({ \ u8 length; \ switch (ACPI_FADT_SPEC_VERSION) { \ case ACPI_FADT_SPEC_VERSION_5_0: \ length = 40; \ break; \ case ACPI_FADT_SPEC_VERSION_5_1: \ length = 76; \ break; \ default: /* use 6.0 size */ \ length = 80; \ } \ length; \ })
So it's just messy and there will be a need for change. Let me think about making this a function instead of a macro; it may make sense to really fix BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable, but it could also be overkill.
So here's my suggestion.
First, make ARM64 boot with 4.2+ in the simplest way possible.
ACK.
Second, set out to fix BAD_MADT_ENTRY() etc. Start with fixing ACPICA to distinguish between the different formats depending on the spec version and follow up from there.
Thanks, Rafael
Yup, that's what I was thinking.
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.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.) Signed-off-by: Al Stone al.stone@linaro.org Reviewed-by: Hanjun Guo hanjun.guo@linaro.org Acked-by: Will Deacon will.deacon@arm.com --- 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 a1883bf..25fc88c 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;
/*
-
Al Stone -
Al Stone -
Catalin Marinas -
Hanjun Guo -
Rafael J. Wysocki