On 20.06.2023 12:34, Simone Ballarin wrote:
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -950,10 +950,10 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void)
> fixed_ctrl_mask |=
> (FIXED_CTR_CTRL_ANYTHREAD_MASK << (FIXED_CTR_CTRL_BITS * i));
>
> - fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1);
> + fixed_counters_mask = ~((1Ull << core2_get_bitwidth_fix_count()) - 1);
What's the point of this adjustment? And if at all, why keep the l-s
lowercase?
> global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) |
> ((1ULL << arch_pmc_cnt) - 1));
> - global_ovf_ctrl_mask = ~(0xC000000000000000 |
> + global_ovf_ctrl_mask = ~(0xC000000000000000U |
If such constants gain a suffix, I think that ought to be UL or ULL.
> --- 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,17 +208,17 @@ 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 0xffU /* 7:0 */
> +#define INTR_INFO_INTR_TYPE_MASK 0x700U /* 10:8 */
> +#define INTR_INFO_DELIVER_CODE_MASK 0x800U /* 11 */
> +#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000 /* 12 */
> +#define INTR_INFO_VALID_MASK 0x80000000U /* 31 */
> +#define INTR_INFO_RESVD_BITS_MASK 0x7ffff000U
I think it would be nice if you took the opportunity and added
zero-padding to these constants.
> /*
> * Exit Qualifications for NOTIFY VM EXIT
> */
> -#define NOTIFY_VM_CONTEXT_INVALID 1u
> +#define NOTIFY_VM_CONTEXT_INVALID 1U
Like above - why this change?
> @@ -247,14 +247,14 @@ 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_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_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 */
And all of these, when it's exactly the two numbers you don't touch
which might want to gain a U (or u) suffix?
Jan
