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

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

[Nicola Vetrini]

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(-)

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);
-- 
2.34.1

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

[Jan Beulich]

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);

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

[Nicola Vetrini]

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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Nicola Vetrini]

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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Jan Beulich]

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

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

[Nicola Vetrini]

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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)