Re: [PATCH] libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> The full structures cannot match in layout, as soon as a 32-bit tool
> stack build comes into play. But it also doesn't need to; the part of
> the layouts that needs to match is merely the union that we memcpy()
> from the sysctl structure to the xc one. Keep (in adjusted form) only
> the relevant ones.
>
> Since the whole block needs touching anyway, move it closer to the
> respective memcpy() and use a wrapper macro to limit verbosity.
>
> Fixes: 2381dfab083f ("xen/sysctl: Nest cpufreq scaling options")
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -248,45 +248,6 @@ int xc_get_cpufreq_para(xc_interface *xc
>      sys_para->freq_num = user_para->freq_num;
>      sys_para->gov_num  = user_para->gov_num;
>
> -    /* Sanity check struct layout */
> -    BUILD_BUG_ON(sizeof(*user_para) != sizeof(*sys_para));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), cpu_num) !=
> -                 offsetof(typeof(*sys_para),  cpu_num));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), freq_num) !=
> -                 offsetof(typeof(*sys_para),  freq_num));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), gov_num) !=
> -                 offsetof(typeof(*sys_para),  gov_num));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), affected_cpus) !=
> -                 offsetof(typeof(*sys_para),  affected_cpus));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_available_frequencies) 
> !=
> -                 offsetof(typeof(*sys_para),  
> scaling_available_frequencies));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_available_governors) !=
> -                 offsetof(typeof(*sys_para),  scaling_available_governors));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_driver) !=
> -                 offsetof(typeof(*sys_para),  scaling_driver));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_cur_freq) !=
> -                 offsetof(typeof(*sys_para),  cpuinfo_cur_freq));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_max_freq) !=
> -                 offsetof(typeof(*sys_para),  cpuinfo_max_freq));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_min_freq) !=
> -                 offsetof(typeof(*sys_para),  cpuinfo_min_freq));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_cur_freq) !=
> -                 offsetof(typeof(*sys_para),  u.s.scaling_cur_freq));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_governor) !=
> -                 offsetof(typeof(*sys_para),  u.s.scaling_governor));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_max_freq) !=
> -                 offsetof(typeof(*sys_para),  u.s.scaling_max_freq));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_min_freq) !=
> -                 offsetof(typeof(*sys_para),  u.s.scaling_min_freq));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.u.userspace) !=
> -                 offsetof(typeof(*sys_para),  u.s.u.userspace));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.u.ondemand) !=
> -                 offsetof(typeof(*sys_para),  u.s.u.ondemand));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), u.cppc_para) !=
> -                 offsetof(typeof(*sys_para),  u.cppc_para));
> -    BUILD_BUG_ON(offsetof(typeof(*user_para), turbo_enabled) !=
> -                 offsetof(typeof(*sys_para),  turbo_enabled));
> -
>      ret = xc_sysctl(xch, &sysctl);
>      if ( ret )
>      {
> @@ -316,6 +277,22 @@ int xc_get_cpufreq_para(xc_interface *xc
>          BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) !=
>                      sizeof(((struct xen_get_cpufreq_para *)0)->u));

This check...

> +        /* Sanity check layout of the union subject to memcpy() below. */
> +        BUILD_BUG_ON(sizeof(user_para->u) != sizeof(sys_para->u));

And this check are the same?  Your newer one is nicer, so maybe drop the first?

> +#define CHK_FIELD(fld) \
> +        BUILD_BUG_ON(offsetof(typeof(user_para->u), fld) != \
> +                     offsetof(typeof(sys_para->u),  fld))
> +
> +        CHK_FIELD(s.scaling_cur_freq);
> +        CHK_FIELD(s.scaling_governor);
> +        CHK_FIELD(s.scaling_max_freq);
> +        CHK_FIELD(s.scaling_min_freq);
> +        CHK_FIELD(s.u.userspace);
> +        CHK_FIELD(s.u.ondemand);
> +        CHK_FIELD(cppc_para);
> +
> +#undef CHK_FIELD
> +
>          memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u));
>      }
>

Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Sorry about the breakage. I started looking at this locally, but you beat me.

Thanks,
Jason