On Sat, May 02, 2020 at 04:05:06AM +0300, Liran Alon wrote:
>
> On 01/05/2020 23:45, Sean Christopherson wrote:
> > On Fri, May 01, 2020 at 11:51:47AM -0700, Forrest Yuan Yu wrote:
> > > The purpose of this new hypercall is to exchange message between
> > > guest and hypervisor. For example, a guest may want to ask hypervisor
> > > to harden security by setting restricted access permission on guest
> > > SLAT entry. In this case, the guest can use this hypercall to send
> > >
> > > a message to the hypervisor which will do its job and send back
> > > anything it wants the guest to know.
> > Hrm, so this reintroduces KVM_EXIT_HYPERCALL without justifying _why_ it
> > needs to be reintroduced. I'm not familiar with the history, but the
> > comments in the documentation advise "use KVM_EXIT_IO or KVM_EXIT_MMIO".
> Both of these options have the disadvantage of requiring instruction
> emulation (Although a trivial one for PIO). Which enlarge host attack
> surface.
> I think this is one of the reasons why Hyper-V defined their PV devices
> (E.g. NetVSC/StorVSC) doorbell kick with hypercall instead of PIO/MMIO.
> (This is currently not much relevant as KVM's instruction emulator is not
> optional and is not even offloaded to host userspace. But relevant for the
> future...)
Good point. Again to me the simplicity of the hypercall interface really
makes it an ideal mechanism for message passing.
> >
> > Off the top of my head, IO and/or MMIO has a few advantages:
> >
> > - Allows the guest kernel to delegate permissions to guest userspace,
> > whereas KVM restrict hypercalls to CPL0.
> > - Allows "pass-through", whereas VMCALL is unconditionally forwarded to
> > L1.
> > - Is vendor agnostic, e.g. VMX and SVM recognized different opcodes for
> > VMCALL vs VMMCALL.
> I agree with all the above (I believe similar rational had led VMware to
> design their Backdoor PIO interface).
>
> Also note that recently AWS introduced Nitro Enclave PV device which is also
> de-facto a PV control-plane interface between guest and host userspace.
> Why similar approach couldn't have been used here?
> (Capability is exposed on a per-VM basis by attaching PV device to VM,
> communication interface is device specific and no KVM changes, only host
> userspace changes).
> > > Signed-off-by: Forrest Yuan Yu <yuanyu@xxxxxxxxxx>
> > > ---
> > > diff --git a/Documentation/virt/kvm/cpuid.rst b/Documentation/virt/kvm/cpuid.rst
> > > index 01b081f6e7ea..ff313f6827bf 100644
> > > --- a/Documentation/virt/kvm/cpuid.rst
> > > +++ b/Documentation/virt/kvm/cpuid.rst
> > > @@ -86,6 +86,9 @@ KVM_FEATURE_PV_SCHED_YIELD 13 guest checks this feature bit
> > > before using paravirtualized
> > > sched yield.
> > > +KVM_FEATURE_UCALL 14 guest checks this feature bit
> > > + before calling hypercall ucall.
> > Why make the UCALL a KVM CPUID feature? I can understand wanting to query
> > KVM support from host userspace, but presumably the guest will care about
> > capabiliteis built on top of the UCALL, not the UCALL itself.
> I agree with this.
> In case of PV device approach, device detection by guest will be the
> capability discovery.
> >
> > > +
> > > KVM_FEATURE_CLOCSOURCE_STABLE_BIT 24 host will warn if no guest-side
> > > per-cpu warps are expeced in
> > > kvmclock
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index c5835f9cb9ad..388a4f89464d 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -3385,6 +3385,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > case KVM_CAP_GET_MSR_FEATURES:
> > > case KVM_CAP_MSR_PLATFORM_INFO:
> > > case KVM_CAP_EXCEPTION_PAYLOAD:
> > > + case KVM_CAP_UCALL:
> > For whatever reason I have a metnal block with UCALL, can we call this
> > KVM_CAP_USERSPACE_HYPERCALL
> +1
> >
> > > r = 1;
> > > break;
> > > case KVM_CAP_SYNC_REGS:
> > > @@ -4895,6 +4896,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> > > kvm->arch.exception_payload_enabled = cap->args[0];
> > > r = 0;
> > > break;
> > > + case KVM_CAP_UCALL:
> > > + kvm->arch.hypercall_ucall_enabled = cap->args[0];
> > > + r = 0;
> > > + break;
> > > default:
> > > r = -EINVAL;
> > > break;
> > > @@ -7554,6 +7559,19 @@ static void kvm_sched_yield(struct kvm *kvm, unsigned long dest_id)
> > > kvm_vcpu_yield_to(target);
> > > }
> > > +static int complete_hypercall(struct kvm_vcpu *vcpu)
> > > +{
> > > + u64 ret = vcpu->run->hypercall.ret;
> > > +
> > > + if (!is_64_bit_mode(vcpu))
> > Do we really anticipate adding support in 32-bit guests? Honest question.
> >
> > > + ret = (u32)ret;
> > > + kvm_rax_write(vcpu, ret);
> > > +
> > > + ++vcpu->stat.hypercalls;
> > > +
> > > + return kvm_skip_emulated_instruction(vcpu);
> > > +}
> > > +
> > > int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > > {
> > > unsigned long nr, a0, a1, a2, a3, ret;
> > > @@ -7605,17 +7623,26 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > > kvm_sched_yield(vcpu->kvm, a0);
> > > ret = 0;
> > > break;
> > > + case KVM_HC_UCALL:
> > > + if (vcpu->kvm->arch.hypercall_ucall_enabled) {
> > > + vcpu->run->hypercall.nr = nr;
> > > + vcpu->run->hypercall.args[0] = a0;
> > > + vcpu->run->hypercall.args[1] = a1;
> > > + vcpu->run->hypercall.args[2] = a2;
> > > + vcpu->run->hypercall.args[3] = a3;
> > If performance is a justification for a more direct userspace exit, why
> > limit it to just four parameters? E.g. why not copy all registers to
> > kvm_sync_regs and reverse the process on the way back in?
> I don't think performance should be relevant for a hypercall interface. It's
> control-plane path.
> If a fast-path is required, guest should use this interface to coordinate a
> separate fast-path (e.g. via ring-buffer on some guest memory page).
>
> Anyway, these kind of questions is another reason why I agree with Sean it
> seems using a PV device is preferred.
I agree. Performance is not an issue here. However, I also don't think
this would make a PV device very suitable for a task like message
passing. When I try to imagine using a device to pass several bytes of
messages, it doesn't feel right ...
> Instead of forcing a general userspace hypercall interface standard, one
> could just implement whatever PV device it wants in host userspace which is
> device specific.
>
> In QEMU's VMPort implementation BTW, userspace calls cpu_synchronize_state()
> which de-facto syncs tons of vCPU state from KVM to userspace. Not just the
> GP registers.
> Because it's a slow-path, it's considered fine. :P
>
> -Liran
>
> >
> > > + vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
> > > + vcpu->arch.complete_userspace_io = complete_hypercall;
> > > + return 0; // message is going to userspace
> > > + }
> > > + ret = -KVM_ENOSYS;
> > > + break;
> > > default:
> > > ret = -KVM_ENOSYS;
> > > break;
> > > }
> > > out:
> > > - if (!op_64_bit)
> > > - ret = (u32)ret;
> > > - kvm_rax_write(vcpu, ret);
> > > -
> > > - ++vcpu->stat.hypercalls;
> > > - return kvm_skip_emulated_instruction(vcpu);
> > > + vcpu->run->hypercall.ret = ret;
> > > + return complete_hypercall(vcpu);
> > > }
> > > EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
