Received-SPF: pass (google.com: domain of linux-kernel+bounces-64614-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: "Li, Xin3" <xin3.li@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "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 v5 1/2] KVM: VMX: Cleanup VMX basic information defines
 and usages
Thread-Topic: [PATCH v5 1/2] KVM: VMX: Cleanup VMX basic information defines
 and usages
Thread-Index: AQHaWS3TKLWZzVt2oEy7w+XgclJ8GbEI6uCAgAAZ+hA=
Date: Wed, 14 Feb 2024 00:46:00 +0000
Message-ID: <SA1PR11MB67347AE5FD9A8710F3921D13A84E2@SA1PR11MB6734.namprd11.prod.outlook.com>
References: <20240206182032.1596-1-xin3.li@intel.com>
 <Zcvxf-fjYhsn_l1F@google.com>
In-Reply-To: <Zcvxf-fjYhsn_l1F@google.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?MhxVGrhcu3QyFA0B+v2g2QTByvvbTrDudFyAQM0i1roW18ZkUzbhJjVIWg1p?=
 =?us-ascii?Q?D8p515Rl+3UOkBFetGE2ivX75IBxtce+H4wAlXCBD0Bwy2rVY2SPrKmi2gu2?=
 =?us-ascii?Q?LSqoMluZH5wDZnozxb9nESuOxeGTZFOytkNa0+N81kufb62mwKdt6IKsSLKu?=
 =?us-ascii?Q?eoFt6zSjy5FZcA1xcjwn0nZlvT/ii3M6hZ/17BWlHDDT9f7V0+GCFTasg0VF?=
 =?us-ascii?Q?UM+DQFCkAAe7MlIWhwdkCWo7YF5laW1SKQ51rso4KDUwjlwGE7k5zbkFoQW6?=
 =?us-ascii?Q?LCZNiZnYWuxPoLfJ5BUjC92gOu5s4nbo5/+PVTt1pTlcTbkO+Opy1b+Z/ulc?=
 =?us-ascii?Q?rKNmFE+/pbjDRMvNx8MDPGzZIIE8xlV/alg6XyXOIWGwdiBfti+S6kLB2JeJ?=
 =?us-ascii?Q?Yc9BdPwfWlpVllpgwSjcPWekDY0ExNYvWcncUPLI1BZhicSHv4D5RgeKnmEE?=
 =?us-ascii?Q?3N//60YVOJgFbaIt03+sXwItDlcihQkiOWZq1QE43VytfvBkK7qR3Y04R3yO?=
 =?us-ascii?Q?8sEH4HFbDeZKQfF8Gk8x1fk/z50eKu6bgMOc4GMk8N3Wm/4YS65FUP+Oe0Y9?=
 =?us-ascii?Q?GHJFNkLfOZJ+zfxM5jQkhJUt0LXSk4BKBVLynXtfQ3mdbx62cd9ueoUm/t1z?=
 =?us-ascii?Q?Gi0eSGzKfYJdVMHQd+l5U03YoQOzWLzNnHZYQaN7nAuZICGYJcIdq6EhDGPS?=
 =?us-ascii?Q?TpiKr64gpGxKHADK4iE1aGJAJki/mAPhv8cmVTRgZDG6LSouv9Dg8pfhJ5PH?=
 =?us-ascii?Q?c5k/GnkCeHxdKTpqainm/GRvgj2hTHVH1pOvNGeMKRlWBRwrjngZUBIMGQTS?=
 =?us-ascii?Q?VgR3JUK20bPkkKSKXVBZshXYWmK7g1yShNegsgfnof09gu80bVcuTNP26NNL?=
 =?us-ascii?Q?/pzj7WeY+Dy0p3FeOieg9/G/ezBBZgOwIXbkJPCTd5qwxW5MsWdd64YVg4h6?=
 =?us-ascii?Q?8PJgioOfni1HfdEIvXzUC3CdF8eF9IIdO/J/N7o76v+GtqqNMelvL01c8p1s?=
 =?us-ascii?Q?VigLY+bLP6v0qmB98c57232Smwl2BaJsATVIZnbTk5ZWzeaIbbfpwerTGZZw?=
 =?us-ascii?Q?rjpczL4LgTtu+maDrOZZrsOsCVLzARFTQKiZHAPY5aeYoVnP8DzlRwQrg7sN?=
 =?us-ascii?Q?6OHtV3d+Hw/WSuZDumKxhvt2067uib/+iqgWWxg/z2DpmzB0+Nv+TrIb9Diy?=
 =?us-ascii?Q?MaEYc9TXVIZODcEvpDdlDmGuSmVyIxvFKu06IoQKVyf/In/OgbeIQv9SsFp9?=
 =?us-ascii?Q?AeRoFDxT6QUwQrYnjxGaFFB7rP/AgBKlmh+2+pN9KAOU3i6DAAf6rjlRuY+d?=
 =?us-ascii?Q?UKr4GZzF5/1B6qXp0fqNjc709jf+DEBVrk+iEWH4Uls+IpvzczLuoW9odzzH?=
 =?us-ascii?Q?YKRMk39t1i55hiVwuYN97AjI8dRCZO5/HMQ8mDzDc89p17aFbLOz5zvIWJ1J?=
 =?us-ascii?Q?7zLRtdXPDkJdhZ94CwcKLiLipdzzTf4j5ypx/AuQ49QIqcUHFI29oOw7YYzo?=
 =?us-ascii?Q?3OVwXml/5aiSUJb3qTLQJDaYEqbxR3a06E/AWPuPyzu2HTrQAAwDttfVcC0p?=
 =?us-ascii?Q?wr+mr3okVV0hoz/Kx30=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: 0229a6e8-0d23-40c9-f1a5-08dc2cf65051
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Feb 2024 00:46:00.0608
 (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: ciWQOaK56i9aP+8IVBMLTN/Zty7pokQ6bMnyZrYbG5U1v2huDkD3fn9/G5ywJwVHJVmDHInaaJNoALtsBSBeNg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYXPR11MB8689
X-OriginatorOrg: intel.com

> Please send cover letters for series with more than one patch, even if th=
ere are
> only two patches.  At the very least, cover letters are a convenient loca=
tion to
> provide feedback/communication for the series as a whole.

Kai also said so...  I'll take it as a standard practice.

>  Instead, I need to put it here:
>=20
> I'll send a v6 with all of my suggestions incorporated.

Perfect!

>  I like the cleanups, but
> there are too many process issues to fixup when applying, a few things th=
at I
> straight up disagree with, and more aggressive memtype related changes th=
at can
> be done in the context of this series.
>=20
> > @@ -505,8 +521,6 @@ enum vmcs_field {
> >  #define VMX_EPTP_PWL_5				0x20ull
> >  #define VMX_EPTP_AD_ENABLE_BIT			(1ull << 6)
> >  #define VMX_EPTP_MT_MASK			0x7ull
> > -#define VMX_EPTP_MT_WB				0x6ull
> > -#define VMX_EPTP_MT_UC				0x0ull
>=20
> I would strongly prefer to keep the VMX_EPTP_MT_WB and VMX_EPTP_MT_UC
> defines,
> at least so long as KVM is open coding reads and writes to the EPTP.  E.g=
 if
> someone wants to do a follow-up series that adds wrappers to decode/encod=
e
> the
> memtype (and other fiels) from/to EPTP values, then I'd be fine dropping =
these.
>=20
> But this:
>=20
>=20
> 	/* Check for memory type validity */
> 	switch (new_eptp & VMX_EPTP_MT_MASK) {
> 	case MEM_TYPE_UC:
> 		if (CC(!(vmx->nested.msrs.ept_caps & VMX_EPTP_UC_BIT)))
> 			return false;
> 		break;
> 	case MEM_TYPE_WB:
> 		if (CC(!(vmx->nested.msrs.ept_caps & VMX_EPTP_WB_BIT)))
> 			return false;
> 		break;
> 	default:
> 		return false;
> 	}
>=20
> looks wrong and is actively confusing, especially when the code below it =
does:
>=20
> 	/* Page-walk levels validity. */
> 	switch (new_eptp & VMX_EPTP_PWL_MASK) {
> 	case VMX_EPTP_PWL_5:
> 		if (CC(!(vmx->nested.msrs.ept_caps &
> VMX_EPT_PAGE_WALK_5_BIT)))
> 			return false;
> 		break;
> 	case VMX_EPTP_PWL_4:
> 		if (CC(!(vmx->nested.msrs.ept_caps &
> VMX_EPT_PAGE_WALK_4_BIT)))
> 			return false;
> 		break;
> 	default:
> 		return false;
> 	}
>

I see your point here.  But "#define VMX_EPTP_MT_WB	0x6ull" seems to define
its own memory type 0x6.  I think what we want is:

/* in a pat/mtrr header */
#define MEM_TYPE_WB 0x6

/* vmx.h */
#define VMX_EPTP_MT_WB	MEM_TYPE_WB

if it's not regarded as another layer of indirect.

> >  static inline bool cpu_has_virtual_nmis(void)
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 994e014f8a50..80fea1875948 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -1226,23 +1226,29 @@ static bool is_bitwise_subset(u64 superset, u64
> subset, u64 mask)
> >  	return (superset | subset) =3D=3D superset;
> >  }
> >
> > +#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
> Looking at this with fresh eyes, I think #defines are overkill.  There is=
 zero
> chance anything other than vmx_restore_vmx_basic() will use these, and th=
e
> feature
> bits mask is rather weird.  It's not a mask of features that KVM supports=
, it's
> a mask of feature *bits* that KVM knows about.
>=20
> So rather than add #defines, I think we can keep "const u64" variables, b=
ut split
> into feature_bits and reserved_bits (the latter will have open coded
> GENMASK_ULL()
> usage, whereas the former will not).
>=20
> BUILD_BUG_ON() is fancy enough that it can detect overlap.

Sounds reasonable to me.

>=20
> > +#define VMX_BSAIC_VMCS12_SIZE	((u64)VMCS12_SIZE << 32)
>=20
> Typo.

Sigh!

>=20
> > +#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50)
>=20
> I don't see any value in either of these.  In fact, I find them both to b=
e far
> more confusing, and much more likely to be incorrectly used.
>=20
> Back in v1, when I said "don't bother with shift #defines", I was very sp=
ecifically
> talking about feature bits where defining the bit shift is an extra, poin=
tless
> layer.  I even (tried) to clarify that.

Another review comment got me lost here:
https://lore.kernel.org/kvm/2158ef3c5ce2de96c970b49802b7e1dba8b704d6.camel@=
intel.com/