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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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