[PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

AMD systems support the Monitor/Mwait instructions and these can be used
for ACPI C1 in the same way as on Intel systems, with appropriate BIOS
support.

Allow ffh_cstate_init() to succeed on AMD systems and make the Cstate
description vendor-agnostic.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/acpi/cstate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index 8a908ae..4c5dd5d 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -109,7 +109,7 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx)
 			cx->type);
 	}
 	snprintf(cx->desc,
-			ACPI_CX_DESC_LEN, "ACPI FFH INTEL MWAIT 0x%x",
+			ACPI_CX_DESC_LEN, "ACPI FFH X86 MWAIT 0x%x",
 			cx->address);
 out:
 	return retval;
@@ -169,7 +169,8 @@ static int __init ffh_cstate_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	if (c->x86_vendor != X86_VENDOR_INTEL &&
+	    c->x86_vendor != X86_VENDOR_AMD)
 		return -1;
 
 	cpu_cstate_entry = alloc_percpu(struct cstate_entry);
-- 
2.7.4

Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Borislav Petkov]

On Wed, May 17, 2017 at 09:20:19AM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> AMD systems support the Monitor/Mwait instructions and these can be used
> for ACPI C1 in the same way as on Intel systems, with appropriate BIOS
> support.
> 
> Allow ffh_cstate_init() to succeed on AMD systems and make the Cstate
> description vendor-agnostic.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/acpi/cstate.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index 8a908ae..4c5dd5d 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -109,7 +109,7 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx)
>  			cx->type);
>  	}
>  	snprintf(cx->desc,
> -			ACPI_CX_DESC_LEN, "ACPI FFH INTEL MWAIT 0x%x",
> +			ACPI_CX_DESC_LEN, "ACPI FFH X86 MWAIT 0x%x",
>  			cx->address);
>  out:
>  	return retval;
> @@ -169,7 +169,8 @@ static int __init ffh_cstate_init(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
>  
> -	if (c->x86_vendor != X86_VENDOR_INTEL)
> +	if (c->x86_vendor != X86_VENDOR_INTEL &&
> +	    c->x86_vendor != X86_VENDOR_AMD)
>  		return -1;
>  
>  	cpu_cstate_entry = alloc_percpu(struct cstate_entry);

What about x86_idle?

That whole select_idle_routine() jumping through hoops. That's still
doing default_idle() on Zen, AFAICT.

Or am I missing something?

Because that still asks prefer_mwait_c1_over_halt() and that needs a
family check or whatever...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Monday, May 22, 2017 12:22 PM
>

...
 
> What about x86_idle?
> 
> That whole select_idle_routine() jumping through hoops. That's still doing
> default_idle() on Zen, AFAICT.
> 
> Or am I missing something?
> 
> Because that still asks prefer_mwait_c1_over_halt() and that needs a family
> check or whatever...
> 

I think we leave HALT as the default idle mechanism and allow BIOS to select
other mechanisms through ACPI. 

On AMD, HALT allows the hardware to automatically manage its Cstates. This
is useful if the OS doesn't define multiple states like when we use
default_idle_call().

On the other hand, MWAIT on AMD limits the hardware to using only certain,
shallower Cstates. This is okay if we define individual states and use MWAIT
for some of them. But it would consume more power if used always.

Thanks,
Yazen

Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Borislav Petkov]

On Mon, May 22, 2017 at 08:20:56PM +0000, Ghannam, Yazen wrote:
> On the other hand, MWAIT on AMD limits the hardware to using only certain,
> shallower Cstates. This is okay if we define individual states and use MWAIT
> for some of them. But it would consume more power if used always.

Let me see if I understand it correctly:

Even though we used to do HLT on previous families as idling with HLT
*is* the preferred method until now, with your change you're moving
*every* AMD machine out there to do MWAIT now.

Lemme look at the F15h BKDG:

"2.5.3.2

C-state Request Interface

C-states are dynamically requested by software and are exposed through
ACPI objects (see 2.5.3.6 [ACPI Processor C-state Objects]). C-states
can be requested on a per-core basis. Software requests a C-state change
in one of two ways, either by executing the HLT instruction or by
reading from an IO address specified by MSRC001_0073[CstateAddr] plus an
offset of 0 through 7 (see D18F4x11[C:8])."

So this doesn't say anything about using MWAIT.

What's up?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On Wed 2017-05-17 09:20:19, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> AMD systems support the Monitor/Mwait instructions and these can be used
> for ACPI C1 in the same way as on Intel systems, with appropriate BIOS
> support.
> 
> Allow ffh_cstate_init() to succeed on AMD systems and make the Cstate
> description vendor-agnostic.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/acpi/cstate.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index 8a908ae..4c5dd5d 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -109,7 +109,7 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx)
>  			cx->type);
>  	}
>  	snprintf(cx->desc,
> -			ACPI_CX_DESC_LEN, "ACPI FFH INTEL MWAIT 0x%x",
> +			ACPI_CX_DESC_LEN, "ACPI FFH X86 MWAIT 0x%x",
>  			cx->address);
>  out:
>  	return retval;

Are you sure no userspace depends on word "INTEL" there?

Does it make sense to include "X86" there?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

RE: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, May 23, 2017 3:59 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-pm@vger.kernel.org; x86@kernel.org; linux-
> kernel@vger.kernel.org; rjw@rjwysocki.net; len.brown@intel.com;
> pavel@ucw.cz
> Subject: Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on
> AMD systems
> 
> On Mon, May 22, 2017 at 08:20:56PM +0000, Ghannam, Yazen wrote:
> > On the other hand, MWAIT on AMD limits the hardware to using only
> > certain, shallower Cstates. This is okay if we define individual
> > states and use MWAIT for some of them. But it would consume more
> power if used always.
> 
> Let me see if I understand it correctly:
> 
> Even though we used to do HLT on previous families as idling with HLT
> *is* the preferred method until now, with your change you're moving
> *every* AMD machine out there to do MWAIT now.
> 

No, AMD systems will continue to use HLT unless the BIOS specifies the
use of MWAIT using a FFH entry in the ACPI _CST.

All this change does is *allow* us to use MWAIT through the FFH
implementation if the BIOS defines it. It doesn't *force* a change.

If the BIOS doesn't define the appropriate _CST entry or it defines it
wrong, then we'll fallback to using HLT.

Thanks,
Yazen

Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Borislav Petkov]

On Tue, May 23, 2017 at 12:50:15PM +0000, Ghannam, Yazen wrote:
> No, AMD systems will continue to use HLT unless the BIOS specifies the
> use of MWAIT using a FFH entry in the ACPI _CST.
> 
> All this change does is *allow* us to use MWAIT through the FFH
> implementation if the BIOS defines it. It doesn't *force* a change.

So that could very well be part of the commit message as it explains
what exactly you're changing.

> If the BIOS doesn't define the appropriate _CST entry or it defines it
> wrong, then we'll fallback to using HLT.

I'm assuming you've tested it on older families to make sure they still
work as expected? I wouldn't trust the BIOS...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD systems

[Ghannam, Yazen]

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Tuesday, May 23, 2017 8:25 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-pm@vger.kernel.org; x86@kernel.org; linux-
> kernel@vger.kernel.org; rjw@rjwysocki.net; len.brown@intel.com
> Subject: Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on
> AMD systems
> 
> On Wed 2017-05-17 09:20:19, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > AMD systems support the Monitor/Mwait instructions and these can be
> > used for ACPI C1 in the same way as on Intel systems, with appropriate
> > BIOS support.
> >
> > Allow ffh_cstate_init() to succeed on AMD systems and make the Cstate
> > description vendor-agnostic.
> >
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> >  arch/x86/kernel/acpi/cstate.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/acpi/cstate.c
> > b/arch/x86/kernel/acpi/cstate.c index 8a908ae..4c5dd5d 100644
> > --- a/arch/x86/kernel/acpi/cstate.c
> > +++ b/arch/x86/kernel/acpi/cstate.c
> > @@ -109,7 +109,7 @@ static long
> acpi_processor_ffh_cstate_probe_cpu(void *_cx)
> >  			cx->type);
> >  	}
> >  	snprintf(cx->desc,
> > -			ACPI_CX_DESC_LEN, "ACPI FFH INTEL MWAIT 0x%x",
> > +			ACPI_CX_DESC_LEN, "ACPI FFH X86 MWAIT 0x%x",
> >  			cx->address);
> >  out:
> >  	return retval;
> 
> Are you sure no userspace depends on word "INTEL" there?
> 

So far I've only seen this description printed by cpupower, and it's just for
information.

> Does it make sense to include "X86" there?

I think so, since MWAIT is available on systems from both Intel and AMD.
Also, this FFH implementation can be shared by both vendors.

Though, as I said above, this description seems to be purely informational,
so it's probably not significant either way. I can remove this if preferred.

Thanks,
Yazen