Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
On Tue, Jul 25, 2023 at 10:37 AM Jan Beulich wrote: > > On 25.07.2023 15:26, Jason Andryuk wrote: > > #define hwp_err(cpu, fmt, args...) \ > > printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, args) > > ..., just that you're missing the ##: > > #define hwp_err(cpu, fmt, args...) \ > printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ## args) Thanks. I thought I was trying to avoid the use of "##", which sent me off in the wrong direction. Regards, Jason
Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
On 25.07.2023 15:26, Jason Andryuk wrote:
> On Tue, Jul 25, 2023 at 2:27 AM Jan Beulich wrote:
>>
>> On 24.07.2023 21:49, Jason Andryuk wrote:
>>> On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich wrote:
On 24.07.2023 14:58, Jason Andryuk wrote:
> --- /dev/null
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> +#define hwp_err(cpu, fmt, ...) \
> +printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
> +#define hwp_info(fmt, ...)printk(XENLOG_INFO "HWP: " fmt,
> ##__VA_ARGS__)
I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
we have a few instances (mainly in code inherited from elsewhere), but I
think it ought to either be plain C99 style (without the ##) or purely
the gcc extension form (not using __VA_ARGS__).
>>>
>>> Using plain __VA_ARGS__ doesn't work for the cases without arguments:
>>> arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’
>>> token
>>>78 | printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__); \
>>> | ^
>>> arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
>>> 201 | hwp_verbose("disabled: No energy/performance
>>> preference available");
>>> | ^~~
>>
>> Of course.
>>
>>> I can use "__VA_OPT__(,) __VA_ARGS__" though.
>>
>> __VA_OPT__ is yet newer than C99, so this is an option only if all
>> compilers we continue to support actually know of this.
>
> Right, sorry.
>
>> We have no
>> uses of it in the codebase so far, which suggests you might best go
>> with the longstanding gcc extension here.
>
> FTAOD, "##__VA_ARGS__" is the longstanding extension?
No. But you've apparently found it ...
> It's the only
> one I've been able to get to compile. I'm reading
> https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html and it
> mentions a few different extensions.
>
> This part seemed promising:
> """
> This has been fixed in C++20, and GNU CPP also has a pair of
> extensions which deal with this problem.
>
> First, in GNU CPP, and in C++ beginning in C++20, you are allowed to
> leave the variable argument out entirely:
>
> eprintf ("success!\n")
> → fprintf(stderr, "success!\n", );
> """
>
> However, it doesn't seem to actually work for me. I still get an
> error like the one above for plain __VA_ARGS__. That is for:
>
> #define hwp_err(cpu, fmt, args...) \
> printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, args)
..., just that you're missing the ##:
#define hwp_err(cpu, fmt, args...) \
printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ## args)
Jan
Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
On Tue, Jul 25, 2023 at 2:27 AM Jan Beulich wrote:
>
> On 24.07.2023 21:49, Jason Andryuk wrote:
> > On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich wrote:
> >> On 24.07.2023 14:58, Jason Andryuk wrote:
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> >>> +#define hwp_err(cpu, fmt, ...) \
> >>> +printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
> >>> +#define hwp_info(fmt, ...)printk(XENLOG_INFO "HWP: " fmt,
> >>> ##__VA_ARGS__)
> >>
> >> I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
> >> we have a few instances (mainly in code inherited from elsewhere), but I
> >> think it ought to either be plain C99 style (without the ##) or purely
> >> the gcc extension form (not using __VA_ARGS__).
> >
> > Using plain __VA_ARGS__ doesn't work for the cases without arguments:
> > arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’
> > token
> >78 | printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__); \
> > | ^
> > arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
> > 201 | hwp_verbose("disabled: No energy/performance
> > preference available");
> > | ^~~
>
> Of course.
>
> > I can use "__VA_OPT__(,) __VA_ARGS__" though.
>
> __VA_OPT__ is yet newer than C99, so this is an option only if all
> compilers we continue to support actually know of this.
Right, sorry.
> We have no
> uses of it in the codebase so far, which suggests you might best go
> with the longstanding gcc extension here.
FTAOD, "##__VA_ARGS__" is the longstanding extension? It's the only
one I've been able to get to compile. I'm reading
https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html and it
mentions a few different extensions.
This part seemed promising:
"""
This has been fixed in C++20, and GNU CPP also has a pair of
extensions which deal with this problem.
First, in GNU CPP, and in C++ beginning in C++20, you are allowed to
leave the variable argument out entirely:
eprintf ("success!\n")
→ fprintf(stderr, "success!\n", );
"""
However, it doesn't seem to actually work for me. I still get an
error like the one above for plain __VA_ARGS__. That is for:
#define hwp_err(cpu, fmt, args...) \
printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, args)
I can drop "fmt" from hwp_info() and hwp_verbose() to just use
__VA_ARGS__, but that doesn't work for hwp_err() since we want to
re-order fmt and cpu inside the macro.
Thanks,
Jason
Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
On 24.07.2023 21:49, Jason Andryuk wrote:
> On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich wrote:
>> On 24.07.2023 14:58, Jason Andryuk wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
>>> @@ -0,0 +1,521 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * hwp.c cpufreq driver to run Intel Hardware P-States (HWP)
>>> + *
>>> + * Copyright (C) 2021 Jason Andryuk
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +static bool __ro_after_init feature_hwp_notification;
>>> +static bool __ro_after_init feature_hwp_activity_window;
>>> +
>>> +static bool __ro_after_init feature_hdc;
>>> +
>>> +static bool __ro_after_init opt_cpufreq_hdc = true;
>>> +
>>> +union hwp_request
>>> +{
>>> +struct
>>> +{
>>> +unsigned int min_perf:8;
>>> +unsigned int max_perf:8;
>>> +unsigned int desired:8;
>>> +unsigned int energy_perf:8;
>>> +unsigned int activity_window:10;
>>> +bool package_control:1;
>>> +unsigned int :16;
>>> +bool activity_window_valid:1;
>>> +bool energy_perf_valid:1;
>>> +bool desired_valid:1;
>>> +bool max_perf_valid:1;
>>> +bool min_perf_valid:1;
>>> +};
>>> +uint64_t raw;
>>> +};
>>> +
>>> +struct hwp_drv_data
>>> +{
>>> +union
>>> +{
>>> +uint64_t hwp_caps;
>>> +struct
>>> +{
>>> +unsigned int highest:8;
>>> +unsigned int guaranteed:8;
>>> +unsigned int most_efficient:8;
>>> +unsigned int lowest:8;
>>> +unsigned int :32;
>>> +} hw;
>>> +};
>>> +union hwp_request curr_req;
>>> +int ret;
>>> +uint16_t activity_window;
>>> +uint8_t minimum;
>>> +uint8_t maximum;
>>> +uint8_t desired;
>>> +uint8_t energy_perf;
>>> +};
>>> +static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data);
>>> +
>>> +#define hwp_err(cpu, fmt, ...) \
>>> +printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
>>> +#define hwp_info(fmt, ...)printk(XENLOG_INFO "HWP: " fmt,
>>> ##__VA_ARGS__)
>>
>> I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
>> we have a few instances (mainly in code inherited from elsewhere), but I
>> think it ought to either be plain C99 style (without the ##) or purely
>> the gcc extension form (not using __VA_ARGS__).
>
> Using plain __VA_ARGS__ doesn't work for the cases without arguments:
> arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’ token
>78 | printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__); \
> | ^
> arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
> 201 | hwp_verbose("disabled: No energy/performance
> preference available");
> | ^~~
Of course.
> I can use "__VA_OPT__(,) __VA_ARGS__" though.
__VA_OPT__ is yet newer than C99, so this is an option only if all
compilers we continue to support actually know of this. We have no
uses of it in the codebase so far, which suggests you might best go
with the longstanding gcc extension here.
>>> +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;
>>> +bool first_run = false;
>>> +
>>> +data = xzalloc(struct hwp_drv_data);
>>> +if ( !data )
>>> +return -ENOMEM;
>>> +
>>> +policy->governor = _gov_hwp;
>>> +
>>> +per_cpu(hwp_drv_data, cpu) = data;
>>> +
>>> +on_selected_cpus(cpumask_of(cpu), hwp_init_msrs, policy, 1);
>>> +
>>> +if ( data->curr_req.raw == -1 )
>>> +{
>>> +hwp_err(cpu, "Could not initialize HWP properly\n");
>>> +per_cpu(hwp_drv_data, cpu) = NULL;
>>> +xfree(data);
>>> +return -ENODEV;
>>> +}
>>> +
>>> +data->minimum = data->curr_req.min_perf;
>>> +data->maximum = data->curr_req.max_perf;
>>> +data->desired = data->curr_req.desired;
>>> +data->energy_perf = data->curr_req.energy_perf;
>>> +data->activity_window = data->curr_req.activity_window;
>>> +
>>> +if ( initial_req.raw == 0 )
>>> +{
>>> +hwp_verbose("CPU%u: HWP_CAPABILITIES: %016lx\n", cpu,
>>> data->hwp_caps);
>>> +initial_req = data->curr_req;
>>> +first_run = true;
>>> +}
>>
>> What part of data->curr_req is guaranteed to be non-0 (for the condition
>> around ...
>>
>>> +if ( first_run || data->curr_req.raw != initial_req.raw )
>>> +hwp_verbose("CPU%u: rdmsr HWP_REQUEST %016lx\n", cpu,
>>> +data->curr_req.raw);
>>
>> ... this logging to be effective)?
>
> Hmmm. I think you are correct that there is no guarantee that
> data->curr_req will be non-zero. i.e. a BIOS could zero the whole
> register. In practice, I see 0x8000ff01 -
Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich wrote:
>
> On 24.07.2023 14:58, Jason Andryuk wrote:
> > --- /dev/null
> > +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> > @@ -0,0 +1,521 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * hwp.c cpufreq driver to run Intel Hardware P-States (HWP)
> > + *
> > + * Copyright (C) 2021 Jason Andryuk
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static bool __ro_after_init feature_hwp_notification;
> > +static bool __ro_after_init feature_hwp_activity_window;
> > +
> > +static bool __ro_after_init feature_hdc;
> > +
> > +static bool __ro_after_init opt_cpufreq_hdc = true;
> > +
> > +union hwp_request
> > +{
> > +struct
> > +{
> > +unsigned int min_perf:8;
> > +unsigned int max_perf:8;
> > +unsigned int desired:8;
> > +unsigned int energy_perf:8;
> > +unsigned int activity_window:10;
> > +bool package_control:1;
> > +unsigned int :16;
> > +bool activity_window_valid:1;
> > +bool energy_perf_valid:1;
> > +bool desired_valid:1;
> > +bool max_perf_valid:1;
> > +bool min_perf_valid:1;
> > +};
> > +uint64_t raw;
> > +};
> > +
> > +struct hwp_drv_data
> > +{
> > +union
> > +{
> > +uint64_t hwp_caps;
> > +struct
> > +{
> > +unsigned int highest:8;
> > +unsigned int guaranteed:8;
> > +unsigned int most_efficient:8;
> > +unsigned int lowest:8;
> > +unsigned int :32;
> > +} hw;
> > +};
> > +union hwp_request curr_req;
> > +int ret;
> > +uint16_t activity_window;
> > +uint8_t minimum;
> > +uint8_t maximum;
> > +uint8_t desired;
> > +uint8_t energy_perf;
> > +};
> > +static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data);
> > +
> > +#define hwp_err(cpu, fmt, ...) \
> > +printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
> > +#define hwp_info(fmt, ...)printk(XENLOG_INFO "HWP: " fmt,
> > ##__VA_ARGS__)
>
> I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
> we have a few instances (mainly in code inherited from elsewhere), but I
> think it ought to either be plain C99 style (without the ##) or purely
> the gcc extension form (not using __VA_ARGS__).
Using plain __VA_ARGS__ doesn't work for the cases without arguments:
arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’ token
78 | printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__); \
| ^
arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
201 | hwp_verbose("disabled: No energy/performance
preference available");
| ^~~
I can use "__VA_OPT__(,) __VA_ARGS__" though.
> > +#define hwp_verbose(fmt, ...) \
> > +({\
> > +if ( cpufreq_verbose )\
> > +printk(XENLOG_DEBUG "HWP: " fmt, ##__VA_ARGS__); \
>
> (One more here then.)
>
> > +static bool hwp_handle_option(const char *s, const char *end)
>
> __init?
Yes, thanks.
> > +static void hwp_get_cpu_speeds(struct cpufreq_policy *policy)
> > +{
> > +uint32_t base_khz, max_khz, bus_khz, edx;
> > +
> > +cpuid(0x16, _khz, _khz, _khz, );
> > +
> > +policy->cpuinfo.perf_freq = base_khz * 1000;
> > +policy->cpuinfo.min_freq = base_khz * 1000;
> > +policy->cpuinfo.max_freq = max_khz * 1000;
> > +policy->min = base_khz * 1000;
> > +policy->max = max_khz * 1000;
>
> Earlier on I asked what about cases where the CPUID output yields
> some zero values (as I know can happen). Iirc you said this doesn't
> affect functionality, but wouldn't it make sense to have a comment
> to this effect here (proving the cases were thought through).
Sure.
> > +static void cf_check hwp_init_msrs(void *info)
> > +{
> > +struct cpufreq_policy *policy = info;
> > +struct hwp_drv_data *data = this_cpu(hwp_drv_data);
> > +uint64_t val;
> > +
> > +/*
> > + * Package level MSR, but we don't have a good idea of packages here,
> > so
> > + * just do it everytime.
> > + */
> > +if ( rdmsr_safe(MSR_PM_ENABLE, val) )
> > +{
> > +hwp_err(policy->cpu, "rdmsr_safe(MSR_PM_ENABLE)\n");
> > +data->curr_req.raw = -1;
> > +return;
> > +}
> > +
> > +/* Ensure we don't generate interrupts */
> > +if ( feature_hwp_notification )
> > +wrmsr_safe(MSR_HWP_INTERRUPT, 0);
> > +
> > +if ( !(val & PM_ENABLE_HWP_ENABLE) )
> > +{
> > +val |= PM_ENABLE_HWP_ENABLE;
> > +if ( wrmsr_safe(MSR_PM_ENABLE, val) )
> > +{
> > +hwp_err(policy->cpu, "wrmsr_safe(MSR_PM_ENABLE, %lx)\n", val);
> > +data->curr_req.raw = -1;
> > +return;
> > +
Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
On 24.07.2023 14:58, Jason Andryuk wrote:
> --- /dev/null
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -0,0 +1,521 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * hwp.c cpufreq driver to run Intel Hardware P-States (HWP)
> + *
> + * Copyright (C) 2021 Jason Andryuk
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static bool __ro_after_init feature_hwp_notification;
> +static bool __ro_after_init feature_hwp_activity_window;
> +
> +static bool __ro_after_init feature_hdc;
> +
> +static bool __ro_after_init opt_cpufreq_hdc = true;
> +
> +union hwp_request
> +{
> +struct
> +{
> +unsigned int min_perf:8;
> +unsigned int max_perf:8;
> +unsigned int desired:8;
> +unsigned int energy_perf:8;
> +unsigned int activity_window:10;
> +bool package_control:1;
> +unsigned int :16;
> +bool activity_window_valid:1;
> +bool energy_perf_valid:1;
> +bool desired_valid:1;
> +bool max_perf_valid:1;
> +bool min_perf_valid:1;
> +};
> +uint64_t raw;
> +};
> +
> +struct hwp_drv_data
> +{
> +union
> +{
> +uint64_t hwp_caps;
> +struct
> +{
> +unsigned int highest:8;
> +unsigned int guaranteed:8;
> +unsigned int most_efficient:8;
> +unsigned int lowest:8;
> +unsigned int :32;
> +} hw;
> +};
> +union hwp_request curr_req;
> +int ret;
> +uint16_t activity_window;
> +uint8_t minimum;
> +uint8_t maximum;
> +uint8_t desired;
> +uint8_t energy_perf;
> +};
> +static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data);
> +
> +#define hwp_err(cpu, fmt, ...) \
> +printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
> +#define hwp_info(fmt, ...)printk(XENLOG_INFO "HWP: " fmt, ##__VA_ARGS__)
I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
we have a few instances (mainly in code inherited from elsewhere), but I
think it ought to either be plain C99 style (without the ##) or purely
the gcc extension form (not using __VA_ARGS__).
> +#define hwp_verbose(fmt, ...) \
> +({\
> +if ( cpufreq_verbose )\
> +printk(XENLOG_DEBUG "HWP: " fmt, ##__VA_ARGS__); \
(One more here then.)
> +static bool hwp_handle_option(const char *s, const char *end)
__init?
> +static void hwp_get_cpu_speeds(struct cpufreq_policy *policy)
> +{
> +uint32_t base_khz, max_khz, bus_khz, edx;
> +
> +cpuid(0x16, _khz, _khz, _khz, );
> +
> +policy->cpuinfo.perf_freq = base_khz * 1000;
> +policy->cpuinfo.min_freq = base_khz * 1000;
> +policy->cpuinfo.max_freq = max_khz * 1000;
> +policy->min = base_khz * 1000;
> +policy->max = max_khz * 1000;
Earlier on I asked what about cases where the CPUID output yields
some zero values (as I know can happen). Iirc you said this doesn't
affect functionality, but wouldn't it make sense to have a comment
to this effect here (proving the cases were thought through).
> +static void cf_check hwp_init_msrs(void *info)
> +{
> +struct cpufreq_policy *policy = info;
> +struct hwp_drv_data *data = this_cpu(hwp_drv_data);
> +uint64_t val;
> +
> +/*
> + * Package level MSR, but we don't have a good idea of packages here, so
> + * just do it everytime.
> + */
> +if ( rdmsr_safe(MSR_PM_ENABLE, val) )
> +{
> +hwp_err(policy->cpu, "rdmsr_safe(MSR_PM_ENABLE)\n");
> +data->curr_req.raw = -1;
> +return;
> +}
> +
> +/* Ensure we don't generate interrupts */
> +if ( feature_hwp_notification )
> +wrmsr_safe(MSR_HWP_INTERRUPT, 0);
> +
> +if ( !(val & PM_ENABLE_HWP_ENABLE) )
> +{
> +val |= PM_ENABLE_HWP_ENABLE;
> +if ( wrmsr_safe(MSR_PM_ENABLE, val) )
> +{
> +hwp_err(policy->cpu, "wrmsr_safe(MSR_PM_ENABLE, %lx)\n", val);
> +data->curr_req.raw = -1;
> +return;
> +}
> +}
> +
> +if ( rdmsr_safe(MSR_HWP_CAPABILITIES, data->hwp_caps) )
> +{
> +hwp_err(policy->cpu, "rdmsr_safe(MSR_HWP_CAPABILITIES)\n");
> +goto error;
> +}
> +
> +if ( rdmsr_safe(MSR_HWP_REQUEST, data->curr_req.raw) )
> +{
> +hwp_err(policy->cpu, "rdmsr_safe(MSR_HWP_REQUEST)\n");
> +goto error;
> +}
> +
> +/*
> + * Check for APERF/MPERF support in hardware
> + * also check for boost/turbo support
> + */
> +intel_feature_detect(policy);
Nit: The comment could do with adding at least a comma or semicolon.
> +if ( feature_hdc &&
> + (!hdc_set_pkg_hdc_ctl(policy->cpu, opt_cpufreq_hdc) ||
> + !hdc_set_pm_ctl1(policy->cpu, opt_cpufreq_hdc)) )
> +{
> +hwp_err(policy->cpu, "Disabling HDC support\n");
> +feature_hdc =
[PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver
>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
---
We disable on cpuid_level < 0x16. cpuid(0x16) is used to get the cpu
frequencies for calculating the APERF/MPERF. Without it, things would
still work, but the average cpu frequency output would be wrong.
My 8th & 10th gen test systems both report:
(XEN) HWP: 1 notify: 1 act_window: 1 energy_perf: 1 pkg_level: 0 peci: 0
(XEN) HWP: Hardware Duty Cycling (HDC) supported
(XEN) HWP: HW_FEEDBACK not supported
We can't use parse_boolean() since it requires a single name=val string
and cpufreq_handle_common_option is provided two strings. Use
parse_bool() and manual handle no-hwp.
FAST_IA32_HWP_REQUEST was removed in v2. The check in v1 was wrong,
it's a model specific feature and the CPUID bit is only available
after enabling via the MSR. Support was untested since I don't have
hardware with the feature. Writes are expected to be infrequent, so
just leave it out.
---
v2:
Alphabetize headers
Re-work driver registration
name hwp_drv_data anonymous union "hw"
Drop hwp_verbose_cont
style cleanups
Condense hwp_governor switch
hwp_cpufreq_target remove .raw from hwp_req assignment
Use typed-pointer in a few functions
Pass type to xzalloc
Add HWP_ENERGY_PERF_BALANCE/IA32_ENERGY_BIAS_BALANCE defines
Add XEN_HWP_GOVERNOR define for "hwp-internal"
Capitalize CPUID and MSR defines
Change '_' to '-' for energy-perf & act-window
Read-modify-write MSRs updates
Use FAST_IA32_HWP_REQUEST_MSR_ENABLE define
constify pointer in hwp_set_misc_turbo
Add space after non-fallthrough break in governor switch
Add IA32_ENERGY_BIAS_MASK define
Check CPUID_PM_LEAK for energy bias when needed
Fail initialization with curr_req = -1
Fold hwp_read_capabilities into hwp_init_msrs
Add command line cpufreq=xen:hwp
Add command line cpufreq=xen:hdc
Use per_cpu for hwp_drv_data pointers
Move hwp_energy_perf_bias call into hwp_write_request
energy_perf 0 is valid, so hwp_energy_perf_bias cannot be skipped
Ensure we don't generate interrupts
Remove Fast Write of Uncore MSR
Initialize hwp_drv_data from curr_req
Use SPDX line instead of license text in hwp.c
v3:
Add cf_check to cpufreq_gov_hwp_init() - Marek
Print cpuid_level with %#x - Marek
v4:
Use BIT() for CPUID and MSR bits
Move __initdata after type
Add __ro_after_init to feature_*
Remove aperf/mperf comment
Move feature_hwp_energy_perf { to newline
Remove _IA32_ infix
Use unsigned int & bool for bitfields
Require energy perf pref (Remove ENERGY_PERF_BIAS support)
Initialize activity_window
Return errors on wrmsr failure
Change command line to: cpufreq=xen:hwp
Move hdc into the hwp-specific handle_options
Drop feature_hwp_energy_perf, feature_hwp_pkg_level_ctl & feature_hwp_peci
Print features before exiting when energy/performance preference isn't available
Disable HWP MSR on initialization error
Change hwp_ print macros to add prefixes
Disable HDC when hdc=0 - (opt_hdc no longer initdata)
Mark hwp governor internal and use "hwp" name
Add XEN_HWP_DRIVER
Use top-level cpufreq=hwp command line option
Document that cpufreq=hwp falls back to cpufreq=xen without hardware
Add SPDX suffix GPL-2.0-only
v5:
Use _AC() macro in MSR_IA32_MISC_ENABLE_TURBO_DISENGAGE definition
hwp_err arg re-ordering
Use XEN_HWP_DRIVER_NAME
Use cpufreq.h for all declarations
Clear feature_hdc on failure and print
