Re: [PATCH v1 11/11] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC xen_sysctl_pm_op for amd-pstate driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2024 09:35, Penny Zheng wrote:
> @@ -489,6 +491,117 @@ static int cf_check amd_pstate_epp_set_policy(struct 
> cpufreq_policy *policy)
>      return amd_pstate_epp_update_limit(policy);
>  }
>  
> +int get_amd_cppc_para(unsigned int cpu,
> +                      struct xen_cppc_para *cppc_para)
> +{
> +    struct amd_pstate_drv_data *data = per_cpu(amd_pstate_drv_data, cpu);
> +
> +    if ( data == NULL )
> +        return -ENODATA;
> +
> +    cppc_para->features         = 0;
> +    cppc_para->lowest           = data->hw.lowest_perf;
> +    cppc_para->lowest_nonlinear = data->hw.lowest_nonlinear_perf;
> +    cppc_para->nominal          = data->hw.nominal_perf;
> +    cppc_para->highest          = data->hw.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,
> +                      struct xen_set_cppc_para *set_cppc)
> +{
> +    unsigned int cpu = policy->cpu;
> +    struct amd_pstate_drv_data *data = per_cpu(amd_pstate_drv_data, cpu);
> +    uint8_t max_perf, min_perf, des_perf;
> +    int epp = -1;
> +
> +    if ( data == NULL )
> +        return -ENOENT;
> +
> +    /* Validate all parameters - Disallow reserved bits. */
> +    if ( set_cppc->minimum > 255 || set_cppc->maximum > 255 ||
> +         set_cppc->desired > 255 || set_cppc->energy_perf > 255 )
> +        return -EINVAL;
> +
> +    /* 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;
> +
> +    /* Activity window not supported */
> +    if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW )
> +        return -EINVAL;
> +
> +    /* Return if there is nothing to do. */
> +    if ( set_cppc->set_params == 0 )
> +        return 0;
> +
> +    /* Apply presets */
> +    switch ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_PRESET_MASK )
> +    {
> +    case XEN_SYSCTL_CPPC_SET_PRESET_POWERSAVE:
> +        if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED )
> +            return -EINVAL;
> +        min_perf = data->hw.lowest_perf;
> +        max_perf = data->hw.highest_perf;

These are the same as ...

> +        epp = CPPC_ENERGY_PERF_MAX_POWERSAVE;
> +        des_perf = 0;
> +        break;
> +
> +    case XEN_SYSCTL_CPPC_SET_PRESET_PERFORMANCE:
> +        if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED )
> +            return -EINVAL;
> +        min_perf = data->hw.highest_perf;
> +        max_perf = data->hw.highest_perf;
> +        epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
> +        des_perf = 0;
> +        break;
> +
> +    case XEN_SYSCTL_CPPC_SET_PRESET_BALANCE:
> +        if ( set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED )
> +            return -EINVAL;
> +        min_perf = data->hw.lowest_perf;
> +        max_perf = data->hw.highest_perf;

... these, despite the presets being quite different - why?

> +        epp = CPPC_ENERGY_PERF_BALANCE;
> +        des_perf = 0;
> +        break;
> +
> +    case XEN_SYSCTL_CPPC_SET_PRESET_NONE:
> +        min_perf = data->hw.lowest_nonlinear_perf;
> +        max_perf = data->hw.highest_perf;
> +        break;

Rather than setting des_perf to 0 everywhere except here (thus leaving it
potentially uninitialized), better give the variable an initializer of 0?

> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -198,6 +198,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      char     *scaling_available_governors;
>      struct list_head *pos;
>      uint32_t cpu, i, j = 0;
> +    bool hw_auto = false;
>  
>      pmpt = processor_pminfo[op->cpuid];
>      policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
> @@ -258,7 +259,19 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>           !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
>                    CPUFREQ_NAME_LEN) )
>          ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para);
> -    else
> +    else if ( !strncmp(op->u.get_para.scaling_driver, 
> XEN_AMD_PSTATE_DRIVER_NAME,
> +                       CPUFREQ_NAME_LEN) ||
> +              !strncmp(op->u.get_para.scaling_driver, 
> XEN_AMD_PSTATE_EPP_DRIVER_NAME,
> +                       CPUFREQ_NAME_LEN) )
> +        ret = get_amd_cppc_para(policy->cpu, &op->u.get_para.u.cppc_para);

Like if is here, ...

> +    if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
> +                 CPUFREQ_NAME_LEN) ||
> +         !strncmp(op->u.get_para.scaling_driver, 
> XEN_AMD_PSTATE_EPP_DRIVER_NAME,
> +                 CPUFREQ_NAME_LEN) )
> +        hw_auto = true;
> +
> +    if ( !hw_auto )

... why not use the strncmp()s directly in the if()?

Jan