RE: [PATCH v10 7/8] 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: Thursday, September 25, 2025 2:12 PM
> To: Andryuk, Jason <Jason.Andryuk@xxxxxxx>; 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 v10 7/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC
> xen_sysctl_pm_op for amd-cppc driver
>
> On 23.09.2025 18:47, Jason Andryuk wrote:
> > On 2025-09-23 11:38, Jan Beulich wrote:
> >> On 23.09.2025 06:38, 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] )
> >>
> >> amd_cppc_prepare_policy() may leave ->governor set to NULL afaics, so
> >> I think you need to add a NULL check here alongside with pulling this
> >> out of ...
> >>
> >>> +        strlcpy(op->u.get_para.s.scaling_governor,
> >>> +                policy->governor->name, CPUFREQ_NAME_LEN);
> >>> +    else
> >>> +        strlcpy(op->u.get_para.s.scaling_governor, "Unknown",
> >>> +                CPUFREQ_NAME_LEN);
> >>> +
> >>>       if ( !cpufreq_is_governorless(op->cpuid) )
> >>>       {
> >>
> >> ... this conditional.
> >>
> >> The description also continues to not mention the effect for HWP. I'm
> >> actually somewhat confused, I suppose (Jason, question mainly to you):
> >> HWP falls in the governor-less category, iirc. Yet it doesn't supply
> >> a .setpolicy hook, hence __cpufreq_set_policy() goes through the
> >> normal governor setting logic. What's the deal here? The answer may
> >> affect whether I'd deem the pulling out of the conditional correct
> >> (or at least
> >> benign) here as to HWP.
> >
> > Hi,
> >
> > When I wrote HWP, I didn't realize using .setpolicy would bypass the
> > governor code.  Instead, I implemented the no-op HWP governor, since I
> > thought I needed something as a governor.
> >
> > set_hwp_para() actually changes the configuration.  HWP only
> > implements the equivalent of amd-cppc-epp autonomous (active) mode.
> >
> > So I think HWP could switch to .setpolicy and drop its governor.
> >
> > But looking at this hunk:
> >
> >  > @@ -321,10 +327,12 @@ static int set_cpufreq_cppc(struct  >
> > xen_sysctl_pm_op *op)
> >  >      if ( !policy || !policy->governor )
> >
> > Doesn't this !policy->governor prevent amd-cppc-epp from setting
> > parameters?
>
> Only if amd_cppc_prepare_policy() took the default case path of its switch(), 
> aiui.
> Penny?
>

Hmmm, I shall admit that I omitted the default case.
Actually, in current codes, no matter it is amd-cppc or amd-cppc-epp, or any 
other cpufreq driver(except hwp), in its .init(), we have:
```
policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR;
```
So we have CPUFREQ_DEFAULT_GOVERNOR when no specified governor set through 
cmfline, and it is ondemand... As long as .init() is executed successfully, we 
will never have NULL governor field right now.

> Jan
>
> >  >          return -ENOENT;
> >  >
> >  > -    if ( !hwp_active() )
> >  > -        return -EOPNOTSUPP;
> >  > +    if ( hwp_active() )
> >  > +        return set_hwp_para(policy, &op->u.set_cppc);
> >  > +    if ( processor_pminfo[op->cpuid]->init & XEN_CPPC_INIT )
> >  > +        return amd_cppc_set_para(policy, &op->u.set_cppc);
> >  >
> >  > -    return set_hwp_para(policy, &op->u.set_cppc);
> >  > +    return -EOPNOTSUPP;
> >  >  }
> >
> > So there may be other checks that would need dropping or adjusting to
> > support HWP without a governor.
> >
> > Thanks,
> > Jason