[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 27 Jul 2023 14:15:13 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=7rFGLkvY8Qw30FFpaVhWJtSW9PoM0yeIUsss/wKQnpQ=; b=RAgZ0HymH9ZG7upDb0KrEMz2qHOlhKQprrbGPzD7GnsX/r6g1pjaY6XoVdwA9X1hJig1vVnzEh+1Cav5Y9uU7hyXRJYMHimaHtFq0+bYKE/81JbYjyPXuViK0J1HOgeowglFS28abB3jbmNJVc1A+y7RM+eK7SBfToHzcoJE12HTRnN+OHqZRKdcnTH1CF6aDY8e+fyHNyGedvvYHmxepC+MnYhkuOD2z4/b+BrsC8k+zjzs9YhpIOZThObEnGjdw9i5h74OIilTPaVyzzjku85UN2aY4hgvvpd6e84A5OwWUvI5tuJfCrz6Bqz1hbYXW5ldQoTbL+2Zl4l173jtGw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PR5t8iZQ9AshKDtFG0b7NPqE170QRgpI9UQ8pz/A1+Y5Cbi9n65h/uzW3Oz/eGBP9R6nL2eaQpNfNL95EIZ4rZunR/0vVAs/TMY6GeS9voUQPaYiwuisgdd/OXLX2xg72+wCTSVl+WUcX6Z+IhcZEF4HWHr9Ql412VtY7zT1YzobpHMEdaA/sGwT9lZp9nnNZvSGS3/c0J4APaC3o1saA9/0kUbTH0YpXL4mY/Of7mk8t5h7n2HUpJxw6VtFk4W2KPaEaHWGNdj3b/+AeNmF5v9hoMWPGL3l4BVTvG4ETUyKZDWTk16BU9EJ3imfJkdQRtJOuAg2PU4cKBqQfE6/wQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: consulting@xxxxxxxxxxx, Gianluca Luparini <gianluca.luparini@xxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 27 Jul 2023 12:15:25 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.