[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.