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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

>      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.

Jan