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 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
> @@ -557,6 +557,187 @@ static int cf_check amd_cppc_epp_set_policy(struct 
> cpufreq_policy *policy)
>      return 0;
>  }
>  
> +#ifdef CONFIG_PM_OP
> +int get_amd_cppc_para(const struct cpufreq_policy *policy,
> +                      struct xen_get_cppc_para *cppc_para)

amd_cppc_get_para() and ...

> +{
> +    const struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data,
> +                                                   policy->cpu);
> +
> +    if ( data == NULL )
> +        return -ENODATA;
> +
> +    cppc_para->policy           = policy->policy;
> +    cppc_para->lowest           = data->caps.lowest_perf;
> +    cppc_para->lowest_nonlinear = data->caps.lowest_nonlinear_perf;
> +    cppc_para->nominal          = data->caps.nominal_perf;
> +    cppc_para->highest          = data->caps.highest_perf;
> +    cppc_para->minimum          = data->req.min_perf;
> +    cppc_para->maximum          = data->req.max_perf;
> +    cppc_para->desired          = data->req.des_perf;
> +    cppc_para->energy_perf      = data->req.epp;
> +
> +    return 0;
> +}
> +
> +int set_amd_cppc_para(struct cpufreq_policy *policy,
> +                      const struct xen_set_cppc_para *set_cppc)

... amd_cppc_set_para() would imo be more consistent names, considering how
other functions are named.

> +{
> +    unsigned int cpu = policy->cpu;
> +    struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu);
> +    uint8_t max_perf, min_perf, des_perf, epp;
> +    bool active_mode = cpufreq_is_governorless(cpu);
> +
> +    if ( data == NULL )
> +        return -ENOENT;
> +
> +    /* Return if there is nothing to do. */
> +    if ( set_cppc->set_params == 0 )
> +        return 0;

That is ...

> +    /* 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?

> +    /*
> +     * 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 ...

> +        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->minimum >

... this, one more pair of parentheses may also help there. (Recall:
symmetry where possible.)

> +                        (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM
> +                         ? set_cppc->maximum
> +                         : data->req.max_perf))) )
> +        return -EINVAL;
> +    /*
> +     * Desired performance may be set to any performance value in the range
> +     * [Minimum Performance, Maximum Performance], inclusive.
> +     */
> +    if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED )
> +    {
> +        if ( active_mode )
> +            return -EOPNOTSUPP;
> +
> +        if ( (set_cppc->desired >
> +                        (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM
> +                         ? set_cppc->maximum
> +                         : data->req.max_perf)) ||
> +             (set_cppc->desired <
> +                        (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM
> +                         ? set_cppc->minimum
> +                         : data->req.min_perf)) )
> +            return -EINVAL;
> +    }
> +    /*
> +     * Energy Performance Preference may be set with a range of values
> +     * from 0 to 0xFF
> +     */
> +    if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ENERGY_PERF )
> +    {
> +        if ( !active_mode )
> +            return -EOPNOTSUPP;
> +
> +        if ( set_cppc->energy_perf > UINT8_MAX )
> +            return -EINVAL;
> +    }
> +
> +    /* Activity window not supported in MSR */
> +    if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW )
> +        return -EOPNOTSUPP;
> +
> +    epp = per_cpu(epp_init, cpu);
> +    min_perf = data->caps.lowest_nonlinear_perf;
> +    max_perf = data->caps.highest_perf;
> +    des_perf = data->req.des_perf;
> +    /*
> +     * Apply presets:
> +     * XEN_SYSCTL_CPPC_SET_PRESET_POWERSAVE/PERFORMANCE/ONDEMAND are
> +     * only available when CPPC in active mode
> +     */
> +    switch ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_PRESET_MASK )
> +    {
> +    case XEN_SYSCTL_CPPC_SET_PRESET_POWERSAVE:
> +        if ( !active_mode )
> +            return -EINVAL;
> +        policy->policy = CPUFREQ_POLICY_POWERSAVE;
> +        /*
> +         * Lower max_perf to nonlinear_lowest to achieve
> +         * ultmost power saviongs
> +         */
> +        max_perf = min_perf;
> +        epp = CPPC_ENERGY_PERF_MAX_POWERSAVE;
> +        break;
> +
> +    case XEN_SYSCTL_CPPC_SET_PRESET_PERFORMANCE:
> +        if ( !active_mode )
> +            return -EINVAL;
> +        policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> +        /* Increase min_perf to highest to achieve ultmost performance */
> +        min_perf = max_perf;
> +        epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
> +        break;
> +
> +    case XEN_SYSCTL_CPPC_SET_PRESET_ONDEMAND:
> +        if ( !active_mode )
> +            return -EINVAL;
> +        policy->policy = CPUFREQ_POLICY_ONDEMAND;
> +        /*
> +         * Take medium value to show no preference over
> +         * performance or powersave
> +         */
> +        epp = CPPC_ENERGY_PERF_BALANCE;
> +        break;
> +
> +    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?

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -81,7 +81,18 @@ struct cpufreq_policy {
>      int8_t              turbo;  /* tristate flag: 0 for unsupported
>                                   * -1 for disable, 1 for enabled
>                                   * See CPUFREQ_TURBO_* below for defines */
> -    unsigned int        policy; /* CPUFREQ_POLICY_* */
> +    unsigned int        policy; /* Performance Policy
> +                                 * If cpufreq_driver->target() exists,
> +                                 * the ->governor decides what frequency
> +                                 * within the limits is used.
> +                                 * If cpufreq_driver->setpolicy() exists, 
> these
> +                                 * following policies are available:
> +                                 * CPUFREQ_POLICY_PERFORMANCE represents
> +                                 * maximum performance
> +                                 * CPUFREQ_POLICY_POWERSAVE represents least
> +                                 * power consumption
> +                                 * CPUFREQ_POLICY_ONDEMAND represents no
> +                                 * preference over performance or powersave 
> */

Besides not being a well-formed comment, this is close to unreadable in this
shape. This much text wants putting ahead of the field.

> --- 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 ...

Jan