| Message ID | 20210503192810.36084-7-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 03.05.2021 21:28, Jason Andryuk wrote: > --- a/xen/arch/x86/acpi/cpufreq/hwp.c > +++ b/xen/arch/x86/acpi/cpufreq/hwp.c > @@ -523,6 +523,30 @@ static const struct cpufreq_driver __initconstrel hwp_cpufreq_driver = > .update = hwp_cpufreq_update, > }; > > +int get_hwp_para(struct cpufreq_policy *policy, struct xen_hwp_para *hwp_para) > +{ > + unsigned int cpu = policy->cpu; > + struct hwp_drv_data *data = hwp_drv_data[cpu]; const, perhaps also for the first function parameter, and in general (throughout the series) whenever an item is not meant to be modified. > + if ( data == NULL ) > + return -EINVAL; > + > + hwp_para->hw_feature = > + feature_hwp_activity_window ? XEN_SYSCTL_HWP_FEAT_ACT_WINDOW : 0 | > + feature_hwp_energy_perf ? XEN_SYSCTL_HWP_FEAT_ENERGY_PERF : 0; This needs parentheses added, as | has higher precedence than ?:. > --- a/xen/drivers/acpi/pmstat.c > +++ b/xen/drivers/acpi/pmstat.c > @@ -290,6 +290,12 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) > &op->u.get_para.u.ondemand.sampling_rate, > &op->u.get_para.u.ondemand.up_threshold); > } > + > + if ( !strncasecmp(op->u.get_para.scaling_governor, > + "hwp-internal", CPUFREQ_NAME_LEN) ) > + { > + ret = get_hwp_para(policy, &op->u.get_para.u.hwp_para); > + } > op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid); Nit: Unnecessary parentheses again, and with the leading blank line you also want a trailing one. (As an aside I'm also not overly happy to see the call keyed to the governor name. Is there really no other indication that hwp is in use?) > --- a/xen/include/acpi/cpufreq/cpufreq.h > +++ b/xen/include/acpi/cpufreq/cpufreq.h > @@ -246,4 +246,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq); > void cpufreq_dbs_timer_suspend(void); > void cpufreq_dbs_timer_resume(void); > > +/********************** hwp hypercall helper *************************/ > +int get_hwp_para(struct cpufreq_policy *policy, struct xen_hwp_para *hwp_para); While I can see that the excessive number of stars matches what we have elsewhere in the header, I still wonder if you need to go this far for a single declaration. If you want to stick to this, then to match the rest of the file you want to follow the comment by a blank line. > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -35,7 +35,7 @@ > #include "domctl.h" > #include "physdev.h" > > -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013 > +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014 As long as the size of struct xen_get_cpufreq_para doesn't change (which from the description I imply it doesn't), you don't need to bump the version, I don't think - your change is a pure addition to the interface. > @@ -301,6 +301,23 @@ struct xen_ondemand { > uint32_t up_threshold; > }; > > +struct xen_hwp_para { > + uint16_t activity_window; /* 7bit mantissa and 3bit exponent */ If you go this far with commenting, you should also make the further aspects clear: Which bits these are, and that the exponent is taking 10 as the base (in most other cases one would expect 2). > +#define XEN_SYSCTL_HWP_FEAT_ENERGY_PERF (1 << 0) /* energy_perf range 0-255 if > + 1. Otherwise 0-15 */ > +#define XEN_SYSCTL_HWP_FEAT_ACT_WINDOW (1 << 1) /* activity_window supported > + if 1 */ Style: Comment formatting. You may want to move the comment on separate lines ahead of what they comment. > + uint8_t hw_feature; /* bit flags for features */ > + uint8_t hw_lowest; > + uint8_t hw_most_efficient; > + uint8_t hw_guaranteed; > + uint8_t hw_highest; Any particular reason for the recurring hw_ prefixes? Jan
On Thu, May 27, 2021 at 4:03 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 03.05.2021 21:28, Jason Andryuk wrote: > > --- a/xen/drivers/acpi/pmstat.c > > +++ b/xen/drivers/acpi/pmstat.c > > @@ -290,6 +290,12 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) > > &op->u.get_para.u.ondemand.sampling_rate, > > &op->u.get_para.u.ondemand.up_threshold); > > } > > + > > + if ( !strncasecmp(op->u.get_para.scaling_governor, > > + "hwp-internal", CPUFREQ_NAME_LEN) ) > > + { > > + ret = get_hwp_para(policy, &op->u.get_para.u.hwp_para); > > + } > > op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid); > > Nit: Unnecessary parentheses again, and with the leading blank line > you also want a trailing one. (As an aside I'm also not overly happy > to see the call keyed to the governor name. Is there really no other > indication that hwp is in use?) This is preceded by similar checks for "userspace" and "ondemand", so it is following existing code. Unlike other governors, hwp-internal is static. It could be exported if you want to switch to comparing with cpufreq_driver. > > --- a/xen/include/acpi/cpufreq/cpufreq.h > > +++ b/xen/include/acpi/cpufreq/cpufreq.h > > @@ -246,4 +246,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq); > > void cpufreq_dbs_timer_suspend(void); > > void cpufreq_dbs_timer_resume(void); > > > > +/********************** hwp hypercall helper *************************/ > > +int get_hwp_para(struct cpufreq_policy *policy, struct xen_hwp_para *hwp_para); > > While I can see that the excessive number of stars matches what > we have elsewhere in the header, I still wonder if you need to go > this far for a single declaration. If you want to stick to this, > then to match the rest of the file you want to follow the comment > by a blank line. Will remove. > > --- a/xen/include/public/sysctl.h > > +++ b/xen/include/public/sysctl.h > > @@ -301,6 +301,23 @@ struct xen_ondemand { > > uint32_t up_threshold; > > }; > > > > +struct xen_hwp_para { > > + uint16_t activity_window; /* 7bit mantissa and 3bit exponent */ > > If you go this far with commenting, you should also make the further > aspects clear: Which bits these are, and that the exponent is taking > 10 as the base (in most other cases one would expect 2). Yes, this is much more useful. > > +#define XEN_SYSCTL_HWP_FEAT_ENERGY_PERF (1 << 0) /* energy_perf range 0-255 if > > + 1. Otherwise 0-15 */ > > +#define XEN_SYSCTL_HWP_FEAT_ACT_WINDOW (1 << 1) /* activity_window supported > > + if 1 */ > > Style: Comment formatting. You may want to move the comment on separate > lines ahead of what they comment. > > > + uint8_t hw_feature; /* bit flags for features */ > > + uint8_t hw_lowest; > > + uint8_t hw_most_efficient; > > + uint8_t hw_guaranteed; > > + uint8_t hw_highest; > > Any particular reason for the recurring hw_ prefixes? The idea was to differentiate values provided by CPU hardware from user-configured values. Regards, Jason
On 28.05.2021 15:19, Jason Andryuk wrote: > On Thu, May 27, 2021 at 4:03 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 03.05.2021 21:28, Jason Andryuk wrote: >>> --- a/xen/drivers/acpi/pmstat.c >>> +++ b/xen/drivers/acpi/pmstat.c >>> @@ -290,6 +290,12 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) >>> &op->u.get_para.u.ondemand.sampling_rate, >>> &op->u.get_para.u.ondemand.up_threshold); >>> } >>> + >>> + if ( !strncasecmp(op->u.get_para.scaling_governor, >>> + "hwp-internal", CPUFREQ_NAME_LEN) ) >>> + { >>> + ret = get_hwp_para(policy, &op->u.get_para.u.hwp_para); >>> + } >>> op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid); >> >> Nit: Unnecessary parentheses again, and with the leading blank line >> you also want a trailing one. (As an aside I'm also not overly happy >> to see the call keyed to the governor name. Is there really no other >> indication that hwp is in use?) > > This is preceded by similar checks for "userspace" and "ondemand", so > it is following existing code. Unlike other governors, hwp-internal > is static. It could be exported if you want to switch to comparing > with cpufreq_driver. Hmm, well, then feel free to keep the logic as you have it, except please don't take presence of unnecessary braces as excuse to add more. >>> + uint8_t hw_feature; /* bit flags for features */ >>> + uint8_t hw_lowest; >>> + uint8_t hw_most_efficient; >>> + uint8_t hw_guaranteed; >>> + uint8_t hw_highest; >> >> Any particular reason for the recurring hw_ prefixes? > > The idea was to differentiate values provided by CPU hardware from > user-configured values. I think that follows from their names already without the prefix. I'd prefer if you dropped them, but I'll try to not insist (I may comment on this again in later versions, in case I forgot by then). Jan
diff --git a/xen/arch/x86/acpi/cpufreq/hwp.c b/xen/arch/x86/acpi/cpufreq/hwp.c index f8e6fdbd41..92222d6d85 100644 --- a/xen/arch/x86/acpi/cpufreq/hwp.c +++ b/xen/arch/x86/acpi/cpufreq/hwp.c @@ -523,6 +523,30 @@ static const struct cpufreq_driver __initconstrel hwp_cpufreq_driver = .update = hwp_cpufreq_update, }; +int get_hwp_para(struct cpufreq_policy *policy, struct xen_hwp_para *hwp_para) +{ + unsigned int cpu = policy->cpu; + struct hwp_drv_data *data = hwp_drv_data[cpu]; + + if ( data == NULL ) + return -EINVAL; + + hwp_para->hw_feature = + feature_hwp_activity_window ? XEN_SYSCTL_HWP_FEAT_ACT_WINDOW : 0 | + feature_hwp_energy_perf ? XEN_SYSCTL_HWP_FEAT_ENERGY_PERF : 0; + hwp_para->hw_lowest = data->hw_lowest; + hwp_para->hw_most_efficient = data->hw_most_efficient; + hwp_para->hw_guaranteed = data->hw_guaranteed; + hwp_para->hw_highest = data->hw_highest; + hwp_para->minimum = data->minimum; + hwp_para->maximum = data->maximum; + hwp_para->energy_perf = data->energy_perf; + hwp_para->activity_window = data->activity_window; + hwp_para->desired = data->desired; + + return 0; +} + int hwp_register_driver(void) { int ret; diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c index 1bae635101..3e35c42949 100644 --- a/xen/drivers/acpi/pmstat.c +++ b/xen/drivers/acpi/pmstat.c @@ -290,6 +290,12 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) &op->u.get_para.u.ondemand.sampling_rate, &op->u.get_para.u.ondemand.up_threshold); } + + if ( !strncasecmp(op->u.get_para.scaling_governor, + "hwp-internal", CPUFREQ_NAME_LEN) ) + { + ret = get_hwp_para(policy, &op->u.get_para.u.hwp_para); + } op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid); return ret; diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h index b91859ce5d..42146ca2cf 100644 --- a/xen/include/acpi/cpufreq/cpufreq.h +++ b/xen/include/acpi/cpufreq/cpufreq.h @@ -246,4 +246,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq); void cpufreq_dbs_timer_suspend(void); void cpufreq_dbs_timer_resume(void); +/********************** hwp hypercall helper *************************/ +int get_hwp_para(struct cpufreq_policy *policy, struct xen_hwp_para *hwp_para); + #endif /* __XEN_CPUFREQ_PM_H__ */ diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 039ccf885c..1a6c6397ea 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -35,7 +35,7 @@ #include "domctl.h" #include "physdev.h" -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013 +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014 /* * Read console content from Xen buffer ring. @@ -301,6 +301,23 @@ struct xen_ondemand { uint32_t up_threshold; }; +struct xen_hwp_para { + uint16_t activity_window; /* 7bit mantissa and 3bit exponent */ +#define XEN_SYSCTL_HWP_FEAT_ENERGY_PERF (1 << 0) /* energy_perf range 0-255 if + 1. Otherwise 0-15 */ +#define XEN_SYSCTL_HWP_FEAT_ACT_WINDOW (1 << 1) /* activity_window supported + if 1 */ + uint8_t hw_feature; /* bit flags for features */ + uint8_t hw_lowest; + uint8_t hw_most_efficient; + uint8_t hw_guaranteed; + uint8_t hw_highest; + uint8_t minimum; + uint8_t maximum; + uint8_t desired; + uint8_t energy_perf; +}; + /* * cpufreq para name of this structure named * same as sysfs file name of native linux @@ -332,6 +349,7 @@ struct xen_get_cpufreq_para { union { struct xen_userspace userspace; struct xen_ondemand ondemand; + struct xen_hwp_para hwp_para; } u; int32_t turbo_enabled;
Extend xen_get_cpufreq_para to return hwp parameters. These match the hardware rather closely. We need the hw_features bitmask to indicated fields supported by the actual hardware. The use of uint8_t parameters matches the hardware size. uint32_t entries grows the sysctl_t past the build assertion in setup.c. The uint8_t ranges are supported across multiple generations, so hopefully they won't change. Increment XEN_SYSCTL_INTERFACE_VERSION for the new fields. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- xen/arch/x86/acpi/cpufreq/hwp.c | 24 ++++++++++++++++++++++++ xen/drivers/acpi/pmstat.c | 6 ++++++ xen/include/acpi/cpufreq/cpufreq.h | 3 +++ xen/include/public/sysctl.h | 20 +++++++++++++++++++- 4 files changed, 52 insertions(+), 1 deletion(-)