Re: [XEN PATCH v2 07/13] x86/hvm: address violations of MISRA C Rule 16.3

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

On 24/06/24 17:32, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -339,7 +339,7 @@ static int hvmemul_do_io(
> >       }
> >       case X86EMUL_UNIMPLEMENTED:
> >           ASSERT_UNREACHABLE();
> > -        /* Fall-through */
> > +        fallthrough;
> >       default:
> >           BUG();
> >       }
> This or very similar comment are replaced elsewhere in this patch. I'm
> sure we have more of them. Hence an alternative would be to deviate those
> variations of what we already deviate. I recall there was a mail from
> Julien asking to avoid extending the set, unless some forms are used
> pretty frequently. Sadly nothing towards judgement between the
> alternatives is said in the description.
I found few occurrences of the hypened fallthrough,
It doesn't seem like a very used form to me,
and most of them are in emulate.c, a file I needed to touch anyway.

The fact that the pseudo keyword is the one preferred is mentioned
in deviations.rst, but I can mention this also in the description.

> > @@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> > *hvmemul_ctxt,
> >   
> >       default:
> >           ASSERT_UNREACHABLE();
> > +        break;
> >       }
> >   
> >       if ( hvmemul_ctxt->ctxt.retire.singlestep )
> > @@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long 
> > gla)
> >           /* fallthrough */
> What about this? It doesn't match anything I see in deviations.rst.
The last item for R16.3 in deviations.rst explicitly says that
existing comment of this form are considered as safe (i.e., deviated)
but deprecated, meaning that the pseudo keyword should be used for new
cases. We can consider a rephrasing if it is not clear enough.

> >       default:
> >           hvm_emulate_writeback(&ctxt);
> > +        break;
> >       }
> >   
> >       return rc;
> > @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, 
> > unsigned int trapnr,
> >           memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
> >                  hvio->mmio_insn_bytes);
> >       }
> > -    /* Fall-through */
> > +    fallthrough;
> >       default:
> >           ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
> >           rc = hvm_emulate_one(&ctx, VIO_no_completion);
> > +        break;
> >       }
> While not as much of a problem for the comment, I view a statement like
> this as mis-indented.
>
> > @@ -5283,6 +5287,8 @@ void hvm_get_segment_register(struct vcpu *v, enum 
> > x86_segment seg,
> >            * %cs and %tr are unconditionally present.  SVM ignores these present
> >            * bits and will happily run without them set.
> >            */
> > +        fallthrough;
> > +
> >       case x86_seg_cs:
> >           reg->p = 1;
> >           break;
> Why the extra blank line here, ...
>
> > --- a/xen/arch/x86/hvm/hypercall.c
> > +++ b/xen/arch/x86/hvm/hypercall.c
> > @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
> >       case 8:
> >           eax = regs->rax;
> >           /* Fallthrough to permission check. */
> > +        fallthrough;
> >       case 4:
> >       case 2:
> >           if ( currd->arch.monitor.guest_request_userspace_enabled &&
> ... when e.g. here there's none? I'm afraid this goes back to an
> unfinished discussion as to whether to have blank lines between blocks
> in fall-through situations. My view is that not having them in these
> cases is helping to make the falling through visually noticeable.
I looked ad the context to preserve the style
of the existing cases.

What do you think about:
-keep the existing style when a break needs to be inserted;
-no blank line if a fallthrough needs to inserted.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)