On Thu, Aug 15, 2024, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@xxxxxxxxxx> writes: > > > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote: > >> Sean Christopherson <seanjc@xxxxxxxxxx> writes: > >> > >> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote: > >> >> What I meant is something along these lines (untested): > >> >> > >> >> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h > >> >> index eb48153bfd73..e2d8c67d0cad 100644 > >> >> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h > >> >> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h > >> >> @@ -104,6 +104,14 @@ static inline void evmcs_load(u64 phys_addr) > >> >> struct hv_vp_assist_page *vp_ap = > >> >> hv_get_vp_assist_page(smp_processor_id()); > >> >> > >> >> + /* > >> >> + * When enabling eVMCS, KVM verifies that every CPU has a valid hv_vp_assist_page() > >> >> + * and aborts enabling the feature otherwise. CPU onlining path is also checked in > >> >> + * vmx_hardware_enable(). With this, it is impossible to reach here with vp_ap == NULL > >> >> + * but compilers may still complain. > >> >> + */ > >> >> + BUG_ON(!vp_ap); > >> > > >> > A full BUG_ON() is overkill, and easily avoided. If we want to add a sanity > >> > check here and do more than just WARN, then it's easy enough to plumb in @vcpu > >> > and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk > >> > corrupting the guest somehow. > >> > > >> > >> I'm still acting under the impression this is an absolutely impossible > >> situation :-) > >> > >> AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't > >> have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the > >> plumbing (or am I missing an easy way to go back from @vmcs to > >> @vcpu?). On the other hand, vmcs_load() should not be called that ofter > >> so if we prefer to have @vcpu there for some other reason -- why not. > > > > kvm_get_running_vcpu(), though I honestly purposely didn't suggest it earlier > > because I am not a fan of using kvm_get_running_vcpu() unless it's absolutely > > necessary. But for this situation, I'd be fine with using it. > > Ah, nice, so we don't even need the plumbing then I guess? Compile-tested only: > > diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h > index eb48153bfd73..318f5f95f211 100644 > --- a/arch/x86/kvm/vmx/vmx_onhyperv.h > +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h > @@ -104,6 +104,19 @@ static inline void evmcs_load(u64 phys_addr) > struct hv_vp_assist_page *vp_ap = > hv_get_vp_assist_page(smp_processor_id()); > > + /* > + * When enabling eVMCS, KVM verifies that every CPU has a valid hv_vp_assist_page() > + * and aborts enabling the feature otherwise. CPU onlining path is also checked in > + * vmx_hardware_enable(). With this, it is impossible to reach here with vp_ap == NULL > + * but compilers may still complain. > + */ > + if (!vp_ap) { > + struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); > + > + KVM_BUG_ON(1, vcpu->kvm); > + return; Eh, I would just do: if (KVM_BUG_ON(!vp_ap, kvm_get_running_vcpu()->kvm)) return > + } > + > if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall) > vp_ap->nested_control.features.directhypercall = 1; > vp_ap->current_nested_vmcs = phys_addr; > > (I hope we can't reach here with kvm_running_vcpu unset, can we?) Yes? kvm_running_vcpu is set before kvm_arch_vcpu_load() and cleared after kvm_arch_vcpu_put(), and I can't think of a scenario where it would be legal/sane to invoke vmcs_load() without a running/loaded vCPU. VMX needs the current VMCS to be loaded to ensure guest state can be accessed, so any ioctl() that can touch guest state needs to do vcpu_load(). x86's kvm_arch_vcpu_ioctl() unconditionally does vcpu_load(), and the only ioctls I see in kvm_vcpu_ioctl() that _don't_ do vcpu_load() are KVM_SET_SIGNAL_MASK and KVM_GET_STATS_FD, so I think we're good. And if I'm wrong and the impossible happens twice, so be it, we die on #GP :-)