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

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

Il giorno gio 6 lug 2023 alle ore 10:04 Jan Beulich <jbeulich@xxxxxxxx> 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.

--

Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)