diff mbox series

[v3,10/21] x86/fpu/xstate: Update xstate save function to support dynamic xstate

Message ID 20201223155717.19556-11-chang.seok.bae@intel.com (mailing list archive)
State New, archived
Headers show
Series [v3,01/21] x86/fpu/xstate: Modify initialization helper to handle both static and dynamic buffers | expand

Commit Message

Chang S. Bae Dec. 23, 2020, 3:57 p.m. UTC 
copy_xregs_to_kernel() used to save all user states in a kernel buffer.
When the dynamic user state is enabled, it becomes conditional which state
to be saved.

fpu->state_mask can indicate which state components are reserved to be
saved in XSAVE buffer. Use it as XSAVE's instruction mask to select states.

KVM used to save all xstate via copy_xregs_to_kernel(). Update KVM to set a
valid fpu->state_mask, which will be necessary to correctly handle dynamic
state buffers.

No functional change until the kernel supports dynamic user states.

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 v2:
* Updated the changelog to clarify the KVM code changes.
---
 arch/x86/include/asm/fpu/internal.h |  3 +--
 arch/x86/kernel/fpu/core.c          |  2 +-
 arch/x86/kvm/x86.c                  | 11 ++++++++---
 3 files changed, 10 insertions(+), 6 deletions(-)

Comments

Jing Liu Jan. 7, 2021, 8:41 a.m. UTC | #1 
-----Original Message-----
From: Bae, Chang Seok <chang.seok.bae@intel.com> 
Sent: Wednesday, December 23, 2020 11:57 PM
To: bp@suse.de; luto@kernel.org; tglx@linutronix.de; mingo@kernel.org; x86@kernel.org
Cc: Brown, Len <len.brown@intel.com>; Hansen, Dave <dave.hansen@intel.com>; Liu, Jing2 <jing2.liu@intel.com>; Shankar, Ravi V <ravi.v.shankar@intel.com>; linux-kernel@vger.kernel.org; Bae, Chang Seok <chang.seok.bae@intel.com>; kvm@vger.kernel.org
Subject: [PATCH v3 10/21] x86/fpu/xstate: Update xstate save function to support dynamic xstate

copy_xregs_to_kernel() used to save all user states in a kernel buffer.
When the dynamic user state is enabled, it becomes conditional which state to be saved.

fpu->state_mask can indicate which state components are reserved to be
saved in XSAVE buffer. Use it as XSAVE's instruction mask to select states.

KVM used to save all xstate via copy_xregs_to_kernel(). Update KVM to set a valid fpu->state_mask, which will be necessary to correctly handle dynamic state buffers.

See comments together below.

No functional change until the kernel supports dynamic user states.

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
[...]
 		/*
 		 * AVX512 state is tracked here because its use is diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4aecfba04bd3..93b5bacad67a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9214,15 +9214,20 @@ static int complete_emulated_mmio(struct kvm_vcpu *vcpu)
 
 static void kvm_save_current_fpu(struct fpu *fpu)  {
+	struct fpu *src_fpu = &current->thread.fpu;
+
 	/*
 	 * If the target FPU state is not resident in the CPU registers, just
 	 * memcpy() from current, else save CPU state directly to the target.
 	 */
-	if (test_thread_flag(TIF_NEED_FPU_LOAD))
-		memcpy(&fpu->state, &current->thread.fpu.state,
+	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
+		memcpy(&fpu->state, &src_fpu->state,
 		       fpu_kernel_xstate_min_size);
For kvm, if we assume that it does not support dynamic features until this series,
memcpy for only fpu->state is correct. 
I think this kind of assumption is reasonable and we only make original xstate work.

-	else
+	} else {
+		if (fpu->state_mask != src_fpu->state_mask)
+			fpu->state_mask = src_fpu->state_mask;

Though dynamic feature is not supported in kvm now, this function still need
consider more things for fpu->state_mask.
I suggest that we can set it before if...else (for both cases) and not change other. 

Thanks,
Jing

 		copy_fpregs_to_fpstate(fpu);
+	}

 }

 
 /* Swap (qemu) user FPU context for the guest FPU context. */
--
2.17.1
Chang S. Bae Jan. 7, 2021, 6:40 p.m. UTC | #2 
> On Jan 7, 2021, at 17:41, Liu, Jing2 <jing2.liu@intel.com> wrote:
> 
> static void kvm_save_current_fpu(struct fpu *fpu)  {
> +	struct fpu *src_fpu = &current->thread.fpu;
> +
> 	/*
> 	 * If the target FPU state is not resident in the CPU registers, just
> 	 * memcpy() from current, else save CPU state directly to the target.
> 	 */
> -	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> -		memcpy(&fpu->state, &current->thread.fpu.state,
> +	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
> +		memcpy(&fpu->state, &src_fpu->state,
> 		       fpu_kernel_xstate_min_size);
> For kvm, if we assume that it does not support dynamic features until this series,
> memcpy for only fpu->state is correct. 
> I think this kind of assumption is reasonable and we only make original xstate work.
> 
> -	else
> +	} else {
> +		if (fpu->state_mask != src_fpu->state_mask)
> +			fpu->state_mask = src_fpu->state_mask;
> 
> Though dynamic feature is not supported in kvm now, this function still need
> consider more things for fpu->state_mask.

Can you elaborate this? Which path might be affected by fpu->state_mask
without dynamic state supported in KVM?

> I suggest that we can set it before if...else (for both cases) and not change other. 

I tried a minimum change here.  The fpu->state_mask value does not impact the
memcpy(). So, why do we need to change it for both?

Thanks,
Chang
Liu, Jing2 Jan. 12, 2021, 2:52 a.m. UTC | #3 
On 1/8/2021 2:40 AM, Bae, Chang Seok wrote:
>> On Jan 7, 2021, at 17:41, Liu, Jing2 <jing2.liu@intel.com> wrote:
>>
>> static void kvm_save_current_fpu(struct fpu *fpu)  {
>> +	struct fpu *src_fpu = &current->thread.fpu;
>> +
>> 	/*
>> 	 * If the target FPU state is not resident in the CPU registers, just
>> 	 * memcpy() from current, else save CPU state directly to the target.
>> 	 */
>> -	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>> -		memcpy(&fpu->state, &current->thread.fpu.state,
>> +	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
>> +		memcpy(&fpu->state, &src_fpu->state,
>> 		       fpu_kernel_xstate_min_size);
>> For kvm, if we assume that it does not support dynamic features until this series,
>> memcpy for only fpu->state is correct.
>> I think this kind of assumption is reasonable and we only make original xstate work.
>>
>> -	else
>> +	} else {
>> +		if (fpu->state_mask != src_fpu->state_mask)
>> +			fpu->state_mask = src_fpu->state_mask;
>>
>> Though dynamic feature is not supported in kvm now, this function still need
>> consider more things for fpu->state_mask.
> Can you elaborate this? Which path might be affected by fpu->state_mask
> without dynamic state supported in KVM?
>
>> I suggest that we can set it before if...else (for both cases) and not change other.
> I tried a minimum change here.  The fpu->state_mask value does not impact the
> memcpy(). So, why do we need to change it for both?

Sure, what I'm considering is that "mask" is the first time introduced 
into "fpu",

representing the usage, so not only set it when needed, but also make it 
as a

representation, in case of anywhere using it (especially between the 
interval

of this series and kvm series in future).

Thanks,

Jing

> Thanks,
> Chang
Chang S. Bae Jan. 15, 2021, 4:59 a.m. UTC | #4 
> On Jan 11, 2021, at 18:52, Liu, Jing2 <jing2.liu@linux.intel.com> wrote:
> 
> On 1/8/2021 2:40 AM, Bae, Chang Seok wrote:
>>> On Jan 7, 2021, at 17:41, Liu, Jing2 <jing2.liu@intel.com> wrote:
>>> 
>>> static void kvm_save_current_fpu(struct fpu *fpu)  {
>>> +	struct fpu *src_fpu = &current->thread.fpu;
>>> +
>>> 	/*
>>> 	 * If the target FPU state is not resident in the CPU registers, just
>>> 	 * memcpy() from current, else save CPU state directly to the target.
>>> 	 */
>>> -	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>>> -		memcpy(&fpu->state, &current->thread.fpu.state,
>>> +	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
>>> +		memcpy(&fpu->state, &src_fpu->state,
>>> 		       fpu_kernel_xstate_min_size);

<snip>

>>> -	else
>>> +	} else {
>>> +		if (fpu->state_mask != src_fpu->state_mask)
>>> +			fpu->state_mask = src_fpu->state_mask;
>>> 
>>> Though dynamic feature is not supported in kvm now, this function still need
>>> consider more things for fpu->state_mask.
>> Can you elaborate this? Which path might be affected by fpu->state_mask
>> without dynamic state supported in KVM?
>> 
>>> I suggest that we can set it before if...else (for both cases) and not change other.
>> I tried a minimum change here.  The fpu->state_mask value does not impact the
>> memcpy(). So, why do we need to change it for both?
> 
> Sure, what I'm considering is that "mask" is the first time introduced into "fpu",
> representing the usage, so not only set it when needed, but also make it as a
> representation, in case of anywhere using it (especially between the interval
> of this series and kvm series in future).

Thank you for the feedback. Sorry, I don't get any logical reason to set the
mask always here. Perhaps, KVM code can be updated like you mentioned when
supporting the dynamic states there.

Please let me know if I’m missing any functional issues.

Thanks,
Chang
Liu, Jing2 Jan. 15, 2021, 5:45 a.m. UTC | #5 
On 1/15/2021 12:59 PM, Bae, Chang Seok wrote:
>> On Jan 11, 2021, at 18:52, Liu, Jing2 <jing2.liu@linux.intel.com> wrote:
>>
>> On 1/8/2021 2:40 AM, Bae, Chang Seok wrote:
>>>> On Jan 7, 2021, at 17:41, Liu, Jing2 <jing2.liu@intel.com> wrote:
>>>>
>>>> static void kvm_save_current_fpu(struct fpu *fpu)  {
>>>> +	struct fpu *src_fpu = &current->thread.fpu;
>>>> +
>>>> 	/*
>>>> 	 * If the target FPU state is not resident in the CPU registers, just
>>>> 	 * memcpy() from current, else save CPU state directly to the target.
>>>> 	 */
>>>> -	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>>>> -		memcpy(&fpu->state, &current->thread.fpu.state,
>>>> +	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
>>>> +		memcpy(&fpu->state, &src_fpu->state,
>>>> 		       fpu_kernel_xstate_min_size);
> <snip>
>
>>>> -	else
>>>> +	} else {
>>>> +		if (fpu->state_mask != src_fpu->state_mask)
>>>> +			fpu->state_mask = src_fpu->state_mask;
>>>>
>>>> Though dynamic feature is not supported in kvm now, this function still need
>>>> consider more things for fpu->state_mask.
>>> Can you elaborate this? Which path might be affected by fpu->state_mask
>>> without dynamic state supported in KVM?
>>>
>>>> I suggest that we can set it before if...else (for both cases) and not change other.
>>> I tried a minimum change here.  The fpu->state_mask value does not impact the
>>> memcpy(). So, why do we need to change it for both?
>> Sure, what I'm considering is that "mask" is the first time introduced into "fpu",
>> representing the usage, so not only set it when needed, but also make it as a
>> representation, in case of anywhere using it (especially between the interval
>> of this series and kvm series in future).
> Thank you for the feedback. Sorry, I don't get any logical reason to set the
> mask always here.

Sure, I'd like to see if fx_init()->memset is the case,

though maybe no hurt so far in test.

Thanks,

Jing

>   Perhaps, KVM code can be updated like you mentioned when
> supporting the dynamic states there.
>
> Please let me know if I’m missing any functional issues.
>
> Thanks,
> Chang
Borislav Petkov Feb. 8, 2021, 12:33 p.m. UTC | #6 
On Wed, Dec 23, 2020 at 07:57:06AM -0800, Chang S. Bae wrote:
> copy_xregs_to_kernel() used to save all user states in a kernel buffer.
> When the dynamic user state is enabled, it becomes conditional which state
> to be saved.
> 
> fpu->state_mask can indicate which state components are reserved to be
> saved in XSAVE buffer. Use it as XSAVE's instruction mask to select states.
> 
> KVM used to save all xstate via copy_xregs_to_kernel(). Update KVM to set a
> valid fpu->state_mask, which will be necessary to correctly handle dynamic
> state buffers.

All this commit message should say is something along the lines of
"extend copy_xregs_to_kernel() to receive a mask argument of which
states to save, in preparation of dynamic states handling."

> No functional change until the kernel supports dynamic user states.

Same comment as before.
Chang S. Bae Feb. 9, 2021, 3:48 p.m. UTC | #7 
On Feb 8, 2021, at 04:33, Borislav Petkov <bp@suse.de> wrote:
> On Wed, Dec 23, 2020 at 07:57:06AM -0800, Chang S. Bae wrote:
>> copy_xregs_to_kernel() used to save all user states in a kernel buffer.
>> When the dynamic user state is enabled, it becomes conditional which state
>> to be saved.
>> 
>> fpu->state_mask can indicate which state components are reserved to be
>> saved in XSAVE buffer. Use it as XSAVE's instruction mask to select states.
>> 
>> KVM used to save all xstate via copy_xregs_to_kernel(). Update KVM to set a
>> valid fpu->state_mask, which will be necessary to correctly handle dynamic
>> state buffers.
> 
> All this commit message should say is something along the lines of
> "extend copy_xregs_to_kernel() to receive a mask argument of which
> states to save, in preparation of dynamic states handling."

Yes, I will change like that. Thanks.

>> No functional change until the kernel supports dynamic user states.
> 
> Same comment as before.

This needs to be removed as per your comment [1].

Chang

[1] https://lore.kernel.org/lkml/20210209124906.GC15909@zn.tnic/
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 67ffd1d7c95e..d409a6ae0c38 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -332,9 +332,8 @@  static inline void copy_kernel_to_xregs_booting(struct xregs_state *xstate)
 /*
  * Save processor xstate to xsave area.
  */
-static inline void copy_xregs_to_kernel(struct xregs_state *xstate)
+static inline void copy_xregs_to_kernel(struct xregs_state *xstate, u64 mask)
 {
-	u64 mask = xfeatures_mask_all;
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
 	int err;
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 8b9d3ec9ac46..5a12e4b22db2 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -99,7 +99,7 @@  int copy_fpregs_to_fpstate(struct fpu *fpu)
 	if (likely(use_xsave())) {
 		struct xregs_state *xsave = &xstate->xsave;
 
-		copy_xregs_to_kernel(xsave);
+		copy_xregs_to_kernel(xsave, fpu->state_mask);
 
 		/*
 		 * AVX512 state is tracked here because its use is
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4aecfba04bd3..93b5bacad67a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9214,15 +9214,20 @@  static int complete_emulated_mmio(struct kvm_vcpu *vcpu)
 
 static void kvm_save_current_fpu(struct fpu *fpu)
 {
+	struct fpu *src_fpu = &current->thread.fpu;
+
 	/*
 	 * If the target FPU state is not resident in the CPU registers, just
 	 * memcpy() from current, else save CPU state directly to the target.
 	 */
-	if (test_thread_flag(TIF_NEED_FPU_LOAD))
-		memcpy(&fpu->state, &current->thread.fpu.state,
+	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
+		memcpy(&fpu->state, &src_fpu->state,
 		       fpu_kernel_xstate_min_size);
-	else
+	} else {
+		if (fpu->state_mask != src_fpu->state_mask)
+			fpu->state_mask = src_fpu->state_mask;
 		copy_fpregs_to_fpstate(fpu);
+	}
 }
 
 /* Swap (qemu) user FPU context for the guest FPU context. */