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
