From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "Christopherson,, Sean" <seanjc@google.com>,
        "Liu, Jing2" <jing2.liu@intel.com>
CC:     "x86@kernel.org" <x86@kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "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>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "shuah@kernel.org" <shuah@kernel.org>,
        "Nakajima, Jun" <jun.nakajima@intel.com>,
        "jing2.liu@linux.intel.com" <jing2.liu@linux.intel.com>,
        "Zeng, Guang" <guang.zeng@intel.com>,
        "Wang, Wei W" <wei.w.wang@intel.com>,
        "Zhong, Yang" <yang.zhong@intel.com>
Subject: RE: [PATCH v3 16/22] kvm: x86: Add XCR0 support for Intel AMX
Thread-Topic: [PATCH v3 16/22] kvm: x86: Add XCR0 support for Intel AMX
Thread-Index: AQHX9zE1CDJu9TXQlki+g1y/a9bZPqxIpY4AgAAqVMA=
Date:   Wed, 29 Dec 2021 03:01:18 +0000
Message-ID: <BN9PR11MB52767A662EE36412B166244B8C449@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20211222124052.644626-1-jing2.liu@intel.com>
 <20211222124052.644626-17-jing2.liu@intel.com> <Ycup8opdHrjCIy1V@google.com>
In-Reply-To: <Ycup8opdHrjCIy1V@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?/ga222Meoz7bPpRQ6XFfGW30+6tqXkzR+2b9TXsJzf0ogmz+RaI0t/NujDyS?=
 =?us-ascii?Q?t93JOXSdh+aN00/DWIiEUZ6tWxCBtJfGi58u1tLewAVPzIAHyGprrcaNbLMn?=
 =?us-ascii?Q?WuITKxfXh0vsR4STRYlptZI5QENaWGYlsAtBviZi52MAWBb9eHqiFMf6Q5ay?=
 =?us-ascii?Q?U0rIo1h9n7EWg1CGMR6YPq0gpzzUsgIa8oVnPXjO1zsZBYXI0n+8tl3FEbkm?=
 =?us-ascii?Q?7KsJBN+D1HNPu/dHy/o7ygfleTKD+dELxZdpokXBFHvcWJfGYgm4HP9nS11K?=
 =?us-ascii?Q?7FYUhZ8Dh5yBDHgKiwqnh809one/TikPSVCYsQ3QK1/glpqbXWzNJGpbNGFt?=
 =?us-ascii?Q?drmcVypi/ll8R6J1yDi+OhtB9q7Vd94LDd2zzVUos7RVFPdcmaGeHQ9pe1Ml?=
 =?us-ascii?Q?DRSNdoUz2kBM48hSDiUM79dJVE07atbtHdK6I/ONpFitmoRuIL44AJU8lsIC?=
 =?us-ascii?Q?ZcDRAKNna52J4pmPTAj3/ywbV1vf+K8mww6xaAylWENKVXb293DemzX50Cpc?=
 =?us-ascii?Q?aB4iIm2IUALQUfYV5v1VCujFgL7Pb1Cc/rAPacv//TMB67VvQ+cZ1OBSZqqp?=
 =?us-ascii?Q?hSFDds3Ov7EUM79K5Ihm32rwEsPicPFL8Eb8wz/D2GAwXpoUpb0XCDXT5OX+?=
 =?us-ascii?Q?0hGSZWczxrvztYQZjm/xiNZyZ0zInn+g/K7vtWELEfCpHlsyx5z4QabRSPAR?=
 =?us-ascii?Q?I8CZHa78ekGpJ/aK45hoWDZ8Inx8d6gcWpDNYBJgmsWiB1+0GQDIm9QP4kXT?=
 =?us-ascii?Q?zPwXE5cPrP4aFgqoExRLn2E5mmPDE8x4+Z4fkG4Z2DtAQ6sGN64DbMXD98S9?=
 =?us-ascii?Q?HIiTaCoaI0XxxBYHqZHOXHjLYR4H63v6DR0AO97V/Pz8530Pkm5cUFqLxR6c?=
 =?us-ascii?Q?/jgd09dqiSHFxSOMzLZAUrF289ZNs9Z3K6UpNbuRY/hL19TYuRHaLb1+L0KT?=
 =?us-ascii?Q?ZnzhwZ9grlV5vQ+ze6I8Zqr06d3iTomSp0P8C44WGCMMQgnf4tGeJFdrs1um?=
 =?us-ascii?Q?CcLyeD1DWXu3JFOWvEXo81jRZfrLVQe7z3PNu9yJ7aOH5iNsWBJbSG6S0YI2?=
 =?us-ascii?Q?rAoeWOt6VrYjP+zfDMunCZr7ZtDQpjflh8G8zryoNkgxQP7FwlrcH0Ywwn9T?=
 =?us-ascii?Q?0mRTZ6XVEWvpg36Jmtfr3pCRtEMGiuA/PieJ+Kx0QnjKc9DXGwbc8XH0ogZl?=
 =?us-ascii?Q?41nichJ7t4MtEyVOAWDkH62Ml1Xf0+U4hqNtA/5IxVQxEihDofsfvx9mNvQi?=
 =?us-ascii?Q?WpfDYDX0yUv9eSyPaJMx+wgxeVhZ4XJTuanmT2ztQ7jA/e6zlQfNJMSZBShS?=
 =?us-ascii?Q?DX6IALhr4gyjnvkdzOcgb4kxi2LO/GH2UFBtIMjezuANh+oP76Rd+qrvMnGJ?=
 =?us-ascii?Q?YI5+LeKdVbnee7dP8OAy0K4bpTVrjb64qjhQDdm9uGqh1XzwF9uMLv2GbPJV?=
 =?us-ascii?Q?dBGecyUn8qBNA9nwFhzHeN107dHI5gXcLcoDwkXMgL1CBIAvf8j++XklmeaN?=
 =?us-ascii?Q?vGX9HDlsaB79bvV/APz2ZYekKVkNWwifcPnGDeP0IXlXKIrY9oLqb9KZiY6Z?=
 =?us-ascii?Q?iHRd3S2au8qVqgVUxZLYYquTl//PXe2KaIKHk3u6hXLAXl4HA00Te54amyvf?=
 =?us-ascii?Q?51v+dfAEv5ADVdbN550ZHuM=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7175855a-0afb-4e52-3cd4-08d9ca777cab
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Dec 2021 03:01:18.9399
 (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: PNa3wkvcgBcUNbJCj8MooQbKlM4MK9QMWTjGK9bKKsd2PCLX9ZR5gqLFf51XcfI5PGx9JNyOD2J8ORFR5JAfbg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1857
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Sean Christopherson <seanjc@google.com>
> Sent: Wednesday, December 29, 2021 8:21 AM
>=20
> On Wed, Dec 22, 2021, Jing Liu wrote:
> > Two XCR0 bits are defined for AMX to support XSAVE mechanism. Bit 17
> > is for tilecfg and bit 18 is for tiledata.
> >
> > The value of XCR0[17:18] is always either 00b or 11b.
>=20
> Is that an SDM requirement, or an arbitrary Linux/KVM requirement?

SDM requirement

>=20
> > Also, SDM
> > recommends that only 64-bit operating systems enable Intel AMX by
> > setting XCR0[18:17]. If a 32-bit guest tries to set dynamic bits, it
>=20
> This is wrong.  It has nothing to do with 32-bit guests.  The restriction=
 is on
> 32-bit _host kernels_, which I'm guessing never set the tile bits in _hos=
t_
> XCR0.

make sense.

>=20
> > fails to pass vcpu->arch.guest_supported_xcr0 check and gets a #GP.
> >
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > Signed-off-by: Jing Liu <jing2.liu@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index a48a89f73027..c558c098979a 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -210,7 +210,7 @@ static struct kvm_user_return_msrs __percpu
> *user_return_msrs;
> >  #define KVM_SUPPORTED_XCR0     (XFEATURE_MASK_FP |
> XFEATURE_MASK_SSE \
> >  				| XFEATURE_MASK_YMM |
> XFEATURE_MASK_BNDREGS \
> >  				| XFEATURE_MASK_BNDCSR |
> XFEATURE_MASK_AVX512 \
> > -				| XFEATURE_MASK_PKRU)
> > +				| XFEATURE_MASK_PKRU |
> XFEATURE_MASK_XTILE)
> >
> >  u64 __read_mostly host_efer;
> >  EXPORT_SYMBOL_GPL(host_efer);
> > @@ -990,6 +990,12 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu,
> u32 index, u64 xcr)
> >  		if ((xcr0 & XFEATURE_MASK_AVX512) !=3D
> XFEATURE_MASK_AVX512)
> >  			return 1;
> >  	}
> > +
> > +#ifdef CONFIG_X86_64
>=20
> Drop the #ifdef, it adds no meaningful value and requires the reader to t=
hink
> far harder than they should have.  Yes, it's technically dead code for 32=
-bit
> KVM,
> but no one cares about performance of 32-bit KVM, and in any case it's
> extremely
> unlikely this will be anything but noise.

ok

>=20
> > +	if ((xcr0 & XFEATURE_MASK_XTILE) &&
> > +	    ((xcr0 & XFEATURE_MASK_XTILE) !=3D XFEATURE_MASK_XTILE))
> > +		return 1;
> > +#endif
> >  	vcpu->arch.xcr0 =3D xcr0;
> >
> >  	if ((xcr0 ^ old_xcr0) & XFEATURE_MASK_EXTEND)
> > --
> > 2.27.0
> >