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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2025 20:17, Jason Andryuk wrote:
> On 2025-09-02 23:14, Penny, Zheng wrote:
>> [Public]
>>
>>> -----Original Message-----
>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>> Sent: Thursday, August 28, 2025 7:07 PM
>>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
>>> <anthony.perard@xxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
>>> Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>>> Subject: Re: [PATCH v8 8/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC
>>> xen_sysctl_pm_op for amd-cppc driver
>>>
>>> On 28.08.2025 12:06, 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] )
>>>> +        strlcpy(op->u.get_para.u.s.scaling_governor,
>>>> +                policy->governor->name, CPUFREQ_NAME_LEN);
>>>> +    else
>>>> +        strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown",
>>>> +                CPUFREQ_NAME_LEN);
>>>
>>> Isn't pulling this ...
>>>
>>>>       if ( !cpufreq_is_governorless(op->cpuid) )
>>>>       {
>>>>           if ( !(scaling_available_governors =
>>>
>>> ... out of this if()'s body going to affect HWP? It's not clear to me 
>>> whether that would
>>> be entirely benign.
>>>
>>
>> HWP has its own unique "hwp" governor. So, imo, it may not affect.
> 
> get_hwp_para() writes into the same union:
> op->u.get_para.u.cppc_para
> op->u.get_para.u.s.scaling_governor

Not anymore as of "tools/cpufreq: extract CPPC para from cpufreq para".

> Which is why I avoided it for hwp.
> 
> I guess writing scaling_governor first and then overwriting it still 
> ends up with the same data in cppc_para.  Seems a little messy though.
> 
> Penny, I'm confused by this comment:
> +    /*
> +     * In CPPC active mode, we are borrowing governor field to indicate
> +     * policy info.
> +     */
> 
> You have CPPC active and passive modes - which uses a governor and which 
> uses get_cppc?
> 
> It seems like only writing the scaling governor inside
> if ( !cpufreq_is_governorless )
> 
> should be correct since it's using the union.  Am I missing something?

The union is now fake; it has just a single member, and hence would better
be dropped. I've commented correspondingly on v9.

Jan