| Message ID | 20230802131003.166670-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/gen-cpuid: Avoid violations of Misra rule 1.3 | expand |
On 02.08.2023 15:10, Andrew Cooper wrote: > Add the script to the X86 section in ./MAINTAINERS. > > Structures or unions without any named members aren't liked by Misra > (nor the C standard). Avoid emitting such for leaves without any known > bits. > > The placeholders are affected similarly, but are only visible to MISRA in the > middle of a patch series adding a new leaf. The absence of a name was > intentional as these defines need to not duplicate names. > > As that's not deemed acceptable any more, move placeholder processing into the > main loop and append the the word number to generate unique names. > > $ diff cpuid-autogen-{before,after}.h > @@ -1034,7 +1034,7 @@ > bool intel_psfd:1, ipred_ctrl:1, rrsba_ctrl:1, ddp_ctrl:1, ... > > #define CPUID_BITFIELD_14 \ > - bool :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, ... > + uint32_t _placeholder_14 > > #define CPUID_BITFIELD_15 \ > bool :1, :1, :1, :1, avx_vnni_int8:1, avx_ne_convert:1, :1, ... > @@ -1043,19 +1043,19 @@ > bool rdcl_no:1, eibrs:1, rsba:1, skip_l1dfl:1, intel_ssb_no:1, ... > > #define CPUID_BITFIELD_17 \ > - bool :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, ... > + uint32_t _placeholder_17 > > #define CPUID_BITFIELD_18 \ > - uint32_t :32 /* placeholder */ > + uint32_t _placeholder_18 > > #define CPUID_BITFIELD_19 \ > - uint32_t :32 /* placeholder */ > + uint32_t _placeholder_19 > > #define CPUID_BITFIELD_20 \ > - uint32_t :32 /* placeholder */ > + uint32_t _placeholder_20 > > #define CPUID_BITFIELD_21 \ > - uint32_t :32 /* placeholder */ > + uint32_t _placeholder_21 > > #endif /* __XEN_X86__FEATURESET_DATA__ */ > > No functional change. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one question below. > v2: > * Write it more pythonically. Yeah, you know I don't speak Python very well. I was glad I got it to work without overly much hassle. > @@ -382,7 +382,10 @@ def crunch_numbers(state): > > names.append(name.lower()) > > - state.bitfields.append("bool " + ":1, ".join(names) + ":1") > + if any(names): > + state.bitfields.append("bool " + ":1, ".join(names) + ":1") > + else: > + state.bitfields.append("uint32_t _placeholder_%s" % (word, )) I thought % could be used here, but then I wouldn't have known to use %s (elsewhere we use %u), nor to add an empty argument (which I see is done in a few other places as well, but not uniformly when %s is used). I assume there's a reason for the specific way you've done it here? Jan
On 02/08/2023 2:22 pm, Jan Beulich wrote: > On 02.08.2023 15:10, Andrew Cooper wrote: >> Add the script to the X86 section in ./MAINTAINERS. >> >> Structures or unions without any named members aren't liked by Misra >> (nor the C standard). Avoid emitting such for leaves without any known >> bits. >> >> The placeholders are affected similarly, but are only visible to MISRA in the >> middle of a patch series adding a new leaf. The absence of a name was >> intentional as these defines need to not duplicate names. >> >> As that's not deemed acceptable any more, move placeholder processing into the >> main loop and append the the word number to generate unique names. >> >> $ diff cpuid-autogen-{before,after}.h >> @@ -1034,7 +1034,7 @@ >> bool intel_psfd:1, ipred_ctrl:1, rrsba_ctrl:1, ddp_ctrl:1, ... >> >> #define CPUID_BITFIELD_14 \ >> - bool :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, ... >> + uint32_t _placeholder_14 >> >> #define CPUID_BITFIELD_15 \ >> bool :1, :1, :1, :1, avx_vnni_int8:1, avx_ne_convert:1, :1, ... >> @@ -1043,19 +1043,19 @@ >> bool rdcl_no:1, eibrs:1, rsba:1, skip_l1dfl:1, intel_ssb_no:1, ... >> >> #define CPUID_BITFIELD_17 \ >> - bool :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, ... >> + uint32_t _placeholder_17 >> >> #define CPUID_BITFIELD_18 \ >> - uint32_t :32 /* placeholder */ >> + uint32_t _placeholder_18 >> >> #define CPUID_BITFIELD_19 \ >> - uint32_t :32 /* placeholder */ >> + uint32_t _placeholder_19 >> >> #define CPUID_BITFIELD_20 \ >> - uint32_t :32 /* placeholder */ >> + uint32_t _placeholder_20 >> >> #define CPUID_BITFIELD_21 \ >> - uint32_t :32 /* placeholder */ >> + uint32_t _placeholder_21 >> >> #endif /* __XEN_X86__FEATURESET_DATA__ */ >> >> No functional change. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > with one question below. > >> v2: >> * Write it more pythonically. > Yeah, you know I don't speak Python very well. I was glad I got it to > work without overly much hassle. Generally speaking, if you've got bool-like variables around loops, there's a neater way of expressing it. This one relies on truthy/falsy values where "" is treated as false. (Same too with None, 0, (,), {} and [] for the other primitive datatypes). > >> @@ -382,7 +382,10 @@ def crunch_numbers(state): >> >> names.append(name.lower()) >> >> - state.bitfields.append("bool " + ":1, ".join(names) + ":1") >> + if any(names): >> + state.bitfields.append("bool " + ":1, ".join(names) + ":1") >> + else: >> + state.bitfields.append("uint32_t _placeholder_%s" % (word, )) > I thought % could be used here, but then I wouldn't have known to use > %s (elsewhere we use %u), nor to add an empty argument (which I see > is done in a few other places as well, but not uniformly when %s is > used). I assume there's a reason for the specific way you've done it > here? Hmm. That was taken from your version. In python, %s means "call str() on whatever you have". Similarly, %r means repr(). str() on an integer behaves as %d. But yes, %u would be better here, and consistent with the rest of the script. Happy to fix on commit. ~Andrew
diff --git a/MAINTAINERS b/MAINTAINERS index d8a02a6c1918..a0805d35cd71 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -601,6 +601,7 @@ F: xen/arch/x86/ F: xen/include/public/arch-x86/ F: xen/include/xen/lib/x86 F: xen/lib/x86 +F: xen/tools/gen-cpuid.py F: tools/firmware/hvmloader/ F: tools/firmware/rombios/ F: tools/firmware/vgabios/ diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py index 72cf11654ba9..5797bc64c803 100755 --- a/xen/tools/gen-cpuid.py +++ b/xen/tools/gen-cpuid.py @@ -363,8 +363,8 @@ def crunch_numbers(state): state.deep_features = deps.keys() state.nr_deep_deps = len(state.deep_deps.keys()) - # Calculate the bitfield name declarations - for word in range(state.nr_entries): + # Calculate the bitfield name declarations. Leave 4 placeholders on the end + for word in range(state.nr_entries + 4): names = [] for bit in range(32): @@ -382,7 +382,10 @@ def crunch_numbers(state): names.append(name.lower()) - state.bitfields.append("bool " + ":1, ".join(names) + ":1") + if any(names): + state.bitfields.append("bool " + ":1, ".join(names) + ":1") + else: + state.bitfields.append("uint32_t _placeholder_%s" % (word, )) def write_results(state): @@ -464,8 +467,6 @@ def write_results(state): """) - state.bitfields += ["uint32_t :32 /* placeholder */"] * 4 - for idx, text in enumerate(state.bitfields): state.output.write( "#define CPUID_BITFIELD_%d \\\n %s\n\n"
Add the script to the X86 section in ./MAINTAINERS. Structures or unions without any named members aren't liked by Misra (nor the C standard). Avoid emitting such for leaves without any known bits. The placeholders are affected similarly, but are only visible to MISRA in the middle of a patch series adding a new leaf. The absence of a name was intentional as these defines need to not duplicate names. As that's not deemed acceptable any more, move placeholder processing into the main loop and append the the word number to generate unique names. $ diff cpuid-autogen-{before,after}.h @@ -1034,7 +1034,7 @@ bool intel_psfd:1, ipred_ctrl:1, rrsba_ctrl:1, ddp_ctrl:1, ... #define CPUID_BITFIELD_14 \ - bool :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, ... + uint32_t _placeholder_14 #define CPUID_BITFIELD_15 \ bool :1, :1, :1, :1, avx_vnni_int8:1, avx_ne_convert:1, :1, ... @@ -1043,19 +1043,19 @@ bool rdcl_no:1, eibrs:1, rsba:1, skip_l1dfl:1, intel_ssb_no:1, ... #define CPUID_BITFIELD_17 \ - bool :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, :1, ... + uint32_t _placeholder_17 #define CPUID_BITFIELD_18 \ - uint32_t :32 /* placeholder */ + uint32_t _placeholder_18 #define CPUID_BITFIELD_19 \ - uint32_t :32 /* placeholder */ + uint32_t _placeholder_19 #define CPUID_BITFIELD_20 \ - uint32_t :32 /* placeholder */ + uint32_t _placeholder_20 #define CPUID_BITFIELD_21 \ - uint32_t :32 /* placeholder */ + uint32_t _placeholder_21 #endif /* __XEN_X86__FEATURESET_DATA__ */ No functional change. Signed-off-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> v2: * Write it more pythonically. Fix up the placeholders too. --- MAINTAINERS | 1 + xen/tools/gen-cpuid.py | 11 ++++++----- 2 files changed, 7 insertions(+), 5 deletions(-) base-commit: 51588938e0cd0e02dbc1d6d8c697c577135ff666