diff mbox series

[v10,04/28] x86/fpu/xstate: Modify address finders to handle both static and dynamic buffers

Message ID 20210825155413.19673-5-chang.seok.bae@intel.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Chang S. Bae Aug. 25, 2021, 3:53 p.m. UTC 
Have all the functions finding XSTATE address take a struct fpu * pointer
in preparation for dynamic state buffer support.

init_fpstate is a special case, which is indicated by a null pointer
parameter to get_xsave_addr() and __raw_xsave_addr().

No functional change.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kvm@vger.kernel.org
---
Changes from v5:
* Adjusted some call sites for the new base.

Changes from v3:
* Updated the changelog. (Borislav Petkov)
* Updated the function comment to use kernel-doc style. (Borislav Petkov)

Changes from v2:
* Updated the changelog with task->fpu removed. (Borislav Petkov)

Changes from v1:
* Rebased on the upstream kernel (5.10)
---
 arch/x86/include/asm/fpu/xstate.h |  2 +-
 arch/x86/kernel/fpu/xstate.c      | 42 ++++++++++++++++++++++++-------
 arch/x86/kvm/x86.c                | 10 +++-----
 3 files changed, 38 insertions(+), 16 deletions(-)

Comments

Thomas Gleixner Oct. 1, 2021, 1:15 p.m. UTC | #1 
Chang,

On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:

> Have all the functions finding XSTATE address take a struct fpu * pointer
> in preparation for dynamic state buffer support.
>
> init_fpstate is a special case, which is indicated by a null pointer
> parameter to get_xsave_addr() and __raw_xsave_addr().

Same comment vs. subject. Prepare ...

> +	if (fpu)
> +		xsave = &fpu->state.xsave;
> +	else
> +		xsave = &init_fpstate.xsave;
> +
> +	return xsave + xstate_comp_offsets[xfeature_nr];

So you have the same conditionals and the same comments vs. that NULL
pointer oddity how many times now all over the place?

That can be completely avoided:

Patch 1:

-union fpregs_state init_fpstate __ro_after_init;
+static union fpregs_state init_fpstate __ro_after_init;
+struct fpu init_fpu = { .state = &init_fpstate } __ro_after_init;

and make all users of init_fpstate access it through init_fpu.

Patches 2..N which change arguments from fpregs_state to fpu:

-	fun(init_fpu->state);
+	fun(&init_fpu);

Patch M which adds state_mask:

@fpu__init_system_xstate()
+	init_fpu.state_mask = xfeatures_mask_all;

Hmm?

Thanks,

        tglx
Chang S. Bae Oct. 3, 2021, 10:35 p.m. UTC | #2 
On Oct 1, 2021, at 06:15, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
> 
>> Have all the functions finding XSTATE address take a struct fpu * pointer
>> in preparation for dynamic state buffer support.
>> 
>> init_fpstate is a special case, which is indicated by a null pointer
>> parameter to get_xsave_addr() and __raw_xsave_addr().
> 
> Same comment vs. subject. Prepare ...

How about:
    "Prepare address finders to handle dynamic features"

>> +	if (fpu)
>> +		xsave = &fpu->state.xsave;
>> +	else
>> +		xsave = &init_fpstate.xsave;
>> +
>> +	return xsave + xstate_comp_offsets[xfeature_nr];
> 
> So you have the same conditionals and the same comments vs. that NULL
> pointer oddity how many times now all over the place?
> 
> That can be completely avoided:
> 
> Patch 1:
> 
> -union fpregs_state init_fpstate __ro_after_init;
> +static union fpregs_state init_fpstate __ro_after_init;
> +struct fpu init_fpu = { .state = &init_fpstate } __ro_after_init;
> 
> and make all users of init_fpstate access it through init_fpu.
> 
> Patches 2..N which change arguments from fpregs_state to fpu:
> 
> -	fun(init_fpu->state);
> +	fun(&init_fpu);
> 
> Patch M which adds state_mask:
> 
> @fpu__init_system_xstate()
> +	init_fpu.state_mask = xfeatures_mask_all;
> 
> Hmm?

Okay, a NULL pointer is odd and it as an argument should be avoided. Defining
a separate struct fpu for the initial state can make every function expect a
valid struct fpu pointer.

I think that the patch set will have such order (once [1] is dropped out) of,
    - patch1 (new): a cleanup patch for fpstate_init_xstate() in patch1
    - patch2 (new): the above init_fpu goes into this, and 
    - patch3-5: changes arguments to fpu,
    - patch6 (new): finally patch 6 to add ->state_mask and ->state_size.
    …

Thanks,
Chang

[1] https://lore.kernel.org/lkml/20210825155413.19673-2-chang.seok.bae@intel.com/
Thomas Gleixner Oct. 4, 2021, 12:54 p.m. UTC | #3 
On Sun, Oct 03 2021 at 22:35, Chang Seok Bae wrote:
> On Oct 1, 2021, at 06:15, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Okay, a NULL pointer is odd and it as an argument should be avoided. Defining
> a separate struct fpu for the initial state can make every function expect a
> valid struct fpu pointer.
>
> I think that the patch set will have such order (once [1] is dropped out) of,
>     - patch1 (new): a cleanup patch for fpstate_init_xstate() in patch1
>     - patch2 (new): the above init_fpu goes into this, and 
>     - patch3-5: changes arguments to fpu,

So actually I sat down over the weekend and looked at that again. Adding this
to struct fpu is wrong. The size and features information belongs into
something like this:

struct fpstate {
	unsigned int		size;
        u64			xfeatures;

        union fpregs_state	regs;
};

Why? Simply because fpstate is the container for the dynamically sized
regs and that's where it semantically belongs.

While staring at that I just started to cleanup stuff all over the place
to make the integration of this simpler.

The patches are completely untested and have no changelogs yet, but if
you want a preview, I've uploaded a patch series to:

    https://tglx.de/~tglx/patches.tar

I'm still staring at some of the dynamic feature integrations, but this
is roughly where this should be heading.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index ede166e9d3f2..2451bccc6cac 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -134,7 +134,7 @@  extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 extern void __init update_regset_xstate_info(unsigned int size,
 					     u64 xstate_mask);
 
-void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
+void *get_xsave_addr(struct fpu *fpu, int xfeature_nr);
 int xfeature_size(int xfeature_nr);
 int copy_uabi_from_kernel_to_xstate(struct fpu *fpu, const void *kbuf);
 int copy_sigframe_from_user_to_xstate(struct fpu *fpu, const void __user *ubuf);
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index df39085e9d05..0a59df0c48e7 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -841,19 +841,34 @@  void fpu__resume_cpu(void)
 	}
 }
 
-/*
+/**
+ * __raw_xsave_addr - Find the address where the feature state is saved.
+ *
  * Given an xstate feature nr, calculate where in the xsave
  * buffer the state is.  Callers should ensure that the buffer
  * is valid.
+ *
+ * If @fpu is NULL, use init_fpstate.
+ *
+ * @fpu:	A struct fpu * pointer
+ *
+ * Return:	An address of the feature state in the buffer
  */
-static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
+static void *__raw_xsave_addr(struct fpu *fpu, int xfeature_nr)
 {
+	void *xsave;
+
 	if (!xfeature_enabled(xfeature_nr)) {
 		WARN_ON_FPU(1);
 		return NULL;
 	}
 
-	return (void *)xsave + xstate_comp_offsets[xfeature_nr];
+	if (fpu)
+		xsave = &fpu->state.xsave;
+	else
+		xsave = &init_fpstate.xsave;
+
+	return xsave + xstate_comp_offsets[xfeature_nr];
 }
 /*
  * Given the xsave area and a state inside, this function returns the
@@ -866,15 +881,18 @@  static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
  * this will return NULL.
  *
  * Inputs:
- *	xstate: the thread's storage area for all FPU data
+ *	fpu: the thread's FPU data to reference xstate buffer(s).
+ *	     (A null pointer parameter indicates init_fpstate.)
  *	xfeature_nr: state which is defined in xsave.h (e.g. XFEATURE_FP,
  *	XFEATURE_SSE, etc...)
  * Output:
  *	address of the state in the xsave area, or NULL if the
  *	field is not present in the xsave buffer.
  */
-void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
+void *get_xsave_addr(struct fpu *fpu, int xfeature_nr)
 {
+	struct xregs_state *xsave;
+
 	/*
 	 * Do we even *have* xsave state?
 	 */
@@ -887,6 +905,12 @@  void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
 	 */
 	WARN_ONCE(!(xfeatures_mask_all & BIT_ULL(xfeature_nr)),
 		  "get of unsupported state");
+
+	if (fpu)
+		xsave = &fpu->state.xsave;
+	else
+		xsave = &init_fpstate.xsave;
+
 	/*
 	 * This assumes the last 'xsave*' instruction to
 	 * have requested that 'xfeature_nr' be saved.
@@ -901,7 +925,7 @@  void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
 	if (!(xsave->header.xfeatures & BIT_ULL(xfeature_nr)))
 		return NULL;
 
-	return __raw_xsave_addr(xsave, xfeature_nr);
+	return __raw_xsave_addr(fpu, xfeature_nr);
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
 
@@ -1061,8 +1085,8 @@  void copy_xstate_to_uabi_buf(struct membuf to, struct task_struct *tsk,
 			membuf_write(&to, &pkru, sizeof(pkru));
 		} else {
 			copy_feature(header.xfeatures & BIT_ULL(i), &to,
-				     __raw_xsave_addr(xsave, i),
-				     __raw_xsave_addr(xinit, i),
+				     __raw_xsave_addr(&tsk->thread.fpu, i),
+				     __raw_xsave_addr(NULL, i),
 				     xstate_sizes[i]);
 		}
 		/*
@@ -1129,7 +1153,7 @@  static int copy_uabi_to_xstate(struct fpu *fpu, const void *kbuf,
 		u64 mask = ((u64)1 << i);
 
 		if (hdr.xfeatures & mask) {
-			void *dst = __raw_xsave_addr(xsave, i);
+			void *dst = __raw_xsave_addr(fpu, i);
 
 			if (!dst)
 				continue;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ce529ab233d7..9c7c59317e5e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4726,7 +4726,7 @@  static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
 			memcpy(dest + offset, &vcpu->arch.pkru,
 			       sizeof(vcpu->arch.pkru));
 		} else {
-			src = get_xsave_addr(xsave, xfeature_nr);
+			src = get_xsave_addr(vcpu->arch.guest_fpu, xfeature_nr);
 			if (src)
 				memcpy(dest + offset, src, size);
 		}
@@ -4769,7 +4769,7 @@  static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
 			memcpy(&vcpu->arch.pkru, src + offset,
 			       sizeof(vcpu->arch.pkru));
 		} else {
-			void *dest = get_xsave_addr(xsave, xfeature_nr);
+			void *dest = get_xsave_addr(vcpu->arch.guest_fpu, xfeature_nr);
 
 			if (dest)
 				memcpy(dest, src + offset, size);
@@ -10840,12 +10840,10 @@  void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 		 */
 		if (init_event)
 			kvm_put_guest_fpu(vcpu);
-		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu->state.xsave,
-					XFEATURE_BNDREGS);
+		mpx_state_buffer = get_xsave_addr(vcpu->arch.guest_fpu, XFEATURE_BNDREGS);
 		if (mpx_state_buffer)
 			memset(mpx_state_buffer, 0, sizeof(struct mpx_bndreg_state));
-		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu->state.xsave,
-					XFEATURE_BNDCSR);
+		mpx_state_buffer = get_xsave_addr(vcpu->arch.guest_fpu, XFEATURE_BNDCSR);
 		if (mpx_state_buffer)
 			memset(mpx_state_buffer, 0, sizeof(struct mpx_bndcsr));
 		if (init_event)