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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
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