Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

Hello Stefano,

On 29/06/23 21:47, Stefano Stabellini wrote:
> On Thu, 29 Jun 2023, Federico Serafini wrote:
> > Change the parameter names of function declarations to be consistent
> > with the names used in the corresponding function definitions
> > so as to fix violations of MISRA C:2012 Rule 8.3.
> >
> > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > ---
> >   
> > diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > index d07fcb2bc9..24bf409d8f 100644
> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > @@ -656,10 +656,10 @@ bool vmx_msr_is_intercepted(struct vmx_msr_bitmap 
> > *msr_bitmap,
> >                               unsigned int msr, bool is_write) __nonnull(1);
> >   void virtual_vmcs_enter(const struct vcpu *);
> >   void virtual_vmcs_exit(const struct vcpu *);
> > -u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
> > +u64 virtual_vmcs_vmread(const struct vcpu *, u32 vmcs_encoding);
> Shouldn't the first parameter be "v" to match the definition?
>
> Or is that a different MISRA C rule?
This is a violation of MISRA C:2012 Rule 8.2:
"function types shall be in prototype form with named parameters".
However, I can propose a new patch version to fix it as well.

> >   enum vmx_insn_errno virtual_vmcs_vmread_safe(const struct vcpu *v,
> >                                                u32 vmcs_encoding, u64 *val);
> > -void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
> > +void virtual_vmcs_vmwrite(const struct vcpu *, u32 vmcs_encoding, u64 val);
> same here
>
>
> >   enum vmx_insn_errno virtual_vmcs_vmwrite_safe(const struct vcpu *v,
> >                                                 u32 vmcs_encoding, u64 val);
> >   
> > diff --git a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
> > index dc9db69258..1e4bbc0d78 100644
> > --- a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
> > @@ -144,9 +144,9 @@ enum vvmcs_encoding_type {
> >       VVMCS_TYPE_HSTATE,
> >   };
> >   
> > -u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
> > +u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding);
> This one should return uint64_t to match the definition. Or is that a
> different MISRA C rule?
This is a violation of the same rule (8.3) but the focus of this patch 
series was intended to be on be only on parameter names.
I can propose a new version of the patch to also address the additional 
violations discussed.
> >   u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
> > -void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
> > +void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val);
> This one should be:
> void set_vvmcs_virtual(void *vvmcs, uint32_t vmcs_encoding, uint64_t val)
>
> Other than these, everything else checks out
Regards

--
Federico Serafini, M.Sc.

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