Re: [PATCH v10 7/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC xen_sysctl_pm_op for amd-cppc driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2025 18:47, Jason Andryuk wrote:
> On 2025-09-23 11:38, Jan Beulich wrote:
>> On 23.09.2025 06:38, Penny Zheng wrote:
>>> @@ -154,6 +156,17 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op 
>>> *op)
>>>       else
>>>           strlcpy(op->u.get_para.scaling_driver, "Unknown", 
>>> CPUFREQ_NAME_LEN);
>>>   
>>> +    /*
>>> +     * In CPPC active mode, we are borrowing governor field to indicate
>>> +     * policy info.
>>> +     */
>>> +    if ( policy->governor->name[0] )
>>
>> amd_cppc_prepare_policy() may leave ->governor set to NULL afaics, so I
>> think you need to add a NULL check here alongside with pulling this out
>> of ...
>>
>>> +        strlcpy(op->u.get_para.s.scaling_governor,
>>> +                policy->governor->name, CPUFREQ_NAME_LEN);
>>> +    else
>>> +        strlcpy(op->u.get_para.s.scaling_governor, "Unknown",
>>> +                CPUFREQ_NAME_LEN);
>>> +
>>>       if ( !cpufreq_is_governorless(op->cpuid) )
>>>       {
>>
>> ... this conditional.
>>
>> The description also continues to not mention the effect for HWP. I'm
>> actually somewhat confused, I suppose (Jason, question mainly to you):
>> HWP falls in the governor-less category, iirc. Yet it doesn't supply
>> a .setpolicy hook, hence __cpufreq_set_policy() goes through the normal
>> governor setting logic. What's the deal here? The answer may affect
>> whether I'd deem the pulling out of the conditional correct (or at least
>> benign) here as to HWP.
> 
> Hi,
> 
> When I wrote HWP, I didn't realize using .setpolicy would bypass the 
> governor code.  Instead, I implemented the no-op HWP governor, since I 
> thought I needed something as a governor.
> 
> set_hwp_para() actually changes the configuration.  HWP only implements 
> the equivalent of amd-cppc-epp autonomous (active) mode.
> 
> So I think HWP could switch to .setpolicy and drop its governor.
> 
> But looking at this hunk:
> 
>  > @@ -321,10 +327,12 @@ static int set_cpufreq_cppc(struct
>  > xen_sysctl_pm_op *op)
>  >      if ( !policy || !policy->governor )
> 
> Doesn't this !policy->governor prevent amd-cppc-epp from setting 
> parameters?

Only if amd_cppc_prepare_policy() took the default case path of its switch(),
aiui. Penny?

Jan

>  >          return -ENOENT;
>  >
>  > -    if ( !hwp_active() )
>  > -        return -EOPNOTSUPP;
>  > +    if ( hwp_active() )
>  > +        return set_hwp_para(policy, &op->u.set_cppc);
>  > +    if ( processor_pminfo[op->cpuid]->init & XEN_CPPC_INIT )
>  > +        return amd_cppc_set_para(policy, &op->u.set_cppc);
>  >
>  > -    return set_hwp_para(policy, &op->u.set_cppc);
>  > +    return -EOPNOTSUPP;
>  >  }
> 
> So there may be other checks that would need dropping or adjusting to 
> support HWP without a governor.
> 
> Thanks,
> Jason