Re: [Linaro-acpi] [RFC] problem with MADT GICC subtables in ACPI 6.0

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