On Wed, Sep 27, 2023 at 05:19:51PM -0700, Sean Christopherson wrote:
Rework how KVM limits guest-unsupported xfeatures to effectively hide only when saving state for userspace (KVM_GET_XSAVE), i.e. to let userspace load all host-supported xfeatures (via KVM_SET_XSAVE) irrespective of what features have been exposed to the guest.
Ok, IIUC your changes provide: - KVM_GET_XSAVE will return only guest-supported xfeatures - KVM_SET_XSAVE will allow user to set any xfeatures supported by host Is that correct?
The effect on KVM_SET_XSAVE was knowingly done by commit ad856280ddea ("x86/kvm/fpu: Limit guest user_xfeatures to supported bits of XCR0"):
As a bonus, it will also fail if userspace tries to set fpu features (with the KVM_SET_XSAVE ioctl) that are not compatible to the guest configuration. Such features will never be returned by KVM_GET_XSAVE or KVM_GET_XSAVE2.Peventing userspace from doing stupid things is usually a good idea, but in this case restricting KVM_SET_XSAVE actually exacerbated the problem that commit ad856280ddea was fixing. As reported by Tyler, rejecting KVM_SET_XSAVE for guest-unsupported xfeatures breaks live migration from a kernel without commit ad856280ddea, to a kernel with ad856280ddea. I.e. from a kernel that saves guest-unsupported xfeatures to a kernel that doesn't allow loading guest-unuspported xfeatures.
So this patch is supposed to fix migration of VM from a host with pre-ad856280ddea (OLD) kernel to a host with ad856280ddea + your set(NEW). Right?
Let's get the scenario here, where all machines are the same: 1 - VM created on OLD kernel with a host-supported xfeature F, which is not guest supported. 2 - VM is migrated to a NEW kernel/host, and KVM_SET_XSAVE xfeature F. 3 - VM will be migrated to another host, qemu requests KVM_GET_XSAVE, which returns only guest-supported xfeatures, and this is passed to next host 4 - VM will be started on 3rd host with guest-supported xfeatures, meaning xfeature F is filtered-out, which is not good, because the VM will have less features compared to boot.
In fact, I notice something would possibly happen between 2 and 3, since qemu will run KVM_GET_XSAVE at kvm_cpu_synchronize_state() and KVM_SET_XSAVE at kvm_cpu_exec(), which happens quite often (when vcpu stops / resumes for some reason).
Also, even if I got something wrong, and for some reason qemu will be able to store the original VM xfeatures between migrations, we have the original issue ad856280ddea was dealing with: newer machines -> older machines migration:
1 - User gets a VM from an OLD kernel, with a newer host (more xfeatures). 2 - User migrates VM to NEW kernel, and we suppose qemu stores original xfeatures (it works). Migration can occur to newer or same gen hosts. 3 - At some point, if migration is attempted to an older host (less xfeatures), qemu will abort the VM.
To make matters even worse, QEMU doesn't terminate if KVM_SET_XSAVE fails, and so the end result is that the live migration results (possibly silent) guest data corruption instead of a failed migration.
And this is something that really needs to be fixed in QEMU side.
Patch 1 refactors the FPU code to let KVM pass in a mask of which xfeatures to save, patch 2 fixes KVM by passing in guest_supported_xcr0 instead of modifying user_xfeatures directly.
At my current understanding of this patchset, I would not recomment merging it, as it would introduce a lot of undesired behaviors.
Please let me know if I got something wrong, so I can review it again.
Thanks! Leo
Patches 3-5 are regression tests.
I have no objection if anyone wants patches 1 and 2 squashed together, I split them purely to make review easier.
Note, this doesn't fix the scenario where a guest is migrated from a "bad" to a "good" kernel and the target host doesn't support the over-saved set of xfeatures. I don't see a way to safely handle that in the kernel without an opt-in, which more or less defeats the purpose of handling it in KVM.
Sean Christopherson (5): x86/fpu: Allow caller to constrain xfeatures when copying to uabi buffer KVM: x86: Constrain guest-supported xfeatures only at KVM_GET_XSAVE{2} KVM: selftests: Touch relevant XSAVE state in guest for state test KVM: selftests: Load XSAVE state into untouched vCPU during state test KVM: selftests: Force load all supported XSAVE state in state test
arch/x86/include/asm/fpu/api.h | 3 +- arch/x86/kernel/fpu/core.c | 5 +- arch/x86/kernel/fpu/xstate.c | 12 +- arch/x86/kernel/fpu/xstate.h | 3 +- arch/x86/kvm/cpuid.c | 8 -- arch/x86/kvm/x86.c | 37 +++--- .../selftests/kvm/include/x86_64/processor.h | 23 ++++ .../testing/selftests/kvm/x86_64/state_test.c | 110 +++++++++++++++++- 8 files changed, 168 insertions(+), 33 deletions(-)
base-commit: 5804c19b80bf625c6a9925317f845e497434d6d3
2.42.0.582.g8ccd20d70d-goog