[PATCH V4 0/4] Fast MMIO eventfd fixes

[PATCH V4 0/4] Fast MMIO eventfd fixes

[Jason Wang]

Hi:

This series fixes two issues of fast mmio eventfd:

1) A single iodev instance were registerd on two buses: KVM_MMIO_BUS
   and KVM_FAST_MMIO_BUS. This will cause double in
   ioeventfd_destructor()
2) A zero length iodev on KVM_MMIO_BUS will never be found but
   kvm_io_bus_cmp(). This will lead e.g the eventfd will be trapped by
   qemu instead of host.

1 is fixed by allocating two instances of iodev. 2 is fixed by ignore
the actual length if the length of iodev is zero in kvm_io_bus_cmp().

Please review.

Changes from V3:

- Don't do search on two buses when trying to do write on
  KVM_MMIO_BUS. This fixes a small regression found by vmexit.flat.
- Since we don't do search on two buses, change kvm_io_bus_cmp() to
  let it can find zero length iodevs.
- Fix the unnecessary lines in tracepoint patch.

Changes from V2:
- Tweak styles and comment suggested by Cornelia.

Changes from v1:
- change ioeventfd_bus_from_flags() to return KVM_FAST_MMIO_BUS when
  needed to save lots of unnecessary changes.

Jason Wang (4):
  kvm: factor out core eventfd assign/deassign logic
  kvm: fix double free for fast mmio eventfd
  kvm: fix zero length mmio searching
  kvm: add tracepoint for fast mmio

 arch/x86/kvm/trace.h |  18 ++++++++
 arch/x86/kvm/vmx.c   |   1 +
 arch/x86/kvm/x86.c   |   1 +
 virt/kvm/eventfd.c   | 124 ++++++++++++++++++++++++++++++---------------------
 virt/kvm/kvm_main.c  |   4 +-
 5 files changed, 96 insertions(+), 52 deletions(-)

-- 
2.1.4

[PATCH V4 1/4] kvm: factor out core eventfd assign/deassign logic

[Jason Wang]

This patch factors out core eventfd assign/deassign logic and leave
the argument checking and bus index selection to callers.

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 virt/kvm/eventfd.c | 83 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 9ff4193..163258d 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -771,40 +771,14 @@ static enum kvm_bus ioeventfd_bus_from_flags(__u32 flags)
 	return KVM_MMIO_BUS;
 }
 
-static int
-kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
+static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
+				enum kvm_bus bus_idx,
+				struct kvm_ioeventfd *args)
 {
-	enum kvm_bus              bus_idx;
-	struct _ioeventfd        *p;
-	struct eventfd_ctx       *eventfd;
-	int                       ret;
-
-	bus_idx = ioeventfd_bus_from_flags(args->flags);
-	/* must be natural-word sized, or 0 to ignore length */
-	switch (args->len) {
-	case 0:
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-		break;
-	default:
-		return -EINVAL;
-	}
 
-	/* check for range overflow */
-	if (args->addr + args->len < args->addr)
-		return -EINVAL;
-
-	/* check for extra flags that we don't understand */
-	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
-		return -EINVAL;
-
-	/* ioeventfd with no length can't be combined with DATAMATCH */
-	if (!args->len &&
-	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
-			   KVM_IOEVENTFD_FLAG_DATAMATCH))
-		return -EINVAL;
+	struct eventfd_ctx *eventfd;
+	struct _ioeventfd *p;
+	int ret;
 
 	eventfd = eventfd_ctx_fdget(args->fd);
 	if (IS_ERR(eventfd))
@@ -873,14 +847,48 @@ fail:
 }
 
 static int
-kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
+kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	enum kvm_bus              bus_idx;
+
+	bus_idx = ioeventfd_bus_from_flags(args->flags);
+	/* must be natural-word sized, or 0 to ignore length */
+	switch (args->len) {
+	case 0:
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* check for range overflow */
+	if (args->addr + args->len < args->addr)
+		return -EINVAL;
+
+	/* check for extra flags that we don't understand */
+	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
+		return -EINVAL;
+
+	/* ioeventfd with no length can't be combined with DATAMATCH */
+	if (!args->len &&
+	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
+			   KVM_IOEVENTFD_FLAG_DATAMATCH))
+		return -EINVAL;
+
+	return kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
+}
+
+static int
+kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
+			   struct kvm_ioeventfd *args)
+{
 	struct _ioeventfd        *p, *tmp;
 	struct eventfd_ctx       *eventfd;
 	int                       ret = -ENOENT;
 
-	bus_idx = ioeventfd_bus_from_flags(args->flags);
 	eventfd = eventfd_ctx_fdget(args->fd);
 	if (IS_ERR(eventfd))
 		return PTR_ERR(eventfd);
@@ -918,6 +926,13 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 	return ret;
 }
 
+static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
+{
+	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
+
+	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
+}
+
 int
 kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
-- 
2.1.4

[PATCH V4 2/4] kvm: fix double free for fast mmio eventfd

[Jason Wang]

We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
and another is KVM_FAST_MMIO_BUS but with a single iodev
instance. This will lead an issue: kvm_io_bus_destroy() knows nothing
about the devices on two buses points to a single dev. Which will lead
double free[1] during exit. Fixing this by using allocate two
instances of iodevs then register one on KVM_MMIO_BUS and another on
KVM_FAST_MMIO_BUS.

CPU: 1 PID: 2894 Comm: qemu-system-x86 Not tainted 3.19.0-26-generic #28-Ubuntu
Hardware name: LENOVO 2356BG6/2356BG6, BIOS G7ET96WW (2.56 ) 09/12/2013
task: ffff88009ae0c4b0 ti: ffff88020e7f0000 task.ti: ffff88020e7f0000
RIP: 0010:[<ffffffffc07e25d8>]  [<ffffffffc07e25d8>] ioeventfd_release+0x28/0x60 [kvm]
RSP: 0018:ffff88020e7f3bc8  EFLAGS: 00010292
RAX: dead000000200200 RBX: ffff8801ec19c900 RCX: 000000018200016d
RDX: ffff8801ec19cf80 RSI: ffffea0008bf1d40 RDI: ffff8801ec19c900
RBP: ffff88020e7f3bd8 R08: 000000002fc75a01 R09: 000000018200016d
R10: ffffffffc07df6ae R11: ffff88022fc75a98 R12: ffff88021e7cc000
R13: ffff88021e7cca48 R14: ffff88021e7cca50 R15: ffff8801ec19c880
FS:  00007fc1ee3e6700(0000) GS:ffff88023e240000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f8f389d8000 CR3: 000000023dc13000 CR4: 00000000001427e0
Stack:
ffff88021e7cc000 0000000000000000 ffff88020e7f3be8 ffffffffc07e2622
ffff88020e7f3c38 ffffffffc07df69a ffff880232524160 ffff88020e792d80
 0000000000000000 ffff880219b78c00 0000000000000008 ffff8802321686a8
Call Trace:
[<ffffffffc07e2622>] ioeventfd_destructor+0x12/0x20 [kvm]
[<ffffffffc07df69a>] kvm_put_kvm+0xca/0x210 [kvm]
[<ffffffffc07df818>] kvm_vcpu_release+0x18/0x20 [kvm]
[<ffffffff811f69f7>] __fput+0xe7/0x250
[<ffffffff811f6bae>] ____fput+0xe/0x10
[<ffffffff81093f04>] task_work_run+0xd4/0xf0
[<ffffffff81079358>] do_exit+0x368/0xa50
[<ffffffff81082c8f>] ? recalc_sigpending+0x1f/0x60
[<ffffffff81079ad5>] do_group_exit+0x45/0xb0
[<ffffffff81085c71>] get_signal+0x291/0x750
[<ffffffff810144d8>] do_signal+0x28/0xab0
[<ffffffff810f3a3b>] ? do_futex+0xdb/0x5d0
[<ffffffff810b7028>] ? __wake_up_locked_key+0x18/0x20
[<ffffffff810f3fa6>] ? SyS_futex+0x76/0x170
[<ffffffff81014fc9>] do_notify_resume+0x69/0xb0
[<ffffffff817cb9af>] int_signal+0x12/0x17
Code: 5d c3 90 0f 1f 44 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 8b 7f 20 e8 06 d6 a5 c0 48 8b 43 08 48 8b 13 48 89 df 48 89 42 08 <48> 89 10 48 b8 00 01 10 00 00
 RIP  [<ffffffffc07e25d8>] ioeventfd_release+0x28/0x60 [kvm]
 RSP <ffff88020e7f3bc8>

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 virt/kvm/eventfd.c | 111 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 52 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 163258d..1a023ac 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -817,16 +817,6 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
 	if (ret < 0)
 		goto unlock_fail;
 
-	/* When length is ignored, MMIO is also put on a separate bus, for
-	 * faster lookups.
-	 */
-	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
-		ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
-					      p->addr, 0, &p->dev);
-		if (ret < 0)
-			goto register_fail;
-	}
-
 	kvm->buses[bus_idx]->ioeventfd_count++;
 	list_add_tail(&p->list, &kvm->ioeventfds);
 
@@ -834,8 +824,6 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
 
 	return 0;
 
-register_fail:
-	kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
 unlock_fail:
 	mutex_unlock(&kvm->slots_lock);
 
@@ -847,41 +835,6 @@ fail:
 }
 
 static int
-kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
-{
-	enum kvm_bus              bus_idx;
-
-	bus_idx = ioeventfd_bus_from_flags(args->flags);
-	/* must be natural-word sized, or 0 to ignore length */
-	switch (args->len) {
-	case 0:
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	/* check for range overflow */
-	if (args->addr + args->len < args->addr)
-		return -EINVAL;
-
-	/* check for extra flags that we don't understand */
-	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
-		return -EINVAL;
-
-	/* ioeventfd with no length can't be combined with DATAMATCH */
-	if (!args->len &&
-	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
-			   KVM_IOEVENTFD_FLAG_DATAMATCH))
-		return -EINVAL;
-
-	return kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
-}
-
-static int
 kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
 			   struct kvm_ioeventfd *args)
 {
@@ -909,10 +862,6 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
 			continue;
 
 		kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
-		if (!p->length) {
-			kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
-						  &p->dev);
-		}
 		kvm->buses[bus_idx]->ioeventfd_count--;
 		ioeventfd_release(p);
 		ret = 0;
@@ -929,8 +878,66 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
 static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
+	int ret = kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
+
+	if (!args->len)
+		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
+
+	return ret;
+}
 
-	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
+static int
+kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
+{
+	enum kvm_bus              bus_idx;
+	int ret;
+
+	bus_idx = ioeventfd_bus_from_flags(args->flags);
+	/* must be natural-word sized, or 0 to ignore length */
+	switch (args->len) {
+	case 0:
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* check for range overflow */
+	if (args->addr + args->len < args->addr)
+		return -EINVAL;
+
+	/* check for extra flags that we don't understand */
+	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
+		return -EINVAL;
+
+	/* ioeventfd with no length can't be combined with DATAMATCH */
+	if (!args->len &&
+	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
+			   KVM_IOEVENTFD_FLAG_DATAMATCH))
+		return -EINVAL;
+
+	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
+	if (ret)
+		goto fail;
+
+	/* When length is ignored, MMIO is also put on a separate bus, for
+	 * faster lookups.
+	 */
+	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
+		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
+		if (ret < 0)
+			goto fast_fail;
+	}
+
+	return 0;
+
+fast_fail:
+	kvm_deassign_ioeventfd(kvm, args);
+fail:
+	return ret;
 }
 
 int
-- 
2.1.4

[PATCH V4 3/4] kvm: fix zero length mmio searching

[Jason Wang]

Currently, if we had a zero length mmio eventfd assigned on
KVM_MMIO_BUS. It will never found by kvm_io_bus_cmp() since it always
compare the kvm_io_range() with the length that guest wrote. This will
lead e.g for vhost, kick will be trapped by qemu userspace instead of
vhost. Fixing this by using zero length if an iodevice is zero length.

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 virt/kvm/kvm_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d8db2f8f..d4c3b66 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3071,9 +3071,11 @@ static void kvm_io_bus_destroy(struct kvm_io_bus *bus)
 static inline int kvm_io_bus_cmp(const struct kvm_io_range *r1,
 				 const struct kvm_io_range *r2)
 {
+	int len = r2->len ? r1->len : 0;
+
 	if (r1->addr < r2->addr)
 		return -1;
-	if (r1->addr + r1->len > r2->addr + r2->len)
+	if (r1->addr + len > r2->addr + r2->len)
 		return 1;
 	return 0;
 }
-- 
2.1.4

[PATCH V4 4/4] kvm: add tracepoint for fast mmio

[Jason Wang]

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 arch/x86/kvm/trace.h | 18 ++++++++++++++++++
 arch/x86/kvm/vmx.c   |  1 +
 arch/x86/kvm/x86.c   |  1 +
 3 files changed, 20 insertions(+)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 4eae7c3..ce4abe3 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -129,6 +129,24 @@ TRACE_EVENT(kvm_pio,
 );
 
 /*
+ * Tracepoint for fast mmio.
+ */
+TRACE_EVENT(kvm_fast_mmio,
+	TP_PROTO(u64 gpa),
+	TP_ARGS(gpa),
+
+	TP_STRUCT__entry(
+		__field(u64,	gpa)
+	),
+
+	TP_fast_assign(
+		__entry->gpa		= gpa;
+	),
+
+	TP_printk("fast mmio at gpa 0x%llx", __entry->gpa)
+);
+
+/*
  * Tracepoint for cpuid.
  */
 TRACE_EVENT(kvm_cpuid,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4a4eec30..cb505b9 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5767,6 +5767,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
 	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
 	if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
 		skip_emulated_instruction(vcpu);
+		trace_kvm_fast_mmio(gpa);
 		return 1;
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1e7e76e..f7c4042 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8013,6 +8013,7 @@ bool kvm_arch_has_noncoherent_dma(struct kvm *kvm)
 EXPORT_SYMBOL_GPL(kvm_arch_has_noncoherent_dma);
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_fast_mmio);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_page_fault);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_msr);
-- 
2.1.4

Re: [PATCH V4 1/4] kvm: factor out core eventfd assign/deassign logic

[Cornelia Huck]

On Fri, 11 Sep 2015 11:17:34 +0800
Jason Wang <jasowang@redhat.com> wrote:

> This patch factors out core eventfd assign/deassign logic and leave
> the argument checking and bus index selection to callers.
> 
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  virt/kvm/eventfd.c | 83 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 49 insertions(+), 34 deletions(-)
> 
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 9ff4193..163258d 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -771,40 +771,14 @@ static enum kvm_bus ioeventfd_bus_from_flags(__u32 flags)
>  	return KVM_MMIO_BUS;
>  }
> 
> -static int
> -kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
> +				enum kvm_bus bus_idx,
> +				struct kvm_ioeventfd *args)
>  {
> -	enum kvm_bus              bus_idx;
> -	struct _ioeventfd        *p;
> -	struct eventfd_ctx       *eventfd;
> -	int                       ret;
> -
> -	bus_idx = ioeventfd_bus_from_flags(args->flags);
> -	/* must be natural-word sized, or 0 to ignore length */
> -	switch (args->len) {
> -	case 0:
> -	case 1:
> -	case 2:
> -	case 4:
> -	case 8:
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> 
> -	/* check for range overflow */
> -	if (args->addr + args->len < args->addr)
> -		return -EINVAL;
> -
> -	/* check for extra flags that we don't understand */
> -	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
> -		return -EINVAL;
> -
> -	/* ioeventfd with no length can't be combined with DATAMATCH */
> -	if (!args->len &&
> -	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
> -			   KVM_IOEVENTFD_FLAG_DATAMATCH))
> -		return -EINVAL;
> +	struct eventfd_ctx *eventfd;
> +	struct _ioeventfd *p;
> +	int ret;
> 
>  	eventfd = eventfd_ctx_fdget(args->fd);
>  	if (IS_ERR(eventfd))
> @@ -873,14 +847,48 @@ fail:
>  }
> 
>  static int
> -kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)

You'll move this function to below the deassign function in patch 2.
Maybe do it already here?

>  {
>  	enum kvm_bus              bus_idx;
> +
> +	bus_idx = ioeventfd_bus_from_flags(args->flags);
> +	/* must be natural-word sized, or 0 to ignore length */
> +	switch (args->len) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* check for range overflow */
> +	if (args->addr + args->len < args->addr)
> +		return -EINVAL;
> +
> +	/* check for extra flags that we don't understand */
> +	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
> +		return -EINVAL;
> +
> +	/* ioeventfd with no length can't be combined with DATAMATCH */
> +	if (!args->len &&
> +	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
> +			   KVM_IOEVENTFD_FLAG_DATAMATCH))
> +		return -EINVAL;
> +
> +	return kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
> +}
> +
> +static int
> +kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
> +			   struct kvm_ioeventfd *args)

While this file uses newline before function name quite often, putting
it on the same line seems more common - don't know which one the
maintainers prefer.

> +{
>  	struct _ioeventfd        *p, *tmp;
>  	struct eventfd_ctx       *eventfd;
>  	int                       ret = -ENOENT;
> 
> -	bus_idx = ioeventfd_bus_from_flags(args->flags);
>  	eventfd = eventfd_ctx_fdget(args->fd);
>  	if (IS_ERR(eventfd))
>  		return PTR_ERR(eventfd);
> @@ -918,6 +926,13 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  	return ret;
>  }
> 
> +static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +{
> +	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
> +
> +	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
> +}
> +
>  int
>  kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  {

Re: [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd

[Cornelia Huck]

On Fri, 11 Sep 2015 11:17:35 +0800
Jason Wang <jasowang@redhat.com> wrote:

> We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
> and another is KVM_FAST_MMIO_BUS but with a single iodev
> instance. This will lead an issue: kvm_io_bus_destroy() knows nothing
> about the devices on two buses points to a single dev. Which will lead

s/points/pointing/

> double free[1] during exit. Fixing this by using allocate two

s/using allocate/allocating/

> instances of iodevs then register one on KVM_MMIO_BUS and another on
> KVM_FAST_MMIO_BUS.
> 
(...)

> @@ -929,8 +878,66 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
>  static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  {
>  	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
> +	int ret = kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
> +
> +	if (!args->len)
> +		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);

I think it would be good to explicitly check for bus_idx ==
KVM_MMIO_BUS here.

> +
> +	return ret;
> +}
> 
> -	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
> +static int
> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +{
> +	enum kvm_bus              bus_idx;
> +	int ret;
> +
> +	bus_idx = ioeventfd_bus_from_flags(args->flags);
> +	/* must be natural-word sized, or 0 to ignore length */
> +	switch (args->len) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* check for range overflow */
> +	if (args->addr + args->len < args->addr)
> +		return -EINVAL;
> +
> +	/* check for extra flags that we don't understand */
> +	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
> +		return -EINVAL;
> +
> +	/* ioeventfd with no length can't be combined with DATAMATCH */
> +	if (!args->len &&
> +	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
> +			   KVM_IOEVENTFD_FLAG_DATAMATCH))
> +		return -EINVAL;
> +
> +	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
> +	if (ret)
> +		goto fail;
> +
> +	/* When length is ignored, MMIO is also put on a separate bus, for
> +	 * faster lookups.
> +	 */
> +	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {

Dito on a positive check for bus_idx == KVM_MMIO_BUS.

> +		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
> +		if (ret < 0)
> +			goto fast_fail;
> +	}
> +
> +	return 0;
> +
> +fast_fail:
> +	kvm_deassign_ioeventfd(kvm, args);

Shouldn't you use kvm_deassign_ioeventfd(kvm, bus_idx, args) here?

> +fail:
> +	return ret;
>  }
> 
>  int

Re: [PATCH V4 0/4] Fast MMIO eventfd fixes

[Michael S. Tsirkin]

On Fri, Sep 11, 2015 at 11:17:33AM +0800, Jason Wang wrote:
> Hi:
> 
> This series fixes two issues of fast mmio eventfd:
> 
> 1) A single iodev instance were registerd on two buses: KVM_MMIO_BUS
>    and KVM_FAST_MMIO_BUS. This will cause double in
>    ioeventfd_destructor()
> 2) A zero length iodev on KVM_MMIO_BUS will never be found but
>    kvm_io_bus_cmp(). This will lead e.g the eventfd will be trapped by
>    qemu instead of host.
> 
> 1 is fixed by allocating two instances of iodev. 2 is fixed by ignore
> the actual length if the length of iodev is zero in kvm_io_bus_cmp().
> 
> Please review.

I think we should add a capability for fast mmio.
This way, userspace can avoid crashing buggy kernels.

> Changes from V3:
> 
> - Don't do search on two buses when trying to do write on
>   KVM_MMIO_BUS. This fixes a small regression found by vmexit.flat.
> - Since we don't do search on two buses, change kvm_io_bus_cmp() to
>   let it can find zero length iodevs.
> - Fix the unnecessary lines in tracepoint patch.
> 
> Changes from V2:
> - Tweak styles and comment suggested by Cornelia.
> 
> Changes from v1:
> - change ioeventfd_bus_from_flags() to return KVM_FAST_MMIO_BUS when
>   needed to save lots of unnecessary changes.
> 
> Jason Wang (4):
>   kvm: factor out core eventfd assign/deassign logic
>   kvm: fix double free for fast mmio eventfd
>   kvm: fix zero length mmio searching
>   kvm: add tracepoint for fast mmio
> 
>  arch/x86/kvm/trace.h |  18 ++++++++
>  arch/x86/kvm/vmx.c   |   1 +
>  arch/x86/kvm/x86.c   |   1 +
>  virt/kvm/eventfd.c   | 124 ++++++++++++++++++++++++++++++---------------------
>  virt/kvm/kvm_main.c  |   4 +-
>  5 files changed, 96 insertions(+), 52 deletions(-)
> 
> -- 
> 2.1.4

Re: [PATCH V4 1/4] kvm: factor out core eventfd assign/deassign logic

[Paolo Bonzini]

On 11/09/2015 09:39, Cornelia Huck wrote:
> > +static int
> > +kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
> > +			   struct kvm_ioeventfd *args)
> 
> While this file uses newline before function name quite often, putting
> it on the same line seems more common - don't know which one the
> maintainers prefer.

I prefer it this way if it doesn't make the declaration one line longer,
which seems to be the case here.

Paolo

Re: [PATCH V4 3/4] kvm: fix zero length mmio searching

[Paolo Bonzini]

On 11/09/2015 05:17, Jason Wang wrote:
> +	int len = r2->len ? r1->len : 0;
> +
>  	if (r1->addr < r2->addr)
>  		return -1;
> -	if (r1->addr + r1->len > r2->addr + r2->len)
> +	if (r1->addr + len > r2->addr + r2->len)
>  		return 1;

Perhaps better:

	gpa_t addr1 = r1->addr;
	gpa_t addr2 = r2->addr;

	if (addr1 < addr2)
		return -1;

	/* If r2->len == 0, match the exact address.  If r2->len != 0,
	 * accept any overlapping write.  Any order is acceptable for
	 * overlapping ranges, because kvm_io_bus_get_first_dev ensures
	 * we process all of them.
	 */
	if (r2->len) {
		addr1 += r1->len;
		addr2 += r2->len;
	}

	if (addr1 > addr2)
		return 1;

	return 0;

Re: [PATCH V4 3/4] kvm: fix zero length mmio searching

[Cornelia Huck]

On Fri, 11 Sep 2015 10:26:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 11/09/2015 05:17, Jason Wang wrote:
> > +	int len = r2->len ? r1->len : 0;
> > +
> >  	if (r1->addr < r2->addr)
> >  		return -1;
> > -	if (r1->addr + r1->len > r2->addr + r2->len)
> > +	if (r1->addr + len > r2->addr + r2->len)
> >  		return 1;
> 
> Perhaps better:
> 
> 	gpa_t addr1 = r1->addr;
> 	gpa_t addr2 = r2->addr;
> 
> 	if (addr1 < addr2)
> 		return -1;
> 
> 	/* If r2->len == 0, match the exact address.  If r2->len != 0,
> 	 * accept any overlapping write.  Any order is acceptable for
> 	 * overlapping ranges, because kvm_io_bus_get_first_dev ensures
> 	 * we process all of them.
> 	 */
> 	if (r2->len) {
> 		addr1 += r1->len;
> 		addr2 += r2->len;
> 	}
> 
> 	if (addr1 > addr2)
> 		return 1;
> 
> 	return 0;
> 

+1 to documenting what the semantics are :)

Re: [PATCH V4 0/4] Fast MMIO eventfd fixes

[Paolo Bonzini]

On 11/09/2015 10:15, Michael S. Tsirkin wrote:
> I think we should add a capability for fast mmio.
> This way, userspace can avoid crashing buggy kernels.

I agree.

Paolo

Re: [PATCH V4 1/4] kvm: factor out core eventfd assign/deassign logic

[Jason Wang]

On 09/11/2015 03:39 PM, Cornelia Huck wrote:
> On Fri, 11 Sep 2015 11:17:34 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> This patch factors out core eventfd assign/deassign logic and leave
>> the argument checking and bus index selection to callers.
>>
>> Cc: Gleb Natapov <gleb@kernel.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  virt/kvm/eventfd.c | 83 ++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 49 insertions(+), 34 deletions(-)
>>
>> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
>> index 9ff4193..163258d 100644
>> --- a/virt/kvm/eventfd.c
>> +++ b/virt/kvm/eventfd.c
>> @@ -771,40 +771,14 @@ static enum kvm_bus ioeventfd_bus_from_flags(__u32 flags)
>>  	return KVM_MMIO_BUS;
>>  }
>>
>> -static int
>> -kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>> +static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
>> +				enum kvm_bus bus_idx,
>> +				struct kvm_ioeventfd *args)
>>  {
>> -	enum kvm_bus              bus_idx;
>> -	struct _ioeventfd        *p;
>> -	struct eventfd_ctx       *eventfd;
>> -	int                       ret;
>> -
>> -	bus_idx = ioeventfd_bus_from_flags(args->flags);
>> -	/* must be natural-word sized, or 0 to ignore length */
>> -	switch (args->len) {
>> -	case 0:
>> -	case 1:
>> -	case 2:
>> -	case 4:
>> -	case 8:
>> -		break;
>> -	default:
>> -		return -EINVAL;
>> -	}
>>
>> -	/* check for range overflow */
>> -	if (args->addr + args->len < args->addr)
>> -		return -EINVAL;
>> -
>> -	/* check for extra flags that we don't understand */
>> -	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
>> -		return -EINVAL;
>> -
>> -	/* ioeventfd with no length can't be combined with DATAMATCH */
>> -	if (!args->len &&
>> -	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
>> -			   KVM_IOEVENTFD_FLAG_DATAMATCH))
>> -		return -EINVAL;
>> +	struct eventfd_ctx *eventfd;
>> +	struct _ioeventfd *p;
>> +	int ret;
>>
>>  	eventfd = eventfd_ctx_fdget(args->fd);
>>  	if (IS_ERR(eventfd))
>> @@ -873,14 +847,48 @@ fail:
>>  }
>>
>>  static int
>> -kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> You'll move this function to below the deassign function in patch 2.
> Maybe do it already here?
>

Yes, this can reduce the changes for patch2.

Re: [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd

[Jason Wang]

On 09/11/2015 03:46 PM, Cornelia Huck wrote:
> On Fri, 11 Sep 2015 11:17:35 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
>> and another is KVM_FAST_MMIO_BUS but with a single iodev
>> instance. This will lead an issue: kvm_io_bus_destroy() knows nothing
>> about the devices on two buses points to a single dev. Which will lead
> s/points/pointing/

Will fix this in V5.

>> double free[1] during exit. Fixing this by using allocate two
> s/using allocate/allocating/

Will fix this in V5.

>
>> instances of iodevs then register one on KVM_MMIO_BUS and another on
>> KVM_FAST_MMIO_BUS.
>>
> (...)
>
>> @@ -929,8 +878,66 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
>>  static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>>  {
>>  	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
>> +	int ret = kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
>> +
>> +	if (!args->len)
>> +		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
> I think it would be good to explicitly check for bus_idx ==
> KVM_MMIO_BUS here.

Ok.

>
>> +
>> +	return ret;
>> +}
>>
>> -	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
>> +static int
>> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>> +{
>> +	enum kvm_bus              bus_idx;
>> +	int ret;
>> +
>> +	bus_idx = ioeventfd_bus_from_flags(args->flags);
>> +	/* must be natural-word sized, or 0 to ignore length */
>> +	switch (args->len) {
>> +	case 0:
>> +	case 1:
>> +	case 2:
>> +	case 4:
>> +	case 8:
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* check for range overflow */
>> +	if (args->addr + args->len < args->addr)
>> +		return -EINVAL;
>> +
>> +	/* check for extra flags that we don't understand */
>> +	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
>> +		return -EINVAL;
>> +
>> +	/* ioeventfd with no length can't be combined with DATAMATCH */
>> +	if (!args->len &&
>> +	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
>> +			   KVM_IOEVENTFD_FLAG_DATAMATCH))
>> +		return -EINVAL;
>> +
>> +	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	/* When length is ignored, MMIO is also put on a separate bus, for
>> +	 * faster lookups.
>> +	 */
>> +	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
> Dito on a positive check for bus_idx == KVM_MMIO_BUS.

I was thinking maybe this should be done in a separate patch on top.
What's your opinion?

>> +		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
>> +		if (ret < 0)
>> +			goto fast_fail;
>> +	}
>> +
>> +	return 0;
>> +
>> +fast_fail:
>> +	kvm_deassign_ioeventfd(kvm, args);
> Shouldn't you use kvm_deassign_ioeventfd(kvm, bus_idx, args) here?

Actually, it's the same. (the deassign of fast mmio will return -ENOENT
and will be ignored.) But I admit do what you suggested here is better.
Will do this.

Thanks

Re: [PATCH V4 3/4] kvm: fix zero length mmio searching

[Jason Wang]

On 09/11/2015 04:31 PM, Cornelia Huck wrote:
> On Fri, 11 Sep 2015 10:26:41 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> On 11/09/2015 05:17, Jason Wang wrote:
>>> +	int len = r2->len ? r1->len : 0;
>>> +
>>>  	if (r1->addr < r2->addr)
>>>  		return -1;
>>> -	if (r1->addr + r1->len > r2->addr + r2->len)
>>> +	if (r1->addr + len > r2->addr + r2->len)
>>>  		return 1;
>> Perhaps better:
>>
>> 	gpa_t addr1 = r1->addr;
>> 	gpa_t addr2 = r2->addr;
>>
>> 	if (addr1 < addr2)
>> 		return -1;
>>
>> 	/* If r2->len == 0, match the exact address.  If r2->len != 0,
>> 	 * accept any overlapping write.  Any order is acceptable for
>> 	 * overlapping ranges, because kvm_io_bus_get_first_dev ensures
>> 	 * we process all of them.
>> 	 */
>> 	if (r2->len) {
>> 		addr1 += r1->len;
>> 		addr2 += r2->len;
>> 	}
>>
>> 	if (addr1 > addr2)
>> 		return 1;
>>
>> 	return 0;
>>
> +1 to documenting what the semantics are :)
>

Right, better. Will fix this in V5.

Re: [PATCH V4 0/4] Fast MMIO eventfd fixes

[Jason Wang]

On 09/11/2015 04:33 PM, Paolo Bonzini wrote:
>
> On 11/09/2015 10:15, Michael S. Tsirkin wrote:
>> I think we should add a capability for fast mmio.
>> This way, userspace can avoid crashing buggy kernels.
> I agree.
>
> Paolo

Right, then qemu will use datamatch eventfd if kenrel dost not have the
capability.

Thanks

Re: [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd

[Cornelia Huck]

On Fri, 11 Sep 2015 17:25:45 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 09/11/2015 03:46 PM, Cornelia Huck wrote:
> > On Fri, 11 Sep 2015 11:17:35 +0800
> > Jason Wang <jasowang@redhat.com> wrote:

> >> +
> >> +	/* When length is ignored, MMIO is also put on a separate bus, for
> >> +	 * faster lookups.
> >> +	 */
> >> +	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
> > Dito on a positive check for bus_idx == KVM_MMIO_BUS.
> 
> I was thinking maybe this should be done in a separate patch on top.
> What's your opinion?

The check is an independent issue, an extra patch is fine (current
usage does not trigger any problems).

> 
> >> +		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
> >> +		if (ret < 0)
> >> +			goto fast_fail;
> >> +	}

Re: [PATCH V4 0/4] Fast MMIO eventfd fixes

[Michael S. Tsirkin]

On Fri, Sep 11, 2015 at 05:28:29PM +0800, Jason Wang wrote:
> 
> 
> On 09/11/2015 04:33 PM, Paolo Bonzini wrote:
> >
> > On 11/09/2015 10:15, Michael S. Tsirkin wrote:
> >> I think we should add a capability for fast mmio.
> >> This way, userspace can avoid crashing buggy kernels.
> > I agree.
> >
> > Paolo
> 
> Right, then qemu will use datamatch eventfd if kenrel dost not have the
> capability.
> 
> Thanks

Wildcard is sufficient I think.

-- 
MST

Re: [PATCH V4 0/4] Fast MMIO eventfd fixes

[Michael S. Tsirkin]

On Fri, Sep 11, 2015 at 10:33:08AM +0200, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 10:15, Michael S. Tsirkin wrote:
> > I think we should add a capability for fast mmio.
> > This way, userspace can avoid crashing buggy kernels.
> 
> I agree.
> 
> Paolo

Having said that, we can merge these patches directly and add
capability on top.
Cc stable for all of them including the capability.