On 27.05.2025 10:48, Penny Zheng wrote:
> In `xenpm get-cpufreq-para <cpuid>`, ->freq_num and ->cpu_num checking are
> tied together via variable "has_num", while ->freq_num only has non-zero 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().
>
> Signed-off-by: Penny Zheng <penny.zh...@amd.com>
> ---
> v3 -> v4:
> - drop the "has_num" condition check
> ---
> v4 -> v5:
> - refactor title and commit
> - make all three pieces (xc_hypercall_bounce_pre()) be as similar as possible
> ---
> tools/libs/ctrl/xc_pm.c | 43 +++++++++++++++++++++++------------------
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
> index 3c9e272aee..cdc072e757 100644
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -212,34 +212,41 @@ 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 ( user_para->gov_num )
> + {
> + ret = xc_hypercall_bounce_pre(xch, scaling_available_governors);
> if ( ret )
> goto unlock_3;
> + }
>
> + if ( user_para->cpu_num )
> set_xen_guest_handle(sys_para->affected_cpus, affected_cpus);
Any reason this can't simply move up to the bottom of the identical conditional
above?
> - set_xen_guest_handle(sys_para->scaling_available_frequencies,
> scaling_available_frequencies);
> - if ( user_para->gov_num )
> - set_xen_guest_handle(sys_para->scaling_available_governors,
> - scaling_available_governors);
> - }
> + if ( user_para->freq_num )
> + set_xen_guest_handle(sys_para->scaling_available_frequencies,
> + scaling_available_frequencies);
> + if ( user_para->gov_num )
> + set_xen_guest_handle(sys_para->scaling_available_governors,
> + scaling_available_governors);
Same for these two then.
Jan
