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 <[email protected]>
Reviewed-by: Len Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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(-)
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 = ¤t->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, ¤t->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. */
--
2.17.1
-----Original Message-----
From: Bae, Chang Seok <[email protected]>
Sent: Wednesday, December 23, 2020 11:57 PM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: Brown, Len <[email protected]>; Hansen, Dave <[email protected]>; Liu, Jing2 <[email protected]>; Shankar, Ravi V <[email protected]>; [email protected]; Bae, Chang Seok <[email protected]>; [email protected]
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 <[email protected]>
Reviewed-by: Len Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
[...]
/*
* 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 = ¤t->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, ¤t->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
> On Jan 7, 2021, at 17:41, Liu, Jing2 <[email protected]> wrote:
>
> static void kvm_save_current_fpu(struct fpu *fpu) {
> + struct fpu *src_fpu = ¤t->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, ¤t->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
On 1/8/2021 2:40 AM, Bae, Chang Seok wrote:
>> On Jan 7, 2021, at 17:41, Liu, Jing2 <[email protected]> wrote:
>>
>> static void kvm_save_current_fpu(struct fpu *fpu) {
>> + struct fpu *src_fpu = ¤t->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, ¤t->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
> On Jan 11, 2021, at 18:52, Liu, Jing2 <[email protected]> wrote:
>
> On 1/8/2021 2:40 AM, Bae, Chang Seok wrote:
>>> On Jan 7, 2021, at 17:41, Liu, Jing2 <[email protected]> wrote:
>>>
>>> static void kvm_save_current_fpu(struct fpu *fpu) {
>>> + struct fpu *src_fpu = ¤t->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, ¤t->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
On 1/15/2021 12:59 PM, Bae, Chang Seok wrote:
>> On Jan 11, 2021, at 18:52, Liu, Jing2 <[email protected]> wrote:
>>
>> On 1/8/2021 2:40 AM, Bae, Chang Seok wrote:
>>>> On Jan 7, 2021, at 17:41, Liu, Jing2 <[email protected]> wrote:
>>>>
>>>> static void kvm_save_current_fpu(struct fpu *fpu) {
>>>> + struct fpu *src_fpu = ¤t->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, ¤t->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
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.
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On Feb 8, 2021, at 04:33, Borislav Petkov <[email protected]> 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/[email protected]/