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

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


  • To: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 6 Jul 2023 10:04:29 +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=hArFJNFoKK19jjNG/YMjS3I+u6H7IIRRIkcKQARosLg=; b=j3THx/cnnML85M3uLYGqBzcnMhi68L2MZvz9rnM3PPXOpbLxG3JSe1QwWk+NzT6soB6nUkO+zmlycGVOonsBRuTk2zdCsFudKgD+hu/+T0Qw6tigiOeY9MBb5nKN1kj2HSesx29qFZv6BFFxcZxBo40O9KShFOecNWF9JlDWaGJpdmPs9eQPh5Rv/t26uuV2nky2bsl+JuMt+XNbhsEeAsFmic2UAk5z5rHLLFTZiA+naVTvTcfEXTmV54+/T64LcuYXrzPdQkM64qjxwg8TsXttPb1abySIZfSTBTNIfsCNc6/uf+4IbA+xMnecRbpOLM9PxhkeZzwpDAQVMAL5Bg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JwkZpv0N2cq9dZYM2hqHQTNuPkXJsRJzAMYer0sz5A0BXJBBd855Dw9K48ZZOd45sJUNPA7pq1rYPNQ1F9clA0t6REKKWqIUZPd+uGGDL8SzdpbWxpkbt32OAS6l7ssYwR3YqZqdVfpesX+Eu39vaC7LfojdVgulKMY37UmIj84pVTD8vQfamQqzNdXlm+UibuzDheSvMag5i1BcaZDdjIy+OYoE2V+W+K1u/RPLsLFXTCepPrO30E/uLtEr1vV3U9v57RIkMXNxL1/qjJcx74q/q/L0/4Wbd7rcb9WKOvcLkYiONQkkVtCyO0pQ4QDa6t3OmpzfeXsDuOUlq1tRvA==
  • 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>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Xenia Ragiadakou <Xenia.Ragiadakou@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 06 Jul 2023 08:04:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.)

> --- 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



 


Rackspace

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