Re: [XEN PATCH v2 7/9] xen/xsm: address violation of MISRA C Rule 16.2

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 4/9/24 16:00, Nicola Vetrini wrote:
> On 2024-04-09 16:02, Daniel P. Smith wrote:
> > On 4/5/24 05: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.
> > To give you a quick response, on cursory review, I do not believe the 
> > two are equivalent. I have never been a fan of the Duff's device in 
> > use here, as it makes reasoning about all the variations difficult. I 
> > unrolled all of this once before, and I recall it being a bit more 
> > intricate than this.
> > v/r,
> > dps
> Hi,
>
> basically what I was trying to accomplish is this: XSM_TARGET will 
> either enter the if and return, or fallthrough to XSM_XS_PRIV, but then 
> it won't enter the if (as it did before because in order to reach this 
> second case's if statement it had to also satisfy the first condition 
> (evaluate_nospec(src == target)), which would then cause the XSM_TARGET 
> phase to return 0). And then if either the action != XSM_XS_PRIV or the 
> other condition is not satisfied we go down to XSM_DM_PRIV, just like 
> before. I may have made a logical mistake somewhere, of course, but it 
> seems the same thing semantically, by relying on the fact that
> case XSM_TARGET:
> if (...) {
>    return 0;
> case XSM_XS_PRIV:
>     ...
> }
>
> can be rewritten as
>
> case XSM_TARGET:
>    if (...) { return 0; }
>    fallthrough;
> case XSM_XS_PRIV:
>    if (action == XSM_XS_PRIV && ...) { ... }
> case XSM_DM_PRIV:
>
> because there wasn't other code after the closed bracket of the if in 
> the XSM_XS_PRIV case (only a fallthough that remained as is).
Yes, upon a closer look, you are correct. I just did a quick read 
earlier since this has been sitting for a few days. I missed that you 
did in fact have it falling all the way down through all the subsequent 
checks for each case "entry point" into the privilege hierarchy. 
Honestly, I probably should look into a comment to document the 
hierarchy of the checks that is buried in there.
Acked-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>