Re: [XEN PATCH v2 07/13] x86/vmx: fix violations of MISRA C:2012 Rule 7.2

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 6 Jul 2023, Jan Beulich wrote:
> On 05.07.2023 17:26, Simone Ballarin wrote:
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -257,14 +257,14 @@ uint64_t get_vvmcs_virtual(void *vvmcs, uint32_t 
> > vmcs_encoding)
> >  
> >      switch ( enc.width ) {
> >      case VVMCS_WIDTH_16:
> > -        res &= 0xffff;
> > +        res &= 0xffffU;
> 
> I don't think the suffix is needed in cases like this one, and ...
> 
> >          break;
> >     case VVMCS_WIDTH_64:
> >          if ( enc.access_type )
> >              res >>= 32;
> >          break;
> >      case VVMCS_WIDTH_32:
> > -        res &= 0xffffffff;
> > +        res &= 0xffffffffU;
> 
> ... while generally I'm suggesting to avoid casts I wonder whether
> casting to uint32_t here wouldn't make things more obviously match
> the purpose. (Same again further down then.)
> 
> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > @@ -207,7 +207,7 @@ void vmx_vmcs_reload(struct vcpu *v);
> >  #define CPU_BASED_ACTIVATE_MSR_BITMAP         0x10000000
> >  #define CPU_BASED_MONITOR_EXITING             0x20000000
> >  #define CPU_BASED_PAUSE_EXITING               0x40000000
> > -#define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS 0x80000000
> > +#define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS 0x80000000U
> 
> Interesting - you don't change adjacent #define-s here, nor ...
> 
> > @@ -257,7 +257,7 @@ extern u32 vmx_vmentry_control;
> >  #define SECONDARY_EXEC_XSAVES                   0x00100000
> >  #define SECONDARY_EXEC_TSC_SCALING              0x02000000
> >  #define SECONDARY_EXEC_BUS_LOCK_DETECTION       0x40000000
> > -#define SECONDARY_EXEC_NOTIFY_VM_EXITING        0x80000000
> > +#define SECONDARY_EXEC_NOTIFY_VM_EXITING        0x80000000U
> 
> ... here. May I ask why that is? (I'm not opposed, but the
> description suggests otherwise.)

Like I wrote in the other email, the requirement is only to add U where
the top bit is set (0x80000000). Adding U to the other constant is
optional and for us to decide.


> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > @@ -136,7 +136,7 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
> >  /*
> >   * Exit Reasons
> >   */
> > -#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000
> > +#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000U
> >  #define VMX_EXIT_REASONS_BUS_LOCK       (1u << 26)
> 
> Along the lines of the latter, perhaps switch to 1u << 31?
> 
> > @@ -246,15 +246,15 @@ typedef union cr_access_qual {
> >  /*
> >   * Access Rights
> >   */
> > -#define X86_SEG_AR_SEG_TYPE     0xf        /* 3:0, segment type */
> > -#define X86_SEG_AR_DESC_TYPE    (1u << 4)  /* 4, descriptor type */
> > -#define X86_SEG_AR_DPL          0x60       /* 6:5, descriptor privilege 
> > level */
> > -#define X86_SEG_AR_SEG_PRESENT  (1u << 7)  /* 7, segment present */
> > -#define X86_SEG_AR_AVL          (1u << 12) /* 12, available for system 
> > software */
> > -#define X86_SEG_AR_CS_LM_ACTIVE (1u << 13) /* 13, long mode active (CS 
> > only) */
> > -#define X86_SEG_AR_DEF_OP_SIZE  (1u << 14) /* 14, default operation size */
> > -#define X86_SEG_AR_GRANULARITY  (1u << 15) /* 15, granularity */
> > -#define X86_SEG_AR_SEG_UNUSABLE (1u << 16) /* 16, segment unusable */
> > +#define X86_SEG_AR_SEG_TYPE     0xfU       /* 3:0, segment type */
> > +#define X86_SEG_AR_DESC_TYPE    (1U << 4)  /* 4, descriptor type */
> > +#define X86_SEG_AR_DPL          0x60U      /* 6:5, descriptor privilege 
> > level */
> > +#define X86_SEG_AR_SEG_PRESENT  (1U << 7)  /* 7, segment present */
> > +#define X86_SEG_AR_AVL          (1U << 12) /* 12, available for system 
> > software */
> > +#define X86_SEG_AR_CS_LM_ACTIVE (1U << 13) /* 13, long mode active (CS 
> > only) */
> > +#define X86_SEG_AR_DEF_OP_SIZE  (1U << 14) /* 14, default operation size */
> > +#define X86_SEG_AR_GRANULARITY  (1U << 15) /* 15, granularity */
> > +#define X86_SEG_AR_SEG_UNUSABLE (1U << 16) /* 16, segment unusable */
> 
> How is this change related to rule 7.2? There are u suffixes already where
> needed (and 0xf and 0x60 don't strictly need one), so there's no violation
> here afaict. A mere style change to switch from u to U imo doesn't belong
> here (and, as mentioned while discussing the rule, is imo hampering
> readability in cases like these ones).

My understanding is that these are not MISRA violations so could be
dropped