* [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic
@ 2024-02-28 10:18 Vitaly Kuznetsov
2024-02-28 10:18 ` [PATCH 1/3] KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper Vitaly Kuznetsov
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Vitaly Kuznetsov @ 2024-02-28 10:18 UTC (permalink / raw)
To: kvm, Paolo Bonzini, Sean Christopherson; +Cc: Li RongQing, linux-kernel
Guest hangs in specific configurations (KVM_X86_DISABLE_EXITS_HLT) are
reported and the issue was bisected to commit ee3a5f9e3d9b ("KVM: x86: Do
runtime CPUID update before updating vcpu->arch.cpuid_entries") which, of
course, carries "No functional change intended" blurb. Turns out, moving
__kvm_update_cpuid_runtime() earlier in kvm_set_cpuid() to tweak the
incoming CPUID data before checking it wasn't innocent as
KVM_FEATURE_PV_UNHALT reset logic relies on cached KVM CPUID base which
gets updated later.
I was not able to reproduce the issue with QEMU myself so I wrote a
selftest to show the problem.
Vitaly Kuznetsov (3):
KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper
KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT
KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with
KVM_X86_DISABLE_EXITS_HLT
arch/x86/kvm/cpuid.c | 42 ++++++++++++-------
.../selftests/kvm/x86_64/kvm_pv_test.c | 42 +++++++++++++++++++
2 files changed, 68 insertions(+), 16 deletions(-)
base-commit: 0cbca1bf44a0b8666c91ce3438f235c6fe70fbf1
--
2.43.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper 2024-02-28 10:18 [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Vitaly Kuznetsov @ 2024-02-28 10:18 ` Vitaly Kuznetsov 2024-02-28 10:18 ` [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT Vitaly Kuznetsov ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Vitaly Kuznetsov @ 2024-02-28 10:18 UTC (permalink / raw) To: kvm, Paolo Bonzini, Sean Christopherson; +Cc: Li RongQing, linux-kernel Similar to kvm_find_kvm_cpuid_features()/__kvm_find_kvm_cpuid_features(), introduce a helper to search for the specific hypervisor signature in any struct kvm_cpuid_entry2 array, not only in vcpu->arch.cpuid_entries. No functional change intended. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/cpuid.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index adba49afb5fe..0e1e3e5df6dd 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -189,15 +189,15 @@ static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 return 0; } -static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcpu, - const char *sig) +static struct kvm_hypervisor_cpuid __kvm_get_hypervisor_cpuid(struct kvm_cpuid_entry2 *entries, + int nent, const char *sig) { struct kvm_hypervisor_cpuid cpuid = {}; struct kvm_cpuid_entry2 *entry; u32 base; for_each_possible_hypervisor_cpuid_base(base) { - entry = kvm_find_cpuid_entry(vcpu, base); + entry = cpuid_entry2_find(entries, nent, base, KVM_CPUID_INDEX_NOT_SIGNIFICANT); if (entry) { u32 signature[3]; @@ -217,6 +217,13 @@ static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcp return cpuid; } +static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcpu, + const char *sig) +{ + return __kvm_get_hypervisor_cpuid(vcpu->arch.cpuid_entries, + vcpu->arch.cpuid_nent, sig); +} + static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries, int nent) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT 2024-02-28 10:18 [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Vitaly Kuznetsov 2024-02-28 10:18 ` [PATCH 1/3] KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper Vitaly Kuznetsov @ 2024-02-28 10:18 ` Vitaly Kuznetsov 2024-02-28 23:27 ` Sean Christopherson 2024-02-28 10:18 ` [PATCH 3/3] KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with KVM_X86_DISABLE_EXITS_HLT Vitaly Kuznetsov 2024-03-08 4:13 ` [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Sean Christopherson 3 siblings, 1 reply; 10+ messages in thread From: Vitaly Kuznetsov @ 2024-02-28 10:18 UTC (permalink / raw) To: kvm, Paolo Bonzini, Sean Christopherson; +Cc: Li RongQing, linux-kernel Commit ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries") moved tweaking of the supplied CPUID data earlier in kvm_set_cpuid() but __kvm_update_cpuid_runtime() actually uses 'vcpu->arch.kvm_cpuid' (though __kvm_find_kvm_cpuid_features()) which gets set later in kvm_set_cpuid(). In some cases, e.g. when kvm_set_cpuid() is called for the first time and 'vcpu->arch.kvm_cpuid' is clear, __kvm_find_kvm_cpuid_features() fails to find KVM PV feature entry and the logic which clears KVM_FEATURE_PV_UNHALT after enabling KVM_X86_DISABLE_EXITS_HLT does not work. The logic, introduced by the commit ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries") must stay: the supplied CPUID data is tweaked by KVM first (__kvm_update_cpuid_runtime()) and checked later (kvm_check_cpuid()) and the actual data (vcpu->arch.cpuid_*, vcpu->arch.kvm_cpuid, vcpu->arch.xen.cpuid,..) is only updated on success. Switch to searching for KVM_SIGNATURE in the supplied CPUID data to discover KVM PV feature entry instead of using stale 'vcpu->arch.kvm_cpuid'. While on it, drop pointless "&& (best->eax & (1 << KVM_FEATURE_PV_UNHALT)" check when clearing KVM_FEATURE_PV_UNHALT bit. Fixes: ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries") Reported-and-tested-by: Li RongQing <lirongqing@baidu.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/cpuid.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0e1e3e5df6dd..bfc0bfcb2bc6 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -224,22 +224,22 @@ static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcp vcpu->arch.cpuid_nent, sig); } -static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu, - struct kvm_cpuid_entry2 *entries, int nent) +static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_cpuid_entry2 *entries, + int nent, u32 kvm_cpuid_base) { - u32 base = vcpu->arch.kvm_cpuid.base; - - if (!base) - return NULL; - - return cpuid_entry2_find(entries, nent, base | KVM_CPUID_FEATURES, + return cpuid_entry2_find(entries, nent, kvm_cpuid_base | KVM_CPUID_FEATURES, KVM_CPUID_INDEX_NOT_SIGNIFICANT); } static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu) { - return __kvm_find_kvm_cpuid_features(vcpu, vcpu->arch.cpuid_entries, - vcpu->arch.cpuid_nent); + u32 base = vcpu->arch.kvm_cpuid.base; + + if (!base) + return NULL; + + return __kvm_find_kvm_cpuid_features(vcpu->arch.cpuid_entries, + vcpu->arch.cpuid_nent, base); } void kvm_update_pv_runtime(struct kvm_vcpu *vcpu) @@ -273,6 +273,7 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e int nent) { struct kvm_cpuid_entry2 *best; + struct kvm_hypervisor_cpuid kvm_cpuid; best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); if (best) { @@ -299,10 +300,12 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e cpuid_entry_has(best, X86_FEATURE_XSAVEC))) best->ebx = xstate_required_size(vcpu->arch.xcr0, true); - best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); - if (kvm_hlt_in_guest(vcpu->kvm) && best && - (best->eax & (1 << KVM_FEATURE_PV_UNHALT))) - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); + kvm_cpuid = __kvm_get_hypervisor_cpuid(entries, nent, KVM_SIGNATURE); + if (kvm_cpuid.base) { + best = __kvm_find_kvm_cpuid_features(entries, nent, kvm_cpuid.base); + if (kvm_hlt_in_guest(vcpu->kvm) && best) + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); + } if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) { best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); -- 2.43.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT 2024-02-28 10:18 ` [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT Vitaly Kuznetsov @ 2024-02-28 23:27 ` Sean Christopherson 2024-02-29 13:20 ` Vitaly Kuznetsov 0 siblings, 1 reply; 10+ messages in thread From: Sean Christopherson @ 2024-02-28 23:27 UTC (permalink / raw) To: Vitaly Kuznetsov; +Cc: kvm, Paolo Bonzini, Li RongQing, linux-kernel On Wed, Feb 28, 2024, Vitaly Kuznetsov wrote: > @@ -273,6 +273,7 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e > int nent) > { > struct kvm_cpuid_entry2 *best; > + struct kvm_hypervisor_cpuid kvm_cpuid; > > best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); > if (best) { > @@ -299,10 +300,12 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e > cpuid_entry_has(best, X86_FEATURE_XSAVEC))) > best->ebx = xstate_required_size(vcpu->arch.xcr0, true); > > - best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); > - if (kvm_hlt_in_guest(vcpu->kvm) && best && > - (best->eax & (1 << KVM_FEATURE_PV_UNHALT))) > - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); > + kvm_cpuid = __kvm_get_hypervisor_cpuid(entries, nent, KVM_SIGNATURE); > + if (kvm_cpuid.base) { > + best = __kvm_find_kvm_cpuid_features(entries, nent, kvm_cpuid.base); > + if (kvm_hlt_in_guest(vcpu->kvm) && best) > + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); > + } > > if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) { > best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); Not now, as we need a minimal fix, but we need to fix the root problem, this is way to brittle. Multiple helpers take @vcpu, including __kvm_update_cpuid_runtime(), before the incoming CPUID is set. That's just asking for new bugs to crop up. Am I missing something, or can we just swap() the new and old, update the new in the context of the vCPU, and then undo the swap() if there's an issue? vcpu->mutex is held, and accessing this state from a different task is wildly unsafe, so I don't see any problem with temporarily having an in-flux state. If we want to be paranoid, we can probably get away with killing the VM if the vCPU has run and the incoming CPUID is "bad", e.g. to guard against something in kvm_set_cpuid() consuming soon-to-be-stale state. And that's actually a feature of sorts, because _if_ something in kvm_set_cpuid() consumes the vCPU's CPUID, then we have a bug _now_ that affects the happy path. Completely untested (I haven't updated the myriad helpers), but this would allow us to revert/remove all of the changes that allow peeking at a CPUID array that lives outside of the vCPU. static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, int nent) { int r, i; swap(vcpu->arch.cpuid_entries, e2); swap(vcpu->arch.cpuid_nent, nent); #ifdef CONFIG_KVM_HYPERV if (kvm_cpuid_has_hyperv(vcpu)) { r = kvm_hv_vcpu_init(vcpu); if (r) goto err; } #endif r = kvm_check_cpuid(vcpu); if (r) goto err; kvm_update_cpuid_runtime(vcpu); /* * KVM does not correctly handle changing guest CPUID after KVM_RUN, as * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't * tracked in kvm_mmu_page_role. As a result, KVM may miss guest page * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with * the core vCPU model on the fly. It would've been better to forbid any * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do * KVM_SET_CPUID{,2} again. To support this legacy behavior, check * whether the supplied CPUID data is equal to what's already set. */ if (kvm_vcpu_has_run(vcpu)) { r = kvm_cpuid_check_equal(vcpu, e2, nent); if (r) goto err; } vcpu->arch.kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE); #ifdef CONFIG_KVM_XEN vcpu->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE); #endif kvm_vcpu_after_set_cpuid(vcpu); kvfree(e2); return 0; err: swap(vcpu->arch.cpuid_entries, e2); swap(vcpu->arch.cpuid_nent, nent); return r; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT 2024-02-28 23:27 ` Sean Christopherson @ 2024-02-29 13:20 ` Vitaly Kuznetsov 2024-02-29 18:54 ` Sean Christopherson 0 siblings, 1 reply; 10+ messages in thread From: Vitaly Kuznetsov @ 2024-02-29 13:20 UTC (permalink / raw) To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, Li RongQing, linux-kernel Sean Christopherson <seanjc@google.com> writes: > On Wed, Feb 28, 2024, Vitaly Kuznetsov wrote: >> @@ -273,6 +273,7 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e >> int nent) >> { >> struct kvm_cpuid_entry2 *best; >> + struct kvm_hypervisor_cpuid kvm_cpuid; >> >> best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); >> if (best) { >> @@ -299,10 +300,12 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e >> cpuid_entry_has(best, X86_FEATURE_XSAVEC))) >> best->ebx = xstate_required_size(vcpu->arch.xcr0, true); >> >> - best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); >> - if (kvm_hlt_in_guest(vcpu->kvm) && best && >> - (best->eax & (1 << KVM_FEATURE_PV_UNHALT))) >> - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); >> + kvm_cpuid = __kvm_get_hypervisor_cpuid(entries, nent, KVM_SIGNATURE); >> + if (kvm_cpuid.base) { >> + best = __kvm_find_kvm_cpuid_features(entries, nent, kvm_cpuid.base); >> + if (kvm_hlt_in_guest(vcpu->kvm) && best) >> + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); >> + } >> >> if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) { >> best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); > > Not now, as we need a minimal fix, but we need to fix the root problem, this is > way to brittle. Multiple helpers take @vcpu, including __kvm_update_cpuid_runtime(), > before the incoming CPUID is set. That's just asking for new bugs to > crop up. Yes, I'm all for making this all more robust but I think it would be nice to have a smaller, backportable fix for the real, observed issue first. > > Am I missing something, or can we just swap() the new and old, update the new > in the context of the vCPU, and then undo the swap() if there's an issue? > vcpu->mutex is held, and accessing this state from a different task is wildly > unsafe, so I don't see any problem with temporarily having an in-flux state. > I don't see why this approach shouldn't work and I agree it looks like it would make things better but I can't say that I'm in love with it. Ideally, I would want to see the following "atomic" workflow for all updates: - Check that the supplied data is correct, return an error if not. No changes to the state on this step. - Tweak the data if needed. - Update the state and apply the side-effects of the update. Ideally, there should be no errors on this step as rollback can be problemmatic. In the real world we will have to handle e.g. failed memory allocations here but in most cases the best course of action is to kill the VM. Well, kvm_set_cpuid() is not like that. At least: - kvm_hv_vcpu_init() is a side-effect but we apply it before all checks are complete. There's no way back. - kvm_check_cpuid() sounds like a pure checker but in reallity we end up mangling guest FPU state in fpstate_realloc() Both are probably "no big deal" but certainly break the atomicity. > If we want to be paranoid, we can probably get away with killing the VM if the > vCPU has run and the incoming CPUID is "bad", e.g. to guard against something > in kvm_set_cpuid() consuming soon-to-be-stale state. And that's actually a > feature of sorts, because _if_ something in kvm_set_cpuid() consumes the vCPU's > CPUID, then we have a bug _now_ that affects the happy path. > > Completely untested (I haven't updated the myriad helpers), but this would allow > us to revert/remove all of the changes that allow peeking at a CPUID array that > lives outside of the vCPU. Thanks, assuming there's no urgency, let me take a look at this in the course of the next week or so. > > static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > int nent) > { > int r, i; > > swap(vcpu->arch.cpuid_entries, e2); > swap(vcpu->arch.cpuid_nent, nent); > > #ifdef CONFIG_KVM_HYPERV > if (kvm_cpuid_has_hyperv(vcpu)) { > r = kvm_hv_vcpu_init(vcpu); > if (r) > goto err; > } > #endif > > r = kvm_check_cpuid(vcpu); > if (r) > goto err; > > kvm_update_cpuid_runtime(vcpu); > > /* > * KVM does not correctly handle changing guest CPUID after KVM_RUN, as > * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't > * tracked in kvm_mmu_page_role. As a result, KVM may miss guest page > * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with > * the core vCPU model on the fly. It would've been better to forbid any > * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately > * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do > * KVM_SET_CPUID{,2} again. To support this legacy behavior, check > * whether the supplied CPUID data is equal to what's already set. > */ > if (kvm_vcpu_has_run(vcpu)) { > r = kvm_cpuid_check_equal(vcpu, e2, nent); > if (r) > goto err; > } > > vcpu->arch.kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE); > #ifdef CONFIG_KVM_XEN > vcpu->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE); > #endif > kvm_vcpu_after_set_cpuid(vcpu); > > kvfree(e2); > return 0; > > err: > swap(vcpu->arch.cpuid_entries, e2); > swap(vcpu->arch.cpuid_nent, nent); > return r; > } > -- Vitaly ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT 2024-02-29 13:20 ` Vitaly Kuznetsov @ 2024-02-29 18:54 ` Sean Christopherson 0 siblings, 0 replies; 10+ messages in thread From: Sean Christopherson @ 2024-02-29 18:54 UTC (permalink / raw) To: Vitaly Kuznetsov; +Cc: kvm, Paolo Bonzini, Li RongQing, linux-kernel On Thu, Feb 29, 2024, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> writes: > > Am I missing something, or can we just swap() the new and old, update the new > > in the context of the vCPU, and then undo the swap() if there's an issue? > > vcpu->mutex is held, and accessing this state from a different task is wildly > > unsafe, so I don't see any problem with temporarily having an in-flux state. > > > > I don't see why this approach shouldn't work and I agree it looks like > it would make things better but I can't say that I'm in love with > it. Agreed, but the lack of atomicity is a pre-existing problem, though as proposed, my idea would make it worse. More below. > Ideally, I would want to see the following "atomic" workflow for all > updates: > > - Check that the supplied data is correct, return an error if not. No > changes to the state on this step. > - Tweak the data if needed. > - Update the state and apply the side-effects of the update. Ideally, > there should be no errors on this step as rollback can be > problemmatic. In the real world we will have to handle e.g. failed > memory allocations here but in most cases the best course of action is > to kill the VM. > > Well, kvm_set_cpuid() is not like that. At least: > - kvm_hv_vcpu_init() is a side-effect but we apply it before all checks > are complete. There's no way back. > - kvm_check_cpuid() sounds like a pure checker but in reallity we end up > mangling guest FPU state in fpstate_realloc() Yeah, I really, really don't like the call to fpu_enable_guest_xfd_features(). But to not make it worse, that call could be hoisted out of kvm_check_cpuid() so that it can be performed after kvm_cpuid_check_equal(), i.e. be kept dead last (and with a comment saying it needs to be dead last due to side effects that are visible to serspace). > Both are probably "no big deal" but certainly break the atomicity. > > > If we want to be paranoid, we can probably get away with killing the VM if the > > vCPU has run and the incoming CPUID is "bad", e.g. to guard against something > > in kvm_set_cpuid() consuming soon-to-be-stale state. And that's actually a > > feature of sorts, because _if_ something in kvm_set_cpuid() consumes the vCPU's > > CPUID, then we have a bug _now_ that affects the happy path. > > > > Completely untested (I haven't updated the myriad helpers), but this would allow > > us to revert/remove all of the changes that allow peeking at a CPUID array that > > lives outside of the vCPU. > > Thanks, assuming there's no urgency Definitely no urgency. > let me take a look at this in the course of the next week or so. No need, it was more of an "FYI, this is what I may go futz with". Specifically, it will impact what I want to do with guest cpu_caps[*], hopefully in a good way. My plan is to play around with it when I get back to that series. [*] https://lore.kernel.org/all/20231110235528.1561679-1-seanjc@google.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with KVM_X86_DISABLE_EXITS_HLT 2024-02-28 10:18 [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Vitaly Kuznetsov 2024-02-28 10:18 ` [PATCH 1/3] KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper Vitaly Kuznetsov 2024-02-28 10:18 ` [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT Vitaly Kuznetsov @ 2024-02-28 10:18 ` Vitaly Kuznetsov 2024-03-08 4:13 ` Sean Christopherson 2024-03-08 4:13 ` [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Sean Christopherson 3 siblings, 1 reply; 10+ messages in thread From: Vitaly Kuznetsov @ 2024-02-28 10:18 UTC (permalink / raw) To: kvm, Paolo Bonzini, Sean Christopherson; +Cc: Li RongQing, linux-kernel KVM_FEATURE_PV_UNHALT is expected to get cleared from KVM PV feature CPUID data when KVM_X86_DISABLE_EXITS_HLT is enabled. Add the corresponding test to kvm_pv_test. Note, the newly added code doesn't actually test KVM_FEATURE_PV_UNHALT and KVM_X86_DISABLE_EXITS_HLT features. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- .../selftests/kvm/x86_64/kvm_pv_test.c | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tools/testing/selftests/kvm/x86_64/kvm_pv_test.c b/tools/testing/selftests/kvm/x86_64/kvm_pv_test.c index 9e2879af7c20..60399454b45e 100644 --- a/tools/testing/selftests/kvm/x86_64/kvm_pv_test.c +++ b/tools/testing/selftests/kvm/x86_64/kvm_pv_test.c @@ -133,6 +133,46 @@ static void enter_guest(struct kvm_vcpu *vcpu) } } +static void test_pv_unhalt(void) +{ + struct kvm_vcpu *vcpu; + struct kvm_vm *vm; + struct kvm_cpuid_entry2 *ent; + u32 kvm_sig_old; + + pr_info("testing KVM_FEATURE_PV_UNHALT\n"); + + TEST_REQUIRE(KVM_CAP_X86_DISABLE_EXITS); + + /* KVM_PV_UNHALT test */ + vm = vm_create_with_one_vcpu(&vcpu, guest_main); + vcpu_set_cpuid_feature(vcpu, X86_FEATURE_KVM_PV_UNHALT); + + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_FEATURES); + + TEST_ASSERT(ent->eax & (1 << KVM_FEATURE_PV_UNHALT), + "Enabling X86_FEATURE_KVM_PV_UNHALT had no effect"); + + /* Make sure KVM clears vcpu->arch.kvm_cpuid */ + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_SIGNATURE); + kvm_sig_old = ent->ebx; + ent->ebx = 0xdeadbeef; + vcpu_set_cpuid(vcpu); + + vm_enable_cap(vm, KVM_CAP_X86_DISABLE_EXITS, KVM_X86_DISABLE_EXITS_HLT); + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_SIGNATURE); + ent->ebx = kvm_sig_old; + vcpu_set_cpuid(vcpu); + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_FEATURES); + + TEST_ASSERT(!(ent->eax & (1 << KVM_FEATURE_PV_UNHALT)), + "KVM_FEATURE_PV_UNHALT is set with KVM_CAP_X86_DISABLE_EXITS"); + + /* FIXME: actually test KVM_FEATURE_PV_UNHALT feature */ + + kvm_vm_free(vm); +} + int main(void) { struct kvm_vcpu *vcpu; @@ -151,4 +191,6 @@ int main(void) enter_guest(vcpu); kvm_vm_free(vm); + + test_pv_unhalt(); } -- 2.43.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with KVM_X86_DISABLE_EXITS_HLT 2024-02-28 10:18 ` [PATCH 3/3] KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with KVM_X86_DISABLE_EXITS_HLT Vitaly Kuznetsov @ 2024-03-08 4:13 ` Sean Christopherson 0 siblings, 0 replies; 10+ messages in thread From: Sean Christopherson @ 2024-03-08 4:13 UTC (permalink / raw) To: Vitaly Kuznetsov; +Cc: kvm, Paolo Bonzini, Li RongQing, linux-kernel Shortlog is a wee bit long, I went with: KVM: selftests: Check that PV_UNHALT is cleared when HLT exiting is disabled On Wed, Feb 28, 2024, Vitaly Kuznetsov wrote: > KVM_FEATURE_PV_UNHALT is expected to get cleared from KVM PV feature CPUID > data when KVM_X86_DISABLE_EXITS_HLT is enabled. Add the corresponding test > to kvm_pv_test. > > Note, the newly added code doesn't actually test KVM_FEATURE_PV_UNHALT and > KVM_X86_DISABLE_EXITS_HLT features. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- ... > + TEST_ASSERT(ent->eax & (1 << KVM_FEATURE_PV_UNHALT), > + "Enabling X86_FEATURE_KVM_PV_UNHALT had no effect"); > + > + /* Make sure KVM clears vcpu->arch.kvm_cpuid */ > + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_SIGNATURE); > + kvm_sig_old = ent->ebx; > + ent->ebx = 0xdeadbeef; > + vcpu_set_cpuid(vcpu); > + > + vm_enable_cap(vm, KVM_CAP_X86_DISABLE_EXITS, KVM_X86_DISABLE_EXITS_HLT); > + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_SIGNATURE); > + ent->ebx = kvm_sig_old; > + vcpu_set_cpuid(vcpu); > + ent = vcpu_get_cpuid_entry(vcpu, KVM_CPUID_FEATURES); > + > + TEST_ASSERT(!(ent->eax & (1 << KVM_FEATURE_PV_UNHALT)), X86_FEATURE_KVM_PV_UNHALT already exists, all we're missing is a helper to get a CPUID feature from host userspace given a vCPU. I added this static inline bool vcpu_cpuid_has(struct kvm_vcpu *vcpu, struct kvm_x86_cpu_feature feature) { struct kvm_cpuid_entry2 *entry; entry = __vcpu_get_cpuid_entry(vcpu, feature.function, feature.index); return *((&entry->eax) + feature.reg) & BIT(feature.bit); } and used it in this test instead of open coding the reg+bit. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic 2024-02-28 10:18 [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Vitaly Kuznetsov ` (2 preceding siblings ...) 2024-02-28 10:18 ` [PATCH 3/3] KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with KVM_X86_DISABLE_EXITS_HLT Vitaly Kuznetsov @ 2024-03-08 4:13 ` Sean Christopherson 2024-03-08 10:44 ` Vitaly Kuznetsov 3 siblings, 1 reply; 10+ messages in thread From: Sean Christopherson @ 2024-03-08 4:13 UTC (permalink / raw) To: Sean Christopherson, kvm, Paolo Bonzini, Vitaly Kuznetsov Cc: Li RongQing, linux-kernel On Wed, 28 Feb 2024 11:18:34 +0100, Vitaly Kuznetsov wrote: > Guest hangs in specific configurations (KVM_X86_DISABLE_EXITS_HLT) are > reported and the issue was bisected to commit ee3a5f9e3d9b ("KVM: x86: Do > runtime CPUID update before updating vcpu->arch.cpuid_entries") which, of > course, carries "No functional change intended" blurb. Turns out, moving > __kvm_update_cpuid_runtime() earlier in kvm_set_cpuid() to tweak the > incoming CPUID data before checking it wasn't innocent as > KVM_FEATURE_PV_UNHALT reset logic relies on cached KVM CPUID base which > gets updated later. > > [...] Applied to kvm-x86 hyperv. I won't send a pull request for this until next week, but I do plan on landing it in 6.9. Holler if the selftests tweaks look wrong (or you just don't like them). Thanks! [1/3] KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper https://github.com/kvm-x86/linux/commit/92e82cf632e8 [2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT https://github.com/kvm-x86/linux/commit/4736d85f0d18 [3/3] KVM: selftests: Check that PV_UNHALT is cleared when HLT exiting is disabled https://github.com/kvm-x86/linux/commit/c2585047c8e1 -- https://github.com/kvm-x86/linux/tree/next ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic 2024-03-08 4:13 ` [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Sean Christopherson @ 2024-03-08 10:44 ` Vitaly Kuznetsov 0 siblings, 0 replies; 10+ messages in thread From: Vitaly Kuznetsov @ 2024-03-08 10:44 UTC (permalink / raw) To: Sean Christopherson; +Cc: Li RongQing, linux-kernel, kvm, Paolo Bonzini Sean Christopherson <seanjc@google.com> writes: > On Wed, 28 Feb 2024 11:18:34 +0100, Vitaly Kuznetsov wrote: >> Guest hangs in specific configurations (KVM_X86_DISABLE_EXITS_HLT) are >> reported and the issue was bisected to commit ee3a5f9e3d9b ("KVM: x86: Do >> runtime CPUID update before updating vcpu->arch.cpuid_entries") which, of >> course, carries "No functional change intended" blurb. Turns out, moving >> __kvm_update_cpuid_runtime() earlier in kvm_set_cpuid() to tweak the >> incoming CPUID data before checking it wasn't innocent as >> KVM_FEATURE_PV_UNHALT reset logic relies on cached KVM CPUID base which >> gets updated later. >> >> [...] > > Applied to kvm-x86 hyperv. I won't send a pull request for this until next week, > but I do plan on landing it in 6.9. Holler if the selftests tweaks look wrong > (or you just don't like them). Looks great, thanks :-) I was also considering introducing 'vcpu_cpuid_has()' first but then I succumbed to my laziness and decided to postpone it until we have a *second* user in the tree). -- Vitaly ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-08 10:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-28 10:18 [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Vitaly Kuznetsov 2024-02-28 10:18 ` [PATCH 1/3] KVM: x86: Introduce __kvm_get_hypervisor_cpuid() helper Vitaly Kuznetsov 2024-02-28 10:18 ` [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT Vitaly Kuznetsov 2024-02-28 23:27 ` Sean Christopherson 2024-02-29 13:20 ` Vitaly Kuznetsov 2024-02-29 18:54 ` Sean Christopherson 2024-02-28 10:18 ` [PATCH 3/3] KVM: selftests: Check that KVM_FEATURE_PV_UNHALT is cleared with KVM_X86_DISABLE_EXITS_HLT Vitaly Kuznetsov 2024-03-08 4:13 ` Sean Christopherson 2024-03-08 4:13 ` [PATCH 0/3] KVM: x86: Fix KVM_FEATURE_PV_UNHALT update logic Sean Christopherson 2024-03-08 10:44 ` Vitaly Kuznetsov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).