Message ID | 20230329174536.6931-1-mario.limonciello@amd.com (mailing list archive) |
---|---|
State | Handled Elsewhere, archived |
Headers | show |
Series | x86/ACPI/boot: Use FADT version to check support for online capable | expand |
On Wed, 2023-03-29 at 12:45 -0500, Mario Limonciello wrote: > ACPI 6.3 introduced the online capable bit, and also introduced MADT > version 5. > > This was used to distinguish whether the offset storing online > capable > could be used. However ACPI 6.2b has MADT version "45" which is for > an errata version of the ACPI 6.2 spec. I made a double check. In https://uefi.org/sites/default/files/resources/ACPI_6_2.pdf, MADT revision is 4. In https://uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf, MADT revision is 45. In https://uefi.org/sites/default/files/resources/ACPI_6_2_B_final_Jan30.pdf MADT revision is 5. So you probably mean 6.2a has MADT revision "45" here? > This means that the Linux code > for detecting availability of MADT will mistakingly flag ACPI 6.2b as > supporting online capable which is inaccurate as it's an ACPI 6.3 > feature. > > Instead use the FADT major and minor revision fields to distingush > this. > > Reported-by: Eric DeVolder <eric.devolder@oracle.com> > Reported-by: Borislav Petkob <bp@alien8.de> > Fixes: aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not online > capable") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > arch/x86/kernel/acpi/boot.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/acpi/boot.c > b/arch/x86/kernel/acpi/boot.c > index 1c38174b5f01..e92e3292fef7 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -146,7 +146,10 @@ static int __init acpi_parse_madt(struct > acpi_table_header *table) > > pr_debug("Local APIC address 0x%08x\n", madt->address); > } > - if (madt->header.revision >= 5) > + > + if (acpi_gbl_FADT.header.revision > 6 || > + (acpi_gbl_FADT.header.revision == 6 && > + acpi_gbl_FADT.minor_revision >= 3)) > acpi_support_online_capable = true; Better to have a comment here? For me, it is hard to understand this by reading the code directly. thanks, rui
On Thu, Mar 30, 2023 at 01:10:07AM +0000, Zhang, Rui wrote: > In https://uefi.org/sites/default/files/resources/ACPI_6_2.pdf, > MADT revision is 4. > > In > https://uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf, > MADT revision is 45. This is a hack to fix some ACPI erratum or whatever. > In > https://uefi.org/sites/default/files/resources/ACPI_6_2_B_final_Jan30.pdf > MADT revision is 5. > > So you probably mean 6.2a has MADT revision "45" here? No, forget MADT. The thing should check whether the ACPI revision is 6.3. And this is what it does below.
On Thu, 2023-03-30 at 10:41 +0200, Borislav Petkov wrote: > On Thu, Mar 30, 2023 at 01:10:07AM +0000, Zhang, Rui wrote: > > In https://uefi.org/sites/default/files/resources/ACPI_6_2.pdf, > > MADT revision is 4. > > > > In > > https://uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf, > > MADT revision is 45. > > This is a hack to fix some ACPI erratum or whatever. > > > In > > https://uefi.org/sites/default/files/resources/ACPI_6_2_B_final_Jan30.pdf > > MADT revision is 5. > > > > So you probably mean 6.2a has MADT revision "45" here? > > No, forget MADT. > > The thing should check whether the ACPI revision is 6.3. And this is > what it does below. Yes, I agree. As the original changelog mentioned that "ACPI spec 6.2b has MADT revision 45", I was just checking if that statement is accurate or not. thanks, rui
On Wed, Mar 29, 2023 at 7:46 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > ACPI 6.3 introduced the online capable bit, and also introduced MADT > version 5. > > This was used to distinguish whether the offset storing online capable > could be used. However ACPI 6.2b has MADT version "45" which is for > an errata version of the ACPI 6.2 spec. This means that the Linux code > for detecting availability of MADT will mistakingly flag ACPI 6.2b as > supporting online capable which is inaccurate as it's an ACPI 6.3 feature. > > Instead use the FADT major and minor revision fields to distingush this. > > Reported-by: Eric DeVolder <eric.devolder@oracle.com> > Reported-by: Borislav Petkob <bp@alien8.de> s/Petkob/Petkov/ I suppose? Would have been nice to CC this to linux-acpi (done now). Anyway, x86 guys, are you going to handle this or do you want me to do that? > Fixes: aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not online capable") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > arch/x86/kernel/acpi/boot.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index 1c38174b5f01..e92e3292fef7 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -146,7 +146,10 @@ static int __init acpi_parse_madt(struct acpi_table_header *table) > > pr_debug("Local APIC address 0x%08x\n", madt->address); > } > - if (madt->header.revision >= 5) > + > + if (acpi_gbl_FADT.header.revision > 6 || > + (acpi_gbl_FADT.header.revision == 6 && > + acpi_gbl_FADT.minor_revision >= 3)) > acpi_support_online_capable = true; > > default_acpi_madt_oem_check(madt->header.oem_id, > -- > 2.34.1 >
On March 30, 2023 8:23:38 PM GMT+02:00, "Rafael J. Wysocki" <rafael@kernel.org> wrote: >s/Petkob/Petkov/ I suppose? Fixed. >Would have been nice to CC this to linux-acpi (done now). Sorry about that. >Anyway, x86 guys, are you going to handle this or do you want me to do that? Yeah, all queued into tip:x86/urgent. Holler if something's still missing. The whole situation got on my nerves so I queued both fixes and am hoping all is fixed now. Ufff.
[Public] > On March 30, 2023 8:23:38 PM GMT+02:00, "Rafael J. Wysocki" > <rafael@kernel.org> wrote: > >s/Petkob/Petkov/ I suppose? > > Fixed. Thx. > > >Would have been nice to CC this to linux-acpi (done now). > > Sorry about that. Sorry, I used ./scripts/get_maintainer.pl and it didn't suggest linux-acpi. > > >Anyway, x86 guys, are you going to handle this or do you want me to do > that? > > Yeah, all queued into tip:x86/urgent. Holler if something's still missing. The > whole situation got on my nerves so I queued both fixes and am hoping all is > fixed now. Ufff. > > -- > Sent from a small device: formatting sucks and brevity is inevitable.
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 1c38174b5f01..e92e3292fef7 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -146,7 +146,10 @@ static int __init acpi_parse_madt(struct acpi_table_header *table) pr_debug("Local APIC address 0x%08x\n", madt->address); } - if (madt->header.revision >= 5) + + if (acpi_gbl_FADT.header.revision > 6 || + (acpi_gbl_FADT.header.revision == 6 && + acpi_gbl_FADT.minor_revision >= 3)) acpi_support_online_capable = true; default_acpi_madt_oem_check(madt->header.oem_id,
ACPI 6.3 introduced the online capable bit, and also introduced MADT version 5. This was used to distinguish whether the offset storing online capable could be used. However ACPI 6.2b has MADT version "45" which is for an errata version of the ACPI 6.2 spec. This means that the Linux code for detecting availability of MADT will mistakingly flag ACPI 6.2b as supporting online capable which is inaccurate as it's an ACPI 6.3 feature. Instead use the FADT major and minor revision fields to distingush this. Reported-by: Eric DeVolder <eric.devolder@oracle.com> Reported-by: Borislav Petkob <bp@alien8.de> Fixes: aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not online capable") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- arch/x86/kernel/acpi/boot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)