[v2] cpufreq: intel_pstate: Add Icelake servers support in no-HWP mode
diff mbox series

Message ID 20210513132051.31465-1-ggherdovich@suse.cz
State In Next
Commit fbdc21e9b038d00d0d56fa4e0f7701d42ae08f00
Headers show
Series
  • [v2] cpufreq: intel_pstate: Add Icelake servers support in no-HWP mode
Related show

Commit Message

Giovanni Gherdovich May 13, 2021, 1:20 p.m. UTC
Users may disable HWP in firmware, in which case intel_pstate wouldn't load
unless the CPU model is explicitly supported.

Add ICELAKE_X to the list of CPUs that can register intel_pstate while not
advertising the HWP capability. Without this change, an ICELAKE_X in no-HWP
mode could only use the acpi_cpufreq frequency scaling driver.

See also commit d8de7a44e11f ("cpufreq: intel_pstate: Add Skylake servers
support").

Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
---
This replaces https://lore.kernel.org/lkml/20210513075930.22657-1-ggherdovich@suse.cz

 drivers/cpufreq/intel_pstate.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Doug Smythies May 14, 2021, 3:31 p.m. UTC | #1
Hi All,

Can I on-board to this patch or do you want me to submit another?
I want to add COMETLAKE (tested), as below:

... Doug

On Thu, May 13, 2021 at 6:21 AM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
>
> Users may disable HWP in firmware, in which case intel_pstate wouldn't load
> unless the CPU model is explicitly supported.
>
> Add ICELAKE_X to the list of CPUs that can register intel_pstate while not
> advertising the HWP capability. Without this change, an ICELAKE_X in no-HWP
> mode could only use the acpi_cpufreq frequency scaling driver.
>
> See also commit d8de7a44e11f ("cpufreq: intel_pstate: Add Skylake servers
> support").
>
> Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> ---
> This replaces https://lore.kernel.org/lkml/20210513075930.22657-1-ggherdovich@suse.cz
>
>  drivers/cpufreq/intel_pstate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f0401064d7aa..28c9733e0dce 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2087,6 +2087,7 @@ static const struct x86_cpu_id intel_pstate_cpu_ids[] = {
>         X86_MATCH(ATOM_GOLDMONT,        core_funcs),
>         X86_MATCH(ATOM_GOLDMONT_PLUS,   core_funcs),
>         X86_MATCH(SKYLAKE_X,            core_funcs),
> +       X86_MATCH(ICELAKE_X,            core_funcs),
   +       X86_MATCH(COMETLAKE,          core_funcs),
>         {}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, intel_pstate_cpu_ids);
> --
> 2.26.2
>
Giovanni Gherdovich May 14, 2021, 8:33 p.m. UTC | #2
On Fri, 2021-05-14 at 08:31 -0700, Doug Smythies wrote:
> Hi All,
> 
> Can I on-board to this patch or do you want me to submit another?
> I want to add COMETLAKE (tested), as below:
> 
> ... Doug

Hello Doug!

Wait, why you don't want to use HWP? It's such a fantastic technology!

:) I'm just teasing you.

More seriously: 

when COMETLAKE is not in that list, can you confirm that if you go into the
BIOS config at boot, and disable HWP from there, then intel_pstate does *not* load?

Does it say "intel_pstate: CPU model not supported" in the dmesg log?

The control may be somewhere around "power mangement" in the BIOS config, and
may be called "Enable/disable Intel Speed Shift".

I'm asking because I've just checked on two Dell laptops, one Skylake and the
other Kabylake, and the menu is there in the BIOS config to disable HWP,
but if I disable it... nothing happens. "lscpu" shows all the hwp flags as usual:

    # lscpu | grep Flags | tr ' ' '\n' | grep hwp
    hwp
    hwp_notify
    hwp_act_window
    hwp_epp

and turbostat gives me:

    # turbostat -Summary -i 1 : 2>&1 | grep MSR_PM_ENABLE
    cpu0: MSR_PM_ENABLE: 0x00000001 (HWP)

Which is to say, on the Intel client machines I have, the firmware doesn't
seem to be able to hide HWP from the OS. Buggy BIOS? Maybe, the fact of the
matter is, I wouldn't need to add, say, KABYLAKE to that list, based on my
experience.

The other side of the issue is that, from my understanding, the
preferred/supported way to disable HWP is to boot with intel_pstate=no_hwp,
and that list is a sort of "known exceptions" that people really can't live
without (it's mostly server CPUs, and mostly because of unfortunate firmware
defaults). Otherwise you'd see the entire intel-family.h file in there.


Cheers,
Giovanni

> 
> On Thu, May 13, 2021 at 6:21 AM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> > Users may disable HWP in firmware, in which case intel_pstate wouldn't load
> > unless the CPU model is explicitly supported.
> > 
> > Add ICELAKE_X to the list of CPUs that can register intel_pstate while not
> > advertising the HWP capability. Without this change, an ICELAKE_X in no-HWP
> > mode could only use the acpi_cpufreq frequency scaling driver.
> > 
> > See also commit d8de7a44e11f ("cpufreq: intel_pstate: Add Skylake servers
> > support").
> > 
> > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> > ---
> > This replaces https://lore.kernel.org/lkml/20210513075930.22657-1-ggherdovich@suse.cz
> > 
> >  drivers/cpufreq/intel_pstate.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index f0401064d7aa..28c9733e0dce 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2087,6 +2087,7 @@ static const struct x86_cpu_id intel_pstate_cpu_ids[] = {
> >         X86_MATCH(ATOM_GOLDMONT,        core_funcs),
> >         X86_MATCH(ATOM_GOLDMONT_PLUS,   core_funcs),
> >         X86_MATCH(SKYLAKE_X,            core_funcs),
> > +       X86_MATCH(ICELAKE_X,            core_funcs),
>    +       X86_MATCH(COMETLAKE,          core_funcs),
> >         {}
> >  };
> >  MODULE_DEVICE_TABLE(x86cpu, intel_pstate_cpu_ids);
> > --
> > 2.26.2
> >
Doug Smythies May 14, 2021, 10:12 p.m. UTC | #3
On Fri, May 14, 2021 at 1:33 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> On Fri, 2021-05-14 at 08:31 -0700, Doug Smythies wrote:
>
> > Can I on-board to this patch or do you want me to submit another?
> > I want to add COMETLAKE (tested), as below:
> >
> > ... Doug
>
> Hello Doug!

Hi Giovanni,
Thank you for your reply.
>
> Wait, why you don't want to use HWP? It's such a fantastic technology!
>
> :) I'm just teasing you.
>
> More seriously:
>
> when COMETLAKE is not in that list, can you confirm that if you go into the
> BIOS config at boot, and disable HWP from there, then intel_pstate does *not* load?

Yes, already tested before my original reply.

> Does it say "intel_pstate: CPU model not supported" in the dmesg log?

That I did not check, but if I boot now with an unmodified kernel
5.13-rc1 (i.e. without this patch):

[    0.369323] intel_pstate: CPU model not supported

> The control may be somewhere around "power mangement" in the BIOS config, and
> may be called "Enable/disable Intel Speed Shift".

Yes.

> I'm asking because I've just checked on two Dell laptops, one Skylake and the
> other Kabylake, and the menu is there in the BIOS config to disable HWP,
> but if I disable it... nothing happens. "lscpu" shows all the hwp flags as usual:

Motherboard here is ASUS PRIME Z490-A.
CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz

>     # lscpu | grep Flags | tr ' ' '\n' | grep hwp
>     hwp
>     hwp_notify
>     hwp_act_window
>     hwp_epp

Here, for some reason I have to do it this way (sudo) or your command
doesn't work properly. Results herein confirmed by looking at the
"Flags" output manually without filtering:

intel_speed_shift = Disabled in BIOS:

doug@s19:~$ sudo lscpu | tr ' ' '\n' | grep hwp
doug@s19:~$

intel_speed_shift = Auto in BIOS

$ sudo lscpu | tr ' ' '\n' | grep hwp
hwp
hwp_notify
hwp_act_window
hwp_epp

> and turbostat gives me:
>
>     # turbostat -Summary -i 1 : 2>&1 | grep MSR_PM_ENABLE
>     cpu0: MSR_PM_ENABLE: 0x00000001 (HWP)

Here:

intel_speed_shift = Disabled in BIOS:

root@s19:/home/doug#
/home/doug/temp-k-git/linux/tools/power/x86/turbostat/turbostat
-Summary -i 1 : 2>&1 | grep MSR_PM_ENABLE
root@s19:/home/doug#

intel_speed_shift = Auto in BIOS (the default setting)

root@s19:/home/doug#
/home/doug/temp-k-git/linux/tools/power/x86/turbostat/turbostat
-Summary -i 1 : 2>&1 | grep MSR_PM_ENABLE
cpu0: MSR_PM_ENABLE: 0x00000001 (HWP)

or with "intel_pstate=no_hwp"

root@s19:/home/doug#
/home/doug/temp-k-git/linux/tools/power/x86/turbostat/turbostat
-Summary -i 1 : 2>&1 | grep MSR_PM_ENABLE
cpu0: MSR_PM_ENABLE: 0x00000000 (No-HWP)

> Which is to say, on the Intel client machines I have, the firmware doesn't
> seem to be able to hide HWP from the OS. Buggy BIOS? Maybe, the fact of the
> matter is, I wouldn't need to add, say, KABYLAKE to that list, based on my
> experience.

My experience (hardware) differs from yours with respect to this.

> The other side of the issue is that, from my understanding, the
> preferred/supported way to disable HWP is to boot with intel_pstate=no_hwp,
> and that list is a sort of "known exceptions" that people really can't live
> without (it's mostly server CPUs, and mostly because of unfortunate firmware
> defaults). Otherwise you'd see the entire intel-family.h file in there.

I'm not sure how to respond here. Yes, I'd expect to see a big list
here, and in the recently added TCC Offset thermal stuff and in the
recently added turbostat patches to deal with a TCC offset. I do not
understand doing things partially only. But that is a bigger/broader
subject than herein.

That said, yes, "intel_pstate=no_hwp" is what I normally do. And my
BIOS normally has "Intel Speed Shift = AUTO", which is the default.

... deleted the rest ...

... Doug
Srinivas Pandruvada May 15, 2021, 2:58 a.m. UTC | #4
On Fri, 2021-05-14 at 22:33 +0200, Giovanni Gherdovich wrote:
> On Fri, 2021-05-14 at 08:31 -0700, Doug Smythies wrote:
> > Hi All,
> > 
> > Can I on-board to this patch or do you want me to submit another?
> > I want to add COMETLAKE (tested), as below:
> > 
> > ... Doug
> 
> Hello Doug!
> 
> Wait, why you don't want to use HWP? It's such a fantastic
> technology!
> 
> :) I'm just teasing you.
> 
> More seriously: 
> 
> when COMETLAKE is not in that list, can you confirm that if you go
> into the
> BIOS config at boot, and disable HWP from there, then intel_pstate
> does *not* load?
> 
> Does it say "intel_pstate: CPU model not supported" in the dmesg log?
> 
> The control may be somewhere around "power mangement" in the BIOS
> config, and
> may be called "Enable/disable Intel Speed Shift".
> 
> I'm asking because I've just checked on two Dell laptops, one Skylake
> and the
> other Kabylake, and the menu is there in the BIOS config to disable
> HWP,
> but if I disable it... nothing happens. "lscpu" shows all the hwp
> flags as usual:
> 
>     # lscpu | grep Flags | tr ' ' '\n' | grep hwp
>     hwp
>     hwp_notify
>     hwp_act_window
>     hwp_epp
> 
> and turbostat gives me:
> 
>     # turbostat -Summary -i 1 : 2>&1 | grep MSR_PM_ENABLE
>     cpu0: MSR_PM_ENABLE: 0x00000001 (HWP)
> 
> Which is to say, on the Intel client machines I have, the firmware
> doesn't
> seem to be able to hide HWP from the OS. Buggy BIOS? Maybe, the fact
> of the
> matter is, I wouldn't need to add, say, KABYLAKE to that list, based
> on my
> experience.

When you disable in BIOS on these systems, it just hides HWP control
via ACPI CPC table. It doesn't disable HWP CPU feature.

Thanks,
Srinivas

> 
> The other side of the issue is that, from my understanding, the
> preferred/supported way to disable HWP is to boot with
> intel_pstate=no_hwp,
> and that list is a sort of "known exceptions" that people really
> can't live
> without (it's mostly server CPUs, and mostly because of unfortunate
> firmware
> defaults). Otherwise you'd see the entire intel-family.h file in
> there.
> 
> 
> Cheers,
> Giovanni
> 
> > 
> > On Thu, May 13, 2021 at 6:21 AM Giovanni Gherdovich <
> > ggherdovich@suse.cz> wrote:
> > > Users may disable HWP in firmware, in which case intel_pstate
> > > wouldn't load
> > > unless the CPU model is explicitly supported.
> > > 
> > > Add ICELAKE_X to the list of CPUs that can register intel_pstate
> > > while not
> > > advertising the HWP capability. Without this change, an ICELAKE_X
> > > in no-HWP
> > > mode could only use the acpi_cpufreq frequency scaling driver.
> > > 
> > > See also commit d8de7a44e11f ("cpufreq: intel_pstate: Add Skylake
> > > servers
> > > support").
> > > 
> > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> > > ---
> > > This replaces 
> > > https://lore.kernel.org/lkml/20210513075930.22657-1-ggherdovich@suse.cz
> > > 
> > >  drivers/cpufreq/intel_pstate.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > b/drivers/cpufreq/intel_pstate.c
> > > index f0401064d7aa..28c9733e0dce 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -2087,6 +2087,7 @@ static const struct x86_cpu_id
> > > intel_pstate_cpu_ids[] = {
> > >         X86_MATCH(ATOM_GOLDMONT,        core_funcs),
> > >         X86_MATCH(ATOM_GOLDMONT_PLUS,   core_funcs),
> > >         X86_MATCH(SKYLAKE_X,            core_funcs),
> > > +       X86_MATCH(ICELAKE_X,            core_funcs),
> >    +       X86_MATCH(COMETLAKE,          core_funcs),
> > >         {}
> > >  };
> > >  MODULE_DEVICE_TABLE(x86cpu, intel_pstate_cpu_ids);
> > > --
> > > 2.26.2
> > > 
>
Rafael J. Wysocki May 17, 2021, 3:26 p.m. UTC | #5
On Fri, May 14, 2021 at 5:31 PM Doug Smythies <dsmythies@telus.net> wrote:
>
> Hi All,
>
> Can I on-board to this patch or do you want me to submit another?

Please send another one.
Giovanni Gherdovich May 18, 2021, 11:33 a.m. UTC | #6
On Mon, 2021-05-17 at 17:26 +0200, Rafael J. Wysocki wrote:
> On Fri, May 14, 2021 at 5:31 PM Doug Smythies <dsmythies@telus.net> wrote:
> > Hi All,
> > 
> > Can I on-board to this patch or do you want me to submit another?
> 
> Please send another one.

Hello Rafael, Doug,

I can resend a series with two patches, one adding ICELAKE_X and the
second adding COMETLAKE to the intel_pstate_cpu_ids array.
I'll prepare it.


Giovanni
Doug Smythies May 19, 2021, 5:37 a.m. UTC | #7
On Tue, May 18, 2021 at 4:33 AM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> On Mon, 2021-05-17 at 17:26 +0200, Rafael J. Wysocki wrote:
> > On Fri, May 14, 2021 at 5:31 PM Doug Smythies <dsmythies@telus.net> wrote:
> > > Hi All,
> > >
> > > Can I on-board to this patch or do you want me to submit another?
> >
> > Please send another one.
>
> Hello Rafael, Doug,
>
> I can resend a series with two patches, one adding ICELAKE_X and the
> second adding COMETLAKE to the intel_pstate_cpu_ids array.
> I'll prepare it.

That is very nice of you. Thanks.

... Doug

Patch
diff mbox series

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index f0401064d7aa..28c9733e0dce 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2087,6 +2087,7 @@  static const struct x86_cpu_id intel_pstate_cpu_ids[] = {
 	X86_MATCH(ATOM_GOLDMONT,	core_funcs),
 	X86_MATCH(ATOM_GOLDMONT_PLUS,	core_funcs),
 	X86_MATCH(SKYLAKE_X,		core_funcs),
+	X86_MATCH(ICELAKE_X,		core_funcs),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, intel_pstate_cpu_ids);