Re: [PATCH v4 13/15] tools/xenpm: fix unnecessary scaling_available_frequencies in CPPC mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> In `xenpm get-cpufreq-para <cpuid>`, para scaling_available_frequencies
> only has meaningful value when cpufreq driver in legacy P-states mode.
> 
> So we drop the "has_num" condition check, and mirror the ->gov_num check for
> both ->freq_num and ->cpu_num in xc_get_cpufreq_para().
> In get_cpufreq_para(), add "freq_num" check to avoid copying data to
> op->u.get_para.scaling_available_frequencies in CPPC mode.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>

The description doesn't clarify where the bug is that's being fixed (from
the word "fix" in the subject). If there was one, surely there would also
want to be a Fixes: tag.

> ---
>  tools/libs/ctrl/xc_pm.c   | 45 +++++++++++++++++++++------------------
>  xen/drivers/acpi/pmstat.c | 11 ++++++----
>  2 files changed, 31 insertions(+), 25 deletions(-)

xenpm is entirely untouched, unlike suggested by the subject prefix.

> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -212,34 +212,39 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
>      DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors,
>                        user_para->scaling_available_governors,
>                        user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), 
> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> -    bool has_num = user_para->cpu_num && user_para->freq_num;
>  
> -    if ( has_num )
> +    if ( (user_para->cpu_num && !user_para->affected_cpus) ||
> +         (user_para->freq_num && !user_para->scaling_available_frequencies) 
> ||
> +         (user_para->gov_num && !user_para->scaling_available_governors) )
> +    {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    if ( user_para->cpu_num )
>      {
> -        if ( (!user_para->affected_cpus)                    ||
> -             (!user_para->scaling_available_frequencies)    ||
> -             (user_para->gov_num && !user_para->scaling_available_governors) 
> )
> -        {
> -            errno = EINVAL;
> -            return -1;
> -        }
>          ret = xc_hypercall_bounce_pre(xch, affected_cpus);
>          if ( ret )
>              return ret;
> +    }
> +    if ( user_para->freq_num )
> +    {
>          ret = xc_hypercall_bounce_pre(xch, scaling_available_frequencies);
>          if ( ret )
>              goto unlock_2;
> -        if ( user_para->gov_num )
> -            ret = xc_hypercall_bounce_pre(xch, scaling_available_governors);
> -        if ( ret )
> -            goto unlock_3;
> +    }
> +    if ( user_para->gov_num )
> +        ret = xc_hypercall_bounce_pre(xch, scaling_available_governors);
> +    if ( ret )
> +        goto unlock_3;

Yes, ret is initialized to 0, so functionally this is okay. But can we please
have all three pieces be as similar as possible, to make apparent that they're
_expected_ to be similar?

> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -228,10 +228,13 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      ret = copy_to_guest(op->u.get_para.affected_cpus,
>                          data, op->u.get_para.cpu_num);
>  
> -    for ( i = 0; i < op->u.get_para.freq_num; i++ )
> -        data[i] = pmpt->perf.states[i].core_frequency * 1000;
> -    ret += copy_to_guest(op->u.get_para.scaling_available_frequencies,
> -                         data, op->u.get_para.freq_num);
> +    if ( op->u.get_para.freq_num )
> +    {
> +        for ( i = 0; i < op->u.get_para.freq_num; i++ )
> +            data[i] = pmpt->perf.states[i].core_frequency * 1000;
> +        ret += copy_to_guest(op->u.get_para.scaling_available_frequencies,
> +                             data, op->u.get_para.freq_num);
> +    }

What's the effect of this change? Without it, the loop will simply have zero
iterations, and zero items will be copied to the guest. (As with the previous
patch, this would better be a tools-only one anyway.)

Else the question would be why the same isn't done for cpu_num, which is
becoming optional now, too (according to the libxc change).

Jan