Re: [PATCH v2 2/2] kvm:x86:hyperv: guest->host event signaling via eventfd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07.12.2017 15:18, Roman Kagan wrote:
> In Hyper-V, the fast guest->host notification mechanism is the
> SIGNAL_EVENT hypercall, with a single parameter of the connection ID to
> signal.
> 
> Currently this hypercall incurs a user exit and requires the userspace
> to decode the parameters and trigger the notification of the potentially
> different I/O context.
> 
> To avoid the costly user exit, process this hypercall and signal the
> corresponding eventfd in KVM, similar to ioeventfd.  The association
> between the connection id and the eventfd is established via the newly
> introduced KVM_HYPERV_EVENTFD ioctl, and maintained in an
> (srcu-protected) IDR.
> 
> Signed-off-by: Roman Kagan <rkagan@xxxxxxxxxxxxx>
> ---
> v1 -> v2:
>  - make data types consistent
>  - get by without the recently dropped struct hv_input_signal_event
>  - fix subject prefix
> 
>  Documentation/virtual/kvm/api.txt |  23 ++++++++
>  arch/x86/include/asm/kvm_host.h   |   2 +
>  arch/x86/kvm/hyperv.h             |   1 +
>  include/uapi/linux/kvm.h          |  13 +++++
>  arch/x86/kvm/hyperv.c             | 115 +++++++++++++++++++++++++++++++++++++-
>  arch/x86/kvm/x86.c                |  10 ++++
>  6 files changed, 163 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index f670e4b9e7f3..e4f319add8b7 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3394,6 +3394,29 @@ invalid, if invalid pages are written to (e.g. after the end of memory)
>  or if no page table is present for the addresses (e.g. when using
>  hugepages).
>  
> +4.109 KVM_HYPERV_EVENTFD
> +
> +Capability: KVM_CAP_HYPERV_EVENTFD
> +Architectures: x86
> +Type: vm ioctl
> +Parameters: struct kvm_hyperv_eventfd (in)
> +Returns: 0 on success, !0 on error
> +
> +This ioctl (un)registers an eventfd to receive notifications from the guest on
> +the specified Hyper-V connection id through the SIGNAL_EVENT hypercall, without
> +causing a user exit.
> +
> +struct kvm_hyperv_eventfd {
> +	__u32 conn_id;
> +	__s32 fd;
> +	__u32 flags;
> +	__u32 padding[3];
> +};
> +
> +The acceptable values for the flags field:
> +
> +#define KVM_HYPERV_EVENTFD_DEASSIGN	(1 << 0)

Do you also want to document the conn_id valid values? And for both
cases, what happens if other values are provided. (-EINVALID)

> +
>  5. The kvm_run structure
>  ------------------------
>  
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 977de5fb968b..d2d318540c87 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -739,6 +739,8 @@ struct kvm_hv {
>  	u64 hv_crash_ctl;
>  
>  	HV_REFERENCE_TSC_PAGE tsc_ref;
> +
> +	struct idr conn_to_evt;
>  };
>  
>  enum kvm_irqchip_mode {
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index cc2468244ca2..837465d69c6d 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -90,5 +90,6 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>  
>  void kvm_hv_init_vm(struct kvm *kvm);
>  void kvm_hv_destroy_vm(struct kvm *kvm);
> +int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args);
>  
>  #endif
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 282d7613fce8..465f45c13cdc 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -932,6 +932,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_HYPERV_SYNIC2 148
>  #define KVM_CAP_HYPERV_VP_INDEX 149
>  #define KVM_CAP_S390_AIS_MIGRATION 150
> +#define KVM_CAP_HYPERV_EVENTFD 151
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -1359,6 +1360,8 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_GET_CMMA_BITS      _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
>  
> +#define KVM_HYPERV_EVENTFD	_IOW(KVMIO,  0xba, struct kvm_hyperv_eventfd)
> +
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>  #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
>  #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
> @@ -1419,4 +1422,14 @@ struct kvm_assigned_msix_entry {
>  #define KVM_ARM_DEV_EL1_PTIMER		(1 << 1)
>  #define KVM_ARM_DEV_PMU			(1 << 2)
>  
> +struct kvm_hyperv_eventfd {
> +	__u32 conn_id;
> +	__s32 fd;
> +	__u32 flags;
> +	__u32 padding[3];
> +};
> +
> +#define KVM_HYPERV_CONN_ID_BITS		24

You could directly use/introduce KVM_HYPERV_CONN_ID_MASK instead, so no
need to do calculations with this value below.

> +#define KVM_HYPERV_EVENTFD_DEASSIGN	(1 << 0)
> +
>  #endif /* __LINUX_KVM_H */
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 015fb06c7522..3b5d4203696b 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -29,6 +29,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/highmem.h>
>  #include <linux/sched/cputime.h>
> +#include <linux/eventfd.h>
>  
>  #include <asm/apicdef.h>
>  #include <trace/events/kvm.h>
> @@ -1226,6 +1227,54 @@ static int kvm_hv_hypercall_complete_userspace(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +static u16 hvcall_sigevent_param(struct kvm_vcpu *vcpu, gpa_t gpa, u64 *param)
> +{
> +	struct page *page;
> +	void *pg;
> +	u64 *msg;
> +
> +	if ((gpa & (__alignof__(*msg) - 1)) ||
> +	    offset_in_page(gpa) + sizeof(*msg) > PAGE_SIZE)
> +		return HV_STATUS_INVALID_ALIGNMENT;
> +
> +	page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT);
> +	if (is_error_page(page))
> +		return HV_STATUS_INSUFFICIENT_MEMORY;
> +
> +	pg = kmap_atomic(page);
> +	msg = pg + offset_in_page(gpa);
> +	*param = *msg;
> +	kunmap_atomic(pg);
> +	return HV_STATUS_SUCCESS;
> +}
> +
> +static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, bool fast, u64 param)
> +{
> +	u16 ret;
> +	u32 conn_id;
> +	int idx;
> +	struct eventfd_ctx *eventfd;
> +
> +	if (unlikely(!fast)) {
> +		u64 gpa = param;
> +		ret = hvcall_sigevent_param(vcpu, gpa, &param);
> +		if (ret != HV_STATUS_SUCCESS)
> +			return ret;
> +	}
> +
> +	conn_id = (param & 0xffffffff) + ((param >> 32) & 0xffff);
> +	if (conn_id & ~((1 << KVM_HYPERV_CONN_ID_BITS) - 1))
> +		return HV_STATUS_INVALID_CONNECTION_ID;
> +
> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> +	eventfd = idr_find(&vcpu->kvm->arch.hyperv.conn_to_evt, conn_id);
> +	if (eventfd)
> +		eventfd_signal(eventfd, 1);
> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +
> +	return eventfd ? HV_STATUS_SUCCESS : HV_STATUS_INVALID_CONNECTION_ID;
> +}
> +
>  int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>  {
>  	u64 param, ingpa, outgpa, ret;
> @@ -1276,8 +1325,12 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>  	case HVCALL_NOTIFY_LONG_SPIN_WAIT:
>  		kvm_vcpu_on_spin(vcpu, true);
>  		break;
> -	case HVCALL_POST_MESSAGE:
>  	case HVCALL_SIGNAL_EVENT:
> +		res = kvm_hvcall_signal_event(vcpu, fast, ingpa);
> +		if (res != HV_STATUS_INVALID_CONNECTION_ID)
> +			break;
> +		/* maybe userspace knows this conn_id: fall through */
> +	case HVCALL_POST_MESSAGE:
>  		/* don't bother userspace if it has no way to handle it */
>  		if (!vcpu_to_synic(vcpu)->active) {
>  			res = HV_STATUS_INVALID_HYPERCALL_CODE;
> @@ -1305,8 +1358,68 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>  void kvm_hv_init_vm(struct kvm *kvm)
>  {
>  	mutex_init(&kvm->arch.hyperv.hv_lock);
> +	idr_init(&kvm->arch.hyperv.conn_to_evt);
>  }
>  
>  void kvm_hv_destroy_vm(struct kvm *kvm)
>  {
> +	int i;

I'd declare i last.

> +	struct eventfd_ctx *eventfd;
> +
> +	idr_for_each_entry(&kvm->arch.hyperv.conn_to_evt, eventfd, i)
> +		eventfd_ctx_put(eventfd);
> +	idr_destroy(&kvm->arch.hyperv.conn_to_evt);
> +}
> +
> +static int kvm_hv_eventfd_assign(struct kvm *kvm, u32 conn_id, int fd)
> +{
> +	int ret;

dito for ret.

> +	struct eventfd_ctx *eventfd;
> +	struct kvm_hv *hv = &kvm->arch.hyperv;
> +
> +	eventfd = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(eventfd))
> +		return PTR_ERR(eventfd);
> +
> +	mutex_lock(&hv->hv_lock);
> +	ret = idr_alloc(&hv->conn_to_evt, eventfd, conn_id, conn_id + 1,
> +			GFP_KERNEL);
> +	mutex_unlock(&hv->hv_lock);
> +
> +	if (ret >= 0)
> +		return 0;
> +
> +	if (ret == -ENOSPC)
> +		ret = -EEXIST;
> +	eventfd_ctx_put(eventfd);
> +	return ret;
> +}
> +
> +static int kvm_hv_eventfd_deassign(struct kvm *kvm, u32 conn_id)
> +{
> +	int ret;

dito

> +	struct eventfd_ctx *eventfd;
> +	struct kvm_hv *hv = &kvm->arch.hyperv;
> +
> +	mutex_lock(&hv->hv_lock);
> +	eventfd = idr_remove(&hv->conn_to_evt, conn_id);
> +	mutex_unlock(&hv->hv_lock);
> +
> +	if (!eventfd)
> +		return -ENOENT;
> +
> +	synchronize_srcu(&kvm->srcu);
> +	eventfd_ctx_put(eventfd);
> +	return ret;
> +}
> +
> +int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args)
> +{
> +	if ((args->flags & ~KVM_HYPERV_EVENTFD_DEASSIGN) ||
> +	    (args->conn_id & ~((1 << KVM_HYPERV_CONN_ID_BITS) - 1)))
> +		return -EINVAL;
> +
> +	return args->flags == KVM_HYPERV_EVENTFD_DEASSIGN ?
> +		kvm_hv_eventfd_deassign(kvm, args->conn_id) :
> +		kvm_hv_eventfd_assign(kvm, args->conn_id, args->fd);

I'd prefer

if(..)
	return X
return Y

same number of lines but improved readability.



-- 

Thanks,

David / dhildenb



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux