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

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

[Roman Kagan]

Make it possible for guests using Hyper-V emulation to do guest->host
notification via EVENT_SIGNAL hypercall without a user exit.

v2 -> v3:
 - expand docs on allowed values and return codes
 - fix uninitialized return
 - style fixes

v1 -> v2:
 - make data types consistent
 - get by without the recently dropped struct hv_input_signal_event
 - fix subject prefixes

Roman Kagan (2):
  kvm: x86: factor out kvm.arch.hyperv (de)init
  kvm: x86: hyperv: guest->host event signaling via eventfd

 Documentation/virtual/kvm/api.txt |  31 ++++++++++
 arch/x86/include/asm/kvm_host.h   |   2 +
 arch/x86/kvm/hyperv.h             |   4 ++
 include/uapi/linux/kvm.h          |  13 ++++
 arch/x86/kvm/hyperv.c             | 123 +++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/x86.c                |  13 +++-
 6 files changed, 184 insertions(+), 2 deletions(-)

-- 
2.14.3

[PATCH v3 1/2] kvm: x86: factor out kvm.arch.hyperv (de)init

[Roman Kagan]

Move kvm.arch.hyperv initialization and cleanup to separate functions.

For now only a mutex is inited in the former, and the latter is empty;
more stuff will go in there in a followup patch.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
v2 -> v3:
 - style fixes

v1 -> v2:
 - fix subject prefix

 arch/x86/kvm/hyperv.h | 3 +++
 arch/x86/kvm/hyperv.c | 9 +++++++++
 arch/x86/kvm/x86.c    | 3 ++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
index e637631a9574..cc2468244ca2 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -88,4 +88,7 @@ void kvm_hv_process_stimers(struct kvm_vcpu *vcpu);
 void kvm_hv_setup_tsc_page(struct kvm *kvm,
 			   struct pvclock_vcpu_time_info *hv_clock);
 
+void kvm_hv_init_vm(struct kvm *kvm);
+void kvm_hv_destroy_vm(struct kvm *kvm);
+
 #endif
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index dc97f2544b6f..015fb06c7522 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1301,3 +1301,12 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
 	kvm_hv_hypercall_set_result(vcpu, ret);
 	return 1;
 }
+
+void kvm_hv_init_vm(struct kvm *kvm)
+{
+	mutex_init(&kvm->arch.hyperv.hv_lock);
+}
+
+void kvm_hv_destroy_vm(struct kvm *kvm)
+{
+}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index eee8e7faf1af..d0ff5e1cf464 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8149,7 +8149,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 	raw_spin_lock_init(&kvm->arch.tsc_write_lock);
 	mutex_init(&kvm->arch.apic_map_lock);
-	mutex_init(&kvm->arch.hyperv.hv_lock);
 	spin_lock_init(&kvm->arch.pvclock_gtod_sync_lock);
 
 	kvm->arch.kvmclock_offset = -ktime_get_boot_ns();
@@ -8158,6 +8157,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
 
+	kvm_hv_init_vm(kvm);
 	kvm_page_track_init(kvm);
 	kvm_mmu_init_vm(kvm);
 
@@ -8292,6 +8292,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	kvfree(rcu_dereference_check(kvm->arch.apic_map, 1));
 	kvm_mmu_uninit_vm(kvm);
 	kvm_page_track_cleanup(kvm);
+	kvm_hv_destroy_vm(kvm);
 }
 
 void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
-- 
2.14.3

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

[Roman Kagan]

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@virtuozzo.com>
---
v2 -> v3:
 - expand docs on allowed values and return codes
 - fix uninitialized return
 - style fixes

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 |  31 +++++++++++
 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             | 114 +++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/x86.c                |  10 ++++
 6 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index f670e4b9e7f3..9c20d5d4a8f8 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3394,6 +3394,37 @@ 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)
+
+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 conn_id field should fit within 24 bits:
+
+#define KVM_HYPERV_CONN_ID_MASK		0x00ffffff
+
+The acceptable values for the flags field are:
+
+#define KVM_HYPERV_EVENTFD_DEASSIGN	(1 << 0)
+
+Returns: 0 on success,
+	-EINVAL if conn_id or flags is outside the allowed range
+	-ENOENT on deassign if the conn_id isn't registered
+	-EEXIST on assign if the conn_id is already registered
+
 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..0830d3d423b6 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_MASK		0x00ffffff
+#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..0303bd8f4d6a 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 & ~KVM_HYPERV_CONN_ID_MASK)
+		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,67 @@ 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)
 {
+	struct eventfd_ctx *eventfd;
+	int i;
+
+	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)
+{
+	struct kvm_hv *hv = &kvm->arch.hyperv;
+	struct eventfd_ctx *eventfd;
+	int ret;
+
+	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)
+{
+	struct kvm_hv *hv = &kvm->arch.hyperv;
+	struct eventfd_ctx *eventfd;
+
+	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 0;
+}
+
+int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args)
+{
+	if ((args->flags & ~KVM_HYPERV_EVENTFD_DEASSIGN) ||
+	    (args->conn_id & ~KVM_HYPERV_CONN_ID_MASK))
+		return -EINVAL;
+
+	if (args->flags == KVM_HYPERV_EVENTFD_DEASSIGN)
+		return kvm_hv_eventfd_deassign(kvm, args->conn_id);
+	return kvm_hv_eventfd_assign(kvm, args->conn_id, args->fd);
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d0ff5e1cf464..45d28b39edb7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2701,6 +2701,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_HYPERV_SYNIC:
 	case KVM_CAP_HYPERV_SYNIC2:
 	case KVM_CAP_HYPERV_VP_INDEX:
+	case KVM_CAP_HYPERV_EVENTFD:
 	case KVM_CAP_PCI_SEGMENT:
 	case KVM_CAP_DEBUGREGS:
 	case KVM_CAP_X86_ROBUST_SINGLESTEP:
@@ -4296,6 +4297,15 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = kvm_vm_ioctl_enable_cap(kvm, &cap);
 		break;
 	}
+	case KVM_HYPERV_EVENTFD: {
+		struct kvm_hyperv_eventfd hvevfd;
+
+		r = -EFAULT;
+		if (copy_from_user(&hvevfd, argp, sizeof(hvevfd)))
+			goto out;
+		r = kvm_vm_ioctl_hv_eventfd(kvm, &hvevfd);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
-- 
2.14.3

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

[David Hildenbrand]

>  #endif /* __LINUX_KVM_H */
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 015fb06c7522..0303bd8f4d6a 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;
> +

Don't we also need srcu_read_lock(&vcpu->kvm->srcu) for
kvm_vcpu_gfn_to_page?

> +	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);

conn_id = param + ((param >> 32) & 0xffff);

(as param will me automatically truncated to 32 bits - sizeof(conn_id))

> +	if (conn_id & ~KVM_HYPERV_CONN_ID_MASK)
> +		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,67 @@ 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)
>  {
> +	struct eventfd_ctx *eventfd;
> +	int i;
> +

At this point we are guaranteed that nobody else can call
kvm_hv_eventfd_assign/unassign? (I assume so, just want to have it
verified that we don't need the lock :) )

> +	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)
> +{
> +	struct kvm_hv *hv = &kvm->arch.hyperv;
> +	struct eventfd_ctx *eventfd;
> +	int ret;
> +
> +	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)
> +{
> +	struct kvm_hv *hv = &kvm->arch.hyperv;

Guess we could move that inline without exceeding any line lengths, but
just my opinion. (same applies in the function above where we already
need two lines either way for idr_alloc)

> +	struct eventfd_ctx *eventfd;
> +
> +	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 0;
> +}
> +


-- 

Thanks,

David / dhildenb

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

[Roman Kagan]

On Mon, Dec 11, 2017 at 10:14:48PM +0100, David Hildenbrand wrote:
> 
> >  #endif /* __LINUX_KVM_H */
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 015fb06c7522..0303bd8f4d6a 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;
> > +
> 
> Don't we also need srcu_read_lock(&vcpu->kvm->srcu) for
> kvm_vcpu_gfn_to_page?

Seems so indeed.  I also think I'll switch to using
kvm_vcpu_read_guest().

> > +	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);
> 
> conn_id = param + ((param >> 32) & 0xffff);
> 
> (as param will me automatically truncated to 32 bits - sizeof(conn_id))

Agreed.  However, it looks like what I really need is

conn_id = (param & KVM_HYPERV_CONN_ID_MASK) + ((param >> 32) & 0xffff);

otherwise the overflown conn_id may pass the following check

> > +	if (conn_id & ~KVM_HYPERV_CONN_ID_MASK)
> > +		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,67 @@ 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)
> >  {
> > +	struct eventfd_ctx *eventfd;
> > +	int i;
> > +
> 
> At this point we are guaranteed that nobody else can call
> kvm_hv_eventfd_assign/unassign? (I assume so, just want to have it
> verified that we don't need the lock :) )

Yes, it's in kvm_destroy_vm, which won't get called until the kvm vm fd
is released, so no concurrency with kvm_vm_ioctl is possible.

> > +	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)
> > +{
> > +	struct kvm_hv *hv = &kvm->arch.hyperv;
> > +	struct eventfd_ctx *eventfd;
> > +	int ret;
> > +
> > +	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)
> > +{
> > +	struct kvm_hv *hv = &kvm->arch.hyperv;
> 
> Guess we could move that inline without exceeding any line lengths, but
> just my opinion. (same applies in the function above where we already
> need two lines either way for idr_alloc)

Right, but it still reads easier with fewer dots and arrows IMHO.

Thanks,
Roman.

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

[David Hildenbrand]

On 12.12.2017 10:02, Roman Kagan wrote:
> On Mon, Dec 11, 2017 at 10:14:48PM +0100, David Hildenbrand wrote:
>>
>>>  #endif /* __LINUX_KVM_H */
>>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>>> index 015fb06c7522..0303bd8f4d6a 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;
>>> +
>>
>> Don't we also need srcu_read_lock(&vcpu->kvm->srcu) for
>> kvm_vcpu_gfn_to_page?
> 
> Seems so indeed.  I also think I'll switch to using
> kvm_vcpu_read_guest().

Probably better (because I think you also missed releasing the page)

> 
>>> +	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);
>>
>> conn_id = param + ((param >> 32) & 0xffff);
>>
>> (as param will me automatically truncated to 32 bits - sizeof(conn_id))
> 
> Agreed.  However, it looks like what I really need is
> 
> conn_id = (param & KVM_HYPERV_CONN_ID_MASK) + ((param >> 32) & 0xffff);
> 
> otherwise the overflown conn_id may pass the following check

That would have been my next question :)

-- 

Thanks,

David / dhildenb

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

[Roman Kagan]

On Tue, Dec 12, 2017 at 10:37:31AM +0100, David Hildenbrand wrote:
> On 12.12.2017 10:02, Roman Kagan wrote:
> > On Mon, Dec 11, 2017 at 10:14:48PM +0100, David Hildenbrand wrote:
> >>
> >>>  #endif /* __LINUX_KVM_H */
> >>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> >>> index 015fb06c7522..0303bd8f4d6a 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;
> >>> +
> >>
> >> Don't we also need srcu_read_lock(&vcpu->kvm->srcu) for
> >> kvm_vcpu_gfn_to_page?
> > 
> > Seems so indeed.  I also think I'll switch to using
> > kvm_vcpu_read_guest().
> 
> Probably better (because I think you also missed releasing the page)

Hmm, I wonder where?  (kvm_vcpu_read_guest ended up being more concise
so I'll stick with it but I'd like to learn from my errors anyway.)

> 
> > 
> >>> +	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);
> >>
> >> conn_id = param + ((param >> 32) & 0xffff);
> >>
> >> (as param will me automatically truncated to 32 bits - sizeof(conn_id))
> > 
> > Agreed.  However, it looks like what I really need is
> > 
> > conn_id = (param & KVM_HYPERV_CONN_ID_MASK) + ((param >> 32) & 0xffff);
> > 
> > otherwise the overflown conn_id may pass the following check
> 
> That would have been my next question :)

Thanks,
Roman.

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

[Roman Kagan]

On Tue, Dec 12, 2017 at 01:17:23PM +0300, Roman Kagan wrote:
> On Tue, Dec 12, 2017 at 10:37:31AM +0100, David Hildenbrand wrote:
> > On 12.12.2017 10:02, Roman Kagan wrote:
> > > On Mon, Dec 11, 2017 at 10:14:48PM +0100, David Hildenbrand wrote:
> > >>> @@ -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;
> > >>> +
> > >>
> > >> Don't we also need srcu_read_lock(&vcpu->kvm->srcu) for
> > >> kvm_vcpu_gfn_to_page?
> > > 
> > > Seems so indeed.  I also think I'll switch to using
> > > kvm_vcpu_read_guest().
> > 
> > Probably better (because I think you also missed releasing the page)
> 
> Hmm, I wonder where?  (kvm_vcpu_read_guest ended up being more concise
> so I'll stick with it but I'd like to learn from my errors anyway.)

Nevermind, I screwed up my branches.  The one I posted was missing
kvm_release_page_clean() indeed.

Thanks,
Roman.