[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH v4 1/4] x86/vmx: address violations of MISRA C:2012 Rule 7.2
- To: xen-devel@xxxxxxxxxxxxxxxxxxxx
- From: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
- Date: Mon, 28 Aug 2023 12:11:28 +0200
- Cc: consulting@xxxxxxxxxxx, Gianluca Luparini <gianluca.luparini@xxxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
- Delivery-date: Mon, 28 Aug 2023 10:12:00 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 27 Jul 2023, Jan Beulich wrote: 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.
Ok. Sorry for that.
> --- 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.
Ok.
> --- 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.
I've used _AC only when strictly required to avoid errors.
In v5 I will use _AC on all constants in the "modern" part of the file.
As a minor aspect, I also don't really see why you insert a 2nd blank
ahead of the comment.
To align the comment with the others below. I see that comment aligning is not done in this file, so I will drop the change in v5.
> #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
|
