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

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

[Mario Limonciello]

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(-)

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

Re: [PATCH] x86/ACPI/boot: Use FADT version to check support for online capable

[Zhang, Rui]

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

Re: [PATCH] x86/ACPI/boot: Use FADT version to check support for online capable

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/ACPI/boot: Use FADT version to check support for online capable

[Zhang, Rui]

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

Re: [PATCH] x86/ACPI/boot: Use FADT version to check support for online capable

[Rafael J. Wysocki]

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
>

Re: [PATCH] x86/ACPI/boot: Use FADT version to check support for online capable

[Borislav Petkov]

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.

-- 
Sent from a small device: formatting sucks and brevity is inevitable.

RE: [PATCH] x86/ACPI/boot: Use FADT version to check support for online capable

[Limonciello, Mario]

[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.