Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1647122 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD  systems Borislav Petkov <bp@alien8.de> - 2017-05-22 18:30 +0200
    RE: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD  systems "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-05-22 22:30 +0200
      Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD  systems Borislav Petkov <bp@alien8.de> - 2017-05-23 10:00 +0200
        RE: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD  systems "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-05-23 15:00 +0200
          Re: [PATCH] x86/ACPI/cstate: Allow ACPI C1 FFH MWAIT use on AMD  systems Borislav Petkov <bp@alien8.de> - 2017-05-23 15:10 +0200

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

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

[toc] | [next] | [standalone]

#1647348

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

[toc] | [prev] | [next] | [standalone]

#1647760

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

[toc] | [prev] | [next] | [standalone]

#1647998

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

[toc] | [prev] | [next] | [standalone]

#1648007

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web