All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches
@ 2015-10-14 21:26 Al Stone
  2015-10-14 21:26 ` [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs Al Stone
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Al Stone @ 2015-10-14 21:26 UTC (permalink / raw)
  To: linux-acpi; +Cc: linaro-acpi, linaro-kernel, patches, Al Stone

Once the patch series "Provide better MADT subtable sanity checks" got
into linux-next (commit b9e11e92b9), several existing platforms were found
where the firmware was doing odd things that aren't exactly correct if
the ACPI specification is being followed precisely.  This patch series
relaxes some of the checks on MADT subtables so that these previously 
working systems (all x86-based) will continue to boot.  For arm64, since
ACPI usage is still relatively new, the stricter checking is left in place.

Al Stone (4):
  ACPI: workaround x86 firmware using reserved MADT subtable IDs
  ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions
  ACPI: workaround FADT always being revision 2
  ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16

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

-- 
2.4.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs
  2015-10-14 21:26 [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Al Stone
@ 2015-10-14 21:26 ` Al Stone
  2015-10-14 23:34   ` Rafael J. Wysocki
  2015-10-14 21:26 ` [PATCH 2/4] ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions Al Stone
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Al Stone @ 2015-10-14 21:26 UTC (permalink / raw)
  To: linux-acpi; +Cc: linaro-acpi, linaro-kernel, patches, Al Stone

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;
+		}
 	}
 
 	/* verify that the table is allowed for this version of the spec */
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions
  2015-10-14 21:26 [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Al Stone
  2015-10-14 21:26 ` [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs Al Stone
@ 2015-10-14 21:26 ` Al Stone
  2015-10-14 23:36   ` Rafael J. Wysocki
  2015-10-14 21:26 ` [PATCH 3/4] ACPI: workaround FADT always being revision 2 Al Stone
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Al Stone @ 2015-10-14 21:26 UTC (permalink / raw)
  To: linux-acpi; +Cc: linaro-acpi, linaro-kernel, patches, Al Stone

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;
+		}
 	}
 
 	if (entry->type >= ms->num_types) {
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] ACPI: workaround FADT always being revision 2
  2015-10-14 21:26 [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Al Stone
  2015-10-14 21:26 ` [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs Al Stone
  2015-10-14 21:26 ` [PATCH 2/4] ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions Al Stone
@ 2015-10-14 21:26 ` Al Stone
  2015-10-14 23:38   ` Rafael J. Wysocki
  2015-10-14 21:26 ` [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16 Al Stone
  2015-10-14 23:44 ` [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Rafael J. Wysocki
  4 siblings, 1 reply; 12+ messages in thread
From: Al Stone @ 2015-10-14 21:26 UTC (permalink / raw)
  To: linux-acpi; +Cc: linaro-acpi, linaro-kernel, patches, Al Stone

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);
+		}
+	}
+
+	/*
+	 * 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) {
+		ms = spec_info;
+		while (ms->num_types != 0)
+			ms++;
+		ms--;
+		len = ms->lengths[entry->type];
 	}
 
 	/* verify that the length is what we expect */
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16
  2015-10-14 21:26 [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Al Stone
                   ` (2 preceding siblings ...)
  2015-10-14 21:26 ` [PATCH 3/4] ACPI: workaround FADT always being revision 2 Al Stone
@ 2015-10-14 21:26 ` Al Stone
  2015-10-14 23:39   ` Rafael J. Wysocki
  2015-10-14 23:44 ` [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Rafael J. Wysocki
  4 siblings, 1 reply; 12+ messages in thread
From: Al Stone @ 2015-10-14 21:26 UTC (permalink / raw)
  To: linux-acpi; +Cc: linaro-acpi, linaro-kernel, patches, Al Stone

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,
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs
  2015-10-14 21:26 ` [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs Al Stone
@ 2015-10-14 23:34   ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2015-10-14 23:34 UTC (permalink / raw)
  To: Al Stone; +Cc: linux-acpi, linaro-acpi, linaro-kernel, patches

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions
  2015-10-14 21:26 ` [PATCH 2/4] ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions Al Stone
@ 2015-10-14 23:36   ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2015-10-14 23:36 UTC (permalink / raw)
  To: Al Stone; +Cc: linux-acpi, linaro-acpi, linaro-kernel, patches

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] ACPI: workaround FADT always being revision 2
  2015-10-14 21:26 ` [PATCH 3/4] ACPI: workaround FADT always being revision 2 Al Stone
@ 2015-10-14 23:38   ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2015-10-14 23:38 UTC (permalink / raw)
  To: Al Stone; +Cc: linux-acpi, linaro-acpi, linaro-kernel, patches

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16
  2015-10-14 21:26 ` [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16 Al Stone
@ 2015-10-14 23:39   ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2015-10-14 23:39 UTC (permalink / raw)
  To: Al Stone; +Cc: linux-acpi, linaro-acpi, linaro-kernel, patches

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches
  2015-10-14 21:26 [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Al Stone
                   ` (3 preceding siblings ...)
  2015-10-14 21:26 ` [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16 Al Stone
@ 2015-10-14 23:44 ` Rafael J. Wysocki
  2015-10-15  0:23   ` Al Stone
  4 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2015-10-14 23:44 UTC (permalink / raw)
  To: Al Stone; +Cc: linux-acpi, linaro-acpi, linaro-kernel, patches

On Wednesday, October 14, 2015 03:26:21 PM Al Stone wrote:
> Once the patch series "Provide better MADT subtable sanity checks" got
> into linux-next (commit b9e11e92b9), several existing platforms were found
> where the firmware was doing odd things that aren't exactly correct if
> the ACPI specification is being followed precisely.  This patch series
> relaxes some of the checks on MADT subtables so that these previously 
> working systems (all x86-based) will continue to boot.  For arm64, since
> ACPI usage is still relatively new, the stricter checking is left in place.
> 
> Al Stone (4):
>   ACPI: workaround x86 firmware using reserved MADT subtable IDs
>   ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions
>   ACPI: workaround FADT always being revision 2
>   ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16
> 
>  drivers/acpi/tables.c | 62 ++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 51 insertions(+), 11 deletions(-)

Honestly, having reviewed this series I'm inclined to drop the original
changes from my tree and ask you to start over.

It seems to have been a mistake to modify the existing behavior for x86 and
goodness only knows about ia64.  The changes for these architectures don't make
us better off in any way.

I understand the motivation to keep ARM64 "fresh and clean", but there must be
a way to do that without affecting the other architectures negatively.

Thanks,
Rafael


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches
  2015-10-14 23:44 ` [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Rafael J. Wysocki
@ 2015-10-15  0:23   ` Al Stone
  2015-10-15  0:37     ` Rafael J. Wysocki
  0 siblings, 1 reply; 12+ messages in thread
From: Al Stone @ 2015-10-15  0:23 UTC (permalink / raw)
  To: Rafael J. Wysocki, Al Stone
  Cc: linux-acpi, linaro-acpi, linaro-kernel, patches

On 10/14/2015 05:44 PM, Rafael J. Wysocki wrote:
> On Wednesday, October 14, 2015 03:26:21 PM Al Stone wrote:
>> Once the patch series "Provide better MADT subtable sanity checks" got
>> into linux-next (commit b9e11e92b9), several existing platforms were found
>> where the firmware was doing odd things that aren't exactly correct if
>> the ACPI specification is being followed precisely.  This patch series
>> relaxes some of the checks on MADT subtables so that these previously 
>> working systems (all x86-based) will continue to boot.  For arm64, since
>> ACPI usage is still relatively new, the stricter checking is left in place.
>>
>> Al Stone (4):
>>   ACPI: workaround x86 firmware using reserved MADT subtable IDs
>>   ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions
>>   ACPI: workaround FADT always being revision 2
>>   ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16
>>
>>  drivers/acpi/tables.c | 62 ++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 51 insertions(+), 11 deletions(-)
> 
> Honestly, having reviewed this series I'm inclined to drop the original
> changes from my tree and ask you to start over.

Yeah, I'm leaning that way, too.  This is getting way too messy.

> It seems to have been a mistake to modify the existing behavior for x86 and
> goodness only knows about ia64.  The changes for these architectures don't make
> us better off in any way.

Right.  It is truly amazing to me how much kruft this has turned up, when the
idea was simply to make sure that the tables used contain proper content -- and
this is only for one table.  I understand why we don't want to break something
that's already working but I am just sort of amazed at how much improperly
written firmware is in use.  I wanted to believe things were better than that.

Oh, well.  I'll turn the optimism knob back down to 1 and the cynicism knob all
the way up to 11...

> I understand the motivation to keep ARM64 "fresh and clean", but there must be
> a way to do that without affecting the other architectures negatively.
> 
> Thanks,
> Rafael

Let me think on that; it can be done, I'm sure, but part of the motivation for
these patches was that all architectures should follow the same ACPI rules.  I
was honestly hoping to avoid a per architecture solution.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches
  2015-10-15  0:23   ` Al Stone
@ 2015-10-15  0:37     ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2015-10-15  0:37 UTC (permalink / raw)
  To: Al Stone
  Cc: Rafael J. Wysocki, Al Stone, ACPI Devel Maling List, linaro-acpi,
	Lists linaro-kernel, patches

On Thu, Oct 15, 2015 at 2:23 AM, Al Stone <ahs3@redhat.com> wrote:
> On 10/14/2015 05:44 PM, Rafael J. Wysocki wrote:
>> On Wednesday, October 14, 2015 03:26:21 PM Al Stone wrote:
>>> Once the patch series "Provide better MADT subtable sanity checks" got
>>> into linux-next (commit b9e11e92b9), several existing platforms were found
>>> where the firmware was doing odd things that aren't exactly correct if
>>> the ACPI specification is being followed precisely.  This patch series
>>> relaxes some of the checks on MADT subtables so that these previously
>>> working systems (all x86-based) will continue to boot.  For arm64, since
>>> ACPI usage is still relatively new, the stricter checking is left in place.
>>>
>>> Al Stone (4):
>>>   ACPI: workaround x86 firmware using reserved MADT subtable IDs
>>>   ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions
>>>   ACPI: workaround FADT always being revision 2
>>>   ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16
>>>
>>>  drivers/acpi/tables.c | 62 ++++++++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 51 insertions(+), 11 deletions(-)
>>
>> Honestly, having reviewed this series I'm inclined to drop the original
>> changes from my tree and ask you to start over.
>
> Yeah, I'm leaning that way, too.  This is getting way too messy.
>
>> It seems to have been a mistake to modify the existing behavior for x86 and
>> goodness only knows about ia64.  The changes for these architectures don't make
>> us better off in any way.
>
> Right.  It is truly amazing to me how much kruft this has turned up, when the
> idea was simply to make sure that the tables used contain proper content -- and
> this is only for one table.  I understand why we don't want to break something
> that's already working but I am just sort of amazed at how much improperly
> written firmware is in use.  I wanted to believe things were better than that.

Well, actually, this is quite consistent with all of my previous
experience as disappointing as that may be ...

> Oh, well.  I'll turn the optimism knob back down to 1 and the cynicism knob all
> the way up to 11...
>
>> I understand the motivation to keep ARM64 "fresh and clean", but there must be
>> a way to do that without affecting the other architectures negatively.
>>
>> Thanks,
>> Rafael
>
> Let me think on that; it can be done, I'm sure, but part of the motivation for
> these patches was that all architectures should follow the same ACPI rules.  I
> was honestly hoping to avoid a per architecture solution.

That hasn't worked out well, though.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-10-15  0:37 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-14 21:26 [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Al Stone
2015-10-14 21:26 ` [PATCH 1/4] ACPI: workaround x86 firmware using reserved MADT subtable IDs Al Stone
2015-10-14 23:34   ` Rafael J. Wysocki
2015-10-14 21:26 ` [PATCH 2/4] ACPI: workaround x86 firmware with mis-matched FADT/MADT revisions Al Stone
2015-10-14 23:36   ` Rafael J. Wysocki
2015-10-14 21:26 ` [PATCH 3/4] ACPI: workaround FADT always being revision 2 Al Stone
2015-10-14 23:38   ` Rafael J. Wysocki
2015-10-14 21:26 ` [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes long, not 16 Al Stone
2015-10-14 23:39   ` Rafael J. Wysocki
2015-10-14 23:44 ` [PATCH 0/4] Fix regressions uncovered by bad_madt_entry() patches Rafael J. Wysocki
2015-10-15  0:23   ` Al Stone
2015-10-15  0:37     ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.