| Message ID | f30ef7c2cda2516d9ef07bb79e5da5513cd90c6c.1688559115.git.gianluca.luparini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fix violations of MISRA C:2012 Rule 7.2 | expand |
On Wed, 5 Jul 2023, Simone Ballarin wrote: > From: Gianluca Luparini <gianluca.luparini@bugseng.com> > > The xen sources contains violations of MISRA C:2012 Rule 7.2 whose > headline states: > "A 'u' or 'U' suffix shall be applied to all integer constants > that are represented in an unsigned type". > > Add the 'U' suffix to integers literals with unsigned type and also to other > literals used in the same contexts or near violations, when their positive > nature is immediately clear. The latter changes are done for the sake of > uniformity. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > Signed-off-by: Gianluca Luparini <gianluca.luparini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v2: > - minor change to commit title > - change commit message > - remove unnecessary changes in 'vpmu_intel.c' and 'vmx.h' > - add 'ULL' suffix in 'vpmu_intel.c' > - add zero-padding to constants in 'vmx.h' > - add missing 'U' in 'vmx.h' > --- > xen/arch/x86/cpu/vpmu_intel.c | 2 +- > xen/arch/x86/hvm/vmx/vmcs.c | 6 ++--- > xen/arch/x86/hvm/vmx/vvmx.c | 12 ++++----- > xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 6 ++--- > xen/arch/x86/include/asm/hvm/vmx/vmx.h | 34 ++++++++++++------------- > 5 files changed, 30 insertions(+), 30 deletions(-) > > diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c > index bda5d511ac..890c279310 100644 > --- a/xen/arch/x86/cpu/vpmu_intel.c > +++ b/xen/arch/x86/cpu/vpmu_intel.c > @@ -946,7 +946,7 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void) > fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1); > global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) | > ((1ULL << arch_pmc_cnt) - 1)); > - global_ovf_ctrl_mask = ~(0xC000000000000000 | > + global_ovf_ctrl_mask = ~(0xC000000000000000ULL | > (((1ULL << fixed_pmc_cnt) - 1) << 32) | > ((1ULL << arch_pmc_cnt) - 1)); > if ( version > 2 ) > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > index b209563625..d5a2b847a9 100644 > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -911,7 +911,7 @@ void vmx_clear_msr_intercept(struct vcpu *v, unsigned int msr, > if ( type & VMX_MSR_W ) > clear_bit(msr, msr_bitmap->write_low); > } > - else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) ) > + else if ( (msr >= 0xc0000000U) && (msr <= 0xc0001fffU) ) > { > msr &= 0x1fff; > if ( type & VMX_MSR_R ) > @@ -939,7 +939,7 @@ void vmx_set_msr_intercept(struct vcpu *v, unsigned int msr, > if ( type & VMX_MSR_W ) > set_bit(msr, msr_bitmap->write_low); > } > - else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) ) > + else if ( (msr >= 0xc0000000U) && (msr <= 0xc0001fffU) ) > { > msr &= 0x1fff; > if ( type & VMX_MSR_R ) > @@ -957,7 +957,7 @@ bool vmx_msr_is_intercepted(struct vmx_msr_bitmap *msr_bitmap, > if ( msr <= 0x1fff ) > return test_bit(msr, is_write ? msr_bitmap->write_low > : msr_bitmap->read_low); > - else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) ) > + else if ( (msr >= 0xc0000000U) && (msr <= 0xc0001fffU) ) > return test_bit(msr & 0x1fff, is_write ? msr_bitmap->write_high > : msr_bitmap->read_high); > else > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index 1034534c97..f59de0f124 100644 > --- 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; > break; > case VVMCS_WIDTH_64: > if ( enc.access_type ) > res >>= 32; > break; > case VVMCS_WIDTH_32: > - res &= 0xffffffff; > + res &= 0xffffffffU; > break; > case VVMCS_WIDTH_NATURAL: > default: > @@ -311,19 +311,19 @@ void set_vvmcs_virtual(void *vvmcs, uint32_t vmcs_encoding, uint64_t val) > > switch ( enc.width ) { > case VVMCS_WIDTH_16: > - res = val & 0xffff; > + res = val & 0xffffU; > break; > case VVMCS_WIDTH_64: > if ( enc.access_type ) > { > - res &= 0xffffffff; > + res &= 0xffffffffU; > res |= val << 32; > } > else > res = val; > break; > case VVMCS_WIDTH_32: > - res = val & 0xffffffff; > + res = val & 0xffffffffU; > break; > case VVMCS_WIDTH_NATURAL: > default: > @@ -2307,7 +2307,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > break; > case MSR_IA32_VMX_CR0_FIXED1: > /* allow 0-settings for all bits */ > - data = 0xffffffff; > + data = 0xffffffffU; > break; > case MSR_IA32_VMX_CR4_FIXED0: > /* VMXE bit must be 1 in VMX operation */ > diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h > index d07fcb2bc9..4acf3970f5 100644 > --- 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 > extern u32 vmx_cpu_based_exec_control; > > #define PIN_BASED_EXT_INTR_MASK 0x00000001 > @@ -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 > extern u32 vmx_secondary_exec_control; > > #define VMX_EPT_EXEC_ONLY_SUPPORTED 0x00000001 > @@ -346,7 +346,7 @@ extern u64 vmx_ept_vpid_cap; > #define cpu_has_vmx_notify_vm_exiting \ > (vmx_secondary_exec_control & SECONDARY_EXEC_NOTIFY_VM_EXITING) > > -#define VMCS_RID_TYPE_MASK 0x80000000 > +#define VMCS_RID_TYPE_MASK 0x80000000U > > /* GUEST_INTERRUPTIBILITY_INFO flags. */ > #define VMX_INTR_SHADOW_STI 0x00000001 > diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h > index 36c108d879..6642688e1d 100644 > --- 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) > > #define EXIT_REASON_EXCEPTION_NMI 0 > @@ -208,12 +208,12 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) > * Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit Qualification > * field for EPT violations, PML full and SPP-related event vmexits. > */ > -#define INTR_INFO_VECTOR_MASK 0xff /* 7:0 */ > -#define INTR_INFO_INTR_TYPE_MASK 0x700 /* 10:8 */ > -#define INTR_INFO_DELIVER_CODE_MASK 0x800 /* 11 */ > -#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000 /* 12 */ > -#define INTR_INFO_VALID_MASK 0x80000000 /* 31 */ > -#define INTR_INFO_RESVD_BITS_MASK 0x7ffff000 > +#define INTR_INFO_VECTOR_MASK 0x000000ffU /* 7:0 */ > +#define INTR_INFO_INTR_TYPE_MASK 0x00000700U /* 10:8 */ > +#define INTR_INFO_DELIVER_CODE_MASK 0x00000800U /* 11 */ > +#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x00001000U /* 12 */ > +#define INTR_INFO_VALID_MASK 0x80000000U /* 31 */ > +#define INTR_INFO_RESVD_BITS_MASK 0x7ffff000U > > /* > * Exit Qualifications for NOTIFY VM EXIT > @@ -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 */ > > #define VMCALL_OPCODE ".byte 0x0f,0x01,0xc1\n" > #define VMCLEAR_OPCODE ".byte 0x66,0x0f,0xc7\n" /* reg/opcode: /6 */ > @@ -606,7 +606,7 @@ static inline void vmx_pi_hooks_assign(struct domain *d) {} > static inline void vmx_pi_hooks_deassign(struct domain *d) {} > #endif > > -#define APIC_INVALID_DEST 0xffffffff > +#define APIC_INVALID_DEST 0xffffffffU > > /* EPT violation qualifications definitions */ > typedef union ept_qual { > -- > 2.41.0 >
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.) > --- 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). Jan
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
On 07.07.2023 00:17, Stefano Stabellini wrote: > On Thu, 6 Jul 2023, Jan Beulich wrote: >> On 05.07.2023 17:26, Simone Ballarin wrote: >>> --- 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. Right, but as said then the description shouldn't suggest things are being done consistently everywhere. (Likely this is going to become easy when splitting by maintainership area, by then simply omitting the respective sentence from the description.) Jan
Il giorno gio 6 lug 2023 alle ore 10:04 Jan Beulich <jbeulich@suse.com> ha scritto: > 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.) > Using the cast is fine. I will change the patch. > > > --- 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.) > > > --- 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). > > Jan > In general, I will remove all non-strictly required U's you have pointed out. I will also amend the commit messages in these cases.
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c index bda5d511ac..890c279310 100644 --- a/xen/arch/x86/cpu/vpmu_intel.c +++ b/xen/arch/x86/cpu/vpmu_intel.c @@ -946,7 +946,7 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void) fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1); global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) | ((1ULL << arch_pmc_cnt) - 1)); - global_ovf_ctrl_mask = ~(0xC000000000000000 | + global_ovf_ctrl_mask = ~(0xC000000000000000ULL | (((1ULL << fixed_pmc_cnt) - 1) << 32) | ((1ULL << arch_pmc_cnt) - 1)); if ( version > 2 ) diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index b209563625..d5a2b847a9 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -911,7 +911,7 @@ void vmx_clear_msr_intercept(struct vcpu *v, unsigned int msr, if ( type & VMX_MSR_W ) clear_bit(msr, msr_bitmap->write_low); } - else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) ) + else if ( (msr >= 0xc0000000U) && (msr <= 0xc0001fffU) ) { msr &= 0x1fff; if ( type & VMX_MSR_R ) @@ -939,7 +939,7 @@ void vmx_set_msr_intercept(struct vcpu *v, unsigned int msr, if ( type & VMX_MSR_W ) set_bit(msr, msr_bitmap->write_low); } - else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) ) + else if ( (msr >= 0xc0000000U) && (msr <= 0xc0001fffU) ) { msr &= 0x1fff; if ( type & VMX_MSR_R ) @@ -957,7 +957,7 @@ bool vmx_msr_is_intercepted(struct vmx_msr_bitmap *msr_bitmap, if ( msr <= 0x1fff ) return test_bit(msr, is_write ? msr_bitmap->write_low : msr_bitmap->read_low); - else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) ) + else if ( (msr >= 0xc0000000U) && (msr <= 0xc0001fffU) ) return test_bit(msr & 0x1fff, is_write ? msr_bitmap->write_high : msr_bitmap->read_high); else diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 1034534c97..f59de0f124 100644 --- 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; break; case VVMCS_WIDTH_64: if ( enc.access_type ) res >>= 32; break; case VVMCS_WIDTH_32: - res &= 0xffffffff; + res &= 0xffffffffU; break; case VVMCS_WIDTH_NATURAL: default: @@ -311,19 +311,19 @@ void set_vvmcs_virtual(void *vvmcs, uint32_t vmcs_encoding, uint64_t val) switch ( enc.width ) { case VVMCS_WIDTH_16: - res = val & 0xffff; + res = val & 0xffffU; break; case VVMCS_WIDTH_64: if ( enc.access_type ) { - res &= 0xffffffff; + res &= 0xffffffffU; res |= val << 32; } else res = val; break; case VVMCS_WIDTH_32: - res = val & 0xffffffff; + res = val & 0xffffffffU; break; case VVMCS_WIDTH_NATURAL: default: @@ -2307,7 +2307,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) break; case MSR_IA32_VMX_CR0_FIXED1: /* allow 0-settings for all bits */ - data = 0xffffffff; + data = 0xffffffffU; break; case MSR_IA32_VMX_CR4_FIXED0: /* VMXE bit must be 1 in VMX operation */ diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h index d07fcb2bc9..4acf3970f5 100644 --- 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 extern u32 vmx_cpu_based_exec_control; #define PIN_BASED_EXT_INTR_MASK 0x00000001 @@ -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 extern u32 vmx_secondary_exec_control; #define VMX_EPT_EXEC_ONLY_SUPPORTED 0x00000001 @@ -346,7 +346,7 @@ extern u64 vmx_ept_vpid_cap; #define cpu_has_vmx_notify_vm_exiting \ (vmx_secondary_exec_control & SECONDARY_EXEC_NOTIFY_VM_EXITING) -#define VMCS_RID_TYPE_MASK 0x80000000 +#define VMCS_RID_TYPE_MASK 0x80000000U /* GUEST_INTERRUPTIBILITY_INFO flags. */ #define VMX_INTR_SHADOW_STI 0x00000001 diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h index 36c108d879..6642688e1d 100644 --- 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) #define EXIT_REASON_EXCEPTION_NMI 0 @@ -208,12 +208,12 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) * Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit Qualification * field for EPT violations, PML full and SPP-related event vmexits. */ -#define INTR_INFO_VECTOR_MASK 0xff /* 7:0 */ -#define INTR_INFO_INTR_TYPE_MASK 0x700 /* 10:8 */ -#define INTR_INFO_DELIVER_CODE_MASK 0x800 /* 11 */ -#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000 /* 12 */ -#define INTR_INFO_VALID_MASK 0x80000000 /* 31 */ -#define INTR_INFO_RESVD_BITS_MASK 0x7ffff000 +#define INTR_INFO_VECTOR_MASK 0x000000ffU /* 7:0 */ +#define INTR_INFO_INTR_TYPE_MASK 0x00000700U /* 10:8 */ +#define INTR_INFO_DELIVER_CODE_MASK 0x00000800U /* 11 */ +#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x00001000U /* 12 */ +#define INTR_INFO_VALID_MASK 0x80000000U /* 31 */ +#define INTR_INFO_RESVD_BITS_MASK 0x7ffff000U /* * Exit Qualifications for NOTIFY VM EXIT @@ -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 */ #define VMCALL_OPCODE ".byte 0x0f,0x01,0xc1\n" #define VMCLEAR_OPCODE ".byte 0x66,0x0f,0xc7\n" /* reg/opcode: /6 */ @@ -606,7 +606,7 @@ static inline void vmx_pi_hooks_assign(struct domain *d) {} static inline void vmx_pi_hooks_deassign(struct domain *d) {} #endif -#define APIC_INVALID_DEST 0xffffffff +#define APIC_INVALID_DEST 0xffffffffU /* EPT violation qualifications definitions */ typedef union ept_qual {