Re: [XEN PATCH 07/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich <jbeulich@xxxxxxxx>]

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