NB: this patch set is for use against the linux-pm bleeding edge branch.
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 v5: -- 0-day found incorrect data in the table describing allowed MADT subtables; this only affected ACPI 1.0 firmware. Corrected the data to meet the 1.0b spec. -- Rebase to bleeding-edge branch for Rafael Wysocki; this patch set now requires that a patch set from Marc Zyngier be applied first: https://lkml.org/lkml/2015/9/28/421 -- Tested on AMD Seattle (linux-pm tree) also
Changes for v4: -- Remove extraneous white space change (Graeme Gregory) -- acpi_parse_entries() changes also needed a check to make sure that only MADT entries used bad_madt_entry() (Sudeep Holla) -- inadvertent use of 01day build noted that bad_madt_entry() can be static, so added it (Sudeep Holla, Fengguang Wu)
Changes for v3: -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts -- Clearer language in error messages (Graeme Gregory, Timur Tabi) -- Double checked that inserting call to bad_madt_entry() into the function acpi_parse_entries() does not impact current behavior (Sudeep Holla)
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/smp.c | 2 - arch/ia64/kernel/acpi.c | 20 ---- arch/x86/kernel/acpi/boot.c | 27 ----- drivers/acpi/tables.c | 247 +++++++++++++++++++++++++++++++++++++++++- drivers/irqchip/irq-gic.c | 3 - include/linux/acpi.h | 4 - 7 files changed, 246 insertions(+), 65 deletions(-)
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 | 247 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 246 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 6c0f079..a2ed38a 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -210,6 +210,247 @@ 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 10 + * NMI Src 0x3 8 8 8 8 8 8 8 + * Local APIC NMI Struct 0x4 6 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.0b */ + .major_version = 1, + .minor_version = 0, + .madt_version = 1, + .num_types = 5, + .lengths = { 8, 12, 10, 8, 6 } + }, + { /* 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 } + } +}; + +static 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 version for either FADT %d.%d or MADT %d\n", + major, minor, madt->header.revision); + 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; + int proper_len = sizeof(struct acpi_madt_local_sapic) + + strlen(lsapic->uid_string) + 1; + + if (proper_len != entry->length) { + pr_err("Variable length MADT subtable %d is wrong length: %d, should be: %d\n", + entry->type, entry->length, proper_len); + return 1; + } + } + } else { + if (entry->length != len) { + pr_err("MADT subtable %d is wrong length: %d, should be: %d\n", + entry->type, entry->length, len); + return 1; + } + } + + return 0; +} + /** * acpi_parse_entries_array - for each proc_num find a suitable subtable * @@ -265,6 +506,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size, if (max_entries && count >= max_entries) break;
+ if (!strncmp(id, ACPI_SIG_MADT, 4) && + bad_madt_entry(table_header, entry)) + return -EINVAL; + for (i = 0; i < proc_num; i++) { if (entry->type != proc[i].id) continue; @@ -407,7 +652,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; }
-/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?)
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 Reviewed-and-tested-by: Sudeep Holla sudeep.holla@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 | 3 --- 3 files changed, 13 deletions(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 6894205..05656fc 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -18,14 +18,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 d4add30..67b7b48 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1206,9 +1206,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.
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; }
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 e759076..e65bcea 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 */ @@ -463,9 +442,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) { @@ -504,9 +480,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? */
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 51a96a8..dd5bd228 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)) - struct acpi_subtable_proc { int id; acpi_tbl_entry_handler handler;
On 2015/9/30 7:45, Al Stone wrote:
NB: this patch set is for use against the linux-pm bleeding edge branch.
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 v5: -- 0-day found incorrect data in the table describing allowed MADT subtables; this only affected ACPI 1.0 firmware. Corrected the data to meet the 1.0b spec. -- Rebase to bleeding-edge branch for Rafael Wysocki; this patch set now requires that a patch set from Marc Zyngier be applied first: https://lkml.org/lkml/2015/9/28/421 -- Tested on AMD Seattle (linux-pm tree) also
Changes for v4: -- Remove extraneous white space change (Graeme Gregory) -- acpi_parse_entries() changes also needed a check to make sure that only MADT entries used bad_madt_entry() (Sudeep Holla) -- inadvertent use of 01day build noted that bad_madt_entry() can be static, so added it (Sudeep Holla, Fengguang Wu)
Changes for v3: -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts -- Clearer language in error messages (Graeme Gregory, Timur Tabi) -- Double checked that inserting call to bad_madt_entry() into the function acpi_parse_entries() does not impact current behavior (Sudeep Holla) 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
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
On 09/30/2015 03:00 AM, Hanjun Guo wrote:
On 2015/9/30 7:45, Al Stone wrote:
NB: this patch set is for use against the linux-pm bleeding edge branch.
[snip...]
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, Hanjun!
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote:
On 2015/9/30 7:45, Al Stone wrote:
NB: this patch set is for use against the linux-pm bleeding edge branch.
[snip...]
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote:
On 2015/9/30 7:45, Al Stone wrote:
NB: this patch set is for use against the linux-pm bleeding edge branch.
[snip...]
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote:
On 2015/9/30 7:45, Al Stone wrote:
NB: this patch set is for use against the linux-pm bleeding edge branch.
[snip...]
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote:
On 2015/9/30 7:45, Al Stone wrote:
NB: this patch set is for use against the linux-pm bleeding edge branch.
[snip...]
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote:
On 2015/9/30 7:45, Al Stone wrote: > NB: this patch set is for use against the linux-pm bleeding edge > branch. >
[snip...]
For this patch set,
Reviewed-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote: > On 2015/9/30 7:45, Al Stone wrote: >> NB: this patch set is for use against the linux-pm bleeding edge >> branch. >>
[snip...]
> > For this patch set, > > Reviewed-by: Hanjun Guo hanjun.guo@linaro.org > > Thanks > Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
Thanks Hanjun
On 12/10/15 08:04, Hanjun Guo wrote:
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: > On 09/30/2015 03:00 AM, Hanjun Guo wrote: >> On 2015/9/30 7:45, Al Stone wrote: >>> NB: this patch set is for use against the linux-pm bleeding edge >>> branch. >>> > > [snip...] > >> >> For this patch set, >> >> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >> >> Thanks >> Hanjun > > Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
Instead of just removing the check completely on x86, IMO restrict it to some newer/later version of ACPI so you can still force vendors to fix their ACPI tables at-least in future.
It would be good to get such sanity check in the tools used to build those tables, but yes since such static tables can be built in many ways, its difficult to deal it in all those tools.
On 10/12/2015 05:44 PM, Sudeep Holla wrote:
On 12/10/15 08:04, Hanjun Guo wrote:
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: > On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>> On 2015/9/30 7:45, Al Stone wrote: >>>> NB: this patch set is for use against the linux-pm bleeding edge >>>> branch. >>>> >> >> [snip...] >> >>> >>> For this patch set, >>> >>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>> >>> Thanks >>> Hanjun >> >> Thanks, Hanjun! > > Series applied, thanks! > > Rafael >
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
Instead of just removing the check completely on x86, IMO restrict it to some newer/later version of ACPI so you can still force vendors to fix their ACPI tables at-least in future.
I agree.
It would be good to get such sanity check in the tools used to build those tables, but yes since such static tables can be built in many ways, its difficult to deal it in all those tools.
At least we can check that in the FWTS. :)
Thanks Hanjun
On Monday, October 12, 2015 10:44:52 AM Sudeep Holla wrote:
On 12/10/15 08:04, Hanjun Guo wrote:
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: > On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>> On 2015/9/30 7:45, Al Stone wrote: >>>> NB: this patch set is for use against the linux-pm bleeding edge >>>> branch. >>>> >> >> [snip...] >> >>> >>> For this patch set, >>> >>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>> >>> Thanks >>> Hanjun >> >> Thanks, Hanjun! > > Series applied, thanks! > > Rafael >
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
Instead of just removing the check completely on x86, IMO restrict it to some newer/later version of ACPI so you can still force vendors to fix their ACPI tables at-least in future.
No, we can't force vendors to fix their ACPI tables. This is completely unrealistic.
We simly need to deal with the bugs in the ACPI tables in the kernel.
It would be good to get such sanity check in the tools used to build those tables, but yes since such static tables can be built in many ways, its difficult to deal it in all those tools.
As I said to Al, we need those checks in firmware test suites. Having them in the kernel is OK too, but they should cause warnings to be printed to the kernel log instead of causing the kernel to panic.
Thanks, Rafael
On 10/12/2015 01:25 PM, Rafael J. Wysocki wrote:
On Monday, October 12, 2015 10:44:52 AM Sudeep Holla wrote:
On 12/10/15 08:04, Hanjun Guo wrote:
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote: > On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: >> On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >>> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>>> On 2015/9/30 7:45, Al Stone wrote: >>>>> NB: this patch set is for use against the linux-pm bleeding edge >>>>> branch. >>>>> >>> >>> [snip...] >>> >>>> >>>> For this patch set, >>>> >>>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>>> >>>> Thanks >>>> Hanjun >>> >>> Thanks, Hanjun! >> >> Series applied, thanks! >> >> Rafael >> > > Thanks, Rafael! >
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
Thanks for sending this!
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
Instead of just removing the check completely on x86, IMO restrict it to some newer/later version of ACPI so you can still force vendors to fix their ACPI tables at-least in future.
No, we can't force vendors to fix their ACPI tables. This is completely unrealistic.
We simly need to deal with the bugs in the ACPI tables in the kernel.
Unfortunately true. I've had a couple of reports to look at and think through apart from this; it's really quite fascinating how much stuff a slightly stricter table check is turning up. A little surprising, too, but fascinating. A fix is in progress, still needs some testing...
It would be good to get such sanity check in the tools used to build those tables, but yes since such static tables can be built in many ways, its difficult to deal it in all those tools.
As I said to Al, we need those checks in firmware test suites. Having them in the kernel is OK too, but they should cause warnings to be printed to the kernel log instead of causing the kernel to panic.
Thanks, Rafael
Yup. Agreed. For x86, we can't induce kernel panics. Since arm64 is new to the game, we'll be stricter since we can afford to be for now.
On 12/10/15 20:25, Rafael J. Wysocki wrote:
On Monday, October 12, 2015 10:44:52 AM Sudeep Holla wrote:
[...]
Instead of just removing the check completely on x86, IMO restrict it to some newer/later version of ACPI so you can still force vendors to fix their ACPI tables at-least in future.
No, we can't force vendors to fix their ACPI tables. This is completely unrealistic.
No, I was referring to the future platforms *only*
We simly need to deal with the bugs in the ACPI tables in the kernel.
Yes sadly true for existing systems, but if we now place a check for ACPIv6.0 and above, we might avoid seeing such broken tables sometime in future once the kernel with this check in place is used for validation.
It would be good to get such sanity check in the tools used to build those tables, but yes since such static tables can be built in many ways, its difficult to deal it in all those tools.
As I said to Al, we need those checks in firmware test suites. Having them in the kernel is OK too, but they should cause warnings to be printed to the kernel log instead of causing the kernel to panic.
Agreed
On Monday, October 12, 2015 03:04:30 PM Hanjun Guo wrote:
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: > On 09/30/2015 03:00 AM, Hanjun Guo wrote: >> On 2015/9/30 7:45, Al Stone wrote: >>> NB: this patch set is for use against the linux-pm bleeding edge >>> branch. >>> > > [snip...] > >> >> For this patch set, >> >> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >> >> Thanks >> Hanjun > > Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
As I said before to Al, we definitely can't break systems that worked before the commit in question.
Thanks, Rafael
On 10/13/2015 03:21 AM, Rafael J. Wysocki wrote:
On Monday, October 12, 2015 03:04:30 PM Hanjun Guo wrote:
On 10/12/2015 11:58 AM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: > On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>> On 2015/9/30 7:45, Al Stone wrote: >>>> NB: this patch set is for use against the linux-pm bleeding edge >>>> branch. >>>> >> >> [snip...] >> >>> >>> For this patch set, >>> >>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>> >>> Thanks >>> Hanjun >> >> Thanks, Hanjun! > > Series applied, thanks! > > Rafael >
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Thanks! I think I had the right guess, the MADT revision is not right for ACPI 4.0:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)] [004h 0004 4] Table Length : 000000BC [008h 0008 1] *Revision : 01*
I encountered such problem before because the table was just copied from previous version, and without the update for table revision.
I think we may need to ignore the table revision for x86, but restrict it for ARM64, I'd like Al and Rafael's suggestion before I send out a patch.
As I said before to Al, we definitely can't break systems that worked before the commit in question.
Sure :)
Thanks Hanjun
On 10/11/2015 09:58 PM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
On 09/30/2015 03:00 AM, Hanjun Guo wrote: > On 2015/9/30 7:45, Al Stone wrote: >> NB: this patch set is for use against the linux-pm bleeding edge >> branch. >>
[snip...]
> > For this patch set, > > Reviewed-by: Hanjun Guo hanjun.guo@linaro.org > > Thanks > Hanjun
Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Pat,
Would you mind sending a copy of the FADT, also, please? The first of the ACPI messages is a check of version correspondence between the FADT and MADT, while the second message is from looking at just an MADT subtable. Thanks for sending the MADT out -- that helps me quite a lot in thinking this through.
BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look at these, also:
[ 0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623) [ 0.000000] ACPI BIOS Warning (bug): Invalid length for FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
Not inherently dangerous, but definitely sloppy and mind-numbingly easy to avoid, IIRC.
On 10/12/2015 01:52 PM, Al Stone wrote:
On 10/11/2015 09:58 PM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: > On 09/30/2015 03:00 AM, Hanjun Guo wrote: >> On 2015/9/30 7:45, Al Stone wrote: >>> NB: this patch set is for use against the linux-pm bleeding edge >>> branch. >>> > > [snip...] > >> >> For this patch set, >> >> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >> >> Thanks >> Hanjun > > Thanks, Hanjun!
Series applied, thanks!
Rafael
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Pat,
Would you mind sending a copy of the FADT, also, please? The first of the ACPI messages is a check of version correspondence between the FADT and MADT, while the second message is from looking at just an MADT subtable. Thanks for sending the MADT out -- that helps me quite a lot in thinking this through.
BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look at these, also:
[ 0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623) [ 0.000000] ACPI BIOS Warning (bug): Invalid length for FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
Not inherently dangerous, but definitely sloppy and mind-numbingly easy to avoid, IIRC.
Here ya go.
On 10/12/2015 10:06 PM, Pat Erley wrote:
On 10/12/2015 01:52 PM, Al Stone wrote:
On 10/11/2015 09:58 PM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote:
On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: > On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>> On 2015/9/30 7:45, Al Stone wrote: >>>> NB: this patch set is for use against the linux-pm bleeding edge >>>> branch. >>>> >> >> [snip...] >> >>> >>> For this patch set, >>> >>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>> >>> Thanks >>> Hanjun >> >> Thanks, Hanjun! > > Series applied, thanks! > > Rafael >
Thanks, Rafael!
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Pat,
Would you mind sending a copy of the FADT, also, please? The first of the ACPI messages is a check of version correspondence between the FADT and MADT, while the second message is from looking at just an MADT subtable. Thanks for sending the MADT out -- that helps me quite a lot in thinking this through.
BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look at these, also:
[ 0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623) [ 0.000000] ACPI BIOS Warning (bug): Invalid length for FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
Not inherently dangerous, but definitely sloppy and mind-numbingly easy to avoid, IIRC.
Here ya go.
Okay. There's just a lot of weird stuff out there in ACPI-land. I've attached four minor fixes for the special cases that have been reported (well, the last one is actually a fix for a typo in the spec, but just the same...).
These should apply on top of linux-next; would you mind trying them out to make sure I didn't break anything else on your laptop? If they behave as I hope they will, I think I'll have covered all the places where the checking of MADT subtables needs to be be relaxed a bit. These work for me on arm64, but if they work for you and a couple of other testers, then I'll send them to Rafael properly.
Many thanks!
On Wednesday, October 14, 2015 02:20:51 PM Al Stone wrote:
This is a multi-part message in MIME format. --------------020400080004050109020606 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit
On 10/12/2015 10:06 PM, Pat Erley wrote:
On 10/12/2015 01:52 PM, Al Stone wrote:
On 10/11/2015 09:58 PM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote:
On 10/05/2015 10:12 AM, Al Stone wrote: > On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: >> On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >>> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>>> On 2015/9/30 7:45, Al Stone wrote: >>>>> NB: this patch set is for use against the linux-pm bleeding edge >>>>> branch. >>>>> >>> >>> [snip...] >>> >>>> >>>> For this patch set, >>>> >>>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>>> >>>> Thanks >>>> Hanjun >>> >>> Thanks, Hanjun! >> >> Series applied, thanks! >> >> Rafael >> > > Thanks, Rafael! >
Just decided to test out linux-next (to see the new nouveau cleanups). This change set prevents my Lenovo W510 from booting properly.
Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to eventually replace the macro"
Gets the system booting again. I'm attaching my dmesg from the failed boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Pat,
Would you mind sending a copy of the FADT, also, please? The first of the ACPI messages is a check of version correspondence between the FADT and MADT, while the second message is from looking at just an MADT subtable. Thanks for sending the MADT out -- that helps me quite a lot in thinking this through.
BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look at these, also:
[ 0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623) [ 0.000000] ACPI BIOS Warning (bug): Invalid length for FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
Not inherently dangerous, but definitely sloppy and mind-numbingly easy to avoid, IIRC.
Here ya go.
Okay. There's just a lot of weird stuff out there in ACPI-land. I've attached four minor fixes for the special cases that have been reported (well, the last one is actually a fix for a typo in the spec, but just the same...).
These should apply on top of linux-next; would you mind trying them out to make sure I didn't break anything else on your laptop? If they behave as I hope they will, I think I'll have covered all the places where the checking of MADT subtables needs to be be relaxed a bit. These work for me on arm64, but if they work for you and a couple of other testers, then I'll send them to Rafael properly.
Well, you might as well submit them properly right away, so I could pick them up and put them into my linux-next branch, which then might make it easier for some people to test them.
Thanks, Rafael
On 10/14/2015 03:25 PM, Rafael J. Wysocki wrote:
On Wednesday, October 14, 2015 02:20:51 PM Al Stone wrote:
This is a multi-part message in MIME format. --------------020400080004050109020606 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit
On 10/12/2015 10:06 PM, Pat Erley wrote:
On 10/12/2015 01:52 PM, Al Stone wrote:
On 10/11/2015 09:58 PM, Pat Erley wrote:
On 10/11/2015 08:49 PM, Hanjun Guo wrote:
On 10/12/2015 11:08 AM, Pat Erley wrote: > On 10/05/2015 10:12 AM, Al Stone wrote: >> On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote: >>> On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote: >>>> On 09/30/2015 03:00 AM, Hanjun Guo wrote: >>>>> On 2015/9/30 7:45, Al Stone wrote: >>>>>> NB: this patch set is for use against the linux-pm bleeding edge >>>>>> branch. >>>>>> >>>> >>>> [snip...] >>>> >>>>> >>>>> For this patch set, >>>>> >>>>> Reviewed-by: Hanjun Guo hanjun.guo@linaro.org >>>>> >>>>> Thanks >>>>> Hanjun >>>> >>>> Thanks, Hanjun! >>> >>> Series applied, thanks! >>> >>> Rafael >>> >> >> Thanks, Rafael! >> > > Just decided to test out linux-next (to see the new nouveau cleanups). > This change set prevents my Lenovo W510 from booting properly. > > Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to > eventually replace the macro" > > Gets the system booting again. I'm attaching my dmesg from the failed > boot, who wants the acpidump?
[ 0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1 [ 0.000000] ACPI: Error parsing LAPIC address override entry [ 0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
Seems the MADT revision is not right, could you dump the ACPI MADT (APIC) table and send it out? I will take a look :)
Thanks Hanjun
Here ya go, enjoy. Feel free to CC me on any patches that might fix it.
Pat,
Would you mind sending a copy of the FADT, also, please? The first of the ACPI messages is a check of version correspondence between the FADT and MADT, while the second message is from looking at just an MADT subtable. Thanks for sending the MADT out -- that helps me quite a lot in thinking this through.
BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look at these, also:
[ 0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623) [ 0.000000] ACPI BIOS Warning (bug): Invalid length for FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
Not inherently dangerous, but definitely sloppy and mind-numbingly easy to avoid, IIRC.
Here ya go.
Okay. There's just a lot of weird stuff out there in ACPI-land. I've attached four minor fixes for the special cases that have been reported (well, the last one is actually a fix for a typo in the spec, but just the same...).
These should apply on top of linux-next; would you mind trying them out to make sure I didn't break anything else on your laptop? If they behave as I hope they will, I think I'll have covered all the places where the checking of MADT subtables needs to be be relaxed a bit. These work for me on arm64, but if they work for you and a couple of other testers, then I'll send them to Rafael properly.
Well, you might as well submit them properly right away, so I could pick them up and put them into my linux-next branch, which then might make it easier for some people to test them.
Thanks, Rafael
Fair enough. I was just being overly cautious about possible further breakage.
Done. Patch series sent to the list.
-
Al Stone -
Al Stone -
Hanjun Guo -
Hanjun Guo -
Pat Erley -
Rafael J. Wysocki -
Sudeep Holla