[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 15/15] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC xen_sysctl_pm_op for amd-cppc driver
On 06.03.2025 09:39, Penny Zheng wrote: > Introduce helper set_amd_cppc_para and get_amd_cppc_para to > SET/GET CPPC-related para for amd-cppc/amd-cppc-epp driver. > > Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx> > --- > v1 -> v2: > - Give the variable des_perf an initializer of 0 > - Use the strncmp()s directly in the if() > --- > xen/arch/x86/acpi/cpufreq/amd-cppc.c | 124 +++++++++++++++++++++++++++ > xen/drivers/acpi/pmstat.c | 20 ++++- > xen/include/acpi/cpufreq/cpufreq.h | 5 ++ > 3 files changed, 145 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/acpi/cpufreq/amd-cppc.c > b/xen/arch/x86/acpi/cpufreq/amd-cppc.c > index 606bb648b3..28c13b09c8 100644 > --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c > @@ -32,6 +32,7 @@ > > static bool __ro_after_init opt_active_mode; > static DEFINE_PER_CPU_READ_MOSTLY(uint8_t, epp_init); > +static bool __ro_after_init amd_cppc_in_use; > > struct amd_cppc_drv_data > { > @@ -513,6 +514,123 @@ static int cf_check amd_cppc_epp_set_policy(struct > cpufreq_policy *policy) > return amd_cppc_epp_update_limit(policy); > } > > +int get_amd_cppc_para(unsigned int cpu, > + struct xen_cppc_para *cppc_para) > +{ > + const struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu); > + > + if ( data == NULL ) > + return -ENODATA; > + > + cppc_para->features = 0; > + cppc_para->lowest = data->caps.lowest_perf; > + cppc_para->lowest_nonlinear = data->caps.lowest_nonlinear_perf; > + cppc_para->nominal = data->caps.nominal_perf; > + cppc_para->highest = data->caps.highest_perf; > + cppc_para->minimum = data->req.min_perf; > + cppc_para->maximum = data->req.max_perf; > + cppc_para->desired = data->req.des_perf; > + cppc_para->energy_perf = data->req.epp; > + > + return 0; > +} > + > +int set_amd_cppc_para(const struct cpufreq_policy *policy, > + const struct xen_set_cppc_para *set_cppc) > +{ > + unsigned int cpu = policy->cpu; > + struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu); > + uint8_t max_perf, min_perf, des_perf = 0, epp; > + > + if ( data == NULL ) > + return -ENOENT; > + > + /* Validate all parameters - Disallow reserved bits. */ > + if ( set_cppc->minimum > UINT8_MAX || set_cppc->maximum > UINT8_MAX || > + set_cppc->desired > UINT8_MAX || set_cppc->energy_perf > UINT8_MAX ) > + return -EINVAL; > + > + /* Only allow values if params bit is set. */ > + if ( (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED) && > + set_cppc->desired) || > + (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM) && > + set_cppc->minimum) || > + (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) && > + set_cppc->maximum) || > + (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ENERGY_PERF) && > + set_cppc->energy_perf) ) > + return -EINVAL; > + > + /* Activity window not supported in MSR */ > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW ) > + return -EOPNOTSUPP; > + > + /* Return if there is nothing to do. */ > + if ( set_cppc->set_params == 0 ) > + return 0; > + > + epp = per_cpu(epp_init, cpu); > + /* Apply presets */ > + /* > + * XEN_SYSCTL_CPPC_SET_PRESET_POWERSAVE/PERFORMANCE/BALANCE are > + * for amd-cppc in active mode, min_perf could be set with lowest_perf > + * representing the T-state range of performance levels, while > + * XEN_SYSCTL_CPPC_SET_PRESET_NONE is for amd-cppc in passive mode, it > + * depends on governor to do performance scaling, setting with > + * lowest_nonlinear_perf to ensures performance in P-state range. > + */ Nit: There are again two consecutive comments here. The active / passive mode distinction mentioned in the comment isn't reflected anywhere in the code. It's the XEN_SYSCTL_CPPC_SET_DESIRED which distinguishes them, yet that flag isn't mentioned in the comment. > + switch ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_PRESET_MASK ) > + { > + case XEN_SYSCTL_CPPC_SET_PRESET_POWERSAVE: > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED ) > + return -EINVAL; > + min_perf = data->caps.lowest_perf; > + max_perf = data->caps.highest_perf; These are still not not both ".lowest_perf", and I still don't understand - due to the lack of a comment - why that is. > + epp = CPPC_ENERGY_PERF_MAX_POWERSAVE; > + break; > + > + case XEN_SYSCTL_CPPC_SET_PRESET_PERFORMANCE: > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED ) > + return -EINVAL; > + min_perf = data->caps.highest_perf; > + max_perf = data->caps.highest_perf; > + epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE; > + break; > + > + case XEN_SYSCTL_CPPC_SET_PRESET_BALANCE: > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED ) > + return -EINVAL; > + min_perf = data->caps.lowest_perf; > + max_perf = data->caps.highest_perf; > + epp = CPPC_ENERGY_PERF_BALANCE; > + break; > + > + case XEN_SYSCTL_CPPC_SET_PRESET_NONE: > + min_perf = data->caps.lowest_nonlinear_perf; > + max_perf = data->caps.highest_perf; > + break; > + > + default: > + return -EINVAL; > + } > + > + /* Further customize presets if needed */ > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM ) > + min_perf = set_cppc->minimum; > + > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM ) > + max_perf = set_cppc->maximum; > + > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ENERGY_PERF ) > + epp = set_cppc->energy_perf; > + > + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED ) > + des_perf = set_cppc->desired; Considering these I even less understand what the comment further up is about. > + return amd_cppc_write_request(cpu, min_perf, des_perf, max_perf, epp); > +} > + > + Nit (not for the first time, I think): No double blank lines please. > @@ -533,6 +651,11 @@ amd_cppc_epp_driver = > .exit = amd_cppc_cpufreq_cpu_exit, > }; > > +bool amd_cppc_active(void) > +{ > + return amd_cppc_in_use; > +} > + > int __init amd_cppc_register_driver(void) > { > int ret; > @@ -552,6 +675,7 @@ int __init amd_cppc_register_driver(void) > > /* Remove possible fallback option */ > xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX; > + amd_cppc_in_use = true; Is this separate flag really needed? Can't you go from xen_processor_pmbits? > --- a/xen/drivers/acpi/pmstat.c > +++ b/xen/drivers/acpi/pmstat.c > @@ -261,7 +261,16 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) > !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME, > CPUFREQ_NAME_LEN) ) > ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para); > - else > + else if ( !strncmp(op->u.get_para.scaling_driver, > XEN_AMD_CPPC_DRIVER_NAME, > + CPUFREQ_NAME_LEN) || > + !strncmp(op->u.get_para.scaling_driver, > XEN_AMD_CPPC_EPP_DRIVER_NAME, Overlong line again. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |