On Mon, Oct 24, 2022, Vipin Sharma wrote:
> On Mon, Oct 24, 2022 at 8:22 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 24, 2022, Vitaly Kuznetsov wrote:
> > > While some 'extended' hypercalls may indeed need to be handled in KVM,
> > > there's no harm done in forwarding all unknown-to-KVM hypercalls to
> > > userspace. The only issue I envision is how would userspace discover
> > > which extended hypercalls are supported by KVM in case it (userspace) is
> > > responsible for handling HvExtCallQueryCapabilities call which returns
> > > the list of supported hypercalls. E.g. in case we decide to implement
> > > HvExtCallMemoryHeatHint in KVM, how are we going to communicate this to
> > > userspace?
> > >
> > > Normally, VMM discovers the availability of Hyper-V features through
> > > KVM_GET_SUPPORTED_HV_CPUID but extended hypercalls are not listed in
> > > CPUID. This can be always be solved by adding new KVM CAPs of
> > > course. Alternatively, we can add a single
> > > "KVM_CAP_HYPERV_EXT_CALL_QUERY" which will just return the list of
> > > extended hypercalls supported by KVM (which Vipin's patch adds anyway to
> > > *set* the list instead).
> >
> > AIUI, the TLFS uses a 64-bit mask to enumerate which extended hypercalls are
> > supported, so a single CAP should be a perfect fit. And KVM can use the capability
> > to enumerate support for _and_ to allow userspace to enable in-kernel handling. E.g.
> >
> > check():
> > case KVM_CAP_HYPERV_EXT_CALL:
> > return KVM_SUPPORTED_HYPERV_EXT_CALL;
> >
> >
> > enable():
> >
> > case KVM_CAP_HYPERV_EXT_CALL:
> > r = -EINVAL;
> > if (mask & ~KVM_SUPPORTED_HYPERV_EXT_CALL)
> > break;
> >
> > mutex_lock(&kvm->lock);
> > if (!kvm->created_vcpus) {
>
> Any reason for setting capability only after vcpus are created?
This only allows setting the capability _before_ vCPUs are created. Attempting
to set the cap after vCPUs are created gets rejected with -EINVAL. This
requirement means vCPUs don't need to take a lock to consume per-VM state, as KVM
prevents the state from changing once vCPUs are created.
> Also, in my patch I wrote the ioctl at kvm_vcpu_ioctl_enable_cap() as
> all of the hyperv related code was there but since this capability is
> a vm setting not a per vcpu setting, should this be at kvm_vm_ioctl()
> as a better choice?
Yep!
> > to_kvm_hv(kvm)->ext_call = cap->args[0];
> > r = 0;
> > }
> > mutex_unlock(&kvm->lock);
> >
> > kvm_hv_hypercall()
> >
> >
> > case HV_EXT_CALL_QUERY_CAPABILITIES ... HV_EXT_CALL_MAX:
> > if (unlikely(hc.fast)) {
> > ret = HV_STATUS_INVALID_PARAMETER;
> > break;
> > }
> > if (!(hc.code & to_kvm_hv(vcpu->kvm)->ext_call))
>
> It won't be directly this. There will be a mapping of hc.code to the
> corresponding bit and then "&" with ext_call.
>
>
> > goto hypercall_userspace_exit;
> >
> > ret = kvm_hv_ext_hypercall(...)
> > break;
> >
> >
> > That maintains backwards compatibility with "exit on everything" as userspace
> > still needs to opt-in to having KVM handle specific hypercalls in-kernel, and it
> > provides the necessary knob for userspace to tell KVM which hypercalls should be
> > allowed, i.e. ensures KVM doesn't violate HV_EXT_CALL_QUERY_CAPABILITIES.
>
> So, should I send a version with KVM capability similar to above
No, the above was just a sketch of how we can extend support if necessary. In
general, we try to avoid creating _any_ APIs before they are strictly required.
For uAPIs, that's pretty much a hard rule.
> or for now just send the version which by default exit to userspace and later
> whenever the need arises KVM capability can be added then?
This one please :-)
