Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1224669 > unrolled thread

[PATCH V6 0/6] Fast mmio eventfd fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH V6 0/6] Fast mmio eventfd fixes Jason Wang <jasowang@redhat.com> - 2015-09-15 08:50 +0200
    [PATCH V6 6/6] kvm: add fast mmio capabilitiy Jason Wang <jasowang@redhat.com> - 2015-09-15 08:50 +0200
      Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy Paolo Bonzini <pbonzini@redhat.com> - 2015-09-15 17:10 +0200
        Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy Cornelia Huck <cornelia.huck@de.ibm.com> - 2015-09-15 18:20 +0200
          Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy Paolo Bonzini <pbonzini@redhat.com> - 2015-09-15 18:30 +0200
            Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy Cornelia Huck <cornelia.huck@de.ibm.com> - 2015-09-15 18:50 +0200
              Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy Paolo Bonzini <pbonzini@redhat.com> - 2015-09-15 18:50 +0200
    [PATCH V6 2/6] kvm: factor out core eventfd assign/deassign logic Jason Wang <jasowang@redhat.com> - 2015-09-15 08:50 +0200
      Re: [PATCH V6 2/6] kvm: factor out core eventfd assign/deassign  logic Cornelia Huck <cornelia.huck@de.ibm.com> - 2015-09-15 09:10 +0200
    [PATCH V6 3/6] kvm: fix double free for fast mmio eventfd Jason Wang <jasowang@redhat.com> - 2015-09-15 08:50 +0200
      Re: [PATCH V6 3/6] kvm: fix double free for fast mmio eventfd Cornelia Huck <cornelia.huck@de.ibm.com> - 2015-09-15 09:20 +0200
    Re: [PATCH V6 0/6] Fast mmio eventfd fixes Paolo Bonzini <pbonzini@redhat.com> - 2015-09-15 17:10 +0200
      Re: [PATCH V6 0/6] Fast mmio eventfd fixes "Michael S. Tsirkin" <mst@redhat.com> - 2015-09-15 21:30 +0200
        Re: [PATCH V6 0/6] Fast mmio eventfd fixes Paolo Bonzini <pbonzini@redhat.com> - 2015-09-16 11:10 +0200

#1224669 — [PATCH V6 0/6] Fast mmio eventfd fixes

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 and introduce a new
capability for userspace. 2 is fixed by ignore the actual length if
the length of iodev is zero in kvm_io_bus_cmp().

Please review.

Changes from V5:
- move patch of explicitly checking for KVM_MMIO_BUS to patch 1 and
  remove the unnecessary checks
- even more grammar and typo fixes
- rabase to kvm.git
- document KVM_CAP_FAST_MMIO

Changes from V4:
- move the location of kvm_assign_ioeventfd() in patch 1 which reduce
  the change set.
- commit log typo fixes
- switch to use kvm_deassign_ioeventfd_id) when fail to register to
  fast mmio bus
- change kvm_io_bus_cmp() as Paolo's suggestions
- introduce a new capability to avoid new userspace crash old kernel
- add a new patch that only try to register mmio eventfd on fast mmio
  bus

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 (6):
  kvm: don't try to register to KVM_FAST_MMIO_BUS for non mmio eventfd
  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
  kvm: add fast mmio capabilitiy

 Documentation/virtual/kvm/api.txt |   7 ++-
 arch/x86/kvm/trace.h              |  18 ++++++
 arch/x86/kvm/vmx.c                |   1 +
 arch/x86/kvm/x86.c                |   1 +
 include/uapi/linux/kvm.h          |   1 +
 virt/kvm/eventfd.c                | 124 ++++++++++++++++++++++----------------
 virt/kvm/kvm_main.c               |  20 +++++-
 7 files changed, 118 insertions(+), 54 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1224671 — [PATCH V6 6/6] kvm: add fast mmio capabilitiy

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 Documentation/virtual/kvm/api.txt | 7 ++++++-
 include/uapi/linux/kvm.h          | 1 +
 virt/kvm/kvm_main.c               | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index d9eccee..26661ef 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1598,7 +1598,7 @@ provided event instead of triggering an exit.
 struct kvm_ioeventfd {
 	__u64 datamatch;
 	__u64 addr;        /* legal pio/mmio address */
-	__u32 len;         /* 1, 2, 4, or 8 bytes    */
+	__u32 len;         /* 0, 1, 2, 4, or 8 bytes    */
 	__s32 fd;
 	__u32 flags;
 	__u8  pad[36];
@@ -1621,6 +1621,11 @@ to the registered address is equal to datamatch in struct kvm_ioeventfd.
 For virtio-ccw devices, addr contains the subchannel id and datamatch the
 virtqueue index.
 
+With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
+kernel to ignore the length of guest write and get a possible faster
+response. Note the speedup may only work on some specific
+architectures and setups. Otherwise, it's as fast as wildcard mmio
+eventfd.
 
 4.60 KVM_DIRTY_TLB
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index a9256f0..ad72a61 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -824,6 +824,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_MULTI_ADDRESS_SPACE 118
 #define KVM_CAP_GUEST_DEBUG_HW_BPS 119
 #define KVM_CAP_GUEST_DEBUG_HW_WPS 120
+#define KVM_CAP_FAST_MMIO 121
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9af68db..645f55d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2717,6 +2717,7 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
 	case KVM_CAP_IRQFD:
 	case KVM_CAP_IRQFD_RESAMPLE:
 #endif
+	case KVM_CAP_FAST_MMIO:
 	case KVM_CAP_CHECK_EXTENSION_VM:
 		return 1;
 #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225273 — Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy

On 15/09/2015 08:41, Jason Wang wrote:
> +With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
> +kernel to ignore the length of guest write and get a possible faster
> +response. Note the speedup may only work on some specific
> +architectures and setups. Otherwise, it's as fast as wildcard mmio
> +eventfd.

I don't really like tying the capability to MMIO, especially since
zero length ioeventfd is already accepted for virtio-ccw.

What about the following?

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7a3cb48a644d..247944071cc8 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1627,11 +1627,10 @@ to the registered address is equal to datamatch in struct kvm_ioeventfd.
 For virtio-ccw devices, addr contains the subchannel id and datamatch the
 virtqueue index.
 
-With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
-kernel to ignore the length of guest write and get a possible faster
-response. Note the speedup may only work on some specific
-architectures and setups. Otherwise, it's as fast as wildcard mmio
-eventfd.
+With KVM_CAP_IOEVENTFD_ANY_LENGTH, a zero length ioeventfd is allowed, and
+the kernel will ignore the length of guest write and get a faster vmexit.
+The speedup may only apply to specific architectures, but the ioeventfd will
+work anyway.
 
 4.60 KVM_DIRTY_TLB
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index b4f6aeaf94a6..03f3618612aa 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -830,7 +830,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_GUEST_DEBUG_HW_BPS 119
 #define KVM_CAP_GUEST_DEBUG_HW_WPS 120
 #define KVM_CAP_SPLIT_IRQCHIP 121
-#define KVM_CAP_FAST_MMIO 122
+#define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 79db45336e3a..1dc8c45d2270 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -914,9 +914,7 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 		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))
+	if (!args->len && (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH))
 		return -EINVAL;
 
 	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 0780d970d087..0b48aadedcee 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2717,7 +2717,7 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
 	case KVM_CAP_IRQFD:
 	case KVM_CAP_IRQFD_RESAMPLE:
 #endif
-	case KVM_CAP_FAST_MMIO:
+	case KVM_CAP_IOEVENTFD_ANY_LENGTH:
 	case KVM_CAP_CHECK_EXTENSION_VM:
 		return 1;
 #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING


Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225385 — Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy

On Tue, 15 Sep 2015 17:07:55 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/09/2015 08:41, Jason Wang wrote:
> > +With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
> > +kernel to ignore the length of guest write and get a possible faster
> > +response. Note the speedup may only work on some specific
> > +architectures and setups. Otherwise, it's as fast as wildcard mmio
> > +eventfd.
> 
> I don't really like tying the capability to MMIO, especially since
> zero length ioeventfd is already accepted for virtio-ccw.

Actually, zero length ioeventfd does not make sense for virtio-ccw; we
just don't check it (although we probably should).

> 
> What about the following?
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 7a3cb48a644d..247944071cc8 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1627,11 +1627,10 @@ to the registered address is equal to datamatch in struct kvm_ioeventfd.
>  For virtio-ccw devices, addr contains the subchannel id and datamatch the
>  virtqueue index.
> 
> -With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
> -kernel to ignore the length of guest write and get a possible faster
> -response. Note the speedup may only work on some specific
> -architectures and setups. Otherwise, it's as fast as wildcard mmio
> -eventfd.
> +With KVM_CAP_IOEVENTFD_ANY_LENGTH, a zero length ioeventfd is allowed, and
> +the kernel will ignore the length of guest write and get a faster vmexit.

s/get/may get/ ?

> +The speedup may only apply to specific architectures, but the ioeventfd will
> +work anyway.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225393 — Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy

On 15/09/2015 18:13, Cornelia Huck wrote:
> On Tue, 15 Sep 2015 17:07:55 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 15/09/2015 08:41, Jason Wang wrote:
>>> +With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
>>> +kernel to ignore the length of guest write and get a possible faster
>>> +response. Note the speedup may only work on some specific
>>> +architectures and setups. Otherwise, it's as fast as wildcard mmio
>>> +eventfd.
>>
>> I don't really like tying the capability to MMIO, especially since
>> zero length ioeventfd is already accepted for virtio-ccw.
> 
> Actually, zero length ioeventfd does not make sense for virtio-ccw;

Can you explain why?  If there is any non-zero valid length, "wildcard
length" (represented by zero) would also make sense.

Paolo

> we just don't check it (although we probably should).
> 
>>
>> What about the following?
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 7a3cb48a644d..247944071cc8 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1627,11 +1627,10 @@ to the registered address is equal to datamatch in struct kvm_ioeventfd.
>>  For virtio-ccw devices, addr contains the subchannel id and datamatch the
>>  virtqueue index.
>>
>> -With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
>> -kernel to ignore the length of guest write and get a possible faster
>> -response. Note the speedup may only work on some specific
>> -architectures and setups. Otherwise, it's as fast as wildcard mmio
>> -eventfd.
>> +With KVM_CAP_IOEVENTFD_ANY_LENGTH, a zero length ioeventfd is allowed, and
>> +the kernel will ignore the length of guest write and get a faster vmexit.
> 
> s/get/may get/ ?
> 
>> +The speedup may only apply to specific architectures, but the ioeventfd will
>> +work anyway.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225408 — Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy

On Tue, 15 Sep 2015 18:29:49 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/09/2015 18:13, Cornelia Huck wrote:
> > On Tue, 15 Sep 2015 17:07:55 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> On 15/09/2015 08:41, Jason Wang wrote:
> >>> +With KVM_CAP_FAST_MMIO, a zero length mmio eventfd is allowed for
> >>> +kernel to ignore the length of guest write and get a possible faster
> >>> +response. Note the speedup may only work on some specific
> >>> +architectures and setups. Otherwise, it's as fast as wildcard mmio
> >>> +eventfd.
> >>
> >> I don't really like tying the capability to MMIO, especially since
> >> zero length ioeventfd is already accepted for virtio-ccw.
> > 
> > Actually, zero length ioeventfd does not make sense for virtio-ccw;
> 
> Can you explain why?  If there is any non-zero valid length, "wildcard
> length" (represented by zero) would also make sense.

What is a wildcard match supposed to mean in this case? The datamatch
field contains the queue index for the device specified in the address
field. The hypercall interface associated with the eventfd always has
device + queue index in its parameters; there is no interface for
"notify device with all its queues".

But maybe I'm just lacking imagination :)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225415 — Re: [PATCH V6 6/6] kvm: add fast mmio capabilitiy

On 15/09/2015 18:44, Cornelia Huck wrote:
>> > Can you explain why?  If there is any non-zero valid length, "wildcard
>> > length" (represented by zero) would also make sense.
> What is a wildcard match supposed to mean in this case? The datamatch
> field contains the queue index for the device specified in the address
> field. The hypercall interface associated with the eventfd always has
> device + queue index in its parameters; there is no interface for
> "notify device with all its queues".

Ah, I see.  Because all valid virtio-ccw ioeventfds are datamatch, no
valid virtio-ccw ioeventfd is wildcard-length.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224674 — [PATCH V6 2/6] kvm: factor out core eventfd assign/deassign logic

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

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

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index e404806..0829c7f 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,13 @@ fail:
 }
 
 static int
-kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
+kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
+			   struct kvm_ioeventfd *args)
 {
-	enum kvm_bus              bus_idx;
 	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 +891,48 @@ 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);
+}
+
+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);
+}
+
 int
 kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224705 — Re: [PATCH V6 2/6] kvm: factor out core eventfd assign/deassign logic

On Tue, 15 Sep 2015 14:41:55 +0800
Jason Wang <jasowang@redhat.com> wrote:

> This patch factors out core eventfd assign/deassign logic and leaves
> the argument checking and bus index selection to callers.
> 
> Cc: stable@vger.kernel.org
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  virt/kvm/eventfd.c | 85 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 35 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224675 — [PATCH V6 3/6] kvm: fix double free for fast mmio eventfd

We register wildcard mmio eventfd on two buses, once for KVM_MMIO_BUS
and once on KVM_FAST_MMIO_BUS but with a single iodev
instance. This will lead to an issue: kvm_io_bus_destroy() knows
nothing about the devices on two buses pointing to a single dev. Which
will lead to double free[1] during exit. Fix this by allocating two
instances of iodevs then registering 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: stable@vger.kernel.org
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 virt/kvm/eventfd.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 0829c7f..79db453 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 && bus_idx == KVM_MMIO_BUS) {
-		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);
 
@@ -874,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 && p->bus_idx == KVM_MMIO_BUS) {
-			kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
-						  &p->dev);
-		}
 		kvm->buses[bus_idx]->ioeventfd_count--;
 		ioeventfd_release(p);
 		ret = 0;
@@ -894,14 +878,19 @@ 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 && bus_idx == KVM_MMIO_BUS)
+		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
 
-	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
+	return ret;
 }
 
 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 */
@@ -930,7 +919,25 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 			   KVM_IOEVENTFD_FLAG_DATAMATCH))
 		return -EINVAL;
 
-	return kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
+	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 && 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_idx(kvm, bus_idx, args);
+fail:
+	return ret;
 }
 
 int
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224716 — Re: [PATCH V6 3/6] kvm: fix double free for fast mmio eventfd

On Tue, 15 Sep 2015 14:41:56 +0800
Jason Wang <jasowang@redhat.com> wrote:

> We register wildcard mmio eventfd on two buses, once for KVM_MMIO_BUS
> and once on KVM_FAST_MMIO_BUS but with a single iodev
> instance. This will lead to an issue: kvm_io_bus_destroy() knows
> nothing about the devices on two buses pointing to a single dev. Which
> will lead to double free[1] during exit. Fix this by allocating two
> instances of iodevs then registering 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: stable@vger.kernel.org
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  virt/kvm/eventfd.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225280

On 15/09/2015 08:41, 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 and introduce a new
> capability for userspace. 2 is fixed by ignore the actual length if
> the length of iodev is zero in kvm_io_bus_cmp().
> 
> Please review.

Applied to kvm/queue and will send patches 1-4 for 4.3-rc.  Thanks!

Paolo

> Changes from V5:
> - move patch of explicitly checking for KVM_MMIO_BUS to patch 1 and
>   remove the unnecessary checks
> - even more grammar and typo fixes
> - rabase to kvm.git
> - document KVM_CAP_FAST_MMIO
> 
> Changes from V4:
> - move the location of kvm_assign_ioeventfd() in patch 1 which reduce
>   the change set.
> - commit log typo fixes
> - switch to use kvm_deassign_ioeventfd_id) when fail to register to
>   fast mmio bus
> - change kvm_io_bus_cmp() as Paolo's suggestions
> - introduce a new capability to avoid new userspace crash old kernel
> - add a new patch that only try to register mmio eventfd on fast mmio
>   bus
> 
> 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 (6):
>   kvm: don't try to register to KVM_FAST_MMIO_BUS for non mmio eventfd
>   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
>   kvm: add fast mmio capabilitiy
> 
>  Documentation/virtual/kvm/api.txt |   7 ++-
>  arch/x86/kvm/trace.h              |  18 ++++++
>  arch/x86/kvm/vmx.c                |   1 +
>  arch/x86/kvm/x86.c                |   1 +
>  include/uapi/linux/kvm.h          |   1 +
>  virt/kvm/eventfd.c                | 124 ++++++++++++++++++++++----------------
>  virt/kvm/kvm_main.c               |  20 +++++-
>  7 files changed, 118 insertions(+), 54 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225519

On Tue, Sep 15, 2015 at 05:08:49PM +0200, Paolo Bonzini wrote:
> 
> 
> On 15/09/2015 08:41, 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 and introduce a new
> > capability for userspace. 2 is fixed by ignore the actual length if
> > the length of iodev is zero in kvm_io_bus_cmp().
> > 
> > Please review.
> 
> Applied to kvm/queue and will send patches 1-4 for 4.3-rc.  Thanks!
> 
> Paolo

I'd prefer at least 6 to be there as well:
without 6 userspace can't safely use the code, and without 5,
it can't trace it.

> > Changes from V5:
> > - move patch of explicitly checking for KVM_MMIO_BUS to patch 1 and
> >   remove the unnecessary checks
> > - even more grammar and typo fixes
> > - rabase to kvm.git
> > - document KVM_CAP_FAST_MMIO
> > 
> > Changes from V4:
> > - move the location of kvm_assign_ioeventfd() in patch 1 which reduce
> >   the change set.
> > - commit log typo fixes
> > - switch to use kvm_deassign_ioeventfd_id) when fail to register to
> >   fast mmio bus
> > - change kvm_io_bus_cmp() as Paolo's suggestions
> > - introduce a new capability to avoid new userspace crash old kernel
> > - add a new patch that only try to register mmio eventfd on fast mmio
> >   bus
> > 
> > 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 (6):
> >   kvm: don't try to register to KVM_FAST_MMIO_BUS for non mmio eventfd
> >   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
> >   kvm: add fast mmio capabilitiy
> > 
> >  Documentation/virtual/kvm/api.txt |   7 ++-
> >  arch/x86/kvm/trace.h              |  18 ++++++
> >  arch/x86/kvm/vmx.c                |   1 +
> >  arch/x86/kvm/x86.c                |   1 +
> >  include/uapi/linux/kvm.h          |   1 +
> >  virt/kvm/eventfd.c                | 124 ++++++++++++++++++++++----------------
> >  virt/kvm/kvm_main.c               |  20 +++++-
> >  7 files changed, 118 insertions(+), 54 deletions(-)
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225854

On 15/09/2015 21:26, Michael S. Tsirkin wrote:
> > Applied to kvm/queue and will send patches 1-4 for 4.3-rc.  Thanks!
> 
> I'd prefer at least 6 to be there as well:
> without 6 userspace can't safely use the code, and without 5,
> it can't trace it.

The idea is to just make old userspace work without crashing.  New
features do not belong in stable releases.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web