So, in ACPI 5.1, the GICC subtable of the MADT is 76 bytes long. In the 6.0 version of the spec, the GICC subtable is 80 bytes long. This causes a problem in the code in linux-next, specifically on the APM Mustang. The same problem could exist for any other ACPI tables on arm64 that are 5.1 tables that we are trying to read.
What happens is that the BAD_MADT_ENTRY() macro will fail in these cases, even though the GICC subtable entry is perfectly valid according to the spec. The macro fail because it compares the subtable length contained in the subtable to the length of the struct in ACPICA. Unfortunately, that causes any 5.1 GICC items to be seen as invalid.
Whilst we try to straighten this out in the spec (this seems like something pretty dumb that should not have happened), I've put together two alternative solutions:
-- per Graeme's suggestion, we change it so that we only use ACPI >= 6.0 on arm64; my only objection to this is that it breaks APM Mustangs that are already in use -- on the positive side, it's the simpler of the two patches.
-- or, replace the use of BAD_MADT_ENTRY with a specific function that works around the spec problem, at least until we straighten out what the spec _should_ do; the downside here is that this may be a short term fix, but on the positive side it keeps the change to the only arch that's affected.
I've attached both patches. Opinions on which one to send upstream?
On 06/10/2015 11:52 AM, Al Stone wrote:
So, in ACPI 5.1, the GICC subtable of the MADT is 76 bytes long. In the 6.0 version of the spec, the GICC subtable is 80 bytes long. This causes a problem in the code in linux-next, specifically on the APM Mustang. The same problem could exist for any other ACPI tables on arm64 that are 5.1 tables that we are trying to read.
What happens is that the BAD_MADT_ENTRY() macro will fail in these cases, even though the GICC subtable entry is perfectly valid according to the spec. The macro fail because it compares the subtable length contained in the subtable to the length of the struct in ACPICA. Unfortunately, that causes any 5.1 GICC items to be seen as invalid.
Whilst we try to straighten this out in the spec (this seems like something pretty dumb that should not have happened), I've put together two alternative solutions:
-- per Graeme's suggestion, we change it so that we only use ACPI >= 6.0 on arm64; my only objection to this is that it breaks APM Mustangs that are already in use -- on the positive side, it's the simpler of the two patches. -- or, replace the use of BAD_MADT_ENTRY with a specific function that works around the spec problem, at least until we straighten out what the spec _should_ do; the downside here is that this may be a short term fix, but on the positive side it keeps the change to the only arch that's affected.
I prefer this one. although there is no shipping product using ACPI 5.1, but there was already platforms using ACPI 5.1 for test, and we can't just cause panic on those platforms and better keep compatible with 5.1.
Thanks Hanjun
On 06/10/2015 11:52 AM, Al Stone wrote:
From c9b3456b2d0aa9fa41d5764b81328aedf0892436 Mon Sep 17 00:00:00 2001 From: Al Stoneahs3@redhat.com Date: Tue, 9 Jun 2015 15:50:27 -0600 Subject: [PATCH 1/2] ARM64 / ACPI : make the sanity checks for MADT GICC more robust
The ACPI 5.1 version of the spec is the first to support arm64 properly. The GICC subtable of the MADT is 76 bytes long in the 5.1 spec. However, in the 6.0 spec, the GICC subtable becomes 80 bytes long. The underlying ACPICA engine only knows about the 6.0 version of this subtable, so that the BAD_MADT_ENTRY() macro now fails with firmware compatible with 5.1, even though it is technically correct. The failure in the macro is that it is checking that the struct length from the MADT matches the struct size in ACPICA so that 5.1-compatible structs end up not being allowed.
So, replace the use of the BAD_MADT_ENTRY() macro with a small function that does essentially the same thing but allows for either a 5.1 or a 6.0 struct size from the firmware.
NB: this correction is in arm64-specific code since it is the only architecture currently using the MADT GICC subtable. Changes to the spec are also in progress that may make this patch short-lived, and may make it fit better in the generic ACPI code. In the meantime, this can cause Linux to not boot on some shipping systems.
Signed-off-by: Al Stoneal.stone@linaro.org
arch/arm64/kernel/acpi.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..38146ce 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -165,6 +165,43 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) }
static int __init
static bool since it returns true and false.
+acpi_bad_madt_gicc_entry(struct acpi_madt_generic_interrupt *gicc,
const unsigned long end)+{
- struct acpi_table_header *madt;
- acpi_status status;
- acpi_size size;
- /*
* Do some very basic sanity checks on the GICC subtable.* Return true if the subtable entry is bad, false otherwise.*/- if ((!gicc) || (unsigned long)gicc + sizeof(*gicc) > end)
return true;- /*
* The ACPI 5.1 spec defines a struct of 76 bytes, but the 6.0* spec uses an 80-byte struct, and both are technically valid.* Check for either case here, specific to revision 3 of MADT.* If the spec fixes this, we might be able to replace this* function with the use of the BAD_MADT_ENTRY() in the future.*/- status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &madt, &size);
- if (ACPI_FAILURE(status)) {
const char *msg = acpi_format_exception(status);pr_err("Failed to get MADT table, %s\n", msg);return true;- }
- if (madt->revision == 3 && (sizeof(*gicc) == 76 || sizeof(*gicc) == 80))
How about using the ACPI spec version to justify the length? It's a bit weird that referring to FADT table in MADT parsing but it's easy understood for me :)
return false;- else
return true;+}
+static int __init acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, const unsigned long end) { @@ -172,7 +209,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
processor = (struct acpi_madt_generic_interrupt *)header;
- if (BAD_MADT_ENTRY(processor, end))
- if (acpi_bad_madt_gicc_entry(processor, end))
acpi_parse_gic_cpu_interface() was already moved to arch/arm64/smp.c, and accepted by Catalin, and also we need to update the BAD_MADT_ENTRY() in GICv2 init code too.
Thanks Hanjun
On 10 June 2015 at 08:05, Hanjun Guo hanjun.guo@linaro.org wrote:
On 06/10/2015 11:52 AM, Al Stone wrote:
From c9b3456b2d0aa9fa41d5764b81328aedf0892436 Mon Sep 17 00:00:00 2001 From: Al Stoneahs3@redhat.com Date: Tue, 9 Jun 2015 15:50:27 -0600 Subject: [PATCH 1/2] ARM64 / ACPI : make the sanity checks for MADT GICC more robust
The ACPI 5.1 version of the spec is the first to support arm64 properly. The GICC subtable of the MADT is 76 bytes long in the 5.1 spec. However, in the 6.0 spec, the GICC subtable becomes 80 bytes long. The underlying ACPICA engine only knows about the 6.0 version of this subtable, so that the BAD_MADT_ENTRY() macro now fails with firmware compatible with 5.1, even though it is technically correct. The failure in the macro is that it is checking that the struct length from the MADT matches the struct size in ACPICA so that 5.1-compatible structs end up not being allowed.
So, replace the use of the BAD_MADT_ENTRY() macro with a small function that does essentially the same thing but allows for either a 5.1 or a 6.0 struct size from the firmware.
NB: this correction is in arm64-specific code since it is the only architecture currently using the MADT GICC subtable. Changes to the spec are also in progress that may make this patch short-lived, and may make it fit better in the generic ACPI code. In the meantime, this can cause Linux to not boot on some shipping systems.
Spec changes cannot retroactively change old firmware. So this is a nonsense statement.
Graeme
Signed-off-by: Al Stoneal.stone@linaro.org
arch/arm64/kernel/acpi.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..38146ce 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -165,6 +165,43 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) }
static int __init
static bool since it returns true and false.
+acpi_bad_madt_gicc_entry(struct acpi_madt_generic_interrupt *gicc,
const unsigned long end)+{
struct acpi_table_header *madt;acpi_status status;acpi_size size;/** Do some very basic sanity checks on the GICC subtable.* Return true if the subtable entry is bad, false otherwise.*/if ((!gicc) || (unsigned long)gicc + sizeof(*gicc) > end)return true;/** The ACPI 5.1 spec defines a struct of 76 bytes, but the 6.0* spec uses an 80-byte struct, and both are technically valid.* Check for either case here, specific to revision 3 of MADT.* If the spec fixes this, we might be able to replace this* function with the use of the BAD_MADT_ENTRY() in the future.*/status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &madt, &size);if (ACPI_FAILURE(status)) {const char *msg = acpi_format_exception(status);pr_err("Failed to get MADT table, %s\n", msg);return true;}if (madt->revision == 3 && (sizeof(*gicc) == 76 || sizeof(*gicc)== 80))
How about using the ACPI spec version to justify the length? It's a bit weird that referring to FADT table in MADT parsing but it's easy understood for me :)
return false;elsereturn true;+}
+static int __init acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, const unsigned long end) { @@ -172,7 +209,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
processor = (struct acpi_madt_generic_interrupt *)header;
if (BAD_MADT_ENTRY(processor, end))
if (acpi_bad_madt_gicc_entry(processor, end))acpi_parse_gic_cpu_interface() was already moved to arch/arm64/smp.c, and accepted by Catalin, and also we need to update the BAD_MADT_ENTRY() in GICv2 init code too.
Thanks Hanjun _______________________________________________ Linaro-acpi mailing list Linaro-acpi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-acpi
On 06/10/2015 01:37 AM, G Gregory wrote:
On 10 June 2015 at 08:05, Hanjun Guo hanjun.guo@linaro.org wrote:
On 06/10/2015 11:52 AM, Al Stone wrote:
From c9b3456b2d0aa9fa41d5764b81328aedf0892436 Mon Sep 17 00:00:00 2001 From: Al Stoneahs3@redhat.com Date: Tue, 9 Jun 2015 15:50:27 -0600 Subject: [PATCH 1/2] ARM64 / ACPI : make the sanity checks for MADT GICC more robust
The ACPI 5.1 version of the spec is the first to support arm64 properly. The GICC subtable of the MADT is 76 bytes long in the 5.1 spec. However, in the 6.0 spec, the GICC subtable becomes 80 bytes long. The underlying ACPICA engine only knows about the 6.0 version of this subtable, so that the BAD_MADT_ENTRY() macro now fails with firmware compatible with 5.1, even though it is technically correct. The failure in the macro is that it is checking that the struct length from the MADT matches the struct size in ACPICA so that 5.1-compatible structs end up not being allowed.
So, replace the use of the BAD_MADT_ENTRY() macro with a small function that does essentially the same thing but allows for either a 5.1 or a 6.0 struct size from the firmware.
NB: this correction is in arm64-specific code since it is the only architecture currently using the MADT GICC subtable. Changes to the spec are also in progress that may make this patch short-lived, and may make it fit better in the generic ACPI code. In the meantime, this can cause Linux to not boot on some shipping systems.
Spec changes cannot retroactively change old firmware. So this is a nonsense statement.
Graeme
Argh. And not what I meant to say. What I meant was that perhaps the spec change will make this patch more straightforward, or that at some point in the future we can ignore the old firmware and simplify the code. But, you are correct: spec changes cannot retroactively change old firmware -- if it could, our lives would be *so* much easier :).
On 10 June 2015 at 08:05, Hanjun Guo hanjun.guo@linaro.org wrote:
On 06/10/2015 11:52 AM, Al Stone wrote:
From c9b3456b2d0aa9fa41d5764b81328aedf0892436 Mon Sep 17 00:00:00 2001 From: Al Stoneahs3@redhat.com Date: Tue, 9 Jun 2015 15:50:27 -0600 Subject: [PATCH 1/2] ARM64 / ACPI : make the sanity checks for MADT GICC more robust
The ACPI 5.1 version of the spec is the first to support arm64 properly. The GICC subtable of the MADT is 76 bytes long in the 5.1 spec. However, in the 6.0 spec, the GICC subtable becomes 80 bytes long. The underlying ACPICA engine only knows about the 6.0 version of this subtable, so that the BAD_MADT_ENTRY() macro now fails with firmware compatible with 5.1, even though it is technically correct. The failure in the macro is that it is checking that the struct length from the MADT matches the struct size in ACPICA so that 5.1-compatible structs end up not being allowed.
So, replace the use of the BAD_MADT_ENTRY() macro with a small function that does essentially the same thing but allows for either a 5.1 or a 6.0 struct size from the firmware.
NB: this correction is in arm64-specific code since it is the only architecture currently using the MADT GICC subtable. Changes to the spec are also in progress that may make this patch short-lived, and may make it fit better in the generic ACPI code. In the meantime, this can cause Linux to not boot on some shipping systems.
Signed-off-by: Al Stoneal.stone@linaro.org
arch/arm64/kernel/acpi.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..38146ce 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -165,6 +165,43 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) }
static int __init
static bool since it returns true and false.
+acpi_bad_madt_gicc_entry(struct acpi_madt_generic_interrupt *gicc,
const unsigned long end)+{
struct acpi_table_header *madt;acpi_status status;acpi_size size;/** Do some very basic sanity checks on the GICC subtable.* Return true if the subtable entry is bad, false otherwise.*/if ((!gicc) || (unsigned long)gicc + sizeof(*gicc) > end)return true;/** The ACPI 5.1 spec defines a struct of 76 bytes, but the 6.0* spec uses an 80-byte struct, and both are technically valid.* Check for either case here, specific to revision 3 of MADT.* If the spec fixes this, we might be able to replace this* function with the use of the BAD_MADT_ENTRY() in the future.*/status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &madt, &size);if (ACPI_FAILURE(status)) {const char *msg = acpi_format_exception(status);pr_err("Failed to get MADT table, %s\n", msg);return true;}if (madt->revision == 3 && (sizeof(*gicc) == 76 || sizeof(*gicc)== 80))
How about using the ACPI spec version to justify the length? It's a bit weird that referring to FADT table in MADT parsing but it's easy understood for me :)
Yes, that would work, maybe we store the FADT globally as acpi_firmware_spec_version? So we don't need to re-parse it.
also sizeof(*gicc) doesn't do what you seem to think it does, those should probably be gicc->length references.
Graeme
return false;elsereturn true;+}
+static int __init acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, const unsigned long end) { @@ -172,7 +209,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
processor = (struct acpi_madt_generic_interrupt *)header;
if (BAD_MADT_ENTRY(processor, end))
if (acpi_bad_madt_gicc_entry(processor, end))acpi_parse_gic_cpu_interface() was already moved to arch/arm64/smp.c, and accepted by Catalin, and also we need to update the BAD_MADT_ENTRY() in GICv2 init code too.
Thanks Hanjun _______________________________________________ Linaro-acpi mailing list Linaro-acpi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-acpi
On 06/10/2015 02:21 AM, G Gregory wrote:
On 10 June 2015 at 08:05, Hanjun Guo hanjun.guo@linaro.org wrote:
On 06/10/2015 11:52 AM, Al Stone wrote:
From c9b3456b2d0aa9fa41d5764b81328aedf0892436 Mon Sep 17 00:00:00 2001 From: Al Stoneahs3@redhat.com Date: Tue, 9 Jun 2015 15:50:27 -0600 Subject: [PATCH 1/2] ARM64 / ACPI : make the sanity checks for MADT GICC more robust
The ACPI 5.1 version of the spec is the first to support arm64 properly. The GICC subtable of the MADT is 76 bytes long in the 5.1 spec. However, in the 6.0 spec, the GICC subtable becomes 80 bytes long. The underlying ACPICA engine only knows about the 6.0 version of this subtable, so that the BAD_MADT_ENTRY() macro now fails with firmware compatible with 5.1, even though it is technically correct. The failure in the macro is that it is checking that the struct length from the MADT matches the struct size in ACPICA so that 5.1-compatible structs end up not being allowed.
So, replace the use of the BAD_MADT_ENTRY() macro with a small function that does essentially the same thing but allows for either a 5.1 or a 6.0 struct size from the firmware.
NB: this correction is in arm64-specific code since it is the only architecture currently using the MADT GICC subtable. Changes to the spec are also in progress that may make this patch short-lived, and may make it fit better in the generic ACPI code. In the meantime, this can cause Linux to not boot on some shipping systems.
Signed-off-by: Al Stoneal.stone@linaro.org
arch/arm64/kernel/acpi.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..38146ce 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -165,6 +165,43 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) }
static int __init
static bool since it returns true and false.
+acpi_bad_madt_gicc_entry(struct acpi_madt_generic_interrupt *gicc,
const unsigned long end)+{
struct acpi_table_header *madt;acpi_status status;acpi_size size;/** Do some very basic sanity checks on the GICC subtable.* Return true if the subtable entry is bad, false otherwise.*/if ((!gicc) || (unsigned long)gicc + sizeof(*gicc) > end)return true;/** The ACPI 5.1 spec defines a struct of 76 bytes, but the 6.0* spec uses an 80-byte struct, and both are technically valid.* Check for either case here, specific to revision 3 of MADT.* If the spec fixes this, we might be able to replace this* function with the use of the BAD_MADT_ENTRY() in the future.*/status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &madt, &size);if (ACPI_FAILURE(status)) {const char *msg = acpi_format_exception(status);pr_err("Failed to get MADT table, %s\n", msg);return true;}if (madt->revision == 3 && (sizeof(*gicc) == 76 || sizeof(*gicc)== 80))
How about using the ACPI spec version to justify the length? It's a bit weird that referring to FADT table in MADT parsing but it's easy understood for me :)
Yes, that would work, maybe we store the FADT globally as acpi_firmware_spec_version? So we don't need to re-parse it.
Yup, as noted on IRC. I'll put that together.
also sizeof(*gicc) doesn't do what you seem to think it does, those should probably be gicc->length references.
Graeme
/me facepalms
Right. Apparently I was relying on the DWIM ("Do What I Mean") part of the compiler...
return false;elsereturn true;+}
+static int __init acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, const unsigned long end) { @@ -172,7 +209,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
processor = (struct acpi_madt_generic_interrupt *)header;
if (BAD_MADT_ENTRY(processor, end))
if (acpi_bad_madt_gicc_entry(processor, end))acpi_parse_gic_cpu_interface() was already moved to arch/arm64/smp.c, and accepted by Catalin, and also we need to update the BAD_MADT_ENTRY() in GICv2 init code too.
Thanks Hanjun _______________________________________________ Linaro-acpi mailing list Linaro-acpi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-acpi
On 06/10/2015 01:05 AM, Hanjun Guo wrote:
On 06/10/2015 11:52 AM, Al Stone wrote:
From c9b3456b2d0aa9fa41d5764b81328aedf0892436 Mon Sep 17 00:00:00 2001 From: Al Stoneahs3@redhat.com Date: Tue, 9 Jun 2015 15:50:27 -0600 Subject: [PATCH 1/2] ARM64 / ACPI : make the sanity checks for MADT GICC more robust
The ACPI 5.1 version of the spec is the first to support arm64 properly. The GICC subtable of the MADT is 76 bytes long in the 5.1 spec. However, in the 6.0 spec, the GICC subtable becomes 80 bytes long. The underlying ACPICA engine only knows about the 6.0 version of this subtable, so that the BAD_MADT_ENTRY() macro now fails with firmware compatible with 5.1, even though it is technically correct. The failure in the macro is that it is checking that the struct length from the MADT matches the struct size in ACPICA so that 5.1-compatible structs end up not being allowed.
So, replace the use of the BAD_MADT_ENTRY() macro with a small function that does essentially the same thing but allows for either a 5.1 or a 6.0 struct size from the firmware.
NB: this correction is in arm64-specific code since it is the only architecture currently using the MADT GICC subtable. Changes to the spec are also in progress that may make this patch short-lived, and may make it fit better in the generic ACPI code. In the meantime, this can cause Linux to not boot on some shipping systems.
Signed-off-by: Al Stoneal.stone@linaro.org
arch/arm64/kernel/acpi.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..38146ce 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -165,6 +165,43 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) }
static int __init
static bool since it returns true and false.
D'oh. Right. Or use 0 and 1 properly :).
+acpi_bad_madt_gicc_entry(struct acpi_madt_generic_interrupt *gicc,
const unsigned long end)+{
- struct acpi_table_header *madt;
- acpi_status status;
- acpi_size size;
- /*
* Do some very basic sanity checks on the GICC subtable.* Return true if the subtable entry is bad, false otherwise.*/- if ((!gicc) || (unsigned long)gicc + sizeof(*gicc) > end)
return true;- /*
* The ACPI 5.1 spec defines a struct of 76 bytes, but the 6.0* spec uses an 80-byte struct, and both are technically valid.* Check for either case here, specific to revision 3 of MADT.* If the spec fixes this, we might be able to replace this* function with the use of the BAD_MADT_ENTRY() in the future.*/- status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &madt, &size);
- if (ACPI_FAILURE(status)) {
const char *msg = acpi_format_exception(status);pr_err("Failed to get MADT table, %s\n", msg);return true;- }
- if (madt->revision == 3 && (sizeof(*gicc) == 76 || sizeof(*gicc) == 80))
How about using the ACPI spec version to justify the length? It's a bit weird that referring to FADT table in MADT parsing but it's easy understood for me :)
Yeah, that's a better idea, and probably more reliable. I was seeing the chat on IRC about preserving the version number from the FADT, too. If you haven't done that patch already, I'll put one in the next version of this.
return false;- else
return true;+}
+static int __init acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, const unsigned long end) { @@ -172,7 +209,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
processor = (struct acpi_madt_generic_interrupt *)header;
- if (BAD_MADT_ENTRY(processor, end))
- if (acpi_bad_madt_gicc_entry(processor, end))
acpi_parse_gic_cpu_interface() was already moved to arch/arm64/smp.c, and accepted by Catalin, and also we need to update the BAD_MADT_ENTRY() in GICv2 init code too.
Huh. That wasn't in the linux-next tree or in acpi.git, and I forgot to look in the ARM kernel tree :(. I'll rebase on top of that and get these, too.
Thanks Hanjun
Thanks for the review, Hanjun.
On 9 June 2015 at 23:52, Al Stone al.stone@linaro.org wrote:
So, in ACPI 5.1, the GICC subtable of the MADT is 76 bytes long. In the 6.0 version of the spec, the GICC subtable is 80 bytes long. This causes a problem in the code in linux-next, specifically on the APM Mustang. The same problem could exist for any other ACPI tables on arm64 that are 5.1 tables that we are trying to read.
What happens is that the BAD_MADT_ENTRY() macro will fail in these cases, even though the GICC subtable entry is perfectly valid according to the spec. The macro fail because it compares the subtable length contained in the subtable to the length of the struct in ACPICA. Unfortunately, that causes any 5.1 GICC items to be seen as invalid.
So, IIUC, this is a problem when v5.1 tables are compiled with v6.0 ACPICA tools?
Whilst we try to straighten this out in the spec (this seems like something pretty dumb that should not have happened), I've put together two alternative solutions:
-- per Graeme's suggestion, we change it so that we only use ACPI >= 6.0 on arm64; my only objection to this is that it breaks APM Mustangs that are already in use -- on the positive side, it's the simpler of the two patches.
-- or, replace the use of BAD_MADT_ENTRY with a specific function that works around the spec problem, at least until we straighten out what the spec _should_ do; the downside here is that this may be a short term fix, but on the positive side it keeps the change to the only arch that's affected.
I've attached both patches. Opinions on which one to send upstream?
I agree, while option 1 is easier, option 2 seems better since there's some platforms ( although not all are SBSA compliant) shipping out there.
Regards, Ashwin.
On 06/10/2015 12:49 PM, Ashwin Chaugule wrote:
On 9 June 2015 at 23:52, Al Stone al.stone@linaro.org wrote:
So, in ACPI 5.1, the GICC subtable of the MADT is 76 bytes long. In the 6.0 version of the spec, the GICC subtable is 80 bytes long. This causes a problem in the code in linux-next, specifically on the APM Mustang. The same problem could exist for any other ACPI tables on arm64 that are 5.1 tables that we are trying to read.
What happens is that the BAD_MADT_ENTRY() macro will fail in these cases, even though the GICC subtable entry is perfectly valid according to the spec. The macro fail because it compares the subtable length contained in the subtable to the length of the struct in ACPICA. Unfortunately, that causes any 5.1 GICC items to be seen as invalid.
So, IIUC, this is a problem when v5.1 tables are compiled with v6.0 ACPICA tools?
Right. If the ASL is 5.1 and 20150515 of acpica-tools is being used, it will complain about the table content. If you disassemble, it's the same problem.
If I can get the MADT version incremented as an errata to ACPI 6.0, then ACPICA will have to correct their code and the problem won't occur.
Whilst we try to straighten this out in the spec (this seems like something pretty dumb that should not have happened), I've put together two alternative solutions:
-- per Graeme's suggestion, we change it so that we only use ACPI >= 6.0 on arm64; my only objection to this is that it breaks APM Mustangs that are already in use -- on the positive side, it's the simpler of the two patches.
-- or, replace the use of BAD_MADT_ENTRY with a specific function that works around the spec problem, at least until we straighten out what the spec _should_ do; the downside here is that this may be a short term fix, but on the positive side it keeps the change to the only arch that's affected.
I've attached both patches. Opinions on which one to send upstream?
I agree, while option 1 is easier, option 2 seems better since there's some platforms ( although not all are SBSA compliant) shipping out there.
Regards, Ashwin.
Okay. Let me revise and re-send. It seems the majority is in favor of the second option.
-
Al Stone -
Ashwin Chaugule -
G Gregory -
Hanjun Guo