On 14.08.2025 09:34, Penny, Zheng wrote: > [Public] > >> -----Original Message----- >> From: Jan Beulich <jbeul...@suse.com> >> Sent: Thursday, August 14, 2025 2:40 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 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. > > It needs to meet the following two criteria: > > 1. caps.lowest_nonlinear <= min_perf <= caps.highest_perf > 2. min_perf <= max_perf. If users don't set max_perf at the same time, we are > using the values stored in req.max_perf, which is the last setting.
Hmm, I see. Yet then what about the case of max being set without also setting min? Overall I'm expecting full symmetry in the checking that's being done. Jan
