| Message ID | 1495030819-4347-1-git-send-email-Yazen.Ghannam@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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...
> -----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
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?
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
> -----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
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...
> -----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
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);