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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, August 26, 2025 12:03 AM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 13/13] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC
> xen_sysctl_pm_op for amd-cppc driver
>
> 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"

> > +    /*
> > +     * 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))) )
```

> > +    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");
+    }
+
```

> Jan