Re: [PATCH v5 04/15] xen/sysctl: Nest cpufreq scaling options

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2023 20:54, Jason Andryuk wrote:
> Add a union and struct so that most of the scaling variables of struct
> xen_get_cpufreq_para are within in a binary-compatible layout.  This
> allows cppc_para to live in the larger union and use uint32_ts - struct
> xen_cppc_para will be 10 uint32_t's.
> 
> The new scaling struct is 3 * uint32_t + 16 bytes CPUFREQ_NAME_LEN + 4 *
> uint32_t for xen_ondemand = 11 uint32_t.  That means the old size is
> retained, int32_t turbo_enabled doesn't move and it's binary compatible.
> 
> The out-of-context memcpy() in xc_get_cpufreq_para() now handles the
> copying of the fields removed there.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Nevertheless I continue to be uncertain about ...

> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para {
>      uint32_t cpuinfo_cur_freq;
>      uint32_t cpuinfo_max_freq;
>      uint32_t cpuinfo_min_freq;
> -    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 {
> -        xc_userspace_t userspace;
> -        xc_ondemand_t ondemand;
> +        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 {
> +                xc_userspace_t userspace;
> +                xc_ondemand_t ondemand;
> +            } u;
> +        } s;
>      } u;
>  
>      int32_t turbo_enabled;

... all of this: Parts of the struct can apparently go out of sync with
the sysctl struct, but other parts have to remain in sync without there
being an appropriate build-time check (checking merely sizes clearly
isn't enough). Therefore I'd really like to have a toolstack side
review / ack here as well.

Jan