Re: [PATCH v10 5/8] tools/cpufreq: extract CPPC para from cpufreq para

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2025 06:38, Penny Zheng wrote:
> We extract cppc info from "struct xen_get_cpufreq_para", where it acts as
> a member of union, and share the space with governor info.
> However, it may fail in amd-cppc passive mode, in which governor info and
> CPPC info could co-exist, and both need to be printed together via xenpm tool.
> If we tried to still put it in "struct xen_get_cpufreq_para" (e.g. just move
> out of union), "struct xen_get_cpufreq_para" will enlarge too much to further
> make xen_sysctl.u exceed 128 bytes.
> 
> So we introduce a new sub-field GET_CPUFREQ_CPPC to dedicatedly acquire
> CPPC-related para, and make get-cpufreq-para invoke GET_CPUFREQ_CPPC
> if available.
> New helpers print_cppc_para() and get_cpufreq_cppc() are introduced to
> extract CPPC-related parameters process from cpufreq para.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx> # hypervisor
> Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> v4 -> v5:
> - new commit
> ---
> v5 -> v6:
> - remove the changes for get-cpufreq-para
> ---
> v6 -> v7:
> - make get-cpufreq-para invoke GET_CPUFREQ_CPPC
> ---
> v7 -> v8:
> - use structure assignment as it is a alias
> - add errno info to the error print
> ---
> v9 -> v10
> - drop the outer union

In the interest of getting this series in I think we will want to take
this patch as is (I yet have to see the other, related patch though),
but ...

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -478,22 +478,19 @@ struct xen_get_cpufreq_para {
>      uint32_t cpuinfo_cur_freq;
>      uint32_t cpuinfo_max_freq;
>      uint32_t cpuinfo_min_freq;
> -    union {
> -        struct {
> -            uint32_t scaling_cur_freq;
> -
> -            char scaling_governor[CPUFREQ_NAME_LEN];
> -            uint32_t scaling_max_freq;
> -            uint32_t scaling_min_freq;
> -
> -            /* for specific governor */
> -            union {
> -                struct  xen_userspace userspace;
> -                struct  xen_ondemand ondemand;
> -            } u;
> -        } s;
> -        struct xen_get_cppc_para cppc_para;
> -    } u;
> +    struct {
> +        uint32_t scaling_cur_freq;
> +
> +        char scaling_governor[CPUFREQ_NAME_LEN];
> +        uint32_t scaling_max_freq;
> +        uint32_t scaling_min_freq;
> +
> +        /* for specific governor */
> +        union {
> +            struct  xen_userspace userspace;
> +            struct  xen_ondemand ondemand;
> +        } u;
> +    } s;

... I don't quite see why we'd need to retain the nested struct now
either. Imo we ought to be cleaning this up for 4.22.

Jan