Received-SPF: pass (google.com: domain of linux-kernel+bounces-29219-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: "Li, Xin3" <xin3.li@intel.com>
To: "Gao, Chao" <chao.gao@intel.com>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "seanjc@google.com"
	<seanjc@google.com>, "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>, "mingo@redhat.com"
	<mingo@redhat.com>, "bp@alien8.de" <bp@alien8.de>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>, "x86@kernel.org"
	<x86@kernel.org>, "hpa@zytor.com" <hpa@zytor.com>, "Yang, Weijiang"
	<weijiang.yang@intel.com>, "Huang, Kai" <kai.huang@intel.com>
Subject: RE: [PATCH v4 1/2] KVM: VMX: Cleanup VMX basic information defines
 and usages
Thread-Topic: [PATCH v4 1/2] KVM: VMX: Cleanup VMX basic information defines
 and usages
Thread-Index: AQHaRT8NfyvFkqdAjk6/fSX6qVKXsrDcFbiAgAIkzVA=
Date: Wed, 17 Jan 2024 16:34:49 +0000
Message-ID: <SA1PR11MB67340002B67910588EE335B1A8722@SA1PR11MB6734.namprd11.prod.outlook.com>
References: <20240112093449.88583-1-xin3.li@intel.com>
 <ZaY0UbFjwCYh4u/r@chao-email>
In-Reply-To: <ZaY0UbFjwCYh4u/r@chao-email>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?1HYcYvwZm5bLQcnCZvIc47JNlPwhf+eC1ynKkJebmu3dtYtkoxcSQlJKoZdv?=
 =?us-ascii?Q?PApat4kM6pXlYcx4b6c6QErw/oLuPAo6GqLgYHgdERZiGbOgPwLqOlv8bYGQ?=
 =?us-ascii?Q?+K6xh29Df0AKpkntAAu4IkxKC15XXibKSKWJ8KOZmDoIeD756uODn0Vjo7tF?=
 =?us-ascii?Q?NNoJZpQGbnxoOB36zWfkx2sQQ2/kFi2Ji2C2+Fky8VYYGo0qIxzxGg2qLaJJ?=
 =?us-ascii?Q?JIqV+/95i85CiSewlgdAlFfHP98Faol2+27uE066EmjiBasxsjc6bIDh51jQ?=
 =?us-ascii?Q?TbmbNiq7WknoX8JRMjLFRruiKpUU9oFWCFevdvEvo2fZ4ch65ottf8zv78eK?=
 =?us-ascii?Q?a5ilQNVhLBf3TR+MKTh1fxUw7YwCNx6mJ2BSpd5rAz4qh9O1Ayv+ZKkna+RF?=
 =?us-ascii?Q?2jRHkT4g0Ucb84sDCaSs1hobAF7nrfl3zzdL0rvPZWk9THULz9hDkSiA0MLT?=
 =?us-ascii?Q?bti1Gy2RGjaQ92i7fuyBcO0z4GeTdEHedbOI8OEfPQpEfEROUH905fNg8eOz?=
 =?us-ascii?Q?k0lL9HYwsfIbnm8x4fKvdr0uvybE7sfTHCiwWJfH1cHYZxPQE56H7v7DleH1?=
 =?us-ascii?Q?VxHmxtD2gBIB3cnItQc3nyRswcEHUDwPSlRdsrBSCKe/hFpFM64YQhi0s8Jy?=
 =?us-ascii?Q?ri0jp79JVrBEE3u75hmjbPHGO4hUNsOYWFKauOAmtb+mIOjpK84RJcy/Oina?=
 =?us-ascii?Q?8yehUObLEfqf6WIaE896it8YKIjfAXhJzQUGgI1ZMXijK8+roiVb+5nCv4FL?=
 =?us-ascii?Q?8X3+uhq7yf4jid8GchCvVCUoLMWyVInGezD28+KI+XkLBxZoknjxs6p7mu9F?=
 =?us-ascii?Q?eXdbPf3hX8vcxNsa10O7yu/t4nst05MRoItD88Q27PDmE4bLuGuLEnqo9/Hz?=
 =?us-ascii?Q?CVLIZOjFyAGzilcxtpmhXY0ArVDTBFbIiEfAFH8aVMG6e2j7rRrwLI9DGjEU?=
 =?us-ascii?Q?K/3mqXWxRF11KEbtqD/C/ZnUl6NavpgjqMihog+syUKKJfzIr2hRyisMtXDm?=
 =?us-ascii?Q?5XEoI9V6TNccx9JeeJ9deUw23RvKv5uqdvBbCLVsFq7LiOrDwJrVVdZjkS2B?=
 =?us-ascii?Q?TD0EvU6Uj/gOKytS0AxNXUpp+ZXPfHOG5+ph5Rf6+dDW7pFDo1QbHR0XY/c/?=
 =?us-ascii?Q?Obmhka7Hen3iHOLDEiOz6rKgxutpPw0bI3cwbLwLu2CB4NkBKGR6mjKvJeem?=
 =?us-ascii?Q?NSeDNo8BCAjEdokcFaPqr4LP7Yl5Wfguz5cdUAuGLOQZx59agYjUuwki6LC9?=
 =?us-ascii?Q?+nT7PXtAcYWFRlLrw0tT4eOyrPuKpQdMmeMG9LCos5+hkV95aty0hEFHZFLA?=
 =?us-ascii?Q?AKl4+mVYS6erOEykKBSIFQNmX0XVyVwmnPX6KiafJ5akUCKpOpdL7ZGuxWYs?=
 =?us-ascii?Q?1Ip/eTcTzOEfaMoH/CKw2gLhyHEWr5qtsbyQiDiPjQKfXAWGrUG1cO+xkZdr?=
 =?us-ascii?Q?UtqFs3zIIfBTkQtpxHkNEcy6I6299U8YlMN8ninGsTIA2fklJnw/+l/ReRVY?=
 =?us-ascii?Q?jACI6p5XKm9mp89AXQNOfO4NKKAya4uQy5vuuz3VQbZQozOj6LPtBS3FCNxp?=
 =?us-ascii?Q?00dcbLXNwDQJvDGluzU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6734.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ead5bef0-c596-4d75-a76d-08dc177a393c
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2024 16:34:49.3322
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: zyYxAHmNVUK7fTidJNyiWCXXz2Zh76x1ESDBbQetAv3vLJy0aI1w2ihJNGAftKMogfgpbMGAiyW/7qKl/na1yw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4571
X-OriginatorOrg: intel.com

> >+#define VMX_BASIC_FEATURES_MASK			\
> >+	(VMX_BASIC_DUAL_MONITOR_TREATMENT |	\
> >+	 VMX_BASIC_INOUT |			\
> >+	 VMX_BASIC_TRUE_CTLS)
> >+
> >+#define VMX_BASIC_RESERVED_BITS			\
> >+	(GENMASK_ULL(63, 56) | GENMASK_ULL(47, 45) | BIT_ULL(31))
>=20
> When we add a new feature (e.g., in CET series, bit 56 is added), the abo=
ve
> two macros need to be modified.
>=20
> Would it be better to use a macro for bits exempt from the bitwise check =
below
> e.g.,
>=20
> #define VMX_BASIC_MULTI_BITS_FEATURES_MASK
>=20
> 	(GENMASK_ULL(53, 50) | GENMASK_ULL(44, 32) | GENMASK_ULL(30, 0))
>=20
> and do
> 	if (!is_bitwise_subset(vmx_basic, data,
> 			       ~VMX_BASIC_MULTI_BITS_FEATURES_MASK)
>=20
> then we don't need to change the macro when adding new features.

Sounds a good idea to me, and just need to add comments about why.

>=20
> >+
> > static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
> > {
> >-	const u64 feature_and_reserved =3D
> >-		/* feature (except bit 48; see below) */
> >-		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
> >-		/* reserved */
> >-		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
> > 	u64 vmx_basic =3D vmcs_config.nested.basic;
> >
> >-	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
> >+	static_assert(!(VMX_BASIC_FEATURES_MASK &
> VMX_BASIC_RESERVED_BITS));
> >+
> >+	if (!is_bitwise_subset(vmx_basic, data,
> >+			       VMX_BASIC_FEATURES_MASK |
> VMX_BASIC_RESERVED_BITS))
> > 		return -EINVAL;
> >
> > 	/*
> > 	 * KVM does not emulate a version of VMX that constrains physical
> > 	 * addresses of VMX structures (e.g. VMCS) to 32-bits.
> > 	 */
> >-	if (data & BIT_ULL(48))
> >+	if (data & VMX_BASIC_32BIT_PHYS_ADDR_ONLY)
> > 		return -EINVAL;
>=20
> Side topic:
>=20
> Actually, there is no need to handle bit 48 as a special case. If we add =
bit 48
> to VMX_BASIC_FEATURES_MASK, the bitwise check will fail if bit 48 of @dat=
a is 1.

Good point!  This is also what you suggested above.