[PATCH v2 0/5] ACPI: Provide better MADT subtable sanity checks

[PATCH v2 0/5] ACPI: Provide better MADT subtable sanity checks

[Al Stone]

Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
check on the various subtables that are defined for the MADT.  The check
compares the size of the subtable data structure as defined by ACPICA to
the length entry in the subtable.  If they are not the same, the assumption
is that the subtable is incorrect.

Over time, the ACPI spec has allowed for MADT subtables where this can
never be true (the local SAPIC subtable, for example).  Or, more recently,
the spec has accumulated some minor flaws where there are three possible 
sizes for a subtable, all of which are valid, but only for specific versions
of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
subtables as bad when they are not.  In order to retain some sanity check
on the MADT subtables, we now have to special case these subtables.  Of
necessity, these special cases have ended up in arch-dependent code (arm64)
or an arch has simply decided to forgo the check (ia64).

This patch set replaces the BAD_MADT_ENTRY macro with a function called
bad_madt_entry().  This function uses a data set of details about the
subtables to provide more sanity checking than before:

	-- is the subtable legal for the version given in the FADT?

	-- is the subtable legal for the revision of the MADT in use?

	-- is the subtable of the proper length (including checking
	   on the one variable length subtable that is currently ignored),
	   given the FADT version and the MADT revision?

Further, this patch set adds in the call to bad_madt_entry() from the 
acpi_table_parse_madt() function, allowing it to be used consistently
by all architectures, for all subtables, and removing the need for each
of the subtable traversal callback functions to use BAD_MADT_ENTRY.

In theory, as the ACPI specification changes, we would only have to add
additional information to the data set describing the MADT subtables in
order to continue providing sanity checks, even when new subtables are
added.

These patches have been tested on an APM Mustang (arm64) and are known to
work there.  They have also been cross-compiled for x86 and ia64 with no
known failures.

Changes for v2:
   -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
   -- Correct faulty end of loop test found by Timur Tabi

Al Stone (5):
  ACPI: add in a bad_madt_entry() function to eventually replace the
    macro
  ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
  ACPI / IA64: remove usage of BAD_MADT_ENTRY
  ACPI / X86: remove usage of BAD_MADT_ENTRY
  ACPI: remove definition of BAD_MADT_ENTRY macro

 arch/arm64/include/asm/acpi.h  |   8 --
 arch/arm64/kernel/perf_event.c |   3 -
 arch/arm64/kernel/smp.c        |   2 -
 arch/ia64/kernel/acpi.c        |  20 ----
 arch/x86/kernel/acpi/boot.c    |  27 -----
 drivers/acpi/tables.c          | 241 +++++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic-v2m.c  |   2 -
 drivers/irqchip/irq-gic.c      |   6 -
 include/linux/acpi.h           |   4 -
 9 files changed, 241 insertions(+), 72 deletions(-)

-- 
2.4.3

[PATCH v2 0/5] ACPI: Provide better MADT subtable sanity checks

[Al Stone]

Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
check on the various subtables that are defined for the MADT.  The check
compares the size of the subtable data structure as defined by ACPICA to
the length entry in the subtable.  If they are not the same, the assumption
is that the subtable is incorrect.

Over time, the ACPI spec has allowed for MADT subtables where this can
never be true (the local SAPIC subtable, for example).  Or, more recently,
the spec has accumulated some minor flaws where there are three possible 
sizes for a subtable, all of which are valid, but only for specific versions
of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
subtables as bad when they are not.  In order to retain some sanity check
on the MADT subtables, we now have to special case these subtables.  Of
necessity, these special cases have ended up in arch-dependent code (arm64)
or an arch has simply decided to forgo the check (ia64).

This patch set replaces the BAD_MADT_ENTRY macro with a function called
bad_madt_entry().  This function uses a data set of details about the
subtables to provide more sanity checking than before:

	-- is the subtable legal for the version given in the FADT?

	-- is the subtable legal for the revision of the MADT in use?

	-- is the subtable of the proper length (including checking
	   on the one variable length subtable that is currently ignored),
	   given the FADT version and the MADT revision?

Further, this patch set adds in the call to bad_madt_entry() from the 
acpi_table_parse_madt() function, allowing it to be used consistently
by all architectures, for all subtables, and removing the need for each
of the subtable traversal callback functions to use BAD_MADT_ENTRY.

In theory, as the ACPI specification changes, we would only have to add
additional information to the data set describing the MADT subtables in
order to continue providing sanity checks, even when new subtables are
added.

These patches have been tested on an APM Mustang (arm64) and are known to
work there.  They have also been cross-compiled for x86 and ia64 with no
known failures.

Changes for v2:
   -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
   -- Correct faulty end of loop test found by Timur Tabi

Al Stone (5):
  ACPI: add in a bad_madt_entry() function to eventually replace the
    macro
  ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
  ACPI / IA64: remove usage of BAD_MADT_ENTRY
  ACPI / X86: remove usage of BAD_MADT_ENTRY
  ACPI: remove definition of BAD_MADT_ENTRY macro

 arch/arm64/include/asm/acpi.h  |   8 --
 arch/arm64/kernel/perf_event.c |   3 -
 arch/arm64/kernel/smp.c        |   2 -
 arch/ia64/kernel/acpi.c        |  20 ----
 arch/x86/kernel/acpi/boot.c    |  27 -----
 drivers/acpi/tables.c          | 241 +++++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic-v2m.c  |   2 -
 drivers/irqchip/irq-gic.c      |   6 -
 include/linux/acpi.h           |   4 -
 9 files changed, 241 insertions(+), 72 deletions(-)

-- 
2.4.3

[PATCH v2 0/5] ACPI: Provide better MADT subtable sanity checks

[Al Stone]

Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
check on the various subtables that are defined for the MADT.  The check
compares the size of the subtable data structure as defined by ACPICA to
the length entry in the subtable.  If they are not the same, the assumption
is that the subtable is incorrect.

Over time, the ACPI spec has allowed for MADT subtables where this can
never be true (the local SAPIC subtable, for example).  Or, more recently,
the spec has accumulated some minor flaws where there are three possible 
sizes for a subtable, all of which are valid, but only for specific versions
of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
subtables as bad when they are not.  In order to retain some sanity check
on the MADT subtables, we now have to special case these subtables.  Of
necessity, these special cases have ended up in arch-dependent code (arm64)
or an arch has simply decided to forgo the check (ia64).

This patch set replaces the BAD_MADT_ENTRY macro with a function called
bad_madt_entry().  This function uses a data set of details about the
subtables to provide more sanity checking than before:

	-- is the subtable legal for the version given in the FADT?

	-- is the subtable legal for the revision of the MADT in use?

	-- is the subtable of the proper length (including checking
	   on the one variable length subtable that is currently ignored),
	   given the FADT version and the MADT revision?

Further, this patch set adds in the call to bad_madt_entry() from the 
acpi_table_parse_madt() function, allowing it to be used consistently
by all architectures, for all subtables, and removing the need for each
of the subtable traversal callback functions to use BAD_MADT_ENTRY.

In theory, as the ACPI specification changes, we would only have to add
additional information to the data set describing the MADT subtables in
order to continue providing sanity checks, even when new subtables are
added.

These patches have been tested on an APM Mustang (arm64) and are known to
work there.  They have also been cross-compiled for x86 and ia64 with no
known failures.

Changes for v2:
   -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
   -- Correct faulty end of loop test found by Timur Tabi

Al Stone (5):
  ACPI: add in a bad_madt_entry() function to eventually replace the
    macro
  ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
  ACPI / IA64: remove usage of BAD_MADT_ENTRY
  ACPI / X86: remove usage of BAD_MADT_ENTRY
  ACPI: remove definition of BAD_MADT_ENTRY macro

 arch/arm64/include/asm/acpi.h  |   8 --
 arch/arm64/kernel/perf_event.c |   3 -
 arch/arm64/kernel/smp.c        |   2 -
 arch/ia64/kernel/acpi.c        |  20 ----
 arch/x86/kernel/acpi/boot.c    |  27 -----
 drivers/acpi/tables.c          | 241 +++++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic-v2m.c  |   2 -
 drivers/irqchip/irq-gic.c      |   6 -
 include/linux/acpi.h           |   4 -
 9 files changed, 241 insertions(+), 72 deletions(-)

-- 
2.4.3

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

The existing BAD_MADT_ENTRY macro only checks that the size of the data
structure for an MADT subtable matches the length entry in the subtable.
This is, unfortunately, not reliable.  Nor, as it turns out, does it have
anything to do with what the length should be in any particular table.

We introduce the bad_madt_entry() function that uses a data set to
do some basic sanity checks on any given MADT subtable.  Over time, as
the spec changes, we should just be able to add entries to the data set
to reflect the changes.

What the data set captures is the allowed MADT subtable length for each
type of subtable, for each revision of the specification.  While there
is a revision number in the MADT that we should be able to use to figure
out the proper subtable length, it was not changed when subtables did.
And, while there is a major and minor revision in the FADT that could
also help, it was not always changed as the subtables changed either.
So, the data set captures for each published version of the ACPI spec
what the FADT revisions numbers should be, the corresponding MADT
revision number, and the subtable types and lengths that were defined
at that time.

The sanity checks done are:
	-- is the length non-zero?
	-- is the subtable type defined/allowed for the revision of
	   the FADT we're using?
	-- is the subtable type defined/allowed for the revision of
	   the MADT we're using?
	-- is the length entry what it should be for this revision
	   of the MADT and FADT?

These checks are more thorough than the previous macro provided, and
are now insulated from data structure size changes by ACPICA, which
have been the source of other patches in the past.

Now that the bad_madt_entry() function is available, we add code to
also invoke it before any subtable handlers are called to use the
info in the subtable.  Subsequent patches will remove the use of the
BAD_MADT_ENTRY macro which is now redundant as a result.  Any ACPI
functions that use acpi_parse_madt_entries() will always have all of
the MADT subtables checked from now on.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 241 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 17a6fa0..d1c0efc 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -210,6 +210,245 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 	}
 }
 
+/*
+ * The Long, Sad, True Story of the MADT
+ *    or
+ * What does bad_madt_entry() actually do?
+ *
+ * Once upon a time in ACPI 1.0, there was the MADT.  It was a nice table,
+ * and it had two subtables all of its own.  But, it was also a pretty
+ * busy table, too, so over time the MADT gathered up other nice little
+ * subtables.  By the time ACPI 6.0 came around, the MADT had 16 of the
+ * little guys.
+ *
+ * Now, the MADT kept a little counter around for the subtables.  In fact,
+ * it kept two counters: one was the revision level, which was supposed to
+ * change when new subtables came to be, or as the ones already around grew
+ * up. The second counter was a type number, because the MADT needed a unique
+ * type for each subtable so he could tell them apart.  But, sometimes the
+ * MADT got so busy, he forgot to increment the revision level when he needed
+ * to.  Fortunately, the type counter kept increasing since that's the only
+ * way the MADT could find each little subtable.  It just wouldn't do to have
+ * every subtable called Number 6.
+ *
+ * In the next valley over, a castle full of wizards was watching the MADT
+ * and made a pact to keep their own counter.  Every time the MADT found a
+ * new subtable, or a subtable grew up, the wizards promised they would
+ * increment their counter.  Well, wizards being the forgetful sort, they
+ * didn't alway do that.  And, since there quite a lot of them, they
+ * couldn't always remember who was supposed to keep track of the MADT,
+ * especially if dinner was coming up soon.  Their counter was called the
+ * spec version.
+ *
+ * Every now and then, the MADT would gather up all its little subtables
+ * and take them in to the cobbler to get new boots.  This was a very, very
+ * meticulous cobbler, so every time they came, he wrote down all the boot
+ * sizes for all of the little subtables.  The cobbler would ask each subtable
+ * for its length, check that against his careful notes, and then go get the
+ * right boots.  Sometimes, a little subtable would change a bit, and their
+ * length did not match what the cobbler had written down.  If the wizards
+ * or the MADT had incremented their counters, the cobbler would breath a
+ * sigh of relief and write down the new length as the right one.  But, if
+ * none of the counters had changed, this would make the cobbler very, very
+ * mad.  He couldn't tell if he had the right size boots or not for the
+ * little subtable.  He would have to *guess* and this really bugged him.
+ *
+ * Well, when the cobbler got mad like this, he would go into hiding.  He
+ * would not make or sell any boots.  He would not go out at all.  Pretty
+ * soon, the coffee shop would have to close because the cobbler wasn't
+ * coming by twice a day any more.  Then the grocery store would have to
+ * close because he wouldn't eat much.  After a while, everyone would panic
+ * and have to move from the village and go live with all their relatives
+ * (usually the ones they didn't like very much).
+ *
+ * Eventually, the cobbler would work his way out of his bad mood, and
+ * open up his boot business again.  Then, everyone else could move back
+ * to the village and restart their lives, too.
+ *
+ * Fortunately, we have been able to collect up all the cobbler's careful
+ * notes (and we wrote them down below).  We'll have to keep checking these
+ * notes over time, too, just as the cobbler does.  But, in the meantime,
+ * we can avoid the panic and the reboot since we can make sure that each
+ * subtable is doing okay.  And that's what bad_madt_entry() does.
+ *
+ *
+ * FADT Major Version ->       1    3    4     4     5     5     6
+ * FADT Minor Version ->       x    x    x     x     x     1     0
+ * MADT revision ->            1    1    2     3     3     3     3
+ * Spec Version ->            1.0  2.0  3.0b  4.0a  5.0b  5.1a  6.0
+ * Subtable Name	Type  Expected Length ->
+ * Processor Local APIC  0x0    8    8    8     8     8     8     8
+ * IO APIC               0x1   12   12   12    12    12    12    12
+ * Int Src Override      0x2        10   10    10    10    10    10
+ * NMI Src               0x3         8    8     8     8     8     8
+ * Local APIC NMI Struct 0x4         6    6     6     6     6     6
+ * Local APIC Addr Ovrrd 0x5        16   12    12    12    12    12
+ * IO SAPIC              0x6        20   16    16    16    16    16
+ * Local SAPIC           0x7         8  >16   >16   >16   >16   >16
+ * Platform Int Src      0x8        16   16    16    16    16    16
+ * Proc Local x2APIC     0x9                   16    16    16    16
+ * Local x2APIC NMI      0xa                   12    12    12    12
+ * GICC CPU I/F          0xb                         40    76    80
+ * GICD                  0xc                         24    24    24
+ * GICv2m MSI            0xd                               24    24
+ * GICR                  0xe                               16    16
+ * GIC ITS               0xf                                     16
+ *
+ * In the table, each length entry is what should be in the length
+ * field of the subtable, and -- in general -- it should match the
+ * size of the struct for the subtable.  Any value that is not set
+ * (i.e., is zero) indicates that the subtable is not defined for
+ * that version of the ACPI spec.
+ *
+ */
+#define SUBTABLE_UNDEFINED	0x00
+#define SUBTABLE_VARIABLE	0xff
+#define NUM_SUBTABLE_TYPES	16
+
+struct acpi_madt_subtable_lengths {
+	unsigned short major_version;	/* from revision in FADT header */
+	unsigned short minor_version;	/* FADT field starting with 5.1 */
+	unsigned short madt_version;	/* MADT revision */
+	unsigned short num_types;	/* types possible for this version */
+	unsigned short lengths[NUM_SUBTABLE_TYPES];
+					/* subtable lengths, indexed by type */
+};
+
+static struct acpi_madt_subtable_lengths spec_info[] = {
+	{ /* for ACPI 1.0 */
+		.major_version = 1,
+		.minor_version = 0,
+		.madt_version = 1,
+		.num_types = 2,
+		.lengths = { 8, 12 }
+	},
+	{ /* for ACPI 2.0 */
+		.major_version = 3,
+		.minor_version = 0,
+		.madt_version = 1,
+		.num_types = 9,
+		.lengths = { 8, 12, 10, 8, 6, 16, 20, 8, 16 }
+	},
+	{ /* for ACPI 3.0b */
+		.major_version = 4,
+		.minor_version = 0,
+		.madt_version = 2,
+		.num_types = 9,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE, 16 }
+	},
+	{ /* for ACPI 4.0a */
+		.major_version = 4,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 11,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12 }
+	},
+	{ /* for ACPI 5.0b */
+		.major_version = 5,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 13,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 40, 24 }
+	},
+	{ /* for ACPI 5.1a */
+		.major_version = 5,
+		.minor_version = 1,
+		.madt_version = 3,
+		.num_types = 15,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 76, 24, 24, 16 }
+	},
+	{ /* for ACPI 6.0 */
+		.major_version = 6,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 16,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 80, 24, 24, 16, 16 }
+	},
+	{ /* terminator */
+		.major_version = 0,
+		.minor_version = 0,
+		.madt_version = 0,
+		.num_types = 0,
+		.lengths = { 0 }
+	}
+};
+
+int __init bad_madt_entry(struct acpi_table_header *table,
+			  struct acpi_subtable_header *entry)
+{
+	struct acpi_madt_subtable_lengths *ms;
+	struct acpi_table_madt *madt;
+	unsigned short major;
+	unsigned short minor;
+	unsigned short len;
+
+	/* simple sanity checking on MADT subtable entries */
+	if (!entry || !table)
+		return 1;
+
+	/* FADT minor numbers were not introduced until ACPI 5.1 */
+	major = acpi_gbl_FADT.header.revision;
+	if (major >= 5 && acpi_gbl_FADT.header.length >= 268)
+		minor = acpi_gbl_FADT.minor_revision;
+	else
+		minor = 0;
+
+	madt = (struct acpi_table_madt *)table;
+	ms = spec_info;
+	while (ms->num_types != 0) {
+		if (ms->major_version == major &&
+		    ms->minor_version == minor &&
+		    ms->madt_version == madt->header.revision)
+			break;
+		ms++;
+	}
+	if (!ms->num_types) {
+		pr_err("undefined FADT version: %d.%d\n", major, minor);
+		return 1;
+	}
+
+	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;
+	}
+
+	/* 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;
+	}
+
+	/* verify that the length is what we expect */
+	if (len == SUBTABLE_VARIABLE) {
+		if (entry->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
+			struct acpi_madt_local_sapic *lsapic =
+				(struct acpi_madt_local_sapic *)entry;
+
+			if (sizeof(struct acpi_madt_local_sapic) +
+			    strlen(lsapic->uid_string) + 1 != entry->length) {
+				pr_err("Variable length MADT subtable %d is wrong size: %d\n",
+				       entry->type, entry->length);
+				return 1;
+			}
+		}
+	} else {
+		if (entry->length != len) {
+			pr_err("MADT subtable %d is wrong size: %d\n",
+			       len, entry->type);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 int __init
 acpi_parse_entries(char *id, unsigned long table_size,
 		acpi_tbl_entry_handler handler,
@@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
 	       table_end) {
 		if (entry->type == entry_id
 		    && (!max_entries || count < max_entries)) {
+			if (bad_madt_entry(table_header, entry))
+				return -EINVAL;
 			if (handler(entry, table_end))
 				return -EINVAL;
 
-- 
2.4.3

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

The existing BAD_MADT_ENTRY macro only checks that the size of the data
structure for an MADT subtable matches the length entry in the subtable.
This is, unfortunately, not reliable.  Nor, as it turns out, does it have
anything to do with what the length should be in any particular table.

We introduce the bad_madt_entry() function that uses a data set to
do some basic sanity checks on any given MADT subtable.  Over time, as
the spec changes, we should just be able to add entries to the data set
to reflect the changes.

What the data set captures is the allowed MADT subtable length for each
type of subtable, for each revision of the specification.  While there
is a revision number in the MADT that we should be able to use to figure
out the proper subtable length, it was not changed when subtables did.
And, while there is a major and minor revision in the FADT that could
also help, it was not always changed as the subtables changed either.
So, the data set captures for each published version of the ACPI spec
what the FADT revisions numbers should be, the corresponding MADT
revision number, and the subtable types and lengths that were defined
at that time.

The sanity checks done are:
	-- is the length non-zero?
	-- is the subtable type defined/allowed for the revision of
	   the FADT we're using?
	-- is the subtable type defined/allowed for the revision of
	   the MADT we're using?
	-- is the length entry what it should be for this revision
	   of the MADT and FADT?

These checks are more thorough than the previous macro provided, and
are now insulated from data structure size changes by ACPICA, which
have been the source of other patches in the past.

Now that the bad_madt_entry() function is available, we add code to
also invoke it before any subtable handlers are called to use the
info in the subtable.  Subsequent patches will remove the use of the
BAD_MADT_ENTRY macro which is now redundant as a result.  Any ACPI
functions that use acpi_parse_madt_entries() will always have all of
the MADT subtables checked from now on.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 241 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 17a6fa0..d1c0efc 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -210,6 +210,245 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 	}
 }
 
+/*
+ * The Long, Sad, True Story of the MADT
+ *    or
+ * What does bad_madt_entry() actually do?
+ *
+ * Once upon a time in ACPI 1.0, there was the MADT.  It was a nice table,
+ * and it had two subtables all of its own.  But, it was also a pretty
+ * busy table, too, so over time the MADT gathered up other nice little
+ * subtables.  By the time ACPI 6.0 came around, the MADT had 16 of the
+ * little guys.
+ *
+ * Now, the MADT kept a little counter around for the subtables.  In fact,
+ * it kept two counters: one was the revision level, which was supposed to
+ * change when new subtables came to be, or as the ones already around grew
+ * up. The second counter was a type number, because the MADT needed a unique
+ * type for each subtable so he could tell them apart.  But, sometimes the
+ * MADT got so busy, he forgot to increment the revision level when he needed
+ * to.  Fortunately, the type counter kept increasing since that's the only
+ * way the MADT could find each little subtable.  It just wouldn't do to have
+ * every subtable called Number 6.
+ *
+ * In the next valley over, a castle full of wizards was watching the MADT
+ * and made a pact to keep their own counter.  Every time the MADT found a
+ * new subtable, or a subtable grew up, the wizards promised they would
+ * increment their counter.  Well, wizards being the forgetful sort, they
+ * didn't alway do that.  And, since there quite a lot of them, they
+ * couldn't always remember who was supposed to keep track of the MADT,
+ * especially if dinner was coming up soon.  Their counter was called the
+ * spec version.
+ *
+ * Every now and then, the MADT would gather up all its little subtables
+ * and take them in to the cobbler to get new boots.  This was a very, very
+ * meticulous cobbler, so every time they came, he wrote down all the boot
+ * sizes for all of the little subtables.  The cobbler would ask each subtable
+ * for its length, check that against his careful notes, and then go get the
+ * right boots.  Sometimes, a little subtable would change a bit, and their
+ * length did not match what the cobbler had written down.  If the wizards
+ * or the MADT had incremented their counters, the cobbler would breath a
+ * sigh of relief and write down the new length as the right one.  But, if
+ * none of the counters had changed, this would make the cobbler very, very
+ * mad.  He couldn't tell if he had the right size boots or not for the
+ * little subtable.  He would have to *guess* and this really bugged him.
+ *
+ * Well, when the cobbler got mad like this, he would go into hiding.  He
+ * would not make or sell any boots.  He would not go out at all.  Pretty
+ * soon, the coffee shop would have to close because the cobbler wasn't
+ * coming by twice a day any more.  Then the grocery store would have to
+ * close because he wouldn't eat much.  After a while, everyone would panic
+ * and have to move from the village and go live with all their relatives
+ * (usually the ones they didn't like very much).
+ *
+ * Eventually, the cobbler would work his way out of his bad mood, and
+ * open up his boot business again.  Then, everyone else could move back
+ * to the village and restart their lives, too.
+ *
+ * Fortunately, we have been able to collect up all the cobbler's careful
+ * notes (and we wrote them down below).  We'll have to keep checking these
+ * notes over time, too, just as the cobbler does.  But, in the meantime,
+ * we can avoid the panic and the reboot since we can make sure that each
+ * subtable is doing okay.  And that's what bad_madt_entry() does.
+ *
+ *
+ * FADT Major Version ->       1    3    4     4     5     5     6
+ * FADT Minor Version ->       x    x    x     x     x     1     0
+ * MADT revision ->            1    1    2     3     3     3     3
+ * Spec Version ->            1.0  2.0  3.0b  4.0a  5.0b  5.1a  6.0
+ * Subtable Name	Type  Expected Length ->
+ * Processor Local APIC  0x0    8    8    8     8     8     8     8
+ * IO APIC               0x1   12   12   12    12    12    12    12
+ * Int Src Override      0x2        10   10    10    10    10    10
+ * NMI Src               0x3         8    8     8     8     8     8
+ * Local APIC NMI Struct 0x4         6    6     6     6     6     6
+ * Local APIC Addr Ovrrd 0x5        16   12    12    12    12    12
+ * IO SAPIC              0x6        20   16    16    16    16    16
+ * Local SAPIC           0x7         8  >16   >16   >16   >16   >16
+ * Platform Int Src      0x8        16   16    16    16    16    16
+ * Proc Local x2APIC     0x9                   16    16    16    16
+ * Local x2APIC NMI      0xa                   12    12    12    12
+ * GICC CPU I/F          0xb                         40    76    80
+ * GICD                  0xc                         24    24    24
+ * GICv2m MSI            0xd                               24    24
+ * GICR                  0xe                               16    16
+ * GIC ITS               0xf                                     16
+ *
+ * In the table, each length entry is what should be in the length
+ * field of the subtable, and -- in general -- it should match the
+ * size of the struct for the subtable.  Any value that is not set
+ * (i.e., is zero) indicates that the subtable is not defined for
+ * that version of the ACPI spec.
+ *
+ */
+#define SUBTABLE_UNDEFINED	0x00
+#define SUBTABLE_VARIABLE	0xff
+#define NUM_SUBTABLE_TYPES	16
+
+struct acpi_madt_subtable_lengths {
+	unsigned short major_version;	/* from revision in FADT header */
+	unsigned short minor_version;	/* FADT field starting with 5.1 */
+	unsigned short madt_version;	/* MADT revision */
+	unsigned short num_types;	/* types possible for this version */
+	unsigned short lengths[NUM_SUBTABLE_TYPES];
+					/* subtable lengths, indexed by type */
+};
+
+static struct acpi_madt_subtable_lengths spec_info[] = {
+	{ /* for ACPI 1.0 */
+		.major_version = 1,
+		.minor_version = 0,
+		.madt_version = 1,
+		.num_types = 2,
+		.lengths = { 8, 12 }
+	},
+	{ /* for ACPI 2.0 */
+		.major_version = 3,
+		.minor_version = 0,
+		.madt_version = 1,
+		.num_types = 9,
+		.lengths = { 8, 12, 10, 8, 6, 16, 20, 8, 16 }
+	},
+	{ /* for ACPI 3.0b */
+		.major_version = 4,
+		.minor_version = 0,
+		.madt_version = 2,
+		.num_types = 9,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE, 16 }
+	},
+	{ /* for ACPI 4.0a */
+		.major_version = 4,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 11,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12 }
+	},
+	{ /* for ACPI 5.0b */
+		.major_version = 5,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 13,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 40, 24 }
+	},
+	{ /* for ACPI 5.1a */
+		.major_version = 5,
+		.minor_version = 1,
+		.madt_version = 3,
+		.num_types = 15,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 76, 24, 24, 16 }
+	},
+	{ /* for ACPI 6.0 */
+		.major_version = 6,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 16,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 80, 24, 24, 16, 16 }
+	},
+	{ /* terminator */
+		.major_version = 0,
+		.minor_version = 0,
+		.madt_version = 0,
+		.num_types = 0,
+		.lengths = { 0 }
+	}
+};
+
+int __init bad_madt_entry(struct acpi_table_header *table,
+			  struct acpi_subtable_header *entry)
+{
+	struct acpi_madt_subtable_lengths *ms;
+	struct acpi_table_madt *madt;
+	unsigned short major;
+	unsigned short minor;
+	unsigned short len;
+
+	/* simple sanity checking on MADT subtable entries */
+	if (!entry || !table)
+		return 1;
+
+	/* FADT minor numbers were not introduced until ACPI 5.1 */
+	major = acpi_gbl_FADT.header.revision;
+	if (major >= 5 && acpi_gbl_FADT.header.length >= 268)
+		minor = acpi_gbl_FADT.minor_revision;
+	else
+		minor = 0;
+
+	madt = (struct acpi_table_madt *)table;
+	ms = spec_info;
+	while (ms->num_types != 0) {
+		if (ms->major_version == major &&
+		    ms->minor_version == minor &&
+		    ms->madt_version == madt->header.revision)
+			break;
+		ms++;
+	}
+	if (!ms->num_types) {
+		pr_err("undefined FADT version: %d.%d\n", major, minor);
+		return 1;
+	}
+
+	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;
+	}
+
+	/* 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;
+	}
+
+	/* verify that the length is what we expect */
+	if (len == SUBTABLE_VARIABLE) {
+		if (entry->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
+			struct acpi_madt_local_sapic *lsapic =
+				(struct acpi_madt_local_sapic *)entry;
+
+			if (sizeof(struct acpi_madt_local_sapic) +
+			    strlen(lsapic->uid_string) + 1 != entry->length) {
+				pr_err("Variable length MADT subtable %d is wrong size: %d\n",
+				       entry->type, entry->length);
+				return 1;
+			}
+		}
+	} else {
+		if (entry->length != len) {
+			pr_err("MADT subtable %d is wrong size: %d\n",
+			       len, entry->type);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 int __init
 acpi_parse_entries(char *id, unsigned long table_size,
 		acpi_tbl_entry_handler handler,
@@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
 	       table_end) {
 		if (entry->type == entry_id
 		    && (!max_entries || count < max_entries)) {
+			if (bad_madt_entry(table_header, entry))
+				return -EINVAL;
 			if (handler(entry, table_end))
 				return -EINVAL;
 
-- 
2.4.3

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

The existing BAD_MADT_ENTRY macro only checks that the size of the data
structure for an MADT subtable matches the length entry in the subtable.
This is, unfortunately, not reliable.  Nor, as it turns out, does it have
anything to do with what the length should be in any particular table.

We introduce the bad_madt_entry() function that uses a data set to
do some basic sanity checks on any given MADT subtable.  Over time, as
the spec changes, we should just be able to add entries to the data set
to reflect the changes.

What the data set captures is the allowed MADT subtable length for each
type of subtable, for each revision of the specification.  While there
is a revision number in the MADT that we should be able to use to figure
out the proper subtable length, it was not changed when subtables did.
And, while there is a major and minor revision in the FADT that could
also help, it was not always changed as the subtables changed either.
So, the data set captures for each published version of the ACPI spec
what the FADT revisions numbers should be, the corresponding MADT
revision number, and the subtable types and lengths that were defined
at that time.

The sanity checks done are:
	-- is the length non-zero?
	-- is the subtable type defined/allowed for the revision of
	   the FADT we're using?
	-- is the subtable type defined/allowed for the revision of
	   the MADT we're using?
	-- is the length entry what it should be for this revision
	   of the MADT and FADT?

These checks are more thorough than the previous macro provided, and
are now insulated from data structure size changes by ACPICA, which
have been the source of other patches in the past.

Now that the bad_madt_entry() function is available, we add code to
also invoke it before any subtable handlers are called to use the
info in the subtable.  Subsequent patches will remove the use of the
BAD_MADT_ENTRY macro which is now redundant as a result.  Any ACPI
functions that use acpi_parse_madt_entries() will always have all of
the MADT subtables checked from now on.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 241 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 17a6fa0..d1c0efc 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -210,6 +210,245 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 	}
 }
 
+/*
+ * The Long, Sad, True Story of the MADT
+ *    or
+ * What does bad_madt_entry() actually do?
+ *
+ * Once upon a time in ACPI 1.0, there was the MADT.  It was a nice table,
+ * and it had two subtables all of its own.  But, it was also a pretty
+ * busy table, too, so over time the MADT gathered up other nice little
+ * subtables.  By the time ACPI 6.0 came around, the MADT had 16 of the
+ * little guys.
+ *
+ * Now, the MADT kept a little counter around for the subtables.  In fact,
+ * it kept two counters: one was the revision level, which was supposed to
+ * change when new subtables came to be, or as the ones already around grew
+ * up. The second counter was a type number, because the MADT needed a unique
+ * type for each subtable so he could tell them apart.  But, sometimes the
+ * MADT got so busy, he forgot to increment the revision level when he needed
+ * to.  Fortunately, the type counter kept increasing since that's the only
+ * way the MADT could find each little subtable.  It just wouldn't do to have
+ * every subtable called Number 6.
+ *
+ * In the next valley over, a castle full of wizards was watching the MADT
+ * and made a pact to keep their own counter.  Every time the MADT found a
+ * new subtable, or a subtable grew up, the wizards promised they would
+ * increment their counter.  Well, wizards being the forgetful sort, they
+ * didn't alway do that.  And, since there quite a lot of them, they
+ * couldn't always remember who was supposed to keep track of the MADT,
+ * especially if dinner was coming up soon.  Their counter was called the
+ * spec version.
+ *
+ * Every now and then, the MADT would gather up all its little subtables
+ * and take them in to the cobbler to get new boots.  This was a very, very
+ * meticulous cobbler, so every time they came, he wrote down all the boot
+ * sizes for all of the little subtables.  The cobbler would ask each subtable
+ * for its length, check that against his careful notes, and then go get the
+ * right boots.  Sometimes, a little subtable would change a bit, and their
+ * length did not match what the cobbler had written down.  If the wizards
+ * or the MADT had incremented their counters, the cobbler would breath a
+ * sigh of relief and write down the new length as the right one.  But, if
+ * none of the counters had changed, this would make the cobbler very, very
+ * mad.  He couldn't tell if he had the right size boots or not for the
+ * little subtable.  He would have to *guess* and this really bugged him.
+ *
+ * Well, when the cobbler got mad like this, he would go into hiding.  He
+ * would not make or sell any boots.  He would not go out at all.  Pretty
+ * soon, the coffee shop would have to close because the cobbler wasn't
+ * coming by twice a day any more.  Then the grocery store would have to
+ * close because he wouldn't eat much.  After a while, everyone would panic
+ * and have to move from the village and go live with all their relatives
+ * (usually the ones they didn't like very much).
+ *
+ * Eventually, the cobbler would work his way out of his bad mood, and
+ * open up his boot business again.  Then, everyone else could move back
+ * to the village and restart their lives, too.
+ *
+ * Fortunately, we have been able to collect up all the cobbler's careful
+ * notes (and we wrote them down below).  We'll have to keep checking these
+ * notes over time, too, just as the cobbler does.  But, in the meantime,
+ * we can avoid the panic and the reboot since we can make sure that each
+ * subtable is doing okay.  And that's what bad_madt_entry() does.
+ *
+ *
+ * FADT Major Version ->       1    3    4     4     5     5     6
+ * FADT Minor Version ->       x    x    x     x     x     1     0
+ * MADT revision ->            1    1    2     3     3     3     3
+ * Spec Version ->            1.0  2.0  3.0b  4.0a  5.0b  5.1a  6.0
+ * Subtable Name	Type  Expected Length ->
+ * Processor Local APIC  0x0    8    8    8     8     8     8     8
+ * IO APIC               0x1   12   12   12    12    12    12    12
+ * Int Src Override      0x2        10   10    10    10    10    10
+ * NMI Src               0x3         8    8     8     8     8     8
+ * Local APIC NMI Struct 0x4         6    6     6     6     6     6
+ * Local APIC Addr Ovrrd 0x5        16   12    12    12    12    12
+ * IO SAPIC              0x6        20   16    16    16    16    16
+ * Local SAPIC           0x7         8  >16   >16   >16   >16   >16
+ * Platform Int Src      0x8        16   16    16    16    16    16
+ * Proc Local x2APIC     0x9                   16    16    16    16
+ * Local x2APIC NMI      0xa                   12    12    12    12
+ * GICC CPU I/F          0xb                         40    76    80
+ * GICD                  0xc                         24    24    24
+ * GICv2m MSI            0xd                               24    24
+ * GICR                  0xe                               16    16
+ * GIC ITS               0xf                                     16
+ *
+ * In the table, each length entry is what should be in the length
+ * field of the subtable, and -- in general -- it should match the
+ * size of the struct for the subtable.  Any value that is not set
+ * (i.e., is zero) indicates that the subtable is not defined for
+ * that version of the ACPI spec.
+ *
+ */
+#define SUBTABLE_UNDEFINED	0x00
+#define SUBTABLE_VARIABLE	0xff
+#define NUM_SUBTABLE_TYPES	16
+
+struct acpi_madt_subtable_lengths {
+	unsigned short major_version;	/* from revision in FADT header */
+	unsigned short minor_version;	/* FADT field starting with 5.1 */
+	unsigned short madt_version;	/* MADT revision */
+	unsigned short num_types;	/* types possible for this version */
+	unsigned short lengths[NUM_SUBTABLE_TYPES];
+					/* subtable lengths, indexed by type */
+};
+
+static struct acpi_madt_subtable_lengths spec_info[] = {
+	{ /* for ACPI 1.0 */
+		.major_version = 1,
+		.minor_version = 0,
+		.madt_version = 1,
+		.num_types = 2,
+		.lengths = { 8, 12 }
+	},
+	{ /* for ACPI 2.0 */
+		.major_version = 3,
+		.minor_version = 0,
+		.madt_version = 1,
+		.num_types = 9,
+		.lengths = { 8, 12, 10, 8, 6, 16, 20, 8, 16 }
+	},
+	{ /* for ACPI 3.0b */
+		.major_version = 4,
+		.minor_version = 0,
+		.madt_version = 2,
+		.num_types = 9,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE, 16 }
+	},
+	{ /* for ACPI 4.0a */
+		.major_version = 4,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 11,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12 }
+	},
+	{ /* for ACPI 5.0b */
+		.major_version = 5,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 13,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 40, 24 }
+	},
+	{ /* for ACPI 5.1a */
+		.major_version = 5,
+		.minor_version = 1,
+		.madt_version = 3,
+		.num_types = 15,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 76, 24, 24, 16 }
+	},
+	{ /* for ACPI 6.0 */
+		.major_version = 6,
+		.minor_version = 0,
+		.madt_version = 3,
+		.num_types = 16,
+		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
+			     16, 16, 12, 80, 24, 24, 16, 16 }
+	},
+	{ /* terminator */
+		.major_version = 0,
+		.minor_version = 0,
+		.madt_version = 0,
+		.num_types = 0,
+		.lengths = { 0 }
+	}
+};
+
+int __init bad_madt_entry(struct acpi_table_header *table,
+			  struct acpi_subtable_header *entry)
+{
+	struct acpi_madt_subtable_lengths *ms;
+	struct acpi_table_madt *madt;
+	unsigned short major;
+	unsigned short minor;
+	unsigned short len;
+
+	/* simple sanity checking on MADT subtable entries */
+	if (!entry || !table)
+		return 1;
+
+	/* FADT minor numbers were not introduced until ACPI 5.1 */
+	major = acpi_gbl_FADT.header.revision;
+	if (major >= 5 && acpi_gbl_FADT.header.length >= 268)
+		minor = acpi_gbl_FADT.minor_revision;
+	else
+		minor = 0;
+
+	madt = (struct acpi_table_madt *)table;
+	ms = spec_info;
+	while (ms->num_types != 0) {
+		if (ms->major_version = major &&
+		    ms->minor_version = minor &&
+		    ms->madt_version = madt->header.revision)
+			break;
+		ms++;
+	}
+	if (!ms->num_types) {
+		pr_err("undefined FADT version: %d.%d\n", major, minor);
+		return 1;
+	}
+
+	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;
+	}
+
+	/* 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;
+	}
+
+	/* verify that the length is what we expect */
+	if (len = SUBTABLE_VARIABLE) {
+		if (entry->type = ACPI_MADT_TYPE_LOCAL_SAPIC) {
+			struct acpi_madt_local_sapic *lsapic +				(struct acpi_madt_local_sapic *)entry;
+
+			if (sizeof(struct acpi_madt_local_sapic) +
+			    strlen(lsapic->uid_string) + 1 != entry->length) {
+				pr_err("Variable length MADT subtable %d is wrong size: %d\n",
+				       entry->type, entry->length);
+				return 1;
+			}
+		}
+	} else {
+		if (entry->length != len) {
+			pr_err("MADT subtable %d is wrong size: %d\n",
+			       len, entry->type);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 int __init
 acpi_parse_entries(char *id, unsigned long table_size,
 		acpi_tbl_entry_handler handler,
@@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
 	       table_end) {
 		if (entry->type = entry_id
 		    && (!max_entries || count < max_entries)) {
+			if (bad_madt_entry(table_header, entry))
+				return -EINVAL;
 			if (handler(entry, table_end))
 				return -EINVAL;
 
-- 
2.4.3

[PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
of arm64, the BAD_MADT_GICC_ENTRY, too.

Signed-off-by: Al Stone <al.stone@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm64/include/asm/acpi.h | 8 --------
 arch/arm64/kernel/smp.c       | 2 --
 drivers/irqchip/irq-gic.c     | 6 ------
 3 files changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 208cec0..ed7e212 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,14 +19,6 @@
 #include <asm/cputype.h>
 #include <asm/smp_plat.h>
 
-/* Macros for consistency checks of the GICC subtable of MADT */
-#define ACPI_MADT_GICC_LENGTH	\
-	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
-
-#define BAD_MADT_GICC_ENTRY(entry, end)						\
-	(!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||	\
-	 (entry)->header.length != ACPI_MADT_GICC_LENGTH)
-
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index dbdaacd..66cc8c4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -451,8 +451,6 @@ 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_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 aa3e7b8..848c44c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1064,9 +1064,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
-	if (BAD_MADT_GICC_ENTRY(processor, end))
-		return -EINVAL;
-
 	/*
 	 * There is no support for non-banked GICv1/2 register in ACPI spec.
 	 * All CPU interface addresses have to be the same.
@@ -1088,9 +1085,6 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
 
 	dist = (struct acpi_madt_generic_distributor *)header;
 
-	if (BAD_MADT_ENTRY(dist, end))
-		return -EINVAL;
-
 	dist_phy_base = dist->base_address;
 	return 0;
 }
-- 
2.4.3

[PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
of arm64, the BAD_MADT_GICC_ENTRY, too.

Signed-off-by: Al Stone <al.stone@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm64/include/asm/acpi.h | 8 --------
 arch/arm64/kernel/smp.c       | 2 --
 drivers/irqchip/irq-gic.c     | 6 ------
 3 files changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 208cec0..ed7e212 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,14 +19,6 @@
 #include <asm/cputype.h>
 #include <asm/smp_plat.h>
 
-/* Macros for consistency checks of the GICC subtable of MADT */
-#define ACPI_MADT_GICC_LENGTH	\
-	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
-
-#define BAD_MADT_GICC_ENTRY(entry, end)						\
-	(!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||	\
-	 (entry)->header.length != ACPI_MADT_GICC_LENGTH)
-
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index dbdaacd..66cc8c4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -451,8 +451,6 @@ 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_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 aa3e7b8..848c44c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1064,9 +1064,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
-	if (BAD_MADT_GICC_ENTRY(processor, end))
-		return -EINVAL;
-
 	/*
 	 * There is no support for non-banked GICv1/2 register in ACPI spec.
 	 * All CPU interface addresses have to be the same.
@@ -1088,9 +1085,6 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
 
 	dist = (struct acpi_madt_generic_distributor *)header;
 
-	if (BAD_MADT_ENTRY(dist, end))
-		return -EINVAL;
-
 	dist_phy_base = dist->base_address;
 	return 0;
 }
-- 
2.4.3

[PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
of arm64, the BAD_MADT_GICC_ENTRY, too.

Signed-off-by: Al Stone <al.stone@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm64/include/asm/acpi.h | 8 --------
 arch/arm64/kernel/smp.c       | 2 --
 drivers/irqchip/irq-gic.c     | 6 ------
 3 files changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 208cec0..ed7e212 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,14 +19,6 @@
 #include <asm/cputype.h>
 #include <asm/smp_plat.h>
 
-/* Macros for consistency checks of the GICC subtable of MADT */
-#define ACPI_MADT_GICC_LENGTH	\
-	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
-
-#define BAD_MADT_GICC_ENTRY(entry, end)						\
-	(!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||	\
-	 (entry)->header.length != ACPI_MADT_GICC_LENGTH)
-
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index dbdaacd..66cc8c4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -451,8 +451,6 @@ 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_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 aa3e7b8..848c44c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1064,9 +1064,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
-	if (BAD_MADT_GICC_ENTRY(processor, end))
-		return -EINVAL;
-
 	/*
 	 * There is no support for non-banked GICv1/2 register in ACPI spec.
 	 * All CPU interface addresses have to be the same.
@@ -1088,9 +1085,6 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
 
 	dist = (struct acpi_madt_generic_distributor *)header;
 
-	if (BAD_MADT_ENTRY(dist, end))
-		return -EINVAL;
-
 	dist_phy_base = dist->base_address;
 	return 0;
 }
-- 
2.4.3

[PATCH v2 3/5] ACPI / IA64: remove usage of BAD_MADT_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/ia64/kernel/acpi.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index b1698bc..efa3f0a 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -184,9 +184,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic, end))
-		return -EINVAL;
-
 	if (lapic->address) {
 		iounmap(ipi_base_addr);
 		ipi_base_addr = ioremap(lapic->address, 0);
@@ -201,8 +198,6 @@ acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	lsapic = (struct acpi_madt_local_sapic *)header;
 
-	/*Skip BAD_MADT_ENTRY check, as lsapic size could vary */
-
 	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
 #ifdef CONFIG_SMP
 		smp_boot_data.cpu_phys_id[available_cpus] =
@@ -222,9 +217,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lacpi_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lacpi_nmi, end))
-		return -EINVAL;
-
 	/* TBD: Support lapic_nmi entries */
 	return 0;
 }
@@ -236,9 +228,6 @@ acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end
 
 	iosapic = (struct acpi_madt_io_sapic *)header;
 
-	if (BAD_MADT_ENTRY(iosapic, end))
-		return -EINVAL;
-
 	return iosapic_init(iosapic->address, iosapic->global_irq_base);
 }
 
@@ -253,9 +242,6 @@ acpi_parse_plat_int_src(struct acpi_subtable_header * header,
 
 	plintsrc = (struct acpi_madt_interrupt_source *)header;
 
-	if (BAD_MADT_ENTRY(plintsrc, end))
-		return -EINVAL;
-
 	/*
 	 * Get vector assignment for this interrupt, set attributes,
 	 * and program the IOSAPIC routing table.
@@ -336,9 +322,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	p = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(p, end))
-		return -EINVAL;
-
 	iosapic_override_isa_irq(p->source_irq, p->global_irq,
 				 ((p->inti_flags & ACPI_MADT_POLARITY_MASK) ==
 				  ACPI_MADT_POLARITY_ACTIVE_LOW) ?
@@ -356,9 +339,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	/* TBD: Support nimsrc entries */
 	return 0;
 }
-- 
2.4.3

[PATCH v2 3/5] ACPI / IA64: remove usage of BAD_MADT_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/ia64/kernel/acpi.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index b1698bc..efa3f0a 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -184,9 +184,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic, end))
-		return -EINVAL;
-
 	if (lapic->address) {
 		iounmap(ipi_base_addr);
 		ipi_base_addr = ioremap(lapic->address, 0);
@@ -201,8 +198,6 @@ acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	lsapic = (struct acpi_madt_local_sapic *)header;
 
-	/*Skip BAD_MADT_ENTRY check, as lsapic size could vary */
-
 	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
 #ifdef CONFIG_SMP
 		smp_boot_data.cpu_phys_id[available_cpus] =
@@ -222,9 +217,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lacpi_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lacpi_nmi, end))
-		return -EINVAL;
-
 	/* TBD: Support lapic_nmi entries */
 	return 0;
 }
@@ -236,9 +228,6 @@ acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end
 
 	iosapic = (struct acpi_madt_io_sapic *)header;
 
-	if (BAD_MADT_ENTRY(iosapic, end))
-		return -EINVAL;
-
 	return iosapic_init(iosapic->address, iosapic->global_irq_base);
 }
 
@@ -253,9 +242,6 @@ acpi_parse_plat_int_src(struct acpi_subtable_header * header,
 
 	plintsrc = (struct acpi_madt_interrupt_source *)header;
 
-	if (BAD_MADT_ENTRY(plintsrc, end))
-		return -EINVAL;
-
 	/*
 	 * Get vector assignment for this interrupt, set attributes,
 	 * and program the IOSAPIC routing table.
@@ -336,9 +322,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	p = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(p, end))
-		return -EINVAL;
-
 	iosapic_override_isa_irq(p->source_irq, p->global_irq,
 				 ((p->inti_flags & ACPI_MADT_POLARITY_MASK) ==
 				  ACPI_MADT_POLARITY_ACTIVE_LOW) ?
@@ -356,9 +339,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	/* TBD: Support nimsrc entries */
 	return 0;
 }
-- 
2.4.3

[PATCH v2 3/5] ACPI / IA64: remove usage of BAD_MADT_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/ia64/kernel/acpi.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index b1698bc..efa3f0a 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -184,9 +184,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic, end))
-		return -EINVAL;
-
 	if (lapic->address) {
 		iounmap(ipi_base_addr);
 		ipi_base_addr = ioremap(lapic->address, 0);
@@ -201,8 +198,6 @@ acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	lsapic = (struct acpi_madt_local_sapic *)header;
 
-	/*Skip BAD_MADT_ENTRY check, as lsapic size could vary */
-
 	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
 #ifdef CONFIG_SMP
 		smp_boot_data.cpu_phys_id[available_cpus] @@ -222,9 +217,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lacpi_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lacpi_nmi, end))
-		return -EINVAL;
-
 	/* TBD: Support lapic_nmi entries */
 	return 0;
 }
@@ -236,9 +228,6 @@ acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end
 
 	iosapic = (struct acpi_madt_io_sapic *)header;
 
-	if (BAD_MADT_ENTRY(iosapic, end))
-		return -EINVAL;
-
 	return iosapic_init(iosapic->address, iosapic->global_irq_base);
 }
 
@@ -253,9 +242,6 @@ acpi_parse_plat_int_src(struct acpi_subtable_header * header,
 
 	plintsrc = (struct acpi_madt_interrupt_source *)header;
 
-	if (BAD_MADT_ENTRY(plintsrc, end))
-		return -EINVAL;
-
 	/*
 	 * Get vector assignment for this interrupt, set attributes,
 	 * and program the IOSAPIC routing table.
@@ -336,9 +322,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	p = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(p, end))
-		return -EINVAL;
-
 	iosapic_override_isa_irq(p->source_irq, p->global_irq,
 				 ((p->inti_flags & ACPI_MADT_POLARITY_MASK) =
 				  ACPI_MADT_POLARITY_ACTIVE_LOW) ?
@@ -356,9 +339,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	/* TBD: Support nimsrc entries */
 	return 0;
 }
-- 
2.4.3

[PATCH v2 4/5] ACPI / X86: remove usage of BAD_MADT_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/acpi/boot.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 75e8bad..f2a70b2 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -194,9 +194,6 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_x2apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	apic_id = processor->local_apic_id;
@@ -227,9 +224,6 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/*
@@ -252,9 +246,6 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_sapic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */
@@ -271,9 +262,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic_addr_ovr = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic_addr_ovr, end))
-		return -EINVAL;
-
 	acpi_lapic_addr = lapic_addr_ovr->address;
 
 	return 0;
@@ -287,9 +275,6 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 
 	x2apic_nmi = (struct acpi_madt_local_x2apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(x2apic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (x2apic_nmi->lint != 1)
@@ -305,9 +290,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lapic_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lapic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (lapic_nmi->lint != 1)
@@ -411,9 +393,6 @@ acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	ioapic = (struct acpi_madt_io_apic *)header;
 
-	if (BAD_MADT_ENTRY(ioapic, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* Statically assign IRQ numbers for IOAPICs hosting legacy IRQs */
@@ -462,9 +441,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	intsrc = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(intsrc, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
@@ -503,9 +479,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* TBD: Support nimsrc entries? */
-- 
2.4.3

[PATCH v2 4/5] ACPI / X86: remove usage of BAD_MADT_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86 at kernel.org
---
 arch/x86/kernel/acpi/boot.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 75e8bad..f2a70b2 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -194,9 +194,6 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_x2apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	apic_id = processor->local_apic_id;
@@ -227,9 +224,6 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/*
@@ -252,9 +246,6 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_sapic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */
@@ -271,9 +262,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic_addr_ovr = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic_addr_ovr, end))
-		return -EINVAL;
-
 	acpi_lapic_addr = lapic_addr_ovr->address;
 
 	return 0;
@@ -287,9 +275,6 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 
 	x2apic_nmi = (struct acpi_madt_local_x2apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(x2apic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (x2apic_nmi->lint != 1)
@@ -305,9 +290,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lapic_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lapic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (lapic_nmi->lint != 1)
@@ -411,9 +393,6 @@ acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	ioapic = (struct acpi_madt_io_apic *)header;
 
-	if (BAD_MADT_ENTRY(ioapic, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* Statically assign IRQ numbers for IOAPICs hosting legacy IRQs */
@@ -462,9 +441,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	intsrc = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(intsrc, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
@@ -503,9 +479,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* TBD: Support nimsrc entries? */
-- 
2.4.3

[PATCH v2 4/5] ACPI / X86: remove usage of BAD_MADT_ENTRY

[Al Stone]

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/acpi/boot.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 75e8bad..f2a70b2 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -194,9 +194,6 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_x2apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	apic_id = processor->local_apic_id;
@@ -227,9 +224,6 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/*
@@ -252,9 +246,6 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_sapic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */
@@ -271,9 +262,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic_addr_ovr = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic_addr_ovr, end))
-		return -EINVAL;
-
 	acpi_lapic_addr = lapic_addr_ovr->address;
 
 	return 0;
@@ -287,9 +275,6 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 
 	x2apic_nmi = (struct acpi_madt_local_x2apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(x2apic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (x2apic_nmi->lint != 1)
@@ -305,9 +290,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lapic_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lapic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (lapic_nmi->lint != 1)
@@ -411,9 +393,6 @@ acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	ioapic = (struct acpi_madt_io_apic *)header;
 
-	if (BAD_MADT_ENTRY(ioapic, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* Statically assign IRQ numbers for IOAPICs hosting legacy IRQs */
@@ -462,9 +441,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	intsrc = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(intsrc, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (intsrc->source_irq = acpi_gbl_FADT.sci_interrupt) {
@@ -503,9 +479,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* TBD: Support nimsrc entries? */
-- 
2.4.3

[PATCH v2 5/5] ACPI: remove definition of BAD_MADT_ENTRY macro

[Al Stone]

Now that we have introduced to bad_madt_entry(), and we have removed
all the usages of the BAD_MADT_ENTRY macro from all of the various
architectures that use it (arm64, ia64, x86), we can remove the macro
definition since it is no longer used.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 include/linux/acpi.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 01e6770..96729d3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -127,10 +127,6 @@ static inline void acpi_initrd_override(void *data, size_t size)
 }
 #endif
 
-#define BAD_MADT_ENTRY(entry, end) (					    \
-		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
-		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);
-- 
2.4.3

[PATCH v2 5/5] ACPI: remove definition of BAD_MADT_ENTRY macro

[Al Stone]

Now that we have introduced to bad_madt_entry(), and we have removed
all the usages of the BAD_MADT_ENTRY macro from all of the various
architectures that use it (arm64, ia64, x86), we can remove the macro
definition since it is no longer used.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 include/linux/acpi.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 01e6770..96729d3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -127,10 +127,6 @@ static inline void acpi_initrd_override(void *data, size_t size)
 }
 #endif
 
-#define BAD_MADT_ENTRY(entry, end) (					    \
-		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
-		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);
-- 
2.4.3

[PATCH v2 5/5] ACPI: remove definition of BAD_MADT_ENTRY macro

[Al Stone]

Now that we have introduced to bad_madt_entry(), and we have removed
all the usages of the BAD_MADT_ENTRY macro from all of the various
architectures that use it (arm64, ia64, x86), we can remove the macro
definition since it is no longer used.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 include/linux/acpi.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 01e6770..96729d3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -127,10 +127,6 @@ static inline void acpi_initrd_override(void *data, size_t size)
 }
 #endif
 
-#define BAD_MADT_ENTRY(entry, end) (					    \
-		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
-		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);
-- 
2.4.3

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

Hi Al,

On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> Now that we have introduced the bad_madt_entry() function, and that
> function is being invoked in acpi_table_parse_madt() for us, there
> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> of arm64, the BAD_MADT_GICC_ENTRY, too.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> ---
>  arch/arm64/include/asm/acpi.h | 8 --------
>  arch/arm64/kernel/smp.c       | 2 --
>  drivers/irqchip/irq-gic.c     | 6 ------
>  3 files changed, 16 deletions(-)

How are you planning to merge this (and which kernel are you targetting?)
You've got Acks for both arm64 and irqchip, so I guess either of those
trees could take it.

Will

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

Hi Al,

On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> Now that we have introduced the bad_madt_entry() function, and that
> function is being invoked in acpi_table_parse_madt() for us, there
> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> of arm64, the BAD_MADT_GICC_ENTRY, too.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> ---
>  arch/arm64/include/asm/acpi.h | 8 --------
>  arch/arm64/kernel/smp.c       | 2 --
>  drivers/irqchip/irq-gic.c     | 6 ------
>  3 files changed, 16 deletions(-)

How are you planning to merge this (and which kernel are you targetting?)
You've got Acks for both arm64 and irqchip, so I guess either of those
trees could take it.

Will

[PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

Hi Al,

On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> Now that we have introduced the bad_madt_entry() function, and that
> function is being invoked in acpi_table_parse_madt() for us, there
> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> of arm64, the BAD_MADT_GICC_ENTRY, too.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> ---
>  arch/arm64/include/asm/acpi.h | 8 --------
>  arch/arm64/kernel/smp.c       | 2 --
>  drivers/irqchip/irq-gic.c     | 6 ------
>  3 files changed, 16 deletions(-)

How are you planning to merge this (and which kernel are you targetting?)
You've got Acks for both arm64 and irqchip, so I guess either of those
trees could take it.

Will

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

Hi Al,

On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> Now that we have introduced the bad_madt_entry() function, and that
> function is being invoked in acpi_table_parse_madt() for us, there
> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> of arm64, the BAD_MADT_GICC_ENTRY, too.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> ---
>  arch/arm64/include/asm/acpi.h | 8 --------
>  arch/arm64/kernel/smp.c       | 2 --
>  drivers/irqchip/irq-gic.c     | 6 ------
>  3 files changed, 16 deletions(-)

How are you planning to merge this (and which kernel are you targetting?)
You've got Acks for both arm64 and irqchip, so I guess either of those
trees could take it.

Will

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

On 08/20/2015 04:13 AM, Will Deacon wrote:
> Hi Al,
> 
> On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
>> Now that we have introduced the bad_madt_entry() function, and that
>> function is being invoked in acpi_table_parse_madt() for us, there
>> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
>> of arm64, the BAD_MADT_GICC_ENTRY, too.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> ---
>>  arch/arm64/include/asm/acpi.h | 8 --------
>>  arch/arm64/kernel/smp.c       | 2 --
>>  drivers/irqchip/irq-gic.c     | 6 ------
>>  3 files changed, 16 deletions(-)
> 
> How are you planning to merge this (and which kernel are you targetting?)
> You've got Acks for both arm64 and irqchip, so I guess either of those
> trees could take it.

Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
but not required -- arm64 already has a usable patch for now, and that's
the only arch affected.  So, 4.3 was my primary target (which is why I
worked with linux-next for these).

Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
the only ones to have provided acks or reviews, however.  I guess I was
assuming this would have to go in via Rafael's ACPI tree since those are
the key parts -- the arch-specific patches would remove safety checks on
MADT subtables without replacing them, if they went in before the ACPI
patches.

Does that make sense?  What do you think?

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

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

On 08/20/2015 04:13 AM, Will Deacon wrote:
> Hi Al,
> 
> On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
>> Now that we have introduced the bad_madt_entry() function, and that
>> function is being invoked in acpi_table_parse_madt() for us, there
>> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
>> of arm64, the BAD_MADT_GICC_ENTRY, too.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> ---
>>  arch/arm64/include/asm/acpi.h | 8 --------
>>  arch/arm64/kernel/smp.c       | 2 --
>>  drivers/irqchip/irq-gic.c     | 6 ------
>>  3 files changed, 16 deletions(-)
> 
> How are you planning to merge this (and which kernel are you targetting?)
> You've got Acks for both arm64 and irqchip, so I guess either of those
> trees could take it.

Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
but not required -- arm64 already has a usable patch for now, and that's
the only arch affected.  So, 4.3 was my primary target (which is why I
worked with linux-next for these).

Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
the only ones to have provided acks or reviews, however.  I guess I was
assuming this would have to go in via Rafael's ACPI tree since those are
the key parts -- the arch-specific patches would remove safety checks on
MADT subtables without replacing them, if they went in before the ACPI
patches.

Does that make sense?  What do you think?

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

[PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

On 08/20/2015 04:13 AM, Will Deacon wrote:
> Hi Al,
> 
> On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
>> Now that we have introduced the bad_madt_entry() function, and that
>> function is being invoked in acpi_table_parse_madt() for us, there
>> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
>> of arm64, the BAD_MADT_GICC_ENTRY, too.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> ---
>>  arch/arm64/include/asm/acpi.h | 8 --------
>>  arch/arm64/kernel/smp.c       | 2 --
>>  drivers/irqchip/irq-gic.c     | 6 ------
>>  3 files changed, 16 deletions(-)
> 
> How are you planning to merge this (and which kernel are you targetting?)
> You've got Acks for both arm64 and irqchip, so I guess either of those
> trees could take it.

Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
but not required -- arm64 already has a usable patch for now, and that's
the only arch affected.  So, 4.3 was my primary target (which is why I
worked with linux-next for these).

Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
the only ones to have provided acks or reviews, however.  I guess I was
assuming this would have to go in via Rafael's ACPI tree since those are
the key parts -- the arch-specific patches would remove safety checks on
MADT subtables without replacing them, if they went in before the ACPI
patches.

Does that make sense?  What do you think?

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

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Al Stone]

On 08/20/2015 04:13 AM, Will Deacon wrote:
> Hi Al,
> 
> On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
>> Now that we have introduced the bad_madt_entry() function, and that
>> function is being invoked in acpi_table_parse_madt() for us, there
>> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
>> of arm64, the BAD_MADT_GICC_ENTRY, too.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> ---
>>  arch/arm64/include/asm/acpi.h | 8 --------
>>  arch/arm64/kernel/smp.c       | 2 --
>>  drivers/irqchip/irq-gic.c     | 6 ------
>>  3 files changed, 16 deletions(-)
> 
> How are you planning to merge this (and which kernel are you targetting?)
> You've got Acks for both arm64 and irqchip, so I guess either of those
> trees could take it.

Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
but not required -- arm64 already has a usable patch for now, and that's
the only arch affected.  So, 4.3 was my primary target (which is why I
worked with linux-next for these).

Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
the only ones to have provided acks or reviews, however.  I guess I was
assuming this would have to go in via Rafael's ACPI tree since those are
the key parts -- the arch-specific patches would remove safety checks on
MADT subtables without replacing them, if they went in before the ACPI
patches.

Does that make sense?  What do you think?

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

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

On Thu, Aug 20, 2015 at 05:57:05PM +0100, Al Stone wrote:
> On 08/20/2015 04:13 AM, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> >> Now that we have introduced the bad_madt_entry() function, and that
> >> function is being invoked in acpi_table_parse_madt() for us, there
> >> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> >> of arm64, the BAD_MADT_GICC_ENTRY, too.
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Jason Cooper <jason@lakedaemon.net>
> >> ---
> >>  arch/arm64/include/asm/acpi.h | 8 --------
> >>  arch/arm64/kernel/smp.c       | 2 --
> >>  drivers/irqchip/irq-gic.c     | 6 ------
> >>  3 files changed, 16 deletions(-)
> > 
> > How are you planning to merge this (and which kernel are you targetting?)
> > You've got Acks for both arm64 and irqchip, so I guess either of those
> > trees could take it.
> 
> Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
> but not required -- arm64 already has a usable patch for now, and that's
> the only arch affected.  So, 4.3 was my primary target (which is why I
> worked with linux-next for these).
> 
> Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
> to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
> the only ones to have provided acks or reviews, however.  I guess I was
> assuming this would have to go in via Rafael's ACPI tree since those are
> the key parts -- the arch-specific patches would remove safety checks on
> MADT subtables without replacing them, if they went in before the ACPI
> patches.
> 
> Does that make sense?  What do you think?

Yup, taking it all via Rafael is fine by me. I just didn't want to end
up in a situation where you thought something was going via the arm64
tree but I hadn't queued it.

Will

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

On Thu, Aug 20, 2015 at 05:57:05PM +0100, Al Stone wrote:
> On 08/20/2015 04:13 AM, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> >> Now that we have introduced the bad_madt_entry() function, and that
> >> function is being invoked in acpi_table_parse_madt() for us, there
> >> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> >> of arm64, the BAD_MADT_GICC_ENTRY, too.
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Jason Cooper <jason@lakedaemon.net>
> >> ---
> >>  arch/arm64/include/asm/acpi.h | 8 --------
> >>  arch/arm64/kernel/smp.c       | 2 --
> >>  drivers/irqchip/irq-gic.c     | 6 ------
> >>  3 files changed, 16 deletions(-)
> > 
> > How are you planning to merge this (and which kernel are you targetting?)
> > You've got Acks for both arm64 and irqchip, so I guess either of those
> > trees could take it.
> 
> Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
> but not required -- arm64 already has a usable patch for now, and that's
> the only arch affected.  So, 4.3 was my primary target (which is why I
> worked with linux-next for these).
> 
> Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
> to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
> the only ones to have provided acks or reviews, however.  I guess I was
> assuming this would have to go in via Rafael's ACPI tree since those are
> the key parts -- the arch-specific patches would remove safety checks on
> MADT subtables without replacing them, if they went in before the ACPI
> patches.
> 
> Does that make sense?  What do you think?

Yup, taking it all via Rafael is fine by me. I just didn't want to end
up in a situation where you thought something was going via the arm64
tree but I hadn't queued it.

Will

[PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

On Thu, Aug 20, 2015 at 05:57:05PM +0100, Al Stone wrote:
> On 08/20/2015 04:13 AM, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> >> Now that we have introduced the bad_madt_entry() function, and that
> >> function is being invoked in acpi_table_parse_madt() for us, there
> >> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> >> of arm64, the BAD_MADT_GICC_ENTRY, too.
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Jason Cooper <jason@lakedaemon.net>
> >> ---
> >>  arch/arm64/include/asm/acpi.h | 8 --------
> >>  arch/arm64/kernel/smp.c       | 2 --
> >>  drivers/irqchip/irq-gic.c     | 6 ------
> >>  3 files changed, 16 deletions(-)
> > 
> > How are you planning to merge this (and which kernel are you targetting?)
> > You've got Acks for both arm64 and irqchip, so I guess either of those
> > trees could take it.
> 
> Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
> but not required -- arm64 already has a usable patch for now, and that's
> the only arch affected.  So, 4.3 was my primary target (which is why I
> worked with linux-next for these).
> 
> Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
> to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
> the only ones to have provided acks or reviews, however.  I guess I was
> assuming this would have to go in via Rafael's ACPI tree since those are
> the key parts -- the arch-specific patches would remove safety checks on
> MADT subtables without replacing them, if they went in before the ACPI
> patches.
> 
> Does that make sense?  What do you think?

Yup, taking it all via Rafael is fine by me. I just didn't want to end
up in a situation where you thought something was going via the arm64
tree but I hadn't queued it.

Will

Re: [PATCH v2 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

[Will Deacon]

On Thu, Aug 20, 2015 at 05:57:05PM +0100, Al Stone wrote:
> On 08/20/2015 04:13 AM, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 11:07:25PM +0100, Al Stone wrote:
> >> Now that we have introduced the bad_madt_entry() function, and that
> >> function is being invoked in acpi_table_parse_madt() for us, there
> >> is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
> >> of arm64, the BAD_MADT_GICC_ENTRY, too.
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Jason Cooper <jason@lakedaemon.net>
> >> ---
> >>  arch/arm64/include/asm/acpi.h | 8 --------
> >>  arch/arm64/kernel/smp.c       | 2 --
> >>  drivers/irqchip/irq-gic.c     | 6 ------
> >>  3 files changed, 16 deletions(-)
> > 
> > How are you planning to merge this (and which kernel are you targetting?)
> > You've got Acks for both arm64 and irqchip, so I guess either of those
> > trees could take it.
> 
> Yeah, this is a little messy.  If I can get into 4.2, that would be nice,
> but not required -- arm64 already has a usable patch for now, and that's
> the only arch affected.  So, 4.3 was my primary target (which is why I
> worked with linux-next for these).
> 
> Which tree?  Yeesh.  1/5 and 5/5 are ACPI only and required for the rest
> to work properly; 2/5 is arm64, 3/5 is ia64, and 4/5 is x86.  ARM folks are
> the only ones to have provided acks or reviews, however.  I guess I was
> assuming this would have to go in via Rafael's ACPI tree since those are
> the key parts -- the arch-specific patches would remove safety checks on
> MADT subtables without replacing them, if they went in before the ACPI
> patches.
> 
> Does that make sense?  What do you think?

Yup, taking it all via Rafael is fine by me. I just didn't want to end
up in a situation where you thought something was going via the arm64
tree but I hadn't queued it.

Will

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Timur Tabi]

On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
> +                                      entry->type, entry->length);
> +                               return 1;
> +                       }
> +               }
> +       } else {
> +               if (entry->length != len) {
> +                       pr_err("MADT subtable %d is wrong size: %d\n",
> +                              len, entry->type);

Can we make these a little more descriptive?

pr_err("Variable length MADT subtable type %d is wrong size: %d,
should be %d\n",
      entry->type, entry->length, len);

pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
      entry->type, entry->length, len);




-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Timur Tabi]

On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
> +                                      entry->type, entry->length);
> +                               return 1;
> +                       }
> +               }
> +       } else {
> +               if (entry->length != len) {
> +                       pr_err("MADT subtable %d is wrong size: %d\n",
> +                              len, entry->type);

Can we make these a little more descriptive?

pr_err("Variable length MADT subtable type %d is wrong size: %d,
should be %d\n",
      entry->type, entry->length, len);

pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
      entry->type, entry->length, len);




-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Timur Tabi]

On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
> +                                      entry->type, entry->length);
> +                               return 1;
> +                       }
> +               }
> +       } else {
> +               if (entry->length != len) {
> +                       pr_err("MADT subtable %d is wrong size: %d\n",
> +                              len, entry->type);

Can we make these a little more descriptive?

pr_err("Variable length MADT subtable type %d is wrong size: %d,
should be %d\n",
      entry->type, entry->length, len);

pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
      entry->type, entry->length, len);




-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Timur Tabi]

On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
> +                                      entry->type, entry->length);
> +                               return 1;
> +                       }
> +               }
> +       } else {
> +               if (entry->length != len) {
> +                       pr_err("MADT subtable %d is wrong size: %d\n",
> +                              len, entry->type);

Can we make these a little more descriptive?

pr_err("Variable length MADT subtable type %d is wrong size: %d,
should be %d\n",
      entry->type, entry->length, len);

pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
      entry->type, entry->length, len);




-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 08/26/2015 09:38 AM, Timur Tabi wrote:
> On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
>> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
>> +                                      entry->type, entry->length);
>> +                               return 1;
>> +                       }
>> +               }
>> +       } else {
>> +               if (entry->length != len) {
>> +                       pr_err("MADT subtable %d is wrong size: %d\n",
>> +                              len, entry->type);
> 
> Can we make these a little more descriptive?
> 
> pr_err("Variable length MADT subtable type %d is wrong size: %d,
> should be %d\n",
>       entry->type, entry->length, len);
> 
> pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
>       entry->type, entry->length, len);
> 

Sure.  It's always a fine line between verbose and not enough info.

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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 08/26/2015 09:38 AM, Timur Tabi wrote:
> On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
>> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
>> +                                      entry->type, entry->length);
>> +                               return 1;
>> +                       }
>> +               }
>> +       } else {
>> +               if (entry->length != len) {
>> +                       pr_err("MADT subtable %d is wrong size: %d\n",
>> +                              len, entry->type);
> 
> Can we make these a little more descriptive?
> 
> pr_err("Variable length MADT subtable type %d is wrong size: %d,
> should be %d\n",
>       entry->type, entry->length, len);
> 
> pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
>       entry->type, entry->length, len);
> 

Sure.  It's always a fine line between verbose and not enough info.

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

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 08/26/2015 09:38 AM, Timur Tabi wrote:
> On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
>> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
>> +                                      entry->type, entry->length);
>> +                               return 1;
>> +                       }
>> +               }
>> +       } else {
>> +               if (entry->length != len) {
>> +                       pr_err("MADT subtable %d is wrong size: %d\n",
>> +                              len, entry->type);
> 
> Can we make these a little more descriptive?
> 
> pr_err("Variable length MADT subtable type %d is wrong size: %d,
> should be %d\n",
>       entry->type, entry->length, len);
> 
> pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
>       entry->type, entry->length, len);
> 

Sure.  It's always a fine line between verbose and not enough info.

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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 08/26/2015 09:38 AM, Timur Tabi wrote:
> On Wed, Aug 19, 2015 at 5:07 PM, Al Stone <al.stone@linaro.org> wrote:
>> +                               pr_err("Variable length MADT subtable %d is wrong size: %d\n",
>> +                                      entry->type, entry->length);
>> +                               return 1;
>> +                       }
>> +               }
>> +       } else {
>> +               if (entry->length != len) {
>> +                       pr_err("MADT subtable %d is wrong size: %d\n",
>> +                              len, entry->type);
> 
> Can we make these a little more descriptive?
> 
> pr_err("Variable length MADT subtable type %d is wrong size: %d,
> should be %d\n",
>       entry->type, entry->length, len);
> 
> pr_err("MADT subtable type %d is wrong size: %d, should be %d\n",
>       entry->type, entry->length, len);
> 

Sure.  It's always a fine line between verbose and not enough info.

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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

Hi Al,

On 19/08/15 23:07, Al Stone wrote:

I finally got a chance to try this series on Juno. Well it exposed a 
firmware bug in MADT table :)

[..]

>                  acpi_tbl_entry_handler handler,
> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>                 table_end) {
>                  if (entry->type == entry_id
>                      && (!max_entries || count < max_entries)) {
> +                       if (bad_madt_entry(table_header, entry))
> +                               return -EINVAL;

Not sure if we can have the above check here unconditionally.
Currently I can see there are 2 other users of acpi_parse_entries i.e.
PCC and NUMA. So may be it can be made conditional or return success for
non-MADT tables from bad_madt_entry ?

Other than that, you can add for ARM64 specific parts:
Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

Hi Al,

On 19/08/15 23:07, Al Stone wrote:

I finally got a chance to try this series on Juno. Well it exposed a 
firmware bug in MADT table :)

[..]

>                  acpi_tbl_entry_handler handler,
> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>                 table_end) {
>                  if (entry->type == entry_id
>                      && (!max_entries || count < max_entries)) {
> +                       if (bad_madt_entry(table_header, entry))
> +                               return -EINVAL;

Not sure if we can have the above check here unconditionally.
Currently I can see there are 2 other users of acpi_parse_entries i.e.
PCC and NUMA. So may be it can be made conditional or return success for
non-MADT tables from bad_madt_entry ?

Other than that, you can add for ARM64 specific parts:
Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

Hi Al,

On 19/08/15 23:07, Al Stone wrote:

I finally got a chance to try this series on Juno. Well it exposed a 
firmware bug in MADT table :)

[..]

>                  acpi_tbl_entry_handler handler,
> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>                 table_end) {
>                  if (entry->type == entry_id
>                      && (!max_entries || count < max_entries)) {
> +                       if (bad_madt_entry(table_header, entry))
> +                               return -EINVAL;

Not sure if we can have the above check here unconditionally.
Currently I can see there are 2 other users of acpi_parse_entries i.e.
PCC and NUMA. So may be it can be made conditional or return success for
non-MADT tables from bad_madt_entry ?

Other than that, you can add for ARM64 specific parts:
Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

Hi Al,

On 19/08/15 23:07, Al Stone wrote:

I finally got a chance to try this series on Juno. Well it exposed a 
firmware bug in MADT table :)

[..]

>                  acpi_tbl_entry_handler handler,
> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>                 table_end) {
>                  if (entry->type = entry_id
>                      && (!max_entries || count < max_entries)) {
> +                       if (bad_madt_entry(table_header, entry))
> +                               return -EINVAL;

Not sure if we can have the above check here unconditionally.
Currently I can see there are 2 other users of acpi_parse_entries i.e.
PCC and NUMA. So may be it can be made conditional or return success for
non-MADT tables from bad_madt_entry ?

Other than that, you can add for ARM64 specific parts:
Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)

What?  The code did what it was supposed to do :-)?  Very cool.  Good to know.

I talked to Graeme a bit, too, and he had some good suggestions for clean up.
I'll post a v3 tomorrow.

> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

I'll double check these uses.  I thought I had before, and based on what I
saw the check would be reasonable.  It never hurts to check again, though.

> Other than that, you can add for ARM64 specific parts:
> Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

> Regards,
> Sudeep
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)

What?  The code did what it was supposed to do :-)?  Very cool.  Good to know.

I talked to Graeme a bit, too, and he had some good suggestions for clean up.
I'll post a v3 tomorrow.

> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

I'll double check these uses.  I thought I had before, and based on what I
saw the check would be reasonable.  It never hurts to check again, though.

> Other than that, you can add for ARM64 specific parts:
> Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

> Regards,
> Sudeep
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)

What?  The code did what it was supposed to do :-)?  Very cool.  Good to know.

I talked to Graeme a bit, too, and he had some good suggestions for clean up.
I'll post a v3 tomorrow.

> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

I'll double check these uses.  I thought I had before, and based on what I
saw the check would be reasonable.  It never hurts to check again, though.

> Other than that, you can add for ARM64 specific parts:
> Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

> Regards,
> Sudeep
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)

What?  The code did what it was supposed to do :-)?  Very cool.  Good to know.

I talked to Graeme a bit, too, and he had some good suggestions for clean up.
I'll post a v3 tomorrow.

> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type = entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

I'll double check these uses.  I thought I had before, and based on what I
saw the check would be reasonable.  It never hurts to check again, though.

> Other than that, you can add for ARM64 specific parts:
> Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

> Regards,
> Sudeep
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)
> 
> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

So, I went back and double checked the other users and they're looking at
the return value for acpi_parse_entries properly; adding in the check above
unconditionally should not cause any behavior change.  Further, despite the
name, acpi_parse_entries is only used to examine MADT subtables.  Granted,
we should probably make the name clearer at some point (too ambiguous as to
which entries are parsed right now).  Nonetheless, current usage seems to
be in order.


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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)
> 
> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

So, I went back and double checked the other users and they're looking at
the return value for acpi_parse_entries properly; adding in the check above
unconditionally should not cause any behavior change.  Further, despite the
name, acpi_parse_entries is only used to examine MADT subtables.  Granted,
we should probably make the name clearer at some point (too ambiguous as to
which entries are parsed right now).  Nonetheless, current usage seems to
be in order.


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

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)
> 
> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

So, I went back and double checked the other users and they're looking at
the return value for acpi_parse_entries properly; adding in the check above
unconditionally should not cause any behavior change.  Further, despite the
name, acpi_parse_entries is only used to examine MADT subtables.  Granted,
we should probably make the name clearer at some point (too ambiguous as to
which entries are parsed right now).  Nonetheless, current usage seems to
be in order.


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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)
> 
> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type = entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

So, I went back and double checked the other users and they're looking at
the return value for acpi_parse_entries properly; adding in the check above
unconditionally should not cause any behavior change.  Further, despite the
name, acpi_parse_entries is only used to examine MADT subtables.  Granted,
we should probably make the name clearer at some point (too ambiguous as to
which entries are parsed right now).  Nonetheless, current usage seems to
be in order.


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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 09/09/15 20:57, Al Stone wrote:
> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>> Hi Al,
>>
>> On 19/08/15 23:07, Al Stone wrote:
>>
>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>> bug in MADT table :)
>>
>> [..]
>>
>>>                   acpi_tbl_entry_handler handler,
>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>                  table_end) {
>>>                   if (entry->type == entry_id
>>>                       && (!max_entries || count < max_entries)) {
>>> +                       if (bad_madt_entry(table_header, entry))
>>> +                               return -EINVAL;
>>
>> Not sure if we can have the above check here unconditionally.
>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>> PCC and NUMA. So may be it can be made conditional or return success for
>> non-MADT tables from bad_madt_entry ?
>
> So, I went back and double checked the other users and they're looking at
> the return value for acpi_parse_entries properly; adding in the check above
> unconditionally should not cause any behavior change.

I disagree. I populated PCCT table on Juno to get this error for
PCCT(PCCT header gets interpreted as MADT header):
"
ACPI: undefined version for either FADT 5.1 or MADT 1
Error parsing PCC subspaces from PCCT
"
And here the stacktrace:
[<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
[<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
[<ffffffc000895af8>] pcc_init+0x70/0x148

> Further, despite the name, acpi_parse_entries is only used to examine MADT
> subtables.  Granted, we should probably make the name clearer at some point
> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
> usage seems to be in order.
>

 From the code inspection, I can see we have 3 users of 
acpi_parse_entries not just MADT but also PCC and NUMA/SRAT

Something like this solves this issue:
-              if (bad_madt_entry(table_header, entry))
+              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
+                      bad_madt_entry(table_header, entry)


Or am I still missing something ?

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 09/09/15 20:57, Al Stone wrote:
> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>> Hi Al,
>>
>> On 19/08/15 23:07, Al Stone wrote:
>>
>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>> bug in MADT table :)
>>
>> [..]
>>
>>>                   acpi_tbl_entry_handler handler,
>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>                  table_end) {
>>>                   if (entry->type == entry_id
>>>                       && (!max_entries || count < max_entries)) {
>>> +                       if (bad_madt_entry(table_header, entry))
>>> +                               return -EINVAL;
>>
>> Not sure if we can have the above check here unconditionally.
>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>> PCC and NUMA. So may be it can be made conditional or return success for
>> non-MADT tables from bad_madt_entry ?
>
> So, I went back and double checked the other users and they're looking at
> the return value for acpi_parse_entries properly; adding in the check above
> unconditionally should not cause any behavior change.

I disagree. I populated PCCT table on Juno to get this error for
PCCT(PCCT header gets interpreted as MADT header):
"
ACPI: undefined version for either FADT 5.1 or MADT 1
Error parsing PCC subspaces from PCCT
"
And here the stacktrace:
[<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
[<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
[<ffffffc000895af8>] pcc_init+0x70/0x148

> Further, despite the name, acpi_parse_entries is only used to examine MADT
> subtables.  Granted, we should probably make the name clearer at some point
> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
> usage seems to be in order.
>

 From the code inspection, I can see we have 3 users of 
acpi_parse_entries not just MADT but also PCC and NUMA/SRAT

Something like this solves this issue:
-              if (bad_madt_entry(table_header, entry))
+              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
+                      bad_madt_entry(table_header, entry)


Or am I still missing something ?

Regards,
Sudeep

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 09/09/15 20:57, Al Stone wrote:
> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>> Hi Al,
>>
>> On 19/08/15 23:07, Al Stone wrote:
>>
>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>> bug in MADT table :)
>>
>> [..]
>>
>>>                   acpi_tbl_entry_handler handler,
>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>                  table_end) {
>>>                   if (entry->type == entry_id
>>>                       && (!max_entries || count < max_entries)) {
>>> +                       if (bad_madt_entry(table_header, entry))
>>> +                               return -EINVAL;
>>
>> Not sure if we can have the above check here unconditionally.
>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>> PCC and NUMA. So may be it can be made conditional or return success for
>> non-MADT tables from bad_madt_entry ?
>
> So, I went back and double checked the other users and they're looking at
> the return value for acpi_parse_entries properly; adding in the check above
> unconditionally should not cause any behavior change.

I disagree. I populated PCCT table on Juno to get this error for
PCCT(PCCT header gets interpreted as MADT header):
"
ACPI: undefined version for either FADT 5.1 or MADT 1
Error parsing PCC subspaces from PCCT
"
And here the stacktrace:
[<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
[<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
[<ffffffc000895af8>] pcc_init+0x70/0x148

> Further, despite the name, acpi_parse_entries is only used to examine MADT
> subtables.  Granted, we should probably make the name clearer at some point
> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
> usage seems to be in order.
>

 From the code inspection, I can see we have 3 users of 
acpi_parse_entries not just MADT but also PCC and NUMA/SRAT

Something like this solves this issue:
-              if (bad_madt_entry(table_header, entry))
+              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
+                      bad_madt_entry(table_header, entry)


Or am I still missing something ?

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 09/09/15 20:57, Al Stone wrote:
> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>> Hi Al,
>>
>> On 19/08/15 23:07, Al Stone wrote:
>>
>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>> bug in MADT table :)
>>
>> [..]
>>
>>>                   acpi_tbl_entry_handler handler,
>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>                  table_end) {
>>>                   if (entry->type = entry_id
>>>                       && (!max_entries || count < max_entries)) {
>>> +                       if (bad_madt_entry(table_header, entry))
>>> +                               return -EINVAL;
>>
>> Not sure if we can have the above check here unconditionally.
>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>> PCC and NUMA. So may be it can be made conditional or return success for
>> non-MADT tables from bad_madt_entry ?
>
> So, I went back and double checked the other users and they're looking at
> the return value for acpi_parse_entries properly; adding in the check above
> unconditionally should not cause any behavior change.

I disagree. I populated PCCT table on Juno to get this error for
PCCT(PCCT header gets interpreted as MADT header):
"
ACPI: undefined version for either FADT 5.1 or MADT 1
Error parsing PCC subspaces from PCCT
"
And here the stacktrace:
[<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
[<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
[<ffffffc000895af8>] pcc_init+0x70/0x148

> Further, despite the name, acpi_parse_entries is only used to examine MADT
> subtables.  Granted, we should probably make the name clearer at some point
> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
> usage seems to be in order.
>

 From the code inspection, I can see we have 3 users of 
acpi_parse_entries not just MADT but also PCC and NUMA/SRAT

Something like this solves this issue:
-              if (bad_madt_entry(table_header, entry))
+              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
+                      bad_madt_entry(table_header, entry)


Or am I still missing something ?

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/10/2015 10:20 AM, Sudeep Holla wrote:
> 
> 
> On 09/09/15 20:57, Al Stone wrote:
>> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 19/08/15 23:07, Al Stone wrote:
>>>
>>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>>> bug in MADT table :)
>>>
>>> [..]
>>>
>>>>                   acpi_tbl_entry_handler handler,
>>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>>                  table_end) {
>>>>                   if (entry->type == entry_id
>>>>                       && (!max_entries || count < max_entries)) {
>>>> +                       if (bad_madt_entry(table_header, entry))
>>>> +                               return -EINVAL;
>>>
>>> Not sure if we can have the above check here unconditionally.
>>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>>> PCC and NUMA. So may be it can be made conditional or return success for
>>> non-MADT tables from bad_madt_entry ?
>>
>> So, I went back and double checked the other users and they're looking at
>> the return value for acpi_parse_entries properly; adding in the check above
>> unconditionally should not cause any behavior change.
> 
> I disagree. I populated PCCT table on Juno to get this error for
> PCCT(PCCT header gets interpreted as MADT header):
> "
> ACPI: undefined version for either FADT 5.1 or MADT 1
> Error parsing PCC subspaces from PCCT
> "
> And here the stacktrace:
> [<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
> [<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
> [<ffffffc000895af8>] pcc_init+0x70/0x148
> 
>> Further, despite the name, acpi_parse_entries is only used to examine MADT
>> subtables.  Granted, we should probably make the name clearer at some point
>> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
>> usage seems to be in order.
>>
> 
> From the code inspection, I can see we have 3 users of acpi_parse_entries not
> just MADT but also PCC and NUMA/SRAT
> 
> Something like this solves this issue:
> -              if (bad_madt_entry(table_header, entry))
> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
> +                      bad_madt_entry(table_header, entry)
> 
> 
> Or am I still missing something ?
> 
> Regards,
> Sudeep

Nope, I missed it.  Your fix above will solve the problem; I misunderstood
how acpi_parse_entries() was being used -- somehow I had it in my head that
only MADT was in use, and just not seeing that it's being used for several
other subtable traversals also.  Sorry about that, Sudeep.  My mistake.

I'll add this fix for a v4, but I'll wait for a few days to see if I get any
additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers
yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
ACPI tables that are in error by using these patches, even with the flaws :).

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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/10/2015 10:20 AM, Sudeep Holla wrote:
> 
> 
> On 09/09/15 20:57, Al Stone wrote:
>> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 19/08/15 23:07, Al Stone wrote:
>>>
>>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>>> bug in MADT table :)
>>>
>>> [..]
>>>
>>>>                   acpi_tbl_entry_handler handler,
>>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>>                  table_end) {
>>>>                   if (entry->type == entry_id
>>>>                       && (!max_entries || count < max_entries)) {
>>>> +                       if (bad_madt_entry(table_header, entry))
>>>> +                               return -EINVAL;
>>>
>>> Not sure if we can have the above check here unconditionally.
>>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>>> PCC and NUMA. So may be it can be made conditional or return success for
>>> non-MADT tables from bad_madt_entry ?
>>
>> So, I went back and double checked the other users and they're looking at
>> the return value for acpi_parse_entries properly; adding in the check above
>> unconditionally should not cause any behavior change.
> 
> I disagree. I populated PCCT table on Juno to get this error for
> PCCT(PCCT header gets interpreted as MADT header):
> "
> ACPI: undefined version for either FADT 5.1 or MADT 1
> Error parsing PCC subspaces from PCCT
> "
> And here the stacktrace:
> [<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
> [<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
> [<ffffffc000895af8>] pcc_init+0x70/0x148
> 
>> Further, despite the name, acpi_parse_entries is only used to examine MADT
>> subtables.  Granted, we should probably make the name clearer at some point
>> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
>> usage seems to be in order.
>>
> 
> From the code inspection, I can see we have 3 users of acpi_parse_entries not
> just MADT but also PCC and NUMA/SRAT
> 
> Something like this solves this issue:
> -              if (bad_madt_entry(table_header, entry))
> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
> +                      bad_madt_entry(table_header, entry)
> 
> 
> Or am I still missing something ?
> 
> Regards,
> Sudeep

Nope, I missed it.  Your fix above will solve the problem; I misunderstood
how acpi_parse_entries() was being used -- somehow I had it in my head that
only MADT was in use, and just not seeing that it's being used for several
other subtable traversals also.  Sorry about that, Sudeep.  My mistake.

I'll add this fix for a v4, but I'll wait for a few days to see if I get any
additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers
yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
ACPI tables that are in error by using these patches, even with the flaws :).

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

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/10/2015 10:20 AM, Sudeep Holla wrote:
> 
> 
> On 09/09/15 20:57, Al Stone wrote:
>> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 19/08/15 23:07, Al Stone wrote:
>>>
>>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>>> bug in MADT table :)
>>>
>>> [..]
>>>
>>>>                   acpi_tbl_entry_handler handler,
>>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>>                  table_end) {
>>>>                   if (entry->type == entry_id
>>>>                       && (!max_entries || count < max_entries)) {
>>>> +                       if (bad_madt_entry(table_header, entry))
>>>> +                               return -EINVAL;
>>>
>>> Not sure if we can have the above check here unconditionally.
>>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>>> PCC and NUMA. So may be it can be made conditional or return success for
>>> non-MADT tables from bad_madt_entry ?
>>
>> So, I went back and double checked the other users and they're looking at
>> the return value for acpi_parse_entries properly; adding in the check above
>> unconditionally should not cause any behavior change.
> 
> I disagree. I populated PCCT table on Juno to get this error for
> PCCT(PCCT header gets interpreted as MADT header):
> "
> ACPI: undefined version for either FADT 5.1 or MADT 1
> Error parsing PCC subspaces from PCCT
> "
> And here the stacktrace:
> [<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
> [<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
> [<ffffffc000895af8>] pcc_init+0x70/0x148
> 
>> Further, despite the name, acpi_parse_entries is only used to examine MADT
>> subtables.  Granted, we should probably make the name clearer at some point
>> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
>> usage seems to be in order.
>>
> 
> From the code inspection, I can see we have 3 users of acpi_parse_entries not
> just MADT but also PCC and NUMA/SRAT
> 
> Something like this solves this issue:
> -              if (bad_madt_entry(table_header, entry))
> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
> +                      bad_madt_entry(table_header, entry)
> 
> 
> Or am I still missing something ?
> 
> Regards,
> Sudeep

Nope, I missed it.  Your fix above will solve the problem; I misunderstood
how acpi_parse_entries() was being used -- somehow I had it in my head that
only MADT was in use, and just not seeing that it's being used for several
other subtable traversals also.  Sorry about that, Sudeep.  My mistake.

I'll add this fix for a v4, but I'll wait for a few days to see if I get any
additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers
yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
ACPI tables that are in error by using these patches, even with the flaws :).

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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Al Stone]

On 09/10/2015 10:20 AM, Sudeep Holla wrote:
> 
> 
> On 09/09/15 20:57, Al Stone wrote:
>> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 19/08/15 23:07, Al Stone wrote:
>>>
>>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>>> bug in MADT table :)
>>>
>>> [..]
>>>
>>>>                   acpi_tbl_entry_handler handler,
>>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>>                  table_end) {
>>>>                   if (entry->type = entry_id
>>>>                       && (!max_entries || count < max_entries)) {
>>>> +                       if (bad_madt_entry(table_header, entry))
>>>> +                               return -EINVAL;
>>>
>>> Not sure if we can have the above check here unconditionally.
>>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>>> PCC and NUMA. So may be it can be made conditional or return success for
>>> non-MADT tables from bad_madt_entry ?
>>
>> So, I went back and double checked the other users and they're looking at
>> the return value for acpi_parse_entries properly; adding in the check above
>> unconditionally should not cause any behavior change.
> 
> I disagree. I populated PCCT table on Juno to get this error for
> PCCT(PCCT header gets interpreted as MADT header):
> "
> ACPI: undefined version for either FADT 5.1 or MADT 1
> Error parsing PCC subspaces from PCCT
> "
> And here the stacktrace:
> [<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
> [<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
> [<ffffffc000895af8>] pcc_init+0x70/0x148
> 
>> Further, despite the name, acpi_parse_entries is only used to examine MADT
>> subtables.  Granted, we should probably make the name clearer at some point
>> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
>> usage seems to be in order.
>>
> 
> From the code inspection, I can see we have 3 users of acpi_parse_entries not
> just MADT but also PCC and NUMA/SRAT
> 
> Something like this solves this issue:
> -              if (bad_madt_entry(table_header, entry))
> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
> +                      bad_madt_entry(table_header, entry)
> 
> 
> Or am I still missing something ?
> 
> Regards,
> Sudeep

Nope, I missed it.  Your fix above will solve the problem; I misunderstood
how acpi_parse_entries() was being used -- somehow I had it in my head that
only MADT was in use, and just not seeing that it's being used for several
other subtable traversals also.  Sorry about that, Sudeep.  My mistake.

I'll add this fix for a v4, but I'll wait for a few days to see if I get any
additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers
yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
ACPI tables that are in error by using these patches, even with the flaws :).

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

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 10/09/15 21:43, Al Stone wrote:
> On 09/10/2015 10:20 AM, Sudeep Holla wrote:
>>

[...]

>>
>>  From the code inspection, I can see we have 3 users of acpi_parse_entries not
>> just MADT but also PCC and NUMA/SRAT
>>
>> Something like this solves this issue:
>> -              if (bad_madt_entry(table_header, entry))
>> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
>> +                      bad_madt_entry(table_header, entry)
>>
>>
>> Or am I still missing something ?
>
> Nope, I missed it.  Your fix above will solve the problem; I misunderstood
> how acpi_parse_entries() was being used -- somehow I had it in my head that
> only MADT was in use, and just not seeing that it's being used for several
> other subtable traversals also.  Sorry about that, Sudeep.  My mistake.
>

No worries.

> I'll add this fix for a v4, but I'll wait for a few days to see if I get any
> additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers

Makes sense.

> yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
> ACPI tables that are in error by using these patches, even with the flaws :).
>

Very much true indeed :)

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 10/09/15 21:43, Al Stone wrote:
> On 09/10/2015 10:20 AM, Sudeep Holla wrote:
>>

[...]

>>
>>  From the code inspection, I can see we have 3 users of acpi_parse_entries not
>> just MADT but also PCC and NUMA/SRAT
>>
>> Something like this solves this issue:
>> -              if (bad_madt_entry(table_header, entry))
>> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
>> +                      bad_madt_entry(table_header, entry)
>>
>>
>> Or am I still missing something ?
>
> Nope, I missed it.  Your fix above will solve the problem; I misunderstood
> how acpi_parse_entries() was being used -- somehow I had it in my head that
> only MADT was in use, and just not seeing that it's being used for several
> other subtable traversals also.  Sorry about that, Sudeep.  My mistake.
>

No worries.

> I'll add this fix for a v4, but I'll wait for a few days to see if I get any
> additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers

Makes sense.

> yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
> ACPI tables that are in error by using these patches, even with the flaws :).
>

Very much true indeed :)

Regards,
Sudeep

[PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 10/09/15 21:43, Al Stone wrote:
> On 09/10/2015 10:20 AM, Sudeep Holla wrote:
>>

[...]

>>
>>  From the code inspection, I can see we have 3 users of acpi_parse_entries not
>> just MADT but also PCC and NUMA/SRAT
>>
>> Something like this solves this issue:
>> -              if (bad_madt_entry(table_header, entry))
>> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
>> +                      bad_madt_entry(table_header, entry)
>>
>>
>> Or am I still missing something ?
>
> Nope, I missed it.  Your fix above will solve the problem; I misunderstood
> how acpi_parse_entries() was being used -- somehow I had it in my head that
> only MADT was in use, and just not seeing that it's being used for several
> other subtable traversals also.  Sorry about that, Sudeep.  My mistake.
>

No worries.

> I'll add this fix for a v4, but I'll wait for a few days to see if I get any
> additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers

Makes sense.

> yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
> ACPI tables that are in error by using these patches, even with the flaws :).
>

Very much true indeed :)

Regards,
Sudeep

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

[Sudeep Holla]

On 10/09/15 21:43, Al Stone wrote:
> On 09/10/2015 10:20 AM, Sudeep Holla wrote:
>>

[...]

>>
>>  From the code inspection, I can see we have 3 users of acpi_parse_entries not
>> just MADT but also PCC and NUMA/SRAT
>>
>> Something like this solves this issue:
>> -              if (bad_madt_entry(table_header, entry))
>> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
>> +                      bad_madt_entry(table_header, entry)
>>
>>
>> Or am I still missing something ?
>
> Nope, I missed it.  Your fix above will solve the problem; I misunderstood
> how acpi_parse_entries() was being used -- somehow I had it in my head that
> only MADT was in use, and just not seeing that it's being used for several
> other subtable traversals also.  Sorry about that, Sudeep.  My mistake.
>

No worries.

> I'll add this fix for a v4, but I'll wait for a few days to see if I get any
> additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers

Makes sense.

> yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
> ACPI tables that are in error by using these patches, even with the flaws :).
>

Very much true indeed :)

Regards,
Sudeep