Re: [XEN PATCH v4 7/9] x86/hvm: address violations of MISRA C Rule 16.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2024 18:48, Federico Serafini wrote:
> MISRA C Rule 16.3 states that "An unconditional `break' statement shall
> terminate every switch-clause".
> 
> Add pseudo keyword fallthrough or missing break statement
> to address violations of the rule.
> 
> As a defensive measure, return an error message or a null pointer in
> case an unreachable return statement is reached.

The two kinds of changes are pretty different in nature. I think that ...

> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> ---
> Changes in v4:
> - do not separate different parts of HVM:
>     a) squash patches 8, 11 and 12 of v3 into this patch;
>     b) address also violations of SVM and VMX;
> - re-arrange fallthrough positioning to comply with Coverity.
> Changes in v3:
> - squashed here modifications of pmtimer.c;

... while the prior splitting by file was indeed unnecessary (when the
main patch's title started with "x86/hvm:"), splitting by measure taken
would be quite helpful. Anything purely mechanical can perhaps stay
together, but everything more involved may want splitting off.

> @@ -2674,6 +2673,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> *hvmemul_ctxt,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        break;
>      }

For example, I'm unconvinced that merely adding "break" is going to be
enough here. Imo at least rc also needs updating, to signal an error to
the caller (which may be what in the description "error message" is
intended to mean). Perhaps the right thing to do here is even to add
"return X86EMUL_*;" instead. Question then is which particular return
value to use. I would have suggested X86EMUL_UNHANDLEABLE, yet its
comment says "No state modified." Then again that may be stale anyway,
so perhaps that's the best we can do here.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1446,6 +1446,7 @@ struct vmx_msr_entry *vmx_find_msr(const struct vcpu 
> *v, uint32_t msr,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        return NULL;
>      }
>  
>      if ( !start )

Right below here there is

        return NULL;

Therefore adding "break" instead may be slightly better.

> @@ -1598,6 +1599,7 @@ int vmx_del_msr(struct vcpu *v, uint32_t msr, enum 
> vmx_msr_list_type type)
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        return -EINVAL;
>      }
>  
>      if ( !start )

Whereas here I agree that we don't want to pass back -ESRCH in such a case.

Jan