| Message ID | 1452595842-20880-5-git-send-email-asmetanin@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
Hello! > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC > event/message pages and to enable/disable SynIC messages/events processing > in userspace. > > + /* KVM_EXIT_HYPERV_HCALL */ > + struct { > + __u64 input; > + __u64 params[2]; > + __u64 result; > + } hv_hcall; > +Indicates that the VCPU exits into userspace to process some guest > +Hyper-V hypercalls. Why introducing this? We already have KVM_EXIT_HYPERCALL, which is exactly the same. AFAIK it's not used at the moment. Additionally, in theory we could have hypercalls handled in userspace for something else except HyperV. And not only for x86. Kind regards, Pavel Fedin Senior Engineer Samsung Electronics Research center Russia
On Thu, Jan 14, 2016 at 11:30:43AM +0300, Pavel Fedin wrote: > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC > > event/message pages and to enable/disable SynIC messages/events processing > > in userspace. > > > > + /* KVM_EXIT_HYPERV_HCALL */ > > + struct { > > + __u64 input; > > + __u64 params[2]; > > + __u64 result; > > + } hv_hcall; > > +Indicates that the VCPU exits into userspace to process some guest > > +Hyper-V hypercalls. > > Why introducing this? We already have KVM_EXIT_HYPERCALL, which is exactly the same. AFAIK it's not used at the moment. > Additionally, in theory we could have hypercalls handled in userspace for something else except HyperV. And not only for x86. We thought reusing KVM_EXIT_HYPERCALL was a bad idea exactly because of that. Hypercalls are not universal, the calling and return conventions are hypervisor-specific. KVM already has to make the decision that the particular vmexit is a HyperV hypercall; it appears unnatural to then pass the data on to userspace in a generic structure and have them make that decision again. Roman.
Hello! > We thought reusing KVM_EXIT_HYPERCALL was a bad idea exactly because of > that. Hypercalls are not universal, the calling and return conventions > are hypervisor-specific. Treatment of them is hypervisor-specific, but from CPUs point of view they are the same. You load something into registers, and execute hypercall instruction. So, you just need to pass registers in your structure. Or, you could even use generic register access APIs. > KVM already has to make the decision that the > particular vmexit is a HyperV hypercall; it appears unnatural to then > pass the data on to userspace in a generic structure and have them make > that decision again. Is it so difficult to make such a decision? The userland already knows what we are emulating. I'm afraid that in future we can end up in having 10 versions of KVM_EXIT_xxx_HYPERCALL with very small difference between them. Will it be good? Kind regards, Pavel Fedin Senior Engineer Samsung Electronics Research center Russia
On Thu, Jan 14, 2016 at 01:50:20PM +0300, Pavel Fedin wrote: > > We thought reusing KVM_EXIT_HYPERCALL was a bad idea exactly because of > > that. Hypercalls are not universal, the calling and return conventions > > are hypervisor-specific. > > Treatment of them is hypervisor-specific, but from CPUs point of view they are the same. You load something into registers, and > execute hypercall instruction. ... and you get something in the registers on return (all that is from guest POV). Right. The differences are, however, which registers are used for parameters and return values, how they are encoded, etc. > So, you just need to pass registers in your structure. Which ones? This is going to be hypervisor- and arch-specific. > Or, you could even use generic register access APIs. With an extra ioctl? This is going to be expensive. > > KVM already has to make the decision that the > > particular vmexit is a HyperV hypercall; it appears unnatural to then > > pass the data on to userspace in a generic structure and have them make > > that decision again. > > Is it so difficult to make such a decision? The userland already knows what we are emulating. It isn't terribly difficult, but KVM has to do all that already, as it handles some of the hypercalls in the kernel. I don't think duplicating this (and also designing a generic marshalling scheme) is profitable compared to saving a vcpu exit code. > I'm afraid that in future we can end up in having 10 versions of KVM_EXIT_xxx_HYPERCALL with very small difference between them. > Will it be good? I don't see it to be a problem. Roman.
On 12/01/2016 11:50, Andrey Smetanin wrote: > The patch implements KVM_EXIT_HV_HCALL functionality > for Hyper-V VMBus hypercalls: HV_X64_HCALL_POST_MESSAGE, > HV_X64_HCALL_SIGNAL_EVENT. > > Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > CC: Gleb Natapov <gleb@kernel.org> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Joerg Roedel <joro@8bytes.org> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Haiyang Zhang <haiyangz@microsoft.com> > CC: Roman Kagan <rkagan@virtuozzo.com> > CC: Denis V. Lunev <den@openvz.org> > CC: qemu-devel@nongnu.org > --- > Documentation/virtual/kvm/api.txt | 8 ++++++++ > arch/x86/kvm/hyperv.c | 28 +++++++++++++++++++++------- > arch/x86/kvm/hyperv.h | 1 + > arch/x86/kvm/x86.c | 3 +++ > include/uapi/linux/kvm.h | 7 +++++++ > 5 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 053f613..23d4b9d 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC > event/message pages and to enable/disable SynIC messages/events processing > in userspace. > > + /* KVM_EXIT_HYPERV_HCALL */ > + struct { > + __u64 input; > + __u64 params[2]; > + __u64 result; > + } hv_hcall; > +Indicates that the VCPU exits into userspace to process some guest > +Hyper-V hypercalls. Is this something other than KVM_EXIT_HYPERV because of the previous discussion about MSRs? Otherwise, it definitely should be a separate type of the existing KVM_EXIT_HYPERV exit. (In fact, I do not think anymore that I will add the MSR exit in 4.5. It's kind of against the KVM design to do "CPU things" outside the kernel. My conjecture is that MSRs are either simple and thus not security sensitive, or complicated and thus require state that resides in the kernel). > /* Fix the size of the union. */ > char padding[256]; > }; > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 0e7c90f..76c9ec4 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -1087,18 +1087,32 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > case HV_X64_HCALL_NOTIFY_LONG_SPIN_WAIT: > kvm_vcpu_on_spin(vcpu); > break; > + case HV_X64_HCALL_POST_MESSAGE: > + case HV_X64_HCALL_SIGNAL_EVENT: > + vcpu->run->exit_reason = KVM_EXIT_HYPERV_HCALL; > + vcpu->run->hv_hcall.input = param; > + vcpu->run->hv_hcall.params[0] = ingpa; > + vcpu->run->hv_hcall.params[1] = outgpa; > + return 0; > default: > res = HV_STATUS_INVALID_HYPERCALL_CODE; > break; > } > > ret = res | (((u64)rep_done & 0xfff) << 32); > - if (longmode) { > - kvm_register_write(vcpu, VCPU_REGS_RAX, ret); > - } else { > - kvm_register_write(vcpu, VCPU_REGS_RDX, ret >> 32); > - kvm_register_write(vcpu, VCPU_REGS_RAX, ret & 0xffffffff); > - } > - > + kvm_hv_hypercall_set_result(vcpu, ret); > return 1; > } > + > +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result) > +{ > + bool longmode; > + > + longmode = is_64_bit_mode(vcpu); > + if (longmode) > + kvm_register_write(vcpu, VCPU_REGS_RAX, result); > + else { > + kvm_register_write(vcpu, VCPU_REGS_RDX, result >> 32); > + kvm_register_write(vcpu, VCPU_REGS_RAX, result & 0xffffffff); > + } > +} > diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h > index 60eccd4..64a4a3b 100644 > --- a/arch/x86/kvm/hyperv.h > +++ b/arch/x86/kvm/hyperv.h > @@ -52,6 +52,7 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata); > > bool kvm_hv_hypercall_enabled(struct kvm *kvm); > int kvm_hv_hypercall(struct kvm_vcpu *vcpu); > +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result); > > void kvm_hv_irq_routing_update(struct kvm *kvm); > int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index fad1d09..6ad3599 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6886,6 +6886,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > } else > WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed); > > + if (unlikely(kvm_run->exit_reason == KVM_EXIT_HYPERV_HCALL)) > + kvm_hv_hypercall_set_result(vcpu, kvm_run->hv_hcall.result); > + > r = vcpu_run(vcpu); > > out: > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 9da9051..a62c4fb 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -199,6 +199,7 @@ struct kvm_hyperv_exit { > #define KVM_EXIT_S390_STSI 25 > #define KVM_EXIT_IOAPIC_EOI 26 > #define KVM_EXIT_HYPERV 27 > +#define KVM_EXIT_HYPERV_HCALL 28 > > /* For KVM_EXIT_INTERNAL_ERROR */ > /* Emulate instruction failed. */ > @@ -355,6 +356,12 @@ struct kvm_run { > } eoi; > /* KVM_EXIT_HYPERV */ > struct kvm_hyperv_exit hyperv; > + /* KVM_EXIT_HYPERV_HCALL */ > + struct { > + __u64 input; > + __u64 params[2]; > + __u64 result; Please put result before params, so that it's possible to add more parameters in the future. Paolo > + } hv_hcall; > /* Fix the size of the union. */ > char padding[256]; > }; >
On 14/01/2016 09:30, Pavel Fedin wrote: > Hello! > >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC >> event/message pages and to enable/disable SynIC messages/events processing >> in userspace. >> >> + /* KVM_EXIT_HYPERV_HCALL */ >> + struct { >> + __u64 input; >> + __u64 params[2]; >> + __u64 result; >> + } hv_hcall; >> +Indicates that the VCPU exits into userspace to process some guest >> +Hyper-V hypercalls. > > Why introducing this? We already have KVM_EXIT_HYPERCALL, which is > exactly the same. AFAIK it's not used at the moment. > Additionally, in theory we could have hypercalls handled in > userspace for something else except HyperV. And not only for x86. Because, as the docs say, we don't want to do that. We want to use KVM_EXIT_IO or KVM_EXIT_MMIO, with two exceptions: s390 and wherever we can't do that for compatibility purposes. This is the latter. So we should not add a new exit (I suggested elsewhere the existing hyper-v exit) but we shouldn't use KVM_EXIT_HYPERCALL either. There is a precedent in fact of using "something else than KVM_EXIT_HYPERCALL" for interoperability/compatibility hypercalls, namely KVM_EXIT_PAPR_HCALL. Paolo
Hello! > Because, as the docs say, we don't want to do that. Where do they say this? I remember that "we want to use KVM_EXIT_IO or KVM_EXIT_MMIO for handling device I/O". In other words - we should not introduce anything that requires any other mechanism (e. g. hypercalls) to handle this. And that's all. By the way, Hyper-V implementation contradicts this rule by itself. And i wonder why we need it at all, but, well, why not. So, Hyper-V violates this rule only because it was designed by other people under different rules. And we have to follow these rules. So we have to support hypercalls. And hypercalls cannot map to EXIT_IO or EXIT_MMIO, so... > We want to use > KVM_EXIT_IO or KVM_EXIT_MMIO, with two exceptions: s390 and wherever we > can't do that for compatibility purposes. This is the latter. Yes, basically this is what i told above... > So we > should not add a new exit (I suggested elsewhere the existing hyper-v > exit) Yes, this would also be more consistent i think, if we think subsystem-centric ("we are implementing Hyper-V") instead of implementation-centric ("we are implementing hypercalls"). Kind regards, Pavel Fedin Senior Engineer Samsung Electronics Research center Russia
On Wed, Jan 20, 2016 at 02:59:08PM +0100, Paolo Bonzini wrote: > >> --- a/Documentation/virtual/kvm/api.txt > >> +++ b/Documentation/virtual/kvm/api.txt > >> @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC > >> event/message pages and to enable/disable SynIC messages/events processing > >> in userspace. > >> > >> + /* KVM_EXIT_HYPERV_HCALL */ > >> + struct { > >> + __u64 input; > >> + __u64 params[2]; > >> + __u64 result; > >> + } hv_hcall; > >> +Indicates that the VCPU exits into userspace to process some guest > >> +Hyper-V hypercalls. > > > > Why introducing this? We already have KVM_EXIT_HYPERCALL, which is > > exactly the same. AFAIK it's not used at the moment. > > Additionally, in theory we could have hypercalls handled in > > userspace for something else except HyperV. And not only for x86. > > Because, as the docs say, we don't want to do that. We want to use > KVM_EXIT_IO or KVM_EXIT_MMIO, with two exceptions: s390 and wherever we > can't do that for compatibility purposes. I must admit I saw this part in the docs but failed to understand it. Is it supposed to mean that we want to avoid using hypercalls as a means of guest communications with the host userspace in general, and use PIO or MMIO instead, unless hypercalls are mandated by a guest implemenation we can't affect (as is the case for Hyper-V)? Because *implementing* Hyper-V hypercalls in terms of KVM_EXIT_IO or KVM_EXIT_MMIO looked hard at best. > So we should not add a new exit Why? VCPU exit codes are not a scarse resource. > (I suggested elsewhere the existing hyper-v exit) So far we've envisaged two reasons for VCPU exit related to hyper-v: one for hyper-v MSRs and the other for hypercalls. Since there was a discussion on implementing generic MSR access by Peter we thought it wiser to introduce a new VCPU exit for hyper-v hypercalls to avoid interfering with the MSR implementation. Thanks, Roman.
On Wed, Jan 20, 2016 at 02:43:01PM +0100, Paolo Bonzini wrote: > > > On 12/01/2016 11:50, Andrey Smetanin wrote: > > The patch implements KVM_EXIT_HV_HCALL functionality > > for Hyper-V VMBus hypercalls: HV_X64_HCALL_POST_MESSAGE, > > HV_X64_HCALL_SIGNAL_EVENT. > > > > Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com> > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > > CC: Gleb Natapov <gleb@kernel.org> > > CC: Paolo Bonzini <pbonzini@redhat.com> > > CC: Joerg Roedel <joro@8bytes.org> > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > CC: Haiyang Zhang <haiyangz@microsoft.com> > > CC: Roman Kagan <rkagan@virtuozzo.com> > > CC: Denis V. Lunev <den@openvz.org> > > CC: qemu-devel@nongnu.org > > --- > > Documentation/virtual/kvm/api.txt | 8 ++++++++ > > arch/x86/kvm/hyperv.c | 28 +++++++++++++++++++++------- > > arch/x86/kvm/hyperv.h | 1 + > > arch/x86/kvm/x86.c | 3 +++ > > include/uapi/linux/kvm.h | 7 +++++++ > > 5 files changed, 40 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > > index 053f613..23d4b9d 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC > > event/message pages and to enable/disable SynIC messages/events processing > > in userspace. > > > > + /* KVM_EXIT_HYPERV_HCALL */ > > + struct { > > + __u64 input; > > + __u64 params[2]; > > + __u64 result; > > + } hv_hcall; > > +Indicates that the VCPU exits into userspace to process some guest > > +Hyper-V hypercalls. > > Is this something other than KVM_EXIT_HYPERV because of the previous > discussion about MSRs? Otherwise, it definitely should be a separate > type of the existing KVM_EXIT_HYPERV exit. Mostly. Besides, it wasn't obvious why we needed another level of dispatch. > (In fact, I do not think anymore that I will add the MSR exit in 4.5. > It's kind of against the KVM design to do "CPU things" outside the > kernel. My conjecture is that MSRs are either simple and thus not > security sensitive, or complicated and thus require state that resides > in the kernel). > > > /* Fix the size of the union. */ > > char padding[256]; > > }; > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > index 0e7c90f..76c9ec4 100644 > > --- a/arch/x86/kvm/hyperv.c > > +++ b/arch/x86/kvm/hyperv.c > > @@ -1087,18 +1087,32 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > > case HV_X64_HCALL_NOTIFY_LONG_SPIN_WAIT: > > kvm_vcpu_on_spin(vcpu); > > break; > > + case HV_X64_HCALL_POST_MESSAGE: > > + case HV_X64_HCALL_SIGNAL_EVENT: > > + vcpu->run->exit_reason = KVM_EXIT_HYPERV_HCALL; > > + vcpu->run->hv_hcall.input = param; > > + vcpu->run->hv_hcall.params[0] = ingpa; > > + vcpu->run->hv_hcall.params[1] = outgpa; > > + return 0; > > default: > > res = HV_STATUS_INVALID_HYPERCALL_CODE; > > break; > > } > > > > ret = res | (((u64)rep_done & 0xfff) << 32); > > - if (longmode) { > > - kvm_register_write(vcpu, VCPU_REGS_RAX, ret); > > - } else { > > - kvm_register_write(vcpu, VCPU_REGS_RDX, ret >> 32); > > - kvm_register_write(vcpu, VCPU_REGS_RAX, ret & 0xffffffff); > > - } > > - > > + kvm_hv_hypercall_set_result(vcpu, ret); > > return 1; > > } > > + > > +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result) > > +{ > > + bool longmode; > > + > > + longmode = is_64_bit_mode(vcpu); > > + if (longmode) > > + kvm_register_write(vcpu, VCPU_REGS_RAX, result); > > + else { > > + kvm_register_write(vcpu, VCPU_REGS_RDX, result >> 32); > > + kvm_register_write(vcpu, VCPU_REGS_RAX, result & 0xffffffff); > > + } > > +} > > diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h > > index 60eccd4..64a4a3b 100644 > > --- a/arch/x86/kvm/hyperv.h > > +++ b/arch/x86/kvm/hyperv.h > > @@ -52,6 +52,7 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata); > > > > bool kvm_hv_hypercall_enabled(struct kvm *kvm); > > int kvm_hv_hypercall(struct kvm_vcpu *vcpu); > > +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result); > > > > void kvm_hv_irq_routing_update(struct kvm *kvm); > > int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint); > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index fad1d09..6ad3599 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -6886,6 +6886,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > > } else > > WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed); > > > > + if (unlikely(kvm_run->exit_reason == KVM_EXIT_HYPERV_HCALL)) > > + kvm_hv_hypercall_set_result(vcpu, kvm_run->hv_hcall.result); > > + > > r = vcpu_run(vcpu); > > > > out: > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > > index 9da9051..a62c4fb 100644 > > --- a/include/uapi/linux/kvm.h > > +++ b/include/uapi/linux/kvm.h > > @@ -199,6 +199,7 @@ struct kvm_hyperv_exit { > > #define KVM_EXIT_S390_STSI 25 > > #define KVM_EXIT_IOAPIC_EOI 26 > > #define KVM_EXIT_HYPERV 27 > > +#define KVM_EXIT_HYPERV_HCALL 28 > > > > /* For KVM_EXIT_INTERNAL_ERROR */ > > /* Emulate instruction failed. */ > > @@ -355,6 +356,12 @@ struct kvm_run { > > } eoi; > > /* KVM_EXIT_HYPERV */ > > struct kvm_hyperv_exit hyperv; > > + /* KVM_EXIT_HYPERV_HCALL */ > > + struct { > > + __u64 input; > > + __u64 params[2]; > > + __u64 result; > > Please put result before params, so that it's possible to add more > parameters in the future. The number of parameters is fixed in the Hyper-V spec: they are either physical addresses of the input and output parameter structures, or (for "fast" hypercalls) up to two input parameters. That said, there's also another variant called "extended fast hypercalls" when extra parameters are placed in XMM registers so if we ever decide to implement those we'll need another field in this structure, so it indeed looks sensible to swap result and params now. Thanks, Roman.
On 20/01/2016 16:20, 'Roman Kagan' wrote: >> > Because, as the docs say, we don't want to do that. We want to use >> > KVM_EXIT_IO or KVM_EXIT_MMIO, with two exceptions: s390 and wherever we >> > can't do that for compatibility purposes. > I must admit I saw this part in the docs but failed to understand it. > > Is it supposed to mean that we want to avoid using hypercalls as a means > of guest communications with the host userspace in general, and use PIO > or MMIO instead, unless hypercalls are mandated by a guest implemenation > we can't affect (as is the case for Hyper-V)? In this case we use hypercalls. :) However, we use a separate exit instead of putting everything under KVM_EXIT_HYPERCALL. This, as you mentioned, has the advantage that you can customize it according to the calling convention (which may help avoiding TOCTTOU issues, too). > Because *implementing* Hyper-V hypercalls in terms of KVM_EXIT_IO or > KVM_EXIT_MMIO looked hard at best. > >> > So we should not add a new exit > Why? VCPU exit codes are not a scarse resource. Indeed, but grouping makes things easier to understand. > So far we've envisaged two reasons for VCPU exit related to hyper-v: one > for hyper-v MSRs and the other for hypercalls. Since there was a > discussion on implementing generic MSR access by Peter we thought it > wiser to introduce a new VCPU exit for hyper-v hypercalls to avoid > interfering with the MSR implementation. That's a good idea. However, I think I'm not going to accept the MSR exit feature, and then the current Hyper-V exit API makes some sense indeed (it's just 3 values, transferring them all at once is not expensive at all). Paolo
On Wed, Jan 20, 2016 at 06:02:25PM +0100, Paolo Bonzini wrote: > > > On 20/01/2016 16:20, 'Roman Kagan' wrote: > >> > So we should not add a new exit > > Why? VCPU exit codes are not a scarse resource. > > Indeed, but grouping makes things easier to understand. > > > So far we've envisaged two reasons for VCPU exit related to hyper-v: one > > for hyper-v MSRs and the other for hypercalls. Since there was a > > discussion on implementing generic MSR access by Peter we thought it > > wiser to introduce a new VCPU exit for hyper-v hypercalls to avoid > > interfering with the MSR implementation. > > That's a good idea. However, I think I'm not going to accept the MSR > exit feature, and then the current Hyper-V exit API makes some sense > indeed (it's just 3 values, transferring them all at once is not > expensive at all). OK can we please sum up (as I'm now a bit confused) what we do now: 1) use a single vcpu exit for both Hyper-V cases (which implies we need to fix this patchset to add the subcode for hypercalls) or 2) use individual vcpu exits for Hyper-V MSRs and for Hyper-V hypercalls (which implies we need to submit an incremental patch dropping the subcode from Hyper-V MSR exit and renaming it to better describe the reality)? Thanks, Roman.
On 20/01/2016 18:31, 'Roman Kagan' wrote: > On Wed, Jan 20, 2016 at 06:02:25PM +0100, Paolo Bonzini wrote: >> >> >> On 20/01/2016 16:20, 'Roman Kagan' wrote: >>>>> So we should not add a new exit >>> Why? VCPU exit codes are not a scarse resource. >> >> Indeed, but grouping makes things easier to understand. >> >>> So far we've envisaged two reasons for VCPU exit related to hyper-v: one >>> for hyper-v MSRs and the other for hypercalls. Since there was a >>> discussion on implementing generic MSR access by Peter we thought it >>> wiser to introduce a new VCPU exit for hyper-v hypercalls to avoid >>> interfering with the MSR implementation. >> >> That's a good idea. However, I think I'm not going to accept the MSR >> exit feature, and then the current Hyper-V exit API makes some sense >> indeed (it's just 3 values, transferring them all at once is not >> expensive at all). > > OK can we please sum up (as I'm now a bit confused) what we do now: > > 1) use a single vcpu exit for both Hyper-V cases (which implies we need > to fix this patchset to add the subcode for hypercalls) This. Paolo > or > > 2) use individual vcpu exits for Hyper-V MSRs and for Hyper-V hypercalls > (which implies we need to submit an incremental patch dropping the > subcode from Hyper-V MSR exit and renaming it to better describe the > reality)? > > Thanks, > Roman. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 053f613..23d4b9d 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to remap SynIC event/message pages and to enable/disable SynIC messages/events processing in userspace. + /* KVM_EXIT_HYPERV_HCALL */ + struct { + __u64 input; + __u64 params[2]; + __u64 result; + } hv_hcall; +Indicates that the VCPU exits into userspace to process some guest +Hyper-V hypercalls. /* Fix the size of the union. */ char padding[256]; }; diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 0e7c90f..76c9ec4 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1087,18 +1087,32 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) case HV_X64_HCALL_NOTIFY_LONG_SPIN_WAIT: kvm_vcpu_on_spin(vcpu); break; + case HV_X64_HCALL_POST_MESSAGE: + case HV_X64_HCALL_SIGNAL_EVENT: + vcpu->run->exit_reason = KVM_EXIT_HYPERV_HCALL; + vcpu->run->hv_hcall.input = param; + vcpu->run->hv_hcall.params[0] = ingpa; + vcpu->run->hv_hcall.params[1] = outgpa; + return 0; default: res = HV_STATUS_INVALID_HYPERCALL_CODE; break; } ret = res | (((u64)rep_done & 0xfff) << 32); - if (longmode) { - kvm_register_write(vcpu, VCPU_REGS_RAX, ret); - } else { - kvm_register_write(vcpu, VCPU_REGS_RDX, ret >> 32); - kvm_register_write(vcpu, VCPU_REGS_RAX, ret & 0xffffffff); - } - + kvm_hv_hypercall_set_result(vcpu, ret); return 1; } + +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result) +{ + bool longmode; + + longmode = is_64_bit_mode(vcpu); + if (longmode) + kvm_register_write(vcpu, VCPU_REGS_RAX, result); + else { + kvm_register_write(vcpu, VCPU_REGS_RDX, result >> 32); + kvm_register_write(vcpu, VCPU_REGS_RAX, result & 0xffffffff); + } +} diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h index 60eccd4..64a4a3b 100644 --- a/arch/x86/kvm/hyperv.h +++ b/arch/x86/kvm/hyperv.h @@ -52,6 +52,7 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata); bool kvm_hv_hypercall_enabled(struct kvm *kvm); int kvm_hv_hypercall(struct kvm_vcpu *vcpu); +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result); void kvm_hv_irq_routing_update(struct kvm *kvm); int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index fad1d09..6ad3599 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6886,6 +6886,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } else WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed); + if (unlikely(kvm_run->exit_reason == KVM_EXIT_HYPERV_HCALL)) + kvm_hv_hypercall_set_result(vcpu, kvm_run->hv_hcall.result); + r = vcpu_run(vcpu); out: diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 9da9051..a62c4fb 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -199,6 +199,7 @@ struct kvm_hyperv_exit { #define KVM_EXIT_S390_STSI 25 #define KVM_EXIT_IOAPIC_EOI 26 #define KVM_EXIT_HYPERV 27 +#define KVM_EXIT_HYPERV_HCALL 28 /* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -355,6 +356,12 @@ struct kvm_run { } eoi; /* KVM_EXIT_HYPERV */ struct kvm_hyperv_exit hyperv; + /* KVM_EXIT_HYPERV_HCALL */ + struct { + __u64 input; + __u64 params[2]; + __u64 result; + } hv_hcall; /* Fix the size of the union. */ char padding[256]; };