From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CF64AC001B0 for ; Fri, 7 Jul 2023 08:19:34 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.560274.876054 (Exim 4.92) (envelope-from ) id 1qHggI-0004BO-Ng; Fri, 07 Jul 2023 08:19:10 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 560274.876054; Fri, 07 Jul 2023 08:19:10 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qHggI-0004BH-L0; Fri, 07 Jul 2023 08:19:10 +0000 Received: by outflank-mailman (input) for mailman id 560274; Fri, 07 Jul 2023 08:19:09 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qHggH-0004BB-GH for xen-devel@lists.xenproject.org; Fri, 07 Jul 2023 08:19:09 +0000 Received: from support.bugseng.com (mail.bugseng.com [162.55.131.47]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id f1a8289b-1c9e-11ee-b237-6b7b168915f2; Fri, 07 Jul 2023 10:19:08 +0200 (CEST) Received: from mail-vk1-f174.google.com (mail-vk1-f174.google.com [209.85.221.174]) by support.bugseng.com (Postfix) with ESMTPSA id B90674EE0C87 for ; Fri, 7 Jul 2023 10:19:07 +0200 (CEST) Received: by mail-vk1-f174.google.com with SMTP id 71dfb90a1353d-47e25709402so638590e0c.0 for ; Fri, 07 Jul 2023 01:19:07 -0700 (PDT) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: f1a8289b-1c9e-11ee-b237-6b7b168915f2 X-Gm-Message-State: ABy/qLYMrwMRUtC4IDYyQ9PHbXPhK2P5CIM4cXhXkiE+t/KiXlBm/iUn Q/B3kpGMPFZ1VKyJ6+gV2fRg4aBt3ZEW31frPhY= X-Google-Smtp-Source: APBJJlERXkLn4rnLZROi+wS0Kk5grHYNuCk9w2E3B1GGUIiH8XHCixQHB7c4QSlTaKg09pTO/orxpb3caKrhoNoCsks= X-Received: by 2002:a05:6102:3a72:b0:443:695d:b91a with SMTP id bf18-20020a0561023a7200b00443695db91amr2889414vsb.28.1688717946615; Fri, 07 Jul 2023 01:19:06 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Simone Ballarin Date: Fri, 7 Jul 2023 10:18:55 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [XEN PATCH v2 07/13] x86/vmx: fix violations of MISRA C:2012 Rule 7.2 To: Jan Beulich Cc: consulting@bugseng.com, Gianluca Luparini , Jun Nakajima , Kevin Tian , Andrew Cooper , =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= , Wei Liu , Stefano Stabellini , Michal Orzel , Xenia Ragiadakou , Ayan Kumar Halder , xen-devel@lists.xenproject.org Content-Type: multipart/alternative; boundary="00000000000055832605ffe14911" --00000000000055832605ffe14911 Content-Type: text/plain; charset="UTF-8" Il giorno gio 6 lug 2023 alle ore 10:04 Jan Beulich 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 ) --00000000000055832605ffe14911 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
Il giorno gio 6 lug 2023 alle ore 10:= 04 Jan Beulich <j= beulich@suse.com> 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)
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 switch ( enc.width ) {
>=C2=A0 =C2=A0 =C2=A0 case VVMCS_WIDTH_16:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 res &=3D 0xffff;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 res &=3D 0xffffU;

I don't think the suffix is needed in cases like this one, and ...
<= /blockquote>

>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0case VVMCS_WIDTH_64:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( enc.access_type )
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 res >>=3D 32; >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case VVMCS_WIDTH_32:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 res &=3D 0xffffffff;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 res &=3D 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);
>=C2=A0 #define CPU_BASED_ACTIVATE_MSR_BITMAP=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A00x10000000
>=C2=A0 #define CPU_BASED_MONITOR_EXITING=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A00x20000000
>=C2=A0 #define CPU_BASED_PAUSE_EXITING=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A00x40000000
> -#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;
>=C2=A0 #define SECONDARY_EXEC_XSAVES=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x00100000
>=C2=A0 #define SECONDARY_EXEC_TSC_SCALING=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 0x02000000
>=C2=A0 #define SECONDARY_EXEC_BUS_LOCK_DETECTION=C2=A0 =C2=A0 =C2=A0 = =C2=A00x40000000
> -#define SECONDARY_EXEC_NOTIFY_VM_EXITING=C2=A0 =C2=A0 =C2=A0 =C2=A0 0= x80000000
> +#define SECONDARY_EXEC_NOTIFY_VM_EXITING=C2=A0 =C2=A0 =C2=A0 =C2=A0 0= x80000000U

... 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)
>=C2=A0 /*
>=C2=A0 =C2=A0* Exit Reasons
>=C2=A0 =C2=A0*/
> -#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000
> +#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000U
>=C2=A0 #define VMX_EXIT_REASONS_BUS_LOCK=C2=A0 =C2=A0 =C2=A0 =C2=A0(1u = << 26)

Along the lines of the latter, perhaps switch to 1u << 31?

> @@ -246,15 +246,15 @@ typedef union cr_access_qual {
>=C2=A0 /*
>=C2=A0 =C2=A0* Access Rights
>=C2=A0 =C2=A0*/
> -#define X86_SEG_AR_SEG_TYPE=C2=A0 =C2=A0 =C2=A00xf=C2=A0 =C2=A0 =C2= =A0 =C2=A0 /* 3:0, segment type */
> -#define X86_SEG_AR_DESC_TYPE=C2=A0 =C2=A0 (1u << 4)=C2=A0 /* 4,= descriptor type */
> -#define X86_SEG_AR_DPL=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 0x60=C2=A0 = =C2=A0 =C2=A0 =C2=A0/* 6:5, descriptor privilege level */
> -#define X86_SEG_AR_SEG_PRESENT=C2=A0 (1u << 7)=C2=A0 /* 7, segm= ent present */
> -#define X86_SEG_AR_AVL=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (1u <<= 12) /* 12, available for system software */
> -#define X86_SEG_AR_CS_LM_ACTIVE (1u << 13) /* 13, long mode act= ive (CS only) */
> -#define X86_SEG_AR_DEF_OP_SIZE=C2=A0 (1u << 14) /* 14, default = operation size */
> -#define X86_SEG_AR_GRANULARITY=C2=A0 (1u << 15) /* 15, granular= ity */
> -#define X86_SEG_AR_SEG_UNUSABLE (1u << 16) /* 16, segment unusa= ble */
> +#define X86_SEG_AR_SEG_TYPE=C2=A0 =C2=A0 =C2=A00xfU=C2=A0 =C2=A0 =C2= =A0 =C2=A0/* 3:0, segment type */
> +#define X86_SEG_AR_DESC_TYPE=C2=A0 =C2=A0 (1U << 4)=C2=A0 /* 4,= descriptor type */
> +#define X86_SEG_AR_DPL=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 0x60U=C2=A0 = =C2=A0 =C2=A0 /* 6:5, descriptor privilege level */
> +#define X86_SEG_AR_SEG_PRESENT=C2=A0 (1U << 7)=C2=A0 /* 7, segm= ent present */
> +#define X86_SEG_AR_AVL=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (1U <<= 12) /* 12, available for system software */
> +#define X86_SEG_AR_CS_LM_ACTIVE (1U << 13) /* 13, long mode act= ive (CS only) */
> +#define X86_SEG_AR_DEF_OP_SIZE=C2=A0 (1U << 14) /* 14, default = operation size */
> +#define X86_SEG_AR_GRANULARITY=C2=A0 (1U << 15) /* 15, granular= ity */
> +#define X86_SEG_AR_SEG_UNUSABLE (1U << 16) /* 16, segment unusa= ble */

How is this change related to rule 7.2? There are u suffixes already where<= br> needed (and 0xf and 0x60 don't strictly need one), so there's no vi= olation
here afaict. A mere style change to switch from u to U imo doesn't belo= ng
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 amen= d the commit messages in these cases.

--
Simone Ballarin, M.Sc.

Field Application Engineer, BU= GSENG (https://bugseng.com= )
--00000000000055832605ffe14911--