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 22/22] kvm: x86: Disable interception for IA32_XFD on
 demand
Thread-Topic: [PATCH v3 22/22] kvm: x86: Disable interception for IA32_XFD on
 demand
Thread-Index: AQHX9zE4gQJMQm2XFEKlfvGX4m1Fn6xIsbuAgAAjG3A=
Date:   Wed, 29 Dec 2021 03:35:27 +0000
Message-ID: <BN9PR11MB527653AF0BEC5BBA007B72408C449@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20211222124052.644626-1-jing2.liu@intel.com>
 <20211222124052.644626-23-jing2.liu@intel.com> <Ycu0KVq9PfuygKKx@google.com>
In-Reply-To: <Ycu0KVq9PfuygKKx@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?iV1e/AUrx8SucTIhI0zT5pGzjUL8WdfTqvUYvVHcI/YdkIyUqsolU0Zj9rFc?=
 =?us-ascii?Q?uzOD3/+Bkn/MZAk2A+H88HIW0q7AN2KOKYwsREvRHb/wMJf6+Mb/Vma2k1tC?=
 =?us-ascii?Q?nfkO7tumz3fyeBV4x3tM276/SEugXLmI5tRRK6B0GuBrAmPJdfFlLjklgNQQ?=
 =?us-ascii?Q?los+NMXCGYYCFo8HFsEVV++FOkNK3xlkRDvr/KUSUZMGjibU9XHvp27pO+RT?=
 =?us-ascii?Q?xT9d5CAU+gqETTM7Y69XlZ50o5jn9bUzlJ2olhzog4aJrDEstFVTYmQx0lYE?=
 =?us-ascii?Q?EvmSZhH4fxyJLVNWliBmort30Ape/P/ojSwn4grpGG5EV9o4qf/slyqW7LUk?=
 =?us-ascii?Q?uCQWco0HC55xmGiUeq7YoAXBDtvCSwauTOzGvmXTsMtdRuTfYNnrzLpBg77E?=
 =?us-ascii?Q?0/d9cNN0R+6GQLrIQBM6Qga+X2jCVQ4xxFGTNqBiEXVVHTEeQ2yp/fq/Kc98?=
 =?us-ascii?Q?UKQGVsyt70J0CwQd4AUWu1Go6FPfnq7cBy15T6dndPt10WNHL7MqX35ioKVv?=
 =?us-ascii?Q?bxQkDciltvWa5I7uI590CNRr9hA+me3wGC5CHywunmkC1KoXqSL2miAgK7O0?=
 =?us-ascii?Q?JJcDYcqCH+Cxb9w/hYF9CLW/RX0sKW3YxGdRxIt20czB0+uzsF59u9mN3PrG?=
 =?us-ascii?Q?H8ohO8OwZbYr44g7Btn+K0LWGi4ID+DcdCCBhNZd6rgkP+GB2QaNFC7CrgUc?=
 =?us-ascii?Q?9xVu9Bl87RhQbpeI76UFuI1XwKdverUPFnRlxCi6oclvKJpablGV/MjpIv++?=
 =?us-ascii?Q?adfN52u5l7KALj6vaLCYzZezI+T7FPIZCV4+ZASkzLFAYdQq2nert9b79OaY?=
 =?us-ascii?Q?f4WGOCQFA6Hwr4gWz0OXENdw+qxRNGVHB3633/Er+qI5EalWQKFSnvOIYT51?=
 =?us-ascii?Q?fuFrF707YieegSMJM5QEjJ2xzkAgw2kQi/Tev6bHpGPSYm2j0/bSGI8QvjWc?=
 =?us-ascii?Q?oND/opX3kYhm/GcURtb+MQGshWV6M4183e3T/76QwVSu7tiyo31r9X7K4KHp?=
 =?us-ascii?Q?W7nSZBXvxOm68nYdbHFlGC5n4KZKbUbXfyr9iRMeAwyFT6FuzEyM8jcGcFJt?=
 =?us-ascii?Q?c3cm592cSuPTibDjbpbtp4i2ewT4PD28ajeADPpJKouYedREiar5gGbDIQB8?=
 =?us-ascii?Q?LnCgmXyHjHUSr7rGuff6QIW3pdz15dBtYpVsyFnhLcTk7nLYe+Nb/2jMcBzW?=
 =?us-ascii?Q?7Zk6+oTP9Pawhq0Y0Iub8P47ONsCjDMXCxNn+0Qgyg3jXaYCx1RQuci9j4wB?=
 =?us-ascii?Q?O1iFgfs0aVd5Ges1V0o8vljxQ7jLMfPv9MraXRNj9MTI191HllqWoN1NM86G?=
 =?us-ascii?Q?qnOczP/l46rpPIHeWS6iExB8VvItRNBmjapJmxsDrcTKS0sueFTpmdAIj1kD?=
 =?us-ascii?Q?CICim3whw7dQPAS9lVrV+IVnb5VjbOyXTA/4pjhRNF0pBQMfoOcRrYxdKn3Y?=
 =?us-ascii?Q?G+WjqfOJ1QwzmW9joJ4z1/HSYztPct0jUJiY8yAphDaS9Sc7WgkUUhbzx/Ov?=
 =?us-ascii?Q?5TJIgPDrh1fod3ixCnOZDbDgyYJoH/PZRXUFWs2rVVFESYYec/xtPKXWOWEw?=
 =?us-ascii?Q?8TyqdvZpPfLSJNBQQcOjQhFjPcQf4oYJZtJJ/tSdO0h8TU+po7ohtxLCf5gF?=
 =?us-ascii?Q?+iuGrRfCXJL47baDFynJQtE=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: b6690ee2-1d1d-47e0-554c-08d9ca7c41bf
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Dec 2021 03:35:27.6324
 (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: C/Ncz1nAu0lk43koeKR+wbWkAnAXMsz4/WnrnpyY9cDYaSMwExguHc8pkjnqESk5fdw1NOiQcIdqggb3D6wnuQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1858
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 9:05 AM
>=20
> On Wed, Dec 22, 2021, Jing Liu wrote:
> > @@ -1968,6 +1969,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
> >  	case MSR_IA32_XFD:
> >  		ret =3D kvm_set_msr_common(vcpu, msr_info);
> >  		if (!ret && data) {
> > +			vmx_disable_intercept_for_msr(vcpu,
> MSR_IA32_XFD, MSR_TYPE_RW);
> > +			vcpu->arch.xfd_out_of_sync =3D true;
>=20
> xfd_out_of_sync is a poor name, as XFD _may_ be out of sync, or it may no=
t.
> It's
> also confusing that it's kept set after XFD is explicitly synchronized in
> vcpu_enter_guest().

yes, sync_xfd_after_exit might be more accurate.

>=20
> > +
> >  			vcpu->arch.trap_nm =3D true;
> >  			vmx_update_exception_bitmap(vcpu);
>=20
> Ah, this is why #NM interception was made sticky many patches ago.  More
> at the end.
>=20
> >  		}
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index bf9d3051cd6c..0a00242a91e7 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -340,7 +340,7 @@ struct vcpu_vmx {
> >  	struct lbr_desc lbr_desc;
> >
> >  	/* Save desired MSR intercept (read: pass-through) state */
> > -#define MAX_POSSIBLE_PASSTHROUGH_MSRS	14
> > +#define MAX_POSSIBLE_PASSTHROUGH_MSRS	15
> >  	struct {
> >  		DECLARE_BITMAP(read,
> MAX_POSSIBLE_PASSTHROUGH_MSRS);
> >  		DECLARE_BITMAP(write,
> MAX_POSSIBLE_PASSTHROUGH_MSRS);
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 3b756ff13103..10a08aa2aa45 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10024,6 +10024,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
> >  	if (vcpu->arch.guest_fpu.xfd_err)
> >  		wrmsrl(MSR_IA32_XFD_ERR, 0);
> >
> > +	if (vcpu->arch.xfd_out_of_sync)
>=20
> Rather than adding a flag that tracks whether or not the MSR can be writt=
en
> by
> the guest, can't this be:
>=20
> 	if (!vmx_test_msr_bitmap_write(vcpu->loaded_vmcs->msr_bitmap))
> 		fpu_sync_guest_vmexit_xfd_state();
>=20

We can use this

> That might be marginally slower than checking a dedicated flag?  But is h=
as
> the
> advantage of doing the correct thing for nested guests instead of penaliz=
ing
> them
> with an unnecessary sync on every exit.  If performance of the check is a=
n
> issue,
> we could add a static key to skip the code unless at least one vCPU has
> triggered
> the XFD crud, a la kvm_has_noapic_vcpu (which may or may not provide any
> real
> performance benefits).
>=20
> Speaking of nested, interception of #NM in vmx_update_exception_bitmap()
> is wrong
> with respect to nested guests.  Until XFD is supported for L2, which I di=
dn't
> see
> in this series, #NM should not be intercepted while L2 is running.

Can you remind what additional thing is required to support XFD for L2?
If only about performance I prefer to the current conservative approach
as the first step. As explained earlier, #NM should be rare if the guest=20
doesn't run AMX applications at all. Adding nested into this picture doesn'=
t=20
make things a lot worser.

>=20
> For the earlier patch that introduced arch.trap_nm, if it's not too gross=
 and
> not
> racy, the code could be:
>=20
> 	if (is_guest_mode(vcpu))
> 		eb |=3D get_vmcs12(vcpu)->exception_bitmap;
>         else {
> 		...
>=20
> 		if (vcpu->arch.guest_fpu.fpstate.xfd)
> 			eb |=3D (1u << NM_VECTOR);
> 	}
>=20
> Though I'm ok with a semi-temporary flag if that's gross/racy.
>=20
> Then this patch can change it to:
>=20
> 	if (is_guest_mode(vcpu))
> 		eb |=3D get_vmcs12(vcpu)->exception_bitmap;
>         else {
> 		...
>=20
> 		if (!vmx_test_msr_bitmap_write(vcpu->vmcs01.msr_bitmap))
> 			eb |=3D (1u << NM_VECTOR);
> 	}
>=20
> > +		fpu_sync_guest_vmexit_xfd_state();
> > +
> >  	/*
> >  	 * Consume any pending interrupts, including the possible source of
> >  	 * VM-Exit on SVM and any ticks that occur between VM-Exit and
> now.
> > --
> > 2.27.0
> >

Thanks
Kevin