Re: [XEN PATCH v4 4/4] xen/x86: address violations of MISRA C:2012 Rule 7.2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2023 13:03, Simone Ballarin wrote:
> @@ -70,15 +70,15 @@ static const uint8_t sr_mask[8] = {
>  };
>  
>  static const uint8_t gr_mask[9] = {
> -    (uint8_t)~0xf0, /* 0x00 */
> -    (uint8_t)~0xf0, /* 0x01 */
> -    (uint8_t)~0xf0, /* 0x02 */
> -    (uint8_t)~0xe0, /* 0x03 */
> -    (uint8_t)~0xfc, /* 0x04 */
> -    (uint8_t)~0x84, /* 0x05 */
> -    (uint8_t)~0xf0, /* 0x06 */
> -    (uint8_t)~0xf0, /* 0x07 */
> -    (uint8_t)~0x00, /* 0x08 */
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xe0,
> +    (uint8_t)~0xfc,
> +    (uint8_t)~0x84,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0x00,
>  };

Hmm, this stray change is _still_ there.

> --- a/xen/arch/x86/include/asm/hvm/trace.h
> +++ b/xen/arch/x86/include/asm/hvm/trace.h
> @@ -58,7 +58,7 @@
>  #define DO_TRC_HVM_VLAPIC           DEFAULT_HVM_MISC
>  
>  
> -#define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
> +#define TRC_PAR_LONG(par) ((uint32_t)par), ((par) >> 32)

You've lost parentheses around "par".

> @@ -93,7 +93,7 @@
>      HVMTRACE_ND(evt, 0, 0)
>  
>  #define HVMTRACE_LONG_1D(evt, d1)                  \
> -                   HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFF, (d1) >> 32)
> +                   HVMTRACE_2D(evt ## 64, (uint32_t)d1, (d1) >> 32)

Same for "d1" here.

Both of these are, btw., indications that you should have dropped Stefano's
R-b.

> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -30,7 +30,7 @@
>  
>  #define MSR_INTEL_CORE_THREAD_COUNT         0x00000035
>  #define  MSR_CTC_THREAD_MASK                0x0000ffff
> -#define  MSR_CTC_CORE_MASK                  0xffff0000
> +#define  MSR_CTC_CORE_MASK                  0xffff0000U
>  
>  #define MSR_SPEC_CTRL                       0x00000048
>  #define  SPEC_CTRL_IBRS                     (_AC(1, ULL) <<  0)
> @@ -168,7 +168,7 @@
>  #define MSR_UARCH_MISC_CTRL                 0x00001b01
>  #define  UARCH_CTRL_DOITM                   (_AC(1, ULL) <<  0)
>  
> -#define MSR_EFER                            0xc0000080 /* Extended Feature 
> Enable Register */
> +#define MSR_EFER                            _AC(0xc0000080, U)  /* Extended 
> Feature Enable Register */

I understand you use _AC() here because the constant is used in assembly
code. But I don't understand why you use it only here, and not throughout
at least the "modern" portion of the file. I wonder what other x86
maintainers think about this.

As a minor aspect, I also don't really see why you insert a 2nd blank
ahead of the comment.

>  #define  EFER_SCE                           (_AC(1, ULL) <<  0) /* SYSCALL 
> Enable */
>  #define  EFER_LME                           (_AC(1, ULL) <<  8) /* Long Mode 
> Enable */
>  #define  EFER_LMA                           (_AC(1, ULL) << 10) /* Long Mode 
> Active */
> @@ -181,35 +181,35 @@
>      (EFER_SCE | EFER_LME | EFER_LMA | EFER_NXE | EFER_SVME | EFER_FFXSE | \
>       EFER_AIBRSE)
>  
> -#define MSR_STAR                            0xc0000081 /* legacy mode 
> SYSCALL target */
> -#define MSR_LSTAR                           0xc0000082 /* long mode SYSCALL 
> target */
> -#define MSR_CSTAR                           0xc0000083 /* compat mode 
> SYSCALL target */
> -#define MSR_SYSCALL_MASK                    0xc0000084 /* EFLAGS mask for 
> syscall */
> -#define MSR_FS_BASE                         0xc0000100 /* 64bit FS base */
> -#define MSR_GS_BASE                         0xc0000101 /* 64bit GS base */
> -#define MSR_SHADOW_GS_BASE                  0xc0000102 /* SwapGS GS shadow */
> -#define MSR_TSC_AUX                         0xc0000103 /* Auxiliary TSC */
> +#define MSR_STAR                            0xc0000081U /* legacy mode 
> SYSCALL target */
> +#define MSR_LSTAR                           0xc0000082U /* long mode SYSCALL 
> target */
> +#define MSR_CSTAR                           0xc0000083U /* compat mode 
> SYSCALL target */
> +#define MSR_SYSCALL_MASK                    0xc0000084U /* EFLAGS mask for 
> syscall */
> +#define MSR_FS_BASE                         0xc0000100U /* 64bit FS base */
> +#define MSR_GS_BASE                         0xc0000101U /* 64bit GS base */
> +#define MSR_SHADOW_GS_BASE                  0xc0000102U /* SwapGS GS shadow 
> */
> +#define MSR_TSC_AUX                         0xc0000103U /* Auxiliary TSC */
>  
> -#define MSR_K8_SYSCFG                       0xc0010010
> +#define MSR_K8_SYSCFG                       0xc0010010U
>  #define  SYSCFG_MTRR_FIX_DRAM_EN            (_AC(1, ULL) << 18)
>  #define  SYSCFG_MTRR_FIX_DRAM_MOD_EN        (_AC(1, ULL) << 19)
>  #define  SYSCFG_MTRR_VAR_DRAM_EN            (_AC(1, ULL) << 20)
>  #define  SYSCFG_MTRR_TOM2_EN                (_AC(1, ULL) << 21)
>  #define  SYSCFG_TOM2_FORCE_WB               (_AC(1, ULL) << 22)
>  
> -#define MSR_K8_IORR_BASE0                   0xc0010016
> -#define MSR_K8_IORR_MASK0                   0xc0010017
> -#define MSR_K8_IORR_BASE1                   0xc0010018
> -#define MSR_K8_IORR_MASK1                   0xc0010019
> +#define MSR_K8_IORR_BASE0                   0xc0010016U
> +#define MSR_K8_IORR_MASK0                   0xc0010017U
> +#define MSR_K8_IORR_BASE1                   0xc0010018U
> +#define MSR_K8_IORR_MASK1                   0xc0010019U
>  
> -#define MSR_K8_TSEG_BASE                    0xc0010112 /* AMD doc: SMMAddr */
> -#define MSR_K8_TSEG_MASK                    0xc0010113 /* AMD doc: SMMMask */
> +#define MSR_K8_TSEG_BASE                    0xc0010112U /* AMD doc: SMMAddr 
> */
> +#define MSR_K8_TSEG_MASK                    0xc0010113U /* AMD doc: SMMMask 
> */
>  
> -#define MSR_K8_VM_CR                        0xc0010114
> +#define MSR_K8_VM_CR                        0xc0010114U
>  #define  VM_CR_INIT_REDIRECTION             (_AC(1, ULL) <<  1)
>  #define  VM_CR_SVM_DISABLE                  (_AC(1, ULL) <<  4)
>  
> -#define MSR_VIRT_SPEC_CTRL                  0xc001011f /* Layout matches 
> MSR_SPEC_CTRL */
> +#define MSR_VIRT_SPEC_CTRL                  0xc001011fU /* Layout matches 
> MSR_SPEC_CTRL */
>  
>  /*
>   * Legacy MSR constants in need of cleanup.  No new MSRs below this comment.

(As to above remark: This is the separator between "modern" [above]
and "historic" [below].)

Jan