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 <jandr...@gmail.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
