From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "Christopherson,, Sean" <seanjc@google.com>,
        "Zhong, Yang" <yang.zhong@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>,
        "Liu, Jing2" <jing2.liu@intel.com>,
        "Zeng, Guang" <guang.zeng@intel.com>,
        "Wang, Wei W" <wei.w.wang@intel.com>
Subject: RE: [PATCH v5 12/21] kvm: x86: Intercept #NM for saving IA32_XFD_ERR
Thread-Topic: [PATCH v5 12/21] kvm: x86: Intercept #NM for saving IA32_XFD_ERR
Thread-Index: AQHYAjDG3grRPUB6WkKC4V40NrLWoqxVAQqAgAAqkbA=
Date:   Thu, 6 Jan 2022 00:55:40 +0000
Message-ID: <BN9PR11MB52760C5B8289B2DEF32AC63C8C4C9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20220105123532.12586-1-yang.zhong@intel.com>
 <20220105123532.12586-13-yang.zhong@intel.com> <YdYaH7buoApEVPOg@google.com>
In-Reply-To: <YdYaH7buoApEVPOg@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?QRzvs0kf+4m/TZG3ztXn+Kmi5nMJY9cfFJOaAKVi397WiwHRpYx+vRRvA6fo?=
 =?us-ascii?Q?6RY2oOGC6zNA0sm7cdHUZ0m9zcIwEv4SEkHOgGpaKqB3mddcpS7cizWY8mUz?=
 =?us-ascii?Q?ixn6Q7igFl0h5Sk8askCrbco726vBbr6+uauwAp3s5+MECqm3vipWpY77u1N?=
 =?us-ascii?Q?URNd2bX5RWcN12ahujoYmcP/NHA0aZAMgaXcGHWsybIDxGSArOV8Mzc4akFj?=
 =?us-ascii?Q?Qjv0JsUtd4VVTkzf/LDAFRou1C6TuUqW0GYu4bEi4zYf/Kk6kK3QM/Ce8rI2?=
 =?us-ascii?Q?efPCQliSsS3ic/YYBfgTpUjspZXScH/IHZfdT02KW7oW2/hmI4H/0/66GJ5Z?=
 =?us-ascii?Q?3gHBVfBI2hX2xLUxz7P/W7IgFz7CNEC6FdRoaQWcAa6p9y4D13lUAfCRKYdQ?=
 =?us-ascii?Q?yuP3raiYXiAdacgKw+QbuOLpp9qja18GufjfaxgZwKvO1Ccp2yqgaWkYZlp8?=
 =?us-ascii?Q?54O92VeNLWo7cHjhaQXf8XdlPxDErWADiiSSscZjLMPKTc+SKv6vU0OpkMjC?=
 =?us-ascii?Q?KZEmVZ7ZCzPqmpqmk5hG3pEMrZMQ8uDRCXcomtLjdU/PWn362Jc+4nM+OEon?=
 =?us-ascii?Q?PjF7+oFBJr0oG58Ot+WHiFT+JdciDxgW1h4tunk8haKjPhWVjasNB+HNDXC/?=
 =?us-ascii?Q?zlC/Lq8y5bdm+JwnsD1Lk5glLJgO7lIj/34rSaPOxvEcEAS7s/UO/B+AoKHc?=
 =?us-ascii?Q?g0UshkUwy7hC2GfucANa2t68izdCmHD1/e1ZU6lucSvvvUaIB2g3KhfHRMHt?=
 =?us-ascii?Q?fhO0pp6efRgtKmDlCQPccomZ4QYTGmB//aU0FoAeBf79p0ha+qo18lxNWREy?=
 =?us-ascii?Q?LDgEgIzNzhYgvjBtiEh7hIssFO6EA+VPyhLx7c9auX9XvoSw/3l5yuNoGRpi?=
 =?us-ascii?Q?lMHQzuMQJbefzY08jmhAK1eKu5n3m+dSs5w8O7biOj6Y5SdbLFL9g0YmfGcI?=
 =?us-ascii?Q?guyjJGviICBOudFJN8TGoe21uQFD1Jft4bYzRTHYF504pqbQafhtxzvHFJHP?=
 =?us-ascii?Q?M/nkL7nKkxOcnFInFAIuiPIOp/HkXeWMrloejBo1JVVv/TsS10B4AeQgffLz?=
 =?us-ascii?Q?BQDnrFdxhHm0nTBQdyxUDYutrBxSM2jIheWUR3W0eeT6iVys0NrAhIKPG9Bm?=
 =?us-ascii?Q?tt6hEXFPJbFMbs3SGxqEM+pr7lQC7/NlsrsPEOH1TJbm7bS6G3ktv3ycZGct?=
 =?us-ascii?Q?hmE555dj35kNaewgv0eRo9mqZIPXfgoVJs2OLO/lxQIubT38i3jBJAvtIudy?=
 =?us-ascii?Q?JTDiL3iZZBfUWMvCFVJM4wzci0Gzc070kpEKTGyOePxb8AGFPeBU2iEWlUlw?=
 =?us-ascii?Q?Y8THP1LCummFGZB9DJjEFUy3j2qtY2waVjo4nYcGnpZVKvQVZI7Jk8gwM/aj?=
 =?us-ascii?Q?KjcH1MbMEZjChs67X+W6BBGS1C+6PS6m38l9vqLcQiKADSaSFfaLUgzWkBWC?=
 =?us-ascii?Q?evEDWgZs2hDycQCIpNZyP1VDeRLbjFJmyKRaZdzU2uQvmYzgFVgnTBS8suSR?=
 =?us-ascii?Q?09lYCaesLbP48XejmK6bR120tLys51yyg4Wa7aiNfvePaXw9N3qxzfB1q1Zq?=
 =?us-ascii?Q?tXtqv5nL9pprn1pYuEfYgxSuPtsrdQecIboDjqSzFeOp5r4XZxaF+Ae60nSS?=
 =?us-ascii?Q?FNsmfdSuuz1ewwbfWTsTRTI=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: 9ef0d11a-2a7f-4a20-d318-08d9d0af4282
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jan 2022 00:55:40.1169
 (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: mKbuX1zb4NWQG/GY8h+SDBLwUU9jJ8AoHwUbkP+OovRdecA1TbMVrU1gdpQyjpE0zdLCkPFVDs3KXti/gbJ5Xw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1393
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: Thursday, January 6, 2022 6:22 AM
>=20
> On Wed, Jan 05, 2022, Yang Zhong wrote:
> > @@ -6399,6 +6424,26 @@ static void handle_interrupt_nmi_irqoff(struct
> kvm_vcpu *vcpu,
> >  	kvm_after_interrupt(vcpu);
> >  }
> >
> > +static void handle_nm_fault_irqoff(struct kvm_vcpu *vcpu)
> > +{
> > +	/*
> > +	 * Save xfd_err to guest_fpu before interrupt is enabled, so the
> > +	 * MSR value is not clobbered by the host activity before the guest
> > +	 * has chance to consume it.
> > +	 *
> > +	 * We should not blindly read xfd_err here, since this exception
>=20
> Nit, avoid "we", and explain what KVM does (or doesn't) do, not what KVM
> "should"
> do, e.g. just say
>=20
> 	 * Do not blindly read ...
>=20
> > +	 * might be caused by L1 interception on a platform which doesn't
> > +	 * support xfd at all.
> > +	 *
> > +	 * Do it conditionally upon guest_fpu::xfd. xfd_err matters
> > +	 * only when xfd contains a non-zero value.
> > +	 *
> > +	 * Queuing exception is done in vmx_handle_exit. See comment
> there.
>=20
> Another nit, it's worth explaining why XFD_ERR needs to be read here
> regardless
> of is_guest_mode().  E.g.
>=20
> 	 * Injecting the #NM back into the guest is handled in the standard
> path
> 	 * as an #NM in L2 may be reflected into L1 as a VM-Exit.  Read
> XFD_ERR
> 	 * even if the #NM is from L2, as L1 may have exposed XFD to L2.

sounds good

>=20
> Side topic, in a follow up series/patch, it's probably worth adding suppo=
rt in
> nested_vmx_prepare_msr_bitmap() to allow passthrough of the MSRs to L2.

will do.

>=20
> > +	 */
> > +	if (vcpu->arch.guest_fpu.fpstate->xfd)
> > +		rdmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
> > +}
> > +
> >  static void handle_exception_nmi_irqoff(struct vcpu_vmx *vmx)
> >  {
> >  	const unsigned long nmi_entry =3D (unsigned long)asm_exc_nmi_noist;
> > @@ -6407,6 +6452,9 @@ static void handle_exception_nmi_irqoff(struct
> vcpu_vmx *vmx)
> >  	/* if exit due to PF check for async PF */
> >  	if (is_page_fault(intr_info))
> >  		vmx->vcpu.arch.apf.host_apf_flags =3D
> kvm_read_and_reset_apf_flags();
> > +	/* if exit due to NM, handle before interrupts are enabled */
>=20
> Nit, drop this comment, it's slightly misleading since the #NM isn't full=
y
> handled
> here.  The comment in handle_nm_fault_irqoff() is more than sufficient.
>=20
> > +	else if (is_nm_fault(intr_info))
> > +		handle_nm_fault_irqoff(&vmx->vcpu);
> >  	/* Handle machine checks before interrupts are enabled */
> >  	else if (is_machine_check(intr_info))
> >  		kvm_machine_check();
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 21ce65220e38..2c988f8ca616 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9953,6 +9953,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
> >  	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> >  		switch_fpu_return();
> >
> > +	if (vcpu->arch.guest_fpu.xfd_err)
> > +		wrmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
> > +
> >  	if (unlikely(vcpu->arch.switch_db_regs)) {
> >  		set_debugreg(0, 7);
> >  		set_debugreg(vcpu->arch.eff_db[0], 0);
> > @@ -10016,6 +10019,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
> >
> >  	static_call(kvm_x86_handle_exit_irqoff)(vcpu);
> >
> > +	if (vcpu->arch.guest_fpu.xfd_err)
> > +		wrmsrl(MSR_IA32_XFD_ERR, 0);
> > +
> >  	/*
> >  	 * Consume any pending interrupts, including the possible source of
> >  	 * VM-Exit on SVM and any ticks that occur between VM-Exit and
> now.