diff mbox series

[14/19] x86/fpu: Prepare for KVM XFD_ERR handling

Message ID 20211208000359.2853257-15-yang.zhong@intel.com (mailing list archive)
State New, archived
Headers show
Series AMX Support in KVM | expand

Commit Message

Yang Zhong Dec. 8, 2021, 12:03 a.m. UTC 
From: Jing Liu <jing2.liu@intel.com>

When XFD causes an instruction to generate #NM, IA32_XFD_ERR
contains information about which disabled state components
are being accessed. The #NM handler is expected to check this
information and then enable the state components by clearing
IA32_XFD for the faulting task (if having permission).

if the XFD_ERR value generated in guest is consumed/clobbered by
the host before the guest itself doing so. This may lead to
non-XFD-related #NM treated as XFD #NM in host (due to non-zero
value in XFD_ERR), or XFD-related #NM treated as non-XFD #NM in
guest (XFD_ERR cleared by the host #NM handler).

This patch provides two helpers to swap the guest XFD_ERR and host
XFD_ERR. Where to call them in KVM will be discussed thoroughly
in next patch.

The guest XFD_ERR value is saved in fpu_guest::xfd_err. There is
no need to save host XFD_ERR because it's always cleared to ZERO
by the host #NM handler (which cannot be preempted by a vCPU
thread to observe a non-zero value).

The lower two bits in fpu_guest::xfd_err is borrowed for special
purposes. The state components (FP and SSE) covered by the two
bits are not XSAVE-enabled feature, thus not XFD-enabled either.
It's impossible to see hardware setting them in XFD_ERR:

  - XFD_ERR_GUEST_DISABLED (bit 0)

    Indicate that XFD extension is not exposed to the guest thus
    no need to save/restore it.

  - XFD_ERR_GUEST_SAVED (bit 1)

    Indicate fpu_guest::xfd_err already contains a saved value
    thus no need for duplicated saving (e.g. when the vCPU thread
    is preempted multiple times before re-enter the guest).

Signed-off-by: Jing Liu <jing2.liu@intel.com>
Signed-off-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 arch/x86/include/asm/fpu/api.h   |  8 ++++++
 arch/x86/include/asm/fpu/types.h | 24 ++++++++++++++++
 arch/x86/kernel/fpu/core.c       | 49 ++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

Comments

Paolo Bonzini Dec. 10, 2021, 4:16 p.m. UTC | #1 
On 12/8/21 01:03, Yang Zhong wrote:
> 
> The guest XFD_ERR value is saved in fpu_guest::xfd_err. There is
> no need to save host XFD_ERR because it's always cleared to ZERO
> by the host #NM handler (which cannot be preempted by a vCPU
> thread to observe a non-zero value).
> 
> The lower two bits in fpu_guest::xfd_err is borrowed for special
> purposes. The state components (FP and SSE) covered by the two
> bits are not XSAVE-enabled feature, thus not XFD-enabled either.
> It's impossible to see hardware setting them in XFD_ERR:
> 
>    - XFD_ERR_GUEST_DISABLED (bit 0)
> 
>      Indicate that XFD extension is not exposed to the guest thus
>      no need to save/restore it.

Can this instead just check if xfeatures includes any dynamic-save features?

Paolo

>    - XFD_ERR_GUEST_SAVED (bit 1)
> 
>      Indicate fpu_guest::xfd_err already contains a saved value
>      thus no need for duplicated saving (e.g. when the vCPU thread
>      is preempted multiple times before re-enter the guest).
Thomas Gleixner Dec. 10, 2021, 11:20 p.m. UTC | #2 
On Tue, Dec 07 2021 at 19:03, Yang Zhong wrote:
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -322,6 +322,55 @@ int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
>  }
>  EXPORT_SYMBOL_GPL(fpu_swap_kvm_fpstate);
>  
> +#ifdef CONFIG_X86_64
> +void fpu_save_guest_xfd_err(struct fpu_guest *guest_fpu)
> +{
> +	if (guest_fpu->xfd_err & XFD_ERR_GUEST_DISABLED)
> +		return;
> +
> +	/* A non-zero value indicates guest XFD_ERR already saved */
> +	if (guest_fpu->xfd_err)
> +		return;
> +
> +	/* Guest XFD_ERR must be saved before switching to host fpstate */
> +	WARN_ON_ONCE(!current->thread.fpu.fpstate->is_guest);

Warn and proceed?

> +	rdmsrl(MSR_IA32_XFD_ERR, guest_fpu->xfd_err);
> +
> +	/*
> +	 * Restore to the host value if guest xfd_err is non-zero.
> +	 * Except in #NM handler, all other places in the kernel
> +	 * should just see xfd_err=0. So just restore to 0.
> +	 */
> +	if (guest_fpu->xfd_err)
> +		wrmsrl(MSR_IA32_XFD_ERR, 0);
> +
> +	guest_fpu->xfd_err |= XFD_ERR_GUEST_SAVED;
> +}
> +EXPORT_SYMBOL_GPL(fpu_save_guest_xfd_err);
> +
> +void fpu_restore_guest_xfd_err(struct fpu_guest *guest_fpu)
> +{
> +	u64 xfd_err = guest_fpu->xfd_err;
> +
> +	if (xfd_err & XFD_ERR_GUEST_DISABLED)
> +		return;
> +
> +	xfd_err &= ~XFD_ERR_GUEST_SAVED;
> +
> +	/*
> +	 * No need to restore a zero value since XFD_ERR
> +	 * is always zero outside of #NM handler in the host.
> +	 */
> +	if (!xfd_err)
> +		return;
> +
> +	wrmsrl(MSR_IA32_XFD_ERR, xfd_err);
> +	guest_fpu->xfd_err = 0;
> +}

Why should any pf this be in the FPU core?

It's a pure guest issue as all of this is related to struct fpu_guest
and not struct fpu or any other core FPU state.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index 999d89026be9..c2e8f2172994 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -147,6 +147,14 @@  extern bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu);
 extern void fpu_free_guest_fpstate(struct fpu_guest *gfpu);
 extern int fpu_swap_kvm_fpstate(struct fpu_guest *gfpu, bool enter_guest);
 
+#ifdef CONFIG_X86_64
+extern void fpu_save_guest_xfd_err(struct fpu_guest *guest_fpu);
+extern void fpu_restore_guest_xfd_err(struct fpu_guest *guest_fpu);
+#else
+static inline void fpu_save_guest_xfd_err(struct fpu_guest *guest_fpu) { }
+static inline void fpu_restore_guest_xfd_err(struct fpu_guest *guest_fpu) { }
+#endif
+
 extern void fpu_copy_guest_fpstate_to_uabi(struct fpu_guest *gfpu, void *buf, unsigned int size, u32 pkru);
 extern int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf, u64 xcr0, u32 *vpkru);
 
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 861cffca3209..5ee98222c103 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -500,6 +500,22 @@  struct fpu {
 	 */
 };
 
+/*
+ * Use @xfd_err:bit0 to indicate whether guest XFD_ERR should be
+ * saved/restored. The x87 state covered by bit 0 is not a
+ * XSAVE-enabled feature, thus is not XFD-enabled either (won't
+ * occur in XFD_ERR).
+ */
+#define XFD_ERR_GUEST_DISABLED		(1 << XFEATURE_FP)
+
+/*
+ * Use @xfd_err:bit1 to indicate the validity of @xfd_err. Used to
+ * avoid duplicated savings in case the vCPU is preempted multiple
+ * times before it re-enters the guest. The SSE state covered by
+ * bit 1 is neither XSAVE-enabled nor XFD-enabled.
+ */
+#define XFD_ERR_GUEST_SAVED		(1 << XFEATURE_SSE)
+
 /*
  * Guest pseudo FPU container
  */
@@ -527,6 +543,14 @@  struct fpu_guest {
 	 */
 	u64				realloc_request;
 
+	/*
+	 * @xfd_err:			save the guest value. bit 0 and bit1
+	 *				have special meaning to indicate the
+	 *				requirement of saving and the validity
+	 *				of the saved value.
+	 */
+	u64				xfd_err;
+
 	/*
 	 * @fpstate:			Pointer to the allocated guest fpstate
 	 */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7a0436a0cb2c..5089f2e7dc22 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -322,6 +322,55 @@  int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
 }
 EXPORT_SYMBOL_GPL(fpu_swap_kvm_fpstate);
 
+#ifdef CONFIG_X86_64
+void fpu_save_guest_xfd_err(struct fpu_guest *guest_fpu)
+{
+	if (guest_fpu->xfd_err & XFD_ERR_GUEST_DISABLED)
+		return;
+
+	/* A non-zero value indicates guest XFD_ERR already saved */
+	if (guest_fpu->xfd_err)
+		return;
+
+	/* Guest XFD_ERR must be saved before switching to host fpstate */
+	WARN_ON_ONCE(!current->thread.fpu.fpstate->is_guest);
+
+	rdmsrl(MSR_IA32_XFD_ERR, guest_fpu->xfd_err);
+
+	/*
+	 * Restore to the host value if guest xfd_err is non-zero.
+	 * Except in #NM handler, all other places in the kernel
+	 * should just see xfd_err=0. So just restore to 0.
+	 */
+	if (guest_fpu->xfd_err)
+		wrmsrl(MSR_IA32_XFD_ERR, 0);
+
+	guest_fpu->xfd_err |= XFD_ERR_GUEST_SAVED;
+}
+EXPORT_SYMBOL_GPL(fpu_save_guest_xfd_err);
+
+void fpu_restore_guest_xfd_err(struct fpu_guest *guest_fpu)
+{
+	u64 xfd_err = guest_fpu->xfd_err;
+
+	if (xfd_err & XFD_ERR_GUEST_DISABLED)
+		return;
+
+	xfd_err &= ~XFD_ERR_GUEST_SAVED;
+
+	/*
+	 * No need to restore a zero value since XFD_ERR
+	 * is always zero outside of #NM handler in the host.
+	 */
+	if (!xfd_err)
+		return;
+
+	wrmsrl(MSR_IA32_XFD_ERR, xfd_err);
+	guest_fpu->xfd_err = 0;
+}
+EXPORT_SYMBOL_GPL(fpu_restore_guest_xfd_err);
+#endif
+
 void fpu_copy_guest_fpstate_to_uabi(struct fpu_guest *gfpu, void *buf,
 				    unsigned int size, u32 pkru)
 {