Re: [RFC PATCH] x86/vlapic: address a violation of MISRA C:2012 Rule 16.2

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > ---
> > 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.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)