On 14.08.2025 05:13, Penny, Zheng wrote: > [Public] > >> -----Original Message----- >> From: Jan Beulich <jbeul...@suse.com> >> Sent: Thursday, July 24, 2025 10:44 PM >> To: Penny, Zheng <penny.zh...@amd.com> >> Cc: Huang, Ray <ray.hu...@amd.com>; Anthony PERARD >> <anthony.per...@vates.tech>; Andrew Cooper <andrew.coop...@citrix.com>; >> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger >> Pau >> Monné <roger....@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>; >> xen- >> de...@lists.xenproject.org >> Subject: Re: [PATCH v6 19/19] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC >> xen_sysctl_pm_op for amd-cppc driver >> >> On 11.07.2025 05:51, 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. >>> >>> In get_cpufreq_cppc()/set_cpufreq_cppc(), we include >>> "processor_pminfo[cpuid]->init & XEN_CPPC_INIT" condition check to >>> deal with cpufreq driver in amd-cppc. >>> >>> Also, a new field "policy" has also been added in "struct xen_get_cppc_para" >>> to describe performance policy in active mode. It gets printed with >>> other cppc paras. Move manifest constants "XEN_CPUFREQ_POLICY_xxx" to >>> public header to let it be used in user space tools. Also add a new >>> anchor "XEN_CPUFREQ_POLICY_xxx" for array overrun check. >> >> If only they indeed had XEN_ prefixes. >> >>> Signed-off-by: Penny Zheng <penny.zh...@amd.com> >>> --- >>> v1 -> v2: >>> - Give the variable des_perf an initializer of 0 >>> - Use the strncmp()s directly in the if() >>> --- >>> v3 -> v4 >>> - refactor comments >>> - remove double blank lines >>> - replace amd_cppc_in_use flag with XEN_PROCESSOR_PM_CPPC >>> --- >>> v4 -> v5: >>> - add new field "policy" in "struct xen_cppc_para" >>> - add new performamce policy XEN_CPUFREQ_POLICY_BALANCE >>> - drop string comparisons with "processor_pminfo[cpuid]->init & >> XEN_CPPC_INIT" >>> and "cpufreq.setpolicy == NULL" >>> - Blank line ahead of the main "return" of a function >>> - refactor comments, commit message and title >>> --- >>> v5 -> v6: >>> - remove duplicated manifest constants, and just move it to public >>> header >>> - use "else if" to avoid confusion that it looks as if both paths >>> could be taken >>> - add check for legitimate perf values >>> - use "unknown" instead of "none" >>> - introduce "CPUFREQ_POLICY_END" for array overrun check in user space >>> tools >>> + (set_cppc->maximum > data->caps.highest_perf || >>> + set_cppc->maximum < data->caps.lowest_nonlinear_perf) ) >>> + return -EINVAL; >>> + /* >>> + * Minimum performance may be set to any performance value in the range >>> + * [Nonlinear Lowest Performance, Highest Performance], inclusive but >>> must >>> + * be set to a value that is less than or equal to Maximum Performance. >>> + */ >>> + if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM && >>> + (set_cppc->minimum < data->caps.lowest_nonlinear_perf || >>> + (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM && >>> + set_cppc->minimum > set_cppc->maximum) || >>> + (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) && >> >> Hmm, I find this confusing to read, and was first thinking the ! was wrong >> here. Imo >> such is better expressed with the conditional operator: >> >> >> set_cppc->minimum > (set_cppc->set_params & >> XEN_SYSCTL_CPPC_SET_MAXIMUM >> ? set_cppc->maximum >> : data->req.max_perf) >> > > Thx, understood! > >> Which also makes it easier to spot that here you use data->req, when in the >> minimum check you use data->caps. Why this difference? >> > > minimum check has two boundary check, > left boundary check is against data->caps.lowest_nonlinear_perf. And right > boundary check is against data->req.max_perf. As it shall not only not larger > than caps.highest_perf , but also req.max_perf. The relation between max_perf > and highest_perf is validated in the maximum check. So here, we are only > considering max_perf
I still don't get why one check is against capabilities (permitted values) why the other is again what's currently set. Jan
