Re: [PATCH v1 4/5] kvm/x86: Hyper-V VMBus hypercall userspace exit

[Paolo Bonzini <pbonzini@redhat.com>]

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];
>  	};
>