1. Patchew
  2. Xen
  3. View series

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

Jan Beulich posted 1 patch 8 months, 2 weeks ago
Failed in applying to current master (apply log)
[PATCH] libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s
Posted by Jan Beulich 8 months, 2 weeks ago 
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@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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));
 
+        /* Sanity check layout of the union subject to memcpy() below. */
+        BUILD_BUG_ON(sizeof(user_para->u) != sizeof(sys_para->u));
+#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));
     }
Re: [PATCH] libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s
Posted by Jason Andryuk 8 months, 2 weeks ago 
On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@suse.com> 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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- 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@gmail.com>

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

Thanks,
Jason
Re: [PATCH] libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s
Posted by Jan Beulich 8 months, 2 weeks ago 
On 23.08.2023 15:57, Jason Andryuk wrote:
> On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>> @@ -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?

Oh, indeed. Will do (and Jürgen, I'll assume this won't invalidate your
R-b).

>> +#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@gmail.com>

Thanks.

Jan
Re: [PATCH] libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s
Posted by Juergen Gross 8 months, 2 weeks ago 
On 23.08.23 16:07, Jan Beulich wrote:
> On 23.08.2023 15:57, Jason Andryuk wrote:
>> On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>>> @@ -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?
> 
> Oh, indeed. Will do (and Jürgen, I'll assume this won't invalidate your
> R-b).

Correct.


Juergen
Re: [PATCH] libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s
Posted by Juergen Gross 8 months, 2 weeks ago 
On 23.08.23 15:47, Jan Beulich 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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.