Re: xen/x86: resolve the last 3 MISRA R16.6 violations

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-02-18 22:42, Stefano Stabellini wrote:
> On Tue, 18 Feb 2025, Jan Beulich wrote:
> > On 18.02.2025 00:12, Stefano Stabellini wrote:
> > > On Mon, 17 Feb 2025, Jan Beulich wrote:
> > >> On 15.02.2025 03:16, Stefano Stabellini wrote:
> > >>> --- a/xen/arch/x86/hvm/hvm.c
> > >>> +++ b/xen/arch/x86/hvm/hvm.c
> > >>> @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int 
> > reg)
> > >>>  {
> > >>>      ASSERT(v == current || !vcpu_runnable(v));
> > >>>
> > >>> -    switch ( reg )
> > >>> -    {
> > >>> -    default:
> > >>> -        return alternative_call(hvm_funcs.get_reg, v, reg);
> > >>> -    }
> > >>> +    return alternative_call(hvm_funcs.get_reg, v, reg);
> > >>>  }
> > >>>
> > >>>  void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
> > >>>  {
> > >>>      ASSERT(v == current || !vcpu_runnable(v));
> > >>>
> > >>> -    switch ( reg )
> > >>> -    {
> > >>> -    default:
> > >>> -        return alternative_vcall(hvm_funcs.set_reg, v, reg, val);
> > >>> -    }
> > >>> +    return alternative_vcall(hvm_funcs.set_reg, v, reg, val);
> > >>>  }
> > >>
> > >> Both of these were, iirc, deliberately written using switch(), to ease
> > >> possible future changes.
> > >
> > > To be honest, I do not see any value in the way they are currently
> > > written. However, if you prefer, I can add a deviation for this, with
> > > one SAF comment for each of these two. The reason for the deviation
> > > would be "deliberate to ease possible future change". Please let me know
> > > how you would like to proceed.
> >
> > Well, best next thing you can do is seek input from the person who has
> > written that code, i.e. Andrew.
> Andrew wrote in chat that he is OK with a deviation and he can live 
> with
> a SAF deviation. Here is the patch.
>
>
> ---
> xen/x86: resolve the last 3 MISRA R16.6 violations
>
> MISRA R16.6 states that "Every switch statement shall have at least two
> switch-clauses". There are only 3 violations left on x86 (zero on ARM).
>
> One of them is only a violation depending on the kconfig configuration.
> So deviate it instead with a SAF comment.
>
> Two of them are deliberate to enable future additions. Deviate them as
> such.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
Looks good to me, from an ECLAIR point of view. Did you have a chance to 
run a pipeline on it to confirm that the SAF comments are recognized 
correctly?
With that,

Reviewed-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>

> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index b8a4f878ea..3d68b59169 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -92,6 +92,22 @@
>          },
>          {
>              "id": "SAF-11-safe",
> +            "analyser": {
> +                "eclair": "MC3A2.R16.6"
> +            },
> +            "name": "Rule 16.6: single clause due to kconfig",
> +            "text": "A switch statement with a single switch clause 
> because other switch clauses are disabled in a given kconfig is safe."
> +        },
> +        {
> +            "id": "SAF-12-safe",
> +            "analyser": {
> +                "eclair": "MC3A2.R16.6"
> +            },
> +            "name": "Rule 16.6: single clause due to future 
> expansion",
> +            "text": "A switch statement with a single switch clause to 
> purposely enable future additions of new cases is safe."
> +        },
> +        {
> +            "id": "SAF-13-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 39e39ce4ce..0f0630769b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3797,6 +3797,7 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int 
> reg)
>  {
>      ASSERT(v == current || !vcpu_runnable(v));
>
> +    /* SAF-12-safe */
>      switch ( reg )
>      {
>      default:
> @@ -3808,6 +3809,7 @@ void hvm_set_reg(struct vcpu *v, unsigned int 
> reg, uint64_t val)
>  {
>      ASSERT(v == current || !vcpu_runnable(v));
>
> +    /* SAF-12-safe */
>      switch ( reg )
>      {
>      default:
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 87b30ce4df..dca11a613d 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -436,6 +436,7 @@ unsigned long get_stack_trace_bottom(unsigned long 
> sp)
>  static unsigned long get_shstk_bottom(unsigned long sp)
>  {
> +    /* SAF-11-safe */
>      switch ( get_stack_page(sp) )
>      {
>  #ifdef CONFIG_XEN_SHSTK
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253