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 :-)
