Re: [PATCH v7 06/15] cpufreq: Add Hardware P-State (HWP) driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2023 19:09, Jason Andryuk wrote:
> From the Intel SDM: "Hardware-Controlled Performance States (HWP), which
> autonomously selects performance states while utilizing OS supplied
> performance guidance hints."
> 
> Enable HWP to run in autonomous mode by poking the correct MSRs.  HWP is
> disabled by default, and cpufreq=hwp enables it.
> 
> cpufreq= parsing is expanded to allow cpufreq=hwp;xen.  This allows
> trying HWP and falling back to xen if not available.  Only hwp and xen
> are supported for this fallback feature.  hdc is a sub-option under hwp
> (i.e.  cpufreq=hwp,hdc=0) as is verbose.
> 
> There is no interface to configure - xen_sysctl_pm_op/xenpm will
> be extended to configure in subsequent patches.  It will run with the
> default values, which should be the default 0x80 (out of 0x0-0xff)
> energy/performance preference.
> 
> Unscientific powertop measurement of an mostly idle, customized OpenXT
> install:
> A 10th gen 6-core laptop showed battery discharge drop from ~9.x to
> ~7.x watts.
> A 8th gen 4-core laptop dropped from ~10 to ~9
> 
> Power usage depends on many factors, especially display brightness, but
> this does show a power saving in balanced mode when CPU utilization is
> low.
> 
> HWP isn't compatible with an external governor - it doesn't take
> explicit frequency requests.  Therefore a minimal internal governor,
> hwp, is also added as a placeholder.
> 
> While adding to the xen-command-line.pandoc entry, un-nest verbose from
> minfreq.  They are independent.
> 
> With cpufreq=hwp,verbose, HWP prints processor capabilities that are not
> used by the code, like HW_FEEDBACK.  This is done because otherwise
> there isn't a convenient way to query the information.
> 
> Xen doesn't use the HWP interrupt, so it is disabled like in the Linux
> pstate driver.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> +static int cf_check hwp_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +    static union hwp_request initial_req;
> +    unsigned int cpu = policy->cpu;
> +    struct hwp_drv_data *data;
> +    static bool first_run = true;

This would probably better be __read_mostly. Iirc there were some small
adjustments requested elsewhere, so I guess I'll wait for v8 before
checking to see how much of this series is ready to go in.

Jan