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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/arch/x86/acpi/cpufreq/amd-cppc.c
>>> +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
>>> +    /* Only allow values if params bit is set. */
>>> +    if ( (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED) &&
>>> +          set_cppc->desired) ||
>>> +         (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM) &&
>>> +          set_cppc->minimum) ||
>>> +         (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) &&
>>> +          set_cppc->maximum) ||
>>> +         (!(set_cppc->set_params &
>> XEN_SYSCTL_CPPC_SET_ENERGY_PERF) &&
>>> +          set_cppc->energy_perf) )
>>> +        return -EINVAL;
>>
>> ... all the errors checked here are to be ignored when no flag is set at all?
> 
> Yes, values are only meaningful when according flag is properly set, which 
> has been described in the comment for "struct xen_set_cppc_para"

Especially since you stripped the initial part of this comment of mine, it feels
as if you misunderstood my request. What it boils down to is the question 
whether
"if ( set_cppc->set_params == 0 )" shouldn't move after the if() you left in
context above.

>>> +    /*
>>> +     * Validate all parameters
>>> +     * Maximum 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 larger than or equal to minimum 
>>> Performance.
>>> +     */
>>> +    if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) &&
>>> +         (set_cppc->maximum > data->caps.highest_perf ||
>>> +          set_cppc->maximum <
>>> +                        (set_cppc->set_params &
>> XEN_SYSCTL_CPPC_SET_MINIMUM
>>> +                         ? set_cppc->minimum
>>> +                         : data->req.min_perf)) )
>>
>> Too deep indentation (more of this throughout the function), and seeing ...
> 
> Maybe four indention is more proper
> ```
>         if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) &&
>              (set_cppc->maximum > data->caps.highest_perf ||
>               (set_cppc->maximum <
>                           (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM
>                     ? set_cppc->minimum
>                     : data->req.min_perf))) )
> ```

No. In expressions you always want to indent according to pending open
parentheses:

        if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) &&
             (set_cppc->maximum > data->caps.highest_perf ||
              (set_cppc->maximum <
               (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM
                ? set_cppc->minimum
                : data->req.min_perf))) )

>>> +    case XEN_SYSCTL_CPPC_SET_PRESET_NONE:
>>> +        if ( active_mode )
>>> +            policy->policy = CPUFREQ_POLICY_UNKNOWN;
>>> +        break;
>>> +
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>
>> Much of this looks very similar to what patch 09 introduces in
>> amd_cppc_epp_set_policy(). Is it not possible to reduce the redundancy?
>>
> 
> I'll add a new helper to amd_cppc_prepare_policy() to extract common
> 
>>> --- 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.

Jan