This patchset comes from a bug I found during qemu guest migration from a
host with newer CPU to a host with an older version of this CPU, and thus
having less FPU features.
When the guests were created, the one with less features is used as
config, so migration is possible.
Patch 1 fix a bug that always happens during this migration, and is
related to the fact that xsave saves all feature flags, but xrstor does
not touch the PKRU flag. It also changes how fpstate->user_xfeatures
is set, going from kvm_check_cpuid() to the later called
kvm_vcpu_after_set_cpuid().
Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
introduced in order to make it easier to read the replaced version.
Patches were compile-tested, and could fix the bug found.
Please let me know of anything to improve!
Best regards,
Leo
--
Changes since v3:
- Add new patch to remove the use of kvm_vcpu_arch.guest_supported_xcr0,
since it is now duplicating guest_fpu.fpstate->user_xfeatures.
- On patch 1, also avoid setting user_xfeatures on kvm_check_cpuid(),
since it is already set in kvm_vcpu_after_set_cpuid() now.
Changes since v2:
- Fix building error because I forgot to EXPORT_SYMBOL(fpu_user_cfg)
Changes since v1:
- Instead of masking xfeatures, mask user_xfeatures instead. This will
only change the value sent to user, instead of the one saved in buf.
- Above change removed the need of the patch 2/2
- Instead of masking the current value of user_xfeatures, save on it
fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0
Leonardo Bras (2):
x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0
x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0
arch/x86/include/asm/kvm_host.h | 1 -
arch/x86/kernel/fpu/xstate.c | 5 ++++-
arch/x86/kvm/cpuid.c | 5 ++++-
arch/x86/kvm/x86.c | 20 +++++++++++++++-----
4 files changed, 23 insertions(+), 8 deletions(-)
--
2.35.1
kvm_vcpu_arch currently contains the guest supported features in both
guest_supported_xcr0 and guest_fpu.fpstate->user_xfeatures field.
Currently both fields are set to the same value in
kvm_vcpu_after_set_cpuid() and are not changed anywhere else after that.
Since it's not good to keep duplicated data, remove guest_supported_xcr0.
To keep the code more readable, introduce kvm_guest_supported_xcr()
and kvm_guest_supported_xfd() to replace the previous usages of
guest_supported_xcr0.
Signed-off-by: Leonardo Bras <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 -
arch/x86/kvm/cpuid.c | 5 +++--
arch/x86/kvm/x86.c | 20 +++++++++++++++-----
3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6dcccb304775..ec9830d2aabf 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -703,7 +703,6 @@ struct kvm_vcpu_arch {
struct fpu_guest guest_fpu;
u64 xcr0;
- u64 guest_supported_xcr0;
struct kvm_pio_request pio;
void *pio_data;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 71125291c578..b8f8d268d058 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -282,6 +282,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
{
struct kvm_lapic *apic = vcpu->arch.apic;
struct kvm_cpuid_entry2 *best;
+ u64 guest_supported_xcr0;
best = kvm_find_cpuid_entry(vcpu, 1, 0);
if (best && apic) {
@@ -293,10 +294,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
kvm_apic_set_version(vcpu);
}
- vcpu->arch.guest_supported_xcr0 =
+ guest_supported_xcr0 =
cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
- vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
+ vcpu->arch.guest_fpu.fpstate->user_xfeatures = guest_supported_xcr0;
kvm_update_pv_runtime(vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 641044db415d..92177e2ff664 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -984,6 +984,18 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_GPL(kvm_load_host_xsave_state);
+static inline u64 kvm_guest_supported_xcr(struct kvm_vcpu *vcpu)
+{
+ u64 guest_supported_xcr0 = vcpu->arch.guest_fpu.fpstate->user_xfeatures;
+
+ return guest_supported_xcr0;
+}
+
+static inline u64 kvm_guest_supported_xfd(struct kvm_vcpu *vcpu)
+{
+ return kvm_guest_supported_xcr(vcpu) & XFEATURE_MASK_USER_DYNAMIC;
+}
+
static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
{
u64 xcr0 = xcr;
@@ -1003,7 +1015,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
* saving. However, xcr0 bit 0 is always set, even if the
* emulated CPU does not support XSAVE (see kvm_vcpu_reset()).
*/
- valid_bits = vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FP;
+ valid_bits = kvm_guest_supported_xcr(vcpu) | XFEATURE_MASK_FP;
if (xcr0 & ~valid_bits)
return 1;
@@ -3706,8 +3718,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
!guest_cpuid_has(vcpu, X86_FEATURE_XFD))
return 1;
- if (data & ~(XFEATURE_MASK_USER_DYNAMIC &
- vcpu->arch.guest_supported_xcr0))
+ if (data & ~(kvm_guest_supported_xfd(vcpu)))
return 1;
fpu_update_guest_xfd(&vcpu->arch.guest_fpu, data);
@@ -3717,8 +3728,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
!guest_cpuid_has(vcpu, X86_FEATURE_XFD))
return 1;
- if (data & ~(XFEATURE_MASK_USER_DYNAMIC &
- vcpu->arch.guest_supported_xcr0))
+ if (data & ~(kvm_guest_supported_xfd(vcpu)))
return 1;
vcpu->arch.guest_fpu.xfd_err = data;
--
2.35.1
On 2/17/22 06:30, Leonardo Bras wrote:
> This patchset comes from a bug I found during qemu guest migration from a
> host with newer CPU to a host with an older version of this CPU, and thus
> having less FPU features.
>
> When the guests were created, the one with less features is used as
> config, so migration is possible.
>
> Patch 1 fix a bug that always happens during this migration, and is
> related to the fact that xsave saves all feature flags, but xrstor does
> not touch the PKRU flag. It also changes how fpstate->user_xfeatures
> is set, going from kvm_check_cpuid() to the later called
> kvm_vcpu_after_set_cpuid().
>
> Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
> duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
> introduced in order to make it easier to read the replaced version.
>
> Patches were compile-tested, and could fix the bug found.
Queued, thanks (for 5.17 of course)! For patch 2, I renamed the
function to kvm_guest_supported_xcr0.
Paolo
> Please let me know of anything to improve!
>
> Best regards,
> Leo
>
> --
> Changes since v3:
> - Add new patch to remove the use of kvm_vcpu_arch.guest_supported_xcr0,
> since it is now duplicating guest_fpu.fpstate->user_xfeatures.
> - On patch 1, also avoid setting user_xfeatures on kvm_check_cpuid(),
> since it is already set in kvm_vcpu_after_set_cpuid() now.
> Changes since v2:
> - Fix building error because I forgot to EXPORT_SYMBOL(fpu_user_cfg)
> Changes since v1:
> - Instead of masking xfeatures, mask user_xfeatures instead. This will
> only change the value sent to user, instead of the one saved in buf.
> - Above change removed the need of the patch 2/2
> - Instead of masking the current value of user_xfeatures, save on it
> fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0
>
> Leonardo Bras (2):
> x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0
> x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0
>
> arch/x86/include/asm/kvm_host.h | 1 -
> arch/x86/kernel/fpu/xstate.c | 5 ++++-
> arch/x86/kvm/cpuid.c | 5 ++++-
> arch/x86/kvm/x86.c | 20 +++++++++++++++-----
> 4 files changed, 23 insertions(+), 8 deletions(-)
>
On Thu, Feb 17, 2022 at 11:52 AM Paolo Bonzini <[email protected]> wrote:
>
> On 2/17/22 06:30, Leonardo Bras wrote:
> > This patchset comes from a bug I found during qemu guest migration from a
> > host with newer CPU to a host with an older version of this CPU, and thus
> > having less FPU features.
> >
> > When the guests were created, the one with less features is used as
> > config, so migration is possible.
> >
> > Patch 1 fix a bug that always happens during this migration, and is
> > related to the fact that xsave saves all feature flags, but xrstor does
> > not touch the PKRU flag. It also changes how fpstate->user_xfeatures
> > is set, going from kvm_check_cpuid() to the later called
> > kvm_vcpu_after_set_cpuid().
> >
> > Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
> > duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
> > introduced in order to make it easier to read the replaced version.
> >
> > Patches were compile-tested, and could fix the bug found.
>
> Queued, thanks (for 5.17 of course)! For patch 2, I renamed the
> function to kvm_guest_supported_xcr0.
>
> Paolo
>
That's great!
Thanks Paolo!