On Wednesday, October 14, 2015 03:26:22 PM Al Stone wrote:

According to the ACPI specification, version 6.0, table 5-46, MADT subtable IDs in the range of 0x10-0x7f are reserved for possible future use by the specification. The function bad_madt_entry() tries to enforce the spec, but it turns out there are x86 machines that use 0x7f even though they should not.

So, continue to enforce this rule for arm64, since we're starting out fresh, but relax it for systems already out there so we don't keep them from booting.

Signed-off-by: Al Stone al.stone@linaro.org

drivers/acpi/tables.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index a2ed38a..e5cfd72 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -413,9 +413,17 @@ static int __init bad_madt_entry(struct acpi_table_header *table, } if (entry->type >= ms->num_types) {

• pr_err("undefined MADT subtable type for FADT %d.%d: %d (length %d)\n",
  

•        major, minor, entry->type, entry->length);
  

• return 1;
  

• if (IS_ENABLED(CONFIG_ARM64)) {
  

• 	/* Enforce this stricture on arm64... */
  

• 	pr_err("undefined MADT subtable type for FADT %d.%d: %d (length %d)\n",
  

• 	       major, minor, entry->type, entry->length);
  

• 	return 1;
  

• } else {
  

• 	/* ... but relax it on legacy systems so they boot */
  

• 	pr_warn("undefined MADT subtable type for FADT %d.%d: %d (length %d)\n",
  

• 	         major, minor, entry->type, entry->length);
  

• 	return 0;
  

I don't see much point in varying the log level of the message depending on the architecture. It can stay pr_err() for all of them as far as I'm concerned and then you only need to change the return line to something like:

return IS_ENABLED(CONFIG_ARM64);

That said, this is getting messy and we may be much better off by having bad_madt_entry() defined per arch. Then, for x86 and ia64 it may just do what was done before and you may use a more sophisticated one for ARM64.

Have you considered doing that?

• }
  

  }

/* verify that the table is allowed for this version of the spec */

Thanks, Rafael

On Wednesday, October 14, 2015 03:26:23 PM Al Stone wrote:

Looking across multiple versions of the ACPI specification, certain versions introduce new revision numbers for the FADT and/or MADT tables. So, for example, an FADT indicating it is revision 4 should not be paired with an MADT revision of anything less than 2.

However, there are systems out there that do not update the revision fields in the FADT and MADT tables as they should. So, for arm64, we can be stricter in complying with the specification, but we need to relax the checking for legacy systems.

Signed-off-by: Al Stone al.stone@linaro.org

drivers/acpi/tables.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index e5cfd72..3b5ddfb 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -407,9 +407,17 @@ static int __init bad_madt_entry(struct acpi_table_header *table, ms++; } if (!ms->num_types) {

• pr_err("undefined version for either FADT %d.%d or MADT %d\n",
  

•        major, minor, madt->header.revision);
  

• return 1;
  

• if (IS_ENABLED(CONFIG_ARM64)) {
  

• 	/* Enforce this stricture on arm64... */
  

• 	pr_err("undefined version for either FADT %d.%d or MADT %d\n",
  

• 	       major, minor, madt->header.revision);
  

• 	return 1;
  

• } else {
  

• 	/* ... but relax it on legacy systems so they boot */
  

• 	pr_warn("undefined version for either FADT %d.%d or MADT %d\n",
  

• 	        major, minor, madt->header.revision);
  

• 	return 0;
  

Same comment as for the [1/4] applies here.

• }
  

  }

if (entry->type >= ms->num_types) {

Thanks, Rafael

On Wednesday, October 14, 2015 03:26:24 PM Al Stone wrote:

In some environments, the FADT revision number is always 2, independent of any other factors indicating that it may be a newer revision. So, we cannot rely on the FADT and MADT revisions being in proper sync. For those environments, relax the checking so we only enforce the size check, even if we do issue warnings on other problems.

If we do not relax the rules, these systems will not boot as they have in the past.

Signed-off-by: Al Stone al.stone@linaro.org

drivers/acpi/tables.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 3b5ddfb..790d4b0 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -416,7 +416,6 @@ static int __init bad_madt_entry(struct acpi_table_header *table, /* ... but relax it on legacy systems so they boot */ pr_warn("undefined version for either FADT %d.%d or MADT %d\n", major, minor, madt->header.revision);

• 	return 0;
  

  } }

@@ -430,16 +429,41 @@ static int __init bad_madt_entry(struct acpi_table_header *table, /* ... but relax it on legacy systems so they boot */ pr_warn("undefined MADT subtable type for FADT %d.%d: %d (length %d)\n", major, minor, entry->type, entry->length);

• 	return 0;
  

  } }

/* verify that the table is allowed for this version of the spec */ len = ms->lengths[entry->type]; if (!len) {

• pr_err("MADT subtable %d not defined for FADT %d.%d\n",
  

• 	 entry->type, major, minor);
  

• return 1;
  

• if (IS_ENABLED(CONFIG_ARM64)) {
  

• 	pr_err("MADT subtable %d not defined for FADT %d.%d\n",
  

• 	       entry->type, major, minor);
  

• 	return 1;
  

• } else {
  

• 	pr_warn("MADT subtable %d not defined for FADT %d.%d\n",
  

• 	        entry->type, major, minor);
  

• }
  

Same comment again.

• }
• /*

• * When we get this far, we may have issued warnings on either
  

• * a mismatch in FADT/MADT revisions, or have noted that the subtable
  

• * ID is not defined for the MADT revision we're using.  On some
  

• * architectures, this is an error, but for legacy systems, we need
  

• * to push on with other checks of the subtable.
  

• *
  

• * In fact, there are environments where the *only* value the FADT
  

• * revision will ever have is 2, regardless of anything else.  So,
  

• * for those systems to boot, we have to pretend the MADT is the
  

• * latest version to allow all known subtables since we have no way
  

• * to determine what revision it should be.
  

• */
  

• if (!IS_ENABLED(CONFIG_ARM64) && major == 2) {

Gosh. No sorry, this isn't going in. Please rethink the approach.

• ms = spec_info;
  

• while (ms->num_types != 0)
  

• 	ms++;
  

• ms--;
  

• len = ms->lengths[entry->type];
  

  }

/* verify that the length is what we expect */

Thanks, Rafael

On Wednesday, October 14, 2015 03:26:25 PM Al Stone wrote:

Correct a typo where bad_madt_entry() expected the MADT GIC ITS subtable to be 16 bytes long, but it is actually 20 bytes. This is a cut'n'paste error picked up from the spec and inadvertently replicated in code.

Signed-off-by: Al Stone al.stone@linaro.org

drivers/acpi/tables.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 790d4b0..1b7c13e 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -292,7 +292,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)

• GICD 0xc 24 24 24
• GICv2m MSI 0xd 24 24
• GICR 0xe 16 16

• • GIC ITS 0xf 16

• • GIC ITS 0xf 20
  • In the table, each length entry is what should be in the length
  • field of the subtable, and -- in general -- it should match the

@@ -366,7 +366,7 @@ static struct acpi_madt_subtable_lengths spec_info[] = { .madt_version = 3, .num_types = 16, .lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,

• 	     16, 16, 12, 80, 24, 24, 16, 16 }
  

• 	     16, 16, 12, 80, 24, 24, 16, 20 }
  

  }, { /* terminator */ .major_version = 0,

That really needs to be folded into the original patch to avoid creating artificial bisection holes.

Thanks, Rafael