| Message ID | 99114c15a4256e6a0f39bd6de232ee4b8ad9b587.1698239734.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] x86/vlapic: address a violation of MISRA C:2012 Rule 16.2 | expand |
On 25.10.2023 15:22, Nicola Vetrini wrote: > The clauses of a switch should be enclosed directly by a switch > statement to make the code more easily understandable and less > prone to errors. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > This patch is mainly indended to probe how the community, especially the > maintainers, would receive such modifications to the code, and whether there > would be consensus on the rule's adoption. Anyone is welcome to > give feedback on this, especially on the x86 side, where this pattern > is used more frequently. The chosen instance (below) is one where it is relatively easy to argue that putting the default label in an inner scope isn't much of a difference as far as overall code size / redundancy is concerned. But there are (perhaps many) other cases where the gains of using what Misra dislikes are much higher. This is another one of the various more recently discussed rules where I think Misra is just going too far, dictating various aspects of style for - in my personal view - no real gain. Furthermore, if you could pick some more involved example (arch/x86/x86_emulate/x86_emulate.c may yield a few "good" examples), I'd like to learn how you propose to change such code, with two up-front constraints: - no added redundancy, - no new goto. Either of them in replacement code would go against what the description above states as a goal. Jan > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1034,10 +1034,10 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) > case APIC_EOI: > case APIC_ESR: > if ( val ) > - { > - default: > return X86EMUL_EXCEPTION; > - } > + break; > + default: > + return X86EMUL_EXCEPTION; > } > > vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val);
On 25/10/2023 15:44, Jan Beulich wrote: > On 25.10.2023 15:22, Nicola Vetrini wrote: >> The clauses of a switch should be enclosed directly by a switch >> statement to make the code more easily understandable and less >> prone to errors. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> This patch is mainly indended to probe how the community, especially >> the >> maintainers, would receive such modifications to the code, and whether >> there >> would be consensus on the rule's adoption. Anyone is welcome to >> give feedback on this, especially on the x86 side, where this pattern >> is used more frequently. > > The chosen instance (below) is one where it is relatively easy to argue > that putting the default label in an inner scope isn't much of a > difference as far as overall code size / redundancy is concerned. But > there are (perhaps many) other cases where the gains of using what > Misra dislikes are much higher. > > This is another one of the various more recently discussed rules where > I think Misra is just going too far, dictating various aspects of style > for - in my personal view - no real gain. Furthermore, if you could > pick some more involved example (arch/x86/x86_emulate/x86_emulate.c may > yield a few "good" examples), I'd like to learn how you propose to > change such code, with two up-front constraints: > - no added redundancy, > - no new goto. > Either of them in replacement code would go against what the > description > above states as a goal. > > Jan > Honestly, I think none of those is attainable in such cases as the ones in x86_emulate.c Take, for instance, index 94caec1d142c..2a70c5f0a197 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1492,10 +1492,10 @@ x86_emulate( if ( ops->rmw && dst.type == OP_MEM ) state->rmw = rmw_add; else - { - case 0x02 ... 0x05: /* add */ emulate_2op_SrcV("add", src, dst, _regs.eflags); - } + break; + case 0x02 ... 0x05: /* add */ + emulate_2op_SrcV("add", src, dst, _regs.eflags); break; or @@ -3504,13 +3504,16 @@ x86_emulate( #if !defined(X86EMUL_NO_MMX) && !defined(X86EMUL_NO_SIMD) case X86EMUL_OPC_66(0x0f, 0x2a): /* cvtpi2pd mm/m64,xmm */ - if ( ea.type == OP_REG ) - { + if ( ea.type == OP_REG ) { + host_and_vcpu_must_have(mmx); + op_bytes = (b & 4) && (vex.pfx & VEX_PREFIX_DOUBLE_MASK) ? 16 : 8; + goto simd_0f_fp; + } + break; case X86EMUL_OPC(0x0f, 0x2a): /* cvtpi2ps mm/m64,xmm */ CASE_SIMD_PACKED_FP(, 0x0f, 0x2c): /* cvttp{s,d}2pi xmm/mem,mm */ CASE_SIMD_PACKED_FP(, 0x0f, 0x2d): /* cvtp{s,d}2pi xmm/mem,mm */ - host_and_vcpu_must_have(mmx); - } + host_and_vcpu_must_have(mmx); op_bytes = (b & 4) && (vex.pfx & VEX_PREFIX_DOUBLE_MASK) ? 16 : 8; goto simd_0f_fp; here, I think some amount of duplication is unavoidable. Global deviation is quite a viable option, and in fact the one I would favour the most.
On 2023-10-25 15:22, Nicola Vetrini wrote: > The clauses of a switch should be enclosed directly by a switch > statement to make the code more easily understandable and less > prone to errors. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > This patch is mainly indended to probe how the community, especially > the > maintainers, would receive such modifications to the code, and whether > there > would be consensus on the rule's adoption. Anyone is welcome to > give feedback on this, especially on the x86 side, where this pattern > is used more frequently. > --- > xen/arch/x86/hvm/vlapic.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > Cc: all the relevant maintainers (mostly x86) An update on the status of this rule's adoption. It has been deemed not beneficial to go after violations in xen/arch/x86/x86_emulate/.* , therefore the only patches to be expected for this rule will be about the few remaining violations in both arm, x86 and common code.
On 25.10.2023 15:22, Nicola Vetrini wrote: > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1034,10 +1034,10 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) > case APIC_EOI: > case APIC_ESR: > if ( val ) > - { > - default: > return X86EMUL_EXCEPTION; > - } > + break; > + default: > + return X86EMUL_EXCEPTION; > } > > vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val); Considering the plan to confine applicability of the rule, one style aspect which would need to be taken into account is that the entire rest of this switch() has blank lines between case blocks. The other is that imo the overall result would be closer to what we have right now if the new code was case APIC_EOI: case APIC_ESR: if ( !val ) break; fallthrough; default: return X86EMUL_EXCEPTION; } at which point the need for the blank line would also disappear. As to the description - isn't this change (whichever way done) also addressing another violation, requiring "break" (or alike according to our interpretation) at the end of each case block? Jan
On 2023-11-21 16:36, Jan Beulich wrote: > On 25.10.2023 15:22, Nicola Vetrini wrote: >> --- a/xen/arch/x86/hvm/vlapic.c >> +++ b/xen/arch/x86/hvm/vlapic.c >> @@ -1034,10 +1034,10 @@ int guest_wrmsr_x2apic(struct vcpu *v, >> uint32_t msr, uint64_t val) >> case APIC_EOI: >> case APIC_ESR: >> if ( val ) >> - { >> - default: >> return X86EMUL_EXCEPTION; >> - } >> + break; >> + default: >> + return X86EMUL_EXCEPTION; >> } >> >> vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val); > > Considering the plan to confine applicability of the rule, one style > aspect > which would need to be taken into account is that the entire rest of > this > switch() has blank lines between case blocks. > > The other is that imo the overall result would be closer to what we > have > right now if the new code was > > case APIC_EOI: > case APIC_ESR: > if ( !val ) > break; > fallthrough; > default: > return X86EMUL_EXCEPTION; > } > > at which point the need for the blank line would also disappear. > This is also a fine solution. I'll keep this in mind when this patch will be revisited. > As to the description - isn't this change (whichever way done) also > addressing another violation, requiring "break" (or alike according to > our interpretation) at the end of each case block? > Correct. It's probably a good idea to mention that, but the "fallthrough" is also a candidate for a deviation from R16.3, so we'll see about that.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index c7ce82d0649a..318dd48577e2 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1034,10 +1034,10 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) case APIC_EOI: case APIC_ESR: if ( val ) - { - default: return X86EMUL_EXCEPTION; - } + break; + default: + return X86EMUL_EXCEPTION; } vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val);
The clauses of a switch should be enclosed directly by a switch statement to make the code more easily understandable and less prone to errors. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- This patch is mainly indended to probe how the community, especially the maintainers, would receive such modifications to the code, and whether there would be consensus on the rule's adoption. Anyone is welcome to give feedback on this, especially on the x86 side, where this pattern is used more frequently. --- xen/arch/x86/hvm/vlapic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)