[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v7 13/13] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC xen_sysctl_pm_op for amd-cppc driver
On 28.08.2025 08:54, Penny, Zheng wrote: > [Public] > >> -----Original Message----- >> From: Jan Beulich <jbeulich@xxxxxxxx> >> Sent: Thursday, August 28, 2025 2:38 PM >> To: Penny, Zheng <penny.zheng@xxxxxxx> >> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD >> <anthony.perard@xxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; >> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger >> Pau >> Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; >> xen- >> devel@xxxxxxxxxxxxxxxxxxxx >> Subject: Re: [PATCH v7 13/13] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC >> xen_sysctl_pm_op for amd-cppc driver >> >> On 28.08.2025 08:35, Jan Beulich wrote: >>> On 28.08.2025 06:06, Penny, Zheng wrote: >>>>> -----Original Message----- >>>>> From: Jan Beulich <jbeulich@xxxxxxxx> >>>>> Sent: Tuesday, August 26, 2025 12:03 AM >>>>> >>>>> On 22.08.2025 12:52, Penny Zheng wrote: >>>>>> --- a/xen/include/public/sysctl.h >>>>>> +++ b/xen/include/public/sysctl.h >>>>>> @@ -336,8 +336,14 @@ struct xen_ondemand { >>>>>> uint32_t up_threshold; >>>>>> }; >>>>>> >>>>>> +#define CPUFREQ_POLICY_UNKNOWN 0 >>>>>> +#define CPUFREQ_POLICY_POWERSAVE 1 >>>>>> +#define CPUFREQ_POLICY_PERFORMANCE 2 >>>>>> +#define CPUFREQ_POLICY_ONDEMAND 3 >>>>> >>>>> Without XEN_ prefixes they shouldn't appear in a public header. But >>>>> do we need ... >>>>> >>>>>> struct xen_get_cppc_para { >>>>>> /* OUT */ >>>>>> + uint32_t policy; /* CPUFREQ_POLICY_xxx */ >>>>> >>>>> ... the new field at all? Can't you synthesize the kind-of-governor >>>>> into struct xen_get_cpufreq_para's respective field? You invoke both >>>>> sub-ops from xenpm now anyway ... >>>>> >>>> >>>> Maybe I could borrow governor field to indicate policy info, like the >>>> following in >> print_cpufreq_para(), then we don't need to add the new filed "policy" >>>> ``` >>>> + /* Translate governor info to policy info in CPPC active mode */ >>>> + if ( is_cppc_active ) >>>> + { >>>> + if ( !strncmp(p_cpufreq->u.s.scaling_governor, >>>> + "ondemand", CPUFREQ_NAME_LEN) ) >>>> + printf("cppc policy : ondemand\n"); >>>> + else if ( !strncmp(p_cpufreq->u.s.scaling_governor, >>>> + "performance", CPUFREQ_NAME_LEN) ) >>>> + printf("cppc policy : performance\n"); >>>> + >>>> + else if ( !strncmp(p_cpufreq->u.s.scaling_governor, >>>> + "powersave", CPUFREQ_NAME_LEN) ) >>>> + printf("cppc policy : powersave\n"); >>>> + else >>>> + printf("cppc policy : unknown\n"); >>>> + } >>>> + >>>> ``` >>> >>> Something like this is what I was thinking of, yes. >> >> Albeit - why the complicated if/else sequence? Why not simply print the >> field the >> hypercall returned? > > userspace governor doesn't have according policy. I could simplify it to > ``` > if ( !strncmp(p_cpufreq->u.s.scaling_governor, > "userspace", CPUFREQ_NAME_LEN) ) > printf("policy : unknown\n"); > else > printf("policy : %s\n", > p_cpufreq->u.s.scaling_governor); > ``` But the hypervisor shouldn't report back "userspace" when the CPPC driver is in use. ANd I think the tool is okay to trust the hypervisor. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |