diff mbox series

x86/ACPI/boot: Use FADT version to check support for online capable

Message ID 20230329174536.6931-1-mario.limonciello@amd.com
State New
Headers
Series x86/ACPI/boot: Use FADT version to check support for online capable |

Commit Message

Mario Limonciello March 29, 2023, 5:45 p.m. UTC 
  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(-)

Comments

Zhang, Rui March 30, 2023, 1:10 a.m. UTC | #1 
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
Borislav Petkov March 30, 2023, 8:41 a.m. UTC | #2 
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.
Zhang, Rui March 30, 2023, 9:29 a.m. UTC | #3 
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
Rafael J. Wysocki March 30, 2023, 6:23 p.m. UTC | #4 
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
>
Borislav Petkov March 30, 2023, 6:39 p.m. UTC | #5 
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.
Mario Limonciello March 30, 2023, 6:51 p.m. UTC | #6 
[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 mbox series

Patch

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,