diff mbox series

[XEN,v2,2/9] x86/cpuid: address violation of MISRA C Rule 16.2

Message ID f957c92d9a00c66df47fc3cac336e378488b9fea.1712305581.git.nicola.vetrini@bugseng.com (mailing list archive)
State Superseded
Headers show
Series address violations of MISRA C Rule 16.2 | expand

Commit Message

Nicola Vetrini April 5, 2024, 9:14 a.m. UTC
Refactor the switch so that a violation of MISRA C Rule 16.2 is resolved
(A switch label shall only be used when the most closely-enclosing
compound statement is the body of a switch statement).
Note that the switch clause ending with the pseudo
keyword "fallthrough" is an allowed exception to Rule 16.3.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/cpuid.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Jan Beulich April 8, 2024, 7:39 a.m. UTC | #1
On 05.04.2024 11:14, Nicola Vetrini wrote:
> Refactor the switch so that a violation of MISRA C Rule 16.2 is resolved
> (A switch label shall only be used when the most closely-enclosing
> compound statement is the body of a switch statement).
> Note that the switch clause ending with the pseudo
> keyword "fallthrough" is an allowed exception to Rule 16.3.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
albeit once again with remarks:

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -331,23 +331,22 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>          switch ( subleaf )
>          {
>          case 1:
> -            if ( p->xstate.xsavec || p->xstate.xsaves )
> -            {
> -                /*
> -                 * TODO: Figure out what to do for XSS state.  VT-x manages
> -                 * host vs guest MSR_XSS automatically, so as soon as we start
> -                 * supporting any XSS states, the wrong XSS will be in
> -                 * context.
> -                 */
> -                BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
> -
> -                /*
> -                 * Read CPUID[0xD,0/1].EBX from hardware.  They vary with
> -                 * enabled XSTATE, and appropraite XCR0|XSS are in context.
> -                 */
> +            if ( !(p->xstate.xsavec || p->xstate.xsaves) )

Personally I dislike with for of writing such. It may not be overly much of a
problem for simple cases like here, but the more complex the expression gets,
the less helpful it is that somewhere far away there's an enclosing !(...). I
may take the liberty to adjust this, should I end up committing this change.

> +                break;
> +            /*
> +             * TODO: Figure out what to do for XSS state.  VT-x manages
> +             * host vs guest MSR_XSS automatically, so as soon as we start
> +             * supporting any XSS states, the wrong XSS will be in
> +             * context.
> +             */

Much like one actually needs to consider re-flowing when increasing indentation
of a comment, it is generally desirable to also to so when decreasing
indentation, which in this case surely would allow at least "context" to be
moved to the earlier line.

Jan
Nicola Vetrini April 9, 2024, 7:48 p.m. UTC | #2
On 2024-04-08 09:39, Jan Beulich wrote:
> On 05.04.2024 11:14, Nicola Vetrini wrote:
>> Refactor the switch so that a violation of MISRA C Rule 16.2 is 
>> resolved
>> (A switch label shall only be used when the most closely-enclosing
>> compound statement is the body of a switch statement).
>> Note that the switch clause ending with the pseudo
>> keyword "fallthrough" is an allowed exception to Rule 16.3.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> albeit once again with remarks:
> 
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -331,23 +331,22 @@ void guest_cpuid(const struct vcpu *v, uint32_t 
>> leaf,
>>          switch ( subleaf )
>>          {
>>          case 1:
>> -            if ( p->xstate.xsavec || p->xstate.xsaves )
>> -            {
>> -                /*
>> -                 * TODO: Figure out what to do for XSS state.  VT-x 
>> manages
>> -                 * host vs guest MSR_XSS automatically, so as soon as 
>> we start
>> -                 * supporting any XSS states, the wrong XSS will be 
>> in
>> -                 * context.
>> -                 */
>> -                BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
>> -
>> -                /*
>> -                 * Read CPUID[0xD,0/1].EBX from hardware.  They vary 
>> with
>> -                 * enabled XSTATE, and appropraite XCR0|XSS are in 
>> context.
>> -                 */
>> +            if ( !(p->xstate.xsavec || p->xstate.xsaves) )
> 
> Personally I dislike with for of writing such. It may not be overly 
> much of a
> problem for simple cases like here, but the more complex the expression 
> gets,
> the less helpful it is that somewhere far away there's an enclosing 
> !(...). I
> may take the liberty to adjust this, should I end up committing this 
> change.
> 

Ok, makes sense. I didn't think about that style aspect.

>> +                break;
>> +            /*
>> +             * TODO: Figure out what to do for XSS state.  VT-x 
>> manages
>> +             * host vs guest MSR_XSS automatically, so as soon as we 
>> start
>> +             * supporting any XSS states, the wrong XSS will be in
>> +             * context.
>> +             */
> 
> Much like one actually needs to consider re-flowing when increasing 
> indentation
> of a comment, it is generally desirable to also to so when decreasing
> indentation, which in this case surely would allow at least "context" 
> to be
> moved to the earlier line.
> 
> Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 7290a979c667..0a7c55199f94 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -331,23 +331,22 @@  void guest_cpuid(const struct vcpu *v, uint32_t leaf,
         switch ( subleaf )
         {
         case 1:
-            if ( p->xstate.xsavec || p->xstate.xsaves )
-            {
-                /*
-                 * TODO: Figure out what to do for XSS state.  VT-x manages
-                 * host vs guest MSR_XSS automatically, so as soon as we start
-                 * supporting any XSS states, the wrong XSS will be in
-                 * context.
-                 */
-                BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
-
-                /*
-                 * Read CPUID[0xD,0/1].EBX from hardware.  They vary with
-                 * enabled XSTATE, and appropraite XCR0|XSS are in context.
-                 */
+            if ( !(p->xstate.xsavec || p->xstate.xsaves) )
+                break;
+            /*
+             * TODO: Figure out what to do for XSS state.  VT-x manages
+             * host vs guest MSR_XSS automatically, so as soon as we start
+             * supporting any XSS states, the wrong XSS will be in
+             * context.
+             */
+            BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
+            fallthrough;
         case 0:
-                res->b = cpuid_count_ebx(leaf, subleaf);
-            }
+            /*
+             * Read CPUID[0xD,0/1].EBX from hardware.  They vary with
+             * enabled XSTATE, and appropriate XCR0|XSS are in context.
+             */
+            res->b = cpuid_count_ebx(leaf, subleaf);
             break;
         }
         break;