Re: [PATCH v10 7/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC xen_sysctl_pm_op for amd-cppc driver

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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