| Message ID | 463572d126a7700e5e90ef3a49104bd4b8c1c389.1696514677.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 10.1 | expand |
On 06/10/2023 9:26 am, Nicola Vetrini wrote: > The COUNT_LEAVES macro is introduced to avoid using an essentially > boolean value in a subtraction. > > No functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h > index bab3eecda6c1..700993cc67e8 100644 > --- a/xen/include/xen/lib/x86/cpu-policy.h > +++ b/xen/include/xen/lib/x86/cpu-policy.h > @@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor); > #define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \ > CPUID_GUEST_NR_EXTD_AMD) > > +#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0)) > /* > * Maximum number of leaves a struct cpu_policy turns into when serialised for > * interaction with the toolstack. (Sum of all leaves in each union, less the > * entries in basic which sub-unions hang off of.) > */ > -#define CPUID_MAX_SERIALISED_LEAVES \ > - (CPUID_GUEST_NR_BASIC + \ > - CPUID_GUEST_NR_FEAT - !!CPUID_GUEST_NR_FEAT + \ > - CPUID_GUEST_NR_CACHE - !!CPUID_GUEST_NR_CACHE + \ > - CPUID_GUEST_NR_TOPO - !!CPUID_GUEST_NR_TOPO + \ > - CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE + \ > +#define CPUID_MAX_SERIALISED_LEAVES \ > + (CPUID_GUEST_NR_BASIC + \ > + COUNT_LEAVES(CPUID_GUEST_NR_FEAT) + \ > + COUNT_LEAVES(CPUID_GUEST_NR_CACHE) + \ > + COUNT_LEAVES(CPUID_GUEST_NR_TOPO) + \ > + COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) + \ > CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ ) This may not have been a MISRA-approved calculation, but encapsulating it like this breaks any ability to follow what's going on. CPUID data in x86 is mostly a sparse 1-D array (BASIC, EXTD, HV blocks), but a couple of elements in the BASIC array have arrays themselves. The struct is laid out for O(1) access, so you can't just say sizeof(struct) / sizeof(element). The BASIC array has elements (0x4, 0x7, 0xb, 0xd) which hold no data because there's actually an array elsewhere containing all the data. So logically, it's: (BASIC + (FEAT - 1) + (CACHE - 1) + (TOPO - 1) + (XSTATE - 1)) + EXTD + 2 And in practice I'd far rather express it with a plain -1 than a - !!NR_, if the latter isn't an option. Presumably MISRA would be happy with that? If so, I can submit a patch. There's also a typo in that the comment that wants fixing. ~Andrew
On 06/10/2023 19:57, Andrew Cooper wrote: > On 06/10/2023 9:26 am, Nicola Vetrini wrote: >> The COUNT_LEAVES macro is introduced to avoid using an essentially >> boolean value in a subtraction. >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/xen/include/xen/lib/x86/cpu-policy.h >> b/xen/include/xen/lib/x86/cpu-policy.h >> index bab3eecda6c1..700993cc67e8 100644 >> --- a/xen/include/xen/lib/x86/cpu-policy.h >> +++ b/xen/include/xen/lib/x86/cpu-policy.h >> @@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int >> vendor); >> #define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \ >> CPUID_GUEST_NR_EXTD_AMD) >> >> +#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0)) >> /* >> * Maximum number of leaves a struct cpu_policy turns into when >> serialised for >> * interaction with the toolstack. (Sum of all leaves in each union, >> less the >> * entries in basic which sub-unions hang off of.) >> */ >> -#define CPUID_MAX_SERIALISED_LEAVES \ >> - (CPUID_GUEST_NR_BASIC + \ >> - CPUID_GUEST_NR_FEAT - !!CPUID_GUEST_NR_FEAT + \ >> - CPUID_GUEST_NR_CACHE - !!CPUID_GUEST_NR_CACHE + \ >> - CPUID_GUEST_NR_TOPO - !!CPUID_GUEST_NR_TOPO + \ >> - CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE + \ >> +#define CPUID_MAX_SERIALISED_LEAVES \ >> + (CPUID_GUEST_NR_BASIC + \ >> + COUNT_LEAVES(CPUID_GUEST_NR_FEAT) + \ >> + COUNT_LEAVES(CPUID_GUEST_NR_CACHE) + \ >> + COUNT_LEAVES(CPUID_GUEST_NR_TOPO) + \ >> + COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) + \ >> CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ ) > > This may not have been a MISRA-approved calculation, but encapsulating > it like this breaks any ability to follow what's going on. > > CPUID data in x86 is mostly a sparse 1-D array (BASIC, EXTD, HV > blocks), > but a couple of elements in the BASIC array have arrays themselves. > > The struct is laid out for O(1) access, so you can't just say > sizeof(struct) / sizeof(element). The BASIC array has elements (0x4, > 0x7, 0xb, 0xd) which hold no data because there's actually an array > elsewhere containing all the data. > > So logically, it's: > > (BASIC + (FEAT - 1) + (CACHE - 1) + (TOPO - 1) + (XSTATE - 1)) + EXTD + > 2 > > And in practice I'd far rather express it with a plain -1 than a - > !!NR_, if the latter isn't an option. > > Presumably MISRA would be happy with that? > > If so, I can submit a patch. There's also a typo in that the comment > that wants fixing. > > ~Andrew Yes, that should be fine. I'll be happy to test that.
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h index bab3eecda6c1..700993cc67e8 100644 --- a/xen/include/xen/lib/x86/cpu-policy.h +++ b/xen/include/xen/lib/x86/cpu-policy.h @@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor); #define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \ CPUID_GUEST_NR_EXTD_AMD) +#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0)) /* * Maximum number of leaves a struct cpu_policy turns into when serialised for * interaction with the toolstack. (Sum of all leaves in each union, less the * entries in basic which sub-unions hang off of.) */ -#define CPUID_MAX_SERIALISED_LEAVES \ - (CPUID_GUEST_NR_BASIC + \ - CPUID_GUEST_NR_FEAT - !!CPUID_GUEST_NR_FEAT + \ - CPUID_GUEST_NR_CACHE - !!CPUID_GUEST_NR_CACHE + \ - CPUID_GUEST_NR_TOPO - !!CPUID_GUEST_NR_TOPO + \ - CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE + \ +#define CPUID_MAX_SERIALISED_LEAVES \ + (CPUID_GUEST_NR_BASIC + \ + COUNT_LEAVES(CPUID_GUEST_NR_FEAT) + \ + COUNT_LEAVES(CPUID_GUEST_NR_CACHE) + \ + COUNT_LEAVES(CPUID_GUEST_NR_TOPO) + \ + COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) + \ CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ ) /* Maximum number of MSRs written when serialising a cpu_policy. */
The COUNT_LEAVES macro is introduced to avoid using an essentially boolean value in a subtraction. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)