Virtualization - Feb 2013 - [PATCH v3 0/5] kvm: Make ioeventfd usable on s390.

On Mon, Feb 25, 2013 at 04:27:45PM +0100, Cornelia Huck
wrote:
> Here's the latest version of my patch series enabling ioeventfds
> on s390, again against kvm-next.
> 
> Patches 1 and 2 (cleaning up initialization and exporting the virtio-ccw
> api) would make sense even independent of the ioeventfd enhancements.
> 
> Patches 3-5 are concerned with adding a new type of ioeventfds for
> virtio-ccw notifications on s390. The naming is now hopefully clearer.
> We won't add ioeventfd support for the legacy s390-virtio transport.
> 
> Please consider applying.
I just had a thought: this makes us lookup the device on the bus
for each notification. It would be better to simply get the
device index from guest instead.

We could validate that it matches the correct device,
if not - fallback to the current linear scan.

We could return the index to guest for the next call.

I know this needs guest changes but it's still not too late to
fix this for 3.9 guests so that we won't need to worry
about compatibility going forward.

Hmm?
> v2 -> v3:
> - Added a patch exporting the virtio-ccw api and use it for the
>   diagnose implementation.
> - Better naming: We're dealing with virtio-ccw notifications only.
> v1 -> v2:
> - Move irqfd initialization from a module init function to kvm_init,
>   eliminating the need for a second module for kvm/s390.
> - Use kvm_io_device for s390 css devices.
> 
> Cornelia Huck (5):
>   KVM: Initialize irqfd from kvm_init().
>   KVM: s390: Export virtio-ccw api.
>   KVM: Introduce KVM_VIRTIO_CCW_NOTIFY_BUS.
>   KVM: ioeventfd for virtio-ccw devices.
>   KVM: s390: Wire up ioeventfd.
> 
>  Documentation/virtual/kvm/api.txt       |  8 ++++++++
>  arch/s390/include/uapi/asm/Kbuild       |  1 +
>  arch/s390/include/uapi/asm/virtio-ccw.h | 21 +++++++++++++++++++++
>  arch/s390/kvm/Kconfig                   |  1 +
>  arch/s390/kvm/Makefile                  |  2 +-
>  arch/s390/kvm/diag.c                    | 26 ++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c                |  1 +
>  drivers/s390/kvm/virtio_ccw.c           |  5 +----
>  include/linux/kvm_host.h                | 14 ++++++++++++++
>  include/uapi/linux/kvm.h                |  3 +++
>  virt/kvm/eventfd.c                      | 21 ++++++++++++++-------
>  virt/kvm/kvm_main.c                     |  6 ++++++
>  12 files changed, 97 insertions(+), 12 deletions(-)
>  create mode 100644 arch/s390/include/uapi/asm/virtio-ccw.h
> 
> -- 
> 1.7.12.4

On Tue, Feb 26, 2013 at 01:04:21PM +0200, Michael S. Tsirkin
wrote:
> On Mon, Feb 25, 2013 at 04:27:45PM +0100, Cornelia Huck wrote:
> > Here's the latest version of my patch series enabling ioeventfds
> > on s390, again against kvm-next.
> > 
> > Patches 1 and 2 (cleaning up initialization and exporting the
virtio-ccw
> > api) would make sense even independent of the ioeventfd enhancements.
> > 
> > Patches 3-5 are concerned with adding a new type of ioeventfds for
> > virtio-ccw notifications on s390. The naming is now hopefully clearer.
> > We won't add ioeventfd support for the legacy s390-virtio
transport.
> > 
> > Please consider applying.
> 
> I just had a thought: this makes us lookup the device on the bus
> for each notification. It would be better to simply get the
> device index from guest instead.
> 
> We could validate that it matches the correct device,
> if not - fallback to the current linear scan.
> 
> We could return the index to guest for the next call.
> 
> I know this needs guest changes but it's still not too late to
> fix this for 3.9 guests so that we won't need to worry
> about compatibility going forward.
> 
> Hmm?
And just to clarify, here's what I mean (BTW, why doesn't
this code use the interfaces from kvm_para.h?)
I think it's a good idea to merge this before 3.9 so we don't
need to worry about legacy going forward.

Completely untested, just to give you the idea.

--->
virtio_ccw: pass a cookie value to kvm hypercall

Lookups by channel/vq pair on host during virtio notifications might be
expensive.  Interpret hypercall return value as a cookie which host can
use to do device lookups for the next notification more efficiently.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>

---

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 2029b6c..1054f3a 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -77,6 +77,7 @@ struct virtio_ccw_vq_info {
 	void *queue;
 	struct vq_info_block *info_block;
 	struct list_head node;
+	long cookie;
 };
 
 #define KVM_VIRTIO_CCW_RING_ALIGN 4096
@@ -145,15 +146,18 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev,
 }
 
 static inline long do_kvm_notify(struct subchannel_id schid,
-				 unsigned long queue_index)
+				 unsigned long queue_index,
+				 long cookie)
 {
 	register unsigned long __nr asm("1") = KVM_S390_VIRTIO_CCW_NOTIFY;
 	register struct subchannel_id __schid asm("2") = schid;
 	register unsigned long __index asm("3") = queue_index;
 	register long __rc asm("2");
+	register long __cookie asm("4") = cookie;
 
 	asm volatile ("diag 2,4,0x500\n"
-		      : "=d" (__rc) : "d" (__nr), "d"
(__schid), "d" (__index)
+		      : "=d" (__rc) : "d" (__nr), "d"
(__schid), "d" (__index),
+		      "d"(__cookie)
 		      : "memory", "cc");
 	return __rc;
 }
@@ -166,7 +170,7 @@ static void virtio_ccw_kvm_notify(struct virtqueue *vq)
 
 	vcdev = to_vc_device(info->vq->vdev);
 	ccw_device_get_schid(vcdev->cdev, &schid);
-	do_kvm_notify(schid, virtqueue_get_queue_index(vq));
+	info->cookie = do_kvm_notify(schid, virtqueue_get_queue_index(vq),
info->cookie);
 }
 
 static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,

On 26/02/13 12:04, Michael S. Tsirkin wrote:
> On Mon, Feb 25, 2013 at 04:27:45PM +0100, Cornelia Huck wrote:
>> Here's the latest version of my patch series enabling ioeventfds
>> on s390, again against kvm-next.
>>
>> Patches 1 and 2 (cleaning up initialization and exporting the
virtio-ccw
>> api) would make sense even independent of the ioeventfd enhancements.
>>
>> Patches 3-5 are concerned with adding a new type of ioeventfds for
>> virtio-ccw notifications on s390. The naming is now hopefully clearer.
>> We won't add ioeventfd support for the legacy s390-virtio
transport.
>>
>> Please consider applying.
> 
> I just had a thought: this makes us lookup the device on the bus
> for each notification. It would be better to simply get the
> device index from guest instead.
> 
> We could validate that it matches the correct device,
> if not - fallback to the current linear scan.
> 
> We could return the index to guest for the next call.
> 
> I know this needs guest changes but it's still not too late to
> fix this for 3.9 guests so that we won't need to worry
> about compatibility going forward.
> 
Hmm, this would certainly have the best scalability, but such
a change would require adotions to the virtio spec and getting this
fully tested till 3.9 seems somewhat dangerous. 
So I would prefer to actually improve the lookup (e.g. with a tree or hash)
in kvm_io_bus_write or something like that as a first step.

We also have some guests in the wild  (sles11sp3 beta) that already
use the current interface, so compatibility is already a an interesting
aspect (does being a beta counts?)

Just thinking loud here, we might be able to do this in a compatible way.
The vq number that we pass is 64bit, but we dont need such a big range. The
guest could use the upper 32 bit as a cookie (old code will have 0 ---> list
traversal). We must have a query cookie function, that will return 0 on
older qemus, though. Such an optimization could be added later on without
needing to rush.

Ideas, opinions?

Christian

On 26/02/13 12:18, Michael S. Tsirkin wrote:
> virtio_ccw: pass a cookie value to kvm hypercall
> 
> Lookups by channel/vq pair on host during virtio notifications might be
> expensive.  Interpret hypercall return value as a cookie which host can
> use to do device lookups for the next notification more efficiently.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> 
> ---
> 
> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index 2029b6c..1054f3a 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -77,6 +77,7 @@ struct virtio_ccw_vq_info {
>  	void *queue;
>  	struct vq_info_block *info_block;
>  	struct list_head node;
> +	long cookie;
>  };
> 
>  #define KVM_VIRTIO_CCW_RING_ALIGN 4096
> @@ -145,15 +146,18 @@ static int ccw_io_helper(struct virtio_ccw_device
*vcdev,
>  }
> 
>  static inline long do_kvm_notify(struct subchannel_id schid,
> -				 unsigned long queue_index)
> +				 unsigned long queue_index,
> +				 long cookie)
>  {
>  	register unsigned long __nr asm("1") =
KVM_S390_VIRTIO_CCW_NOTIFY;
>  	register struct subchannel_id __schid asm("2") = schid;
>  	register unsigned long __index asm("3") = queue_index;
>  	register long __rc asm("2");
> +	register long __cookie asm("4") = cookie;
> 
>  	asm volatile ("diag 2,4,0x500\n"
> -		      : "=d" (__rc) : "d" (__nr), "d"
(__schid), "d" (__index)
> +		      : "=d" (__rc) : "d" (__nr), "d"
(__schid), "d" (__index),
> +		      "d"(__cookie)
>  		      : "memory", "cc");
>  	return __rc;
>  }
> @@ -166,7 +170,7 @@ static void virtio_ccw_kvm_notify(struct virtqueue *vq)
> 
>  	vcdev = to_vc_device(info->vq->vdev);
>  	ccw_device_get_schid(vcdev->cdev, &schid);
> -	do_kvm_notify(schid, virtqueue_get_queue_index(vq));
> +	info->cookie = do_kvm_notify(schid, virtqueue_get_queue_index(vq),
info->cookie);
>  }
> 
>  static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,

Hmmm, forget my last mail. This actually could be even forward and backward
compatible.
In the virtio spec we will not define the cookie format (just 64bit int). That
will allow
qemu or future kernels to use that for other things (as long as a validity check
is
possible) if we dont have a kvm bus.

Now:

old guest, old host: 
works.

old guest, new host: 
the cookie from the guest contains junk, the host needs to detect that the
cookie is
junk and ignores it. It will return the new cookie anyway. 

new guest, old host:
The guest will get a junk cookie and pass it back to the host. But the host will
ignore
it anyway.

new guest, new host:
works.

So...
Acked-by: Christian Borntraeger <borntraeger at de.ibm.com>

On 26/02/13 12:18, Michael S. Tsirkin wrote:
> virtio_ccw: pass a cookie value to kvm hypercall
> 
> Lookups by channel/vq pair on host during virtio notifications might be
> expensive.  Interpret hypercall return value as a cookie which host can
> use to do device lookups for the next notification more efficiently.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Seems to work fine. (as expected).
Tested-by: Christian Borntraeger <borntraeger at de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger at de.ibm.com>
> 
> ---
> 
> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index 2029b6c..1054f3a 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -77,6 +77,7 @@ struct virtio_ccw_vq_info {
>  	void *queue;
>  	struct vq_info_block *info_block;
>  	struct list_head node;
> +	long cookie;
>  };
> 
>  #define KVM_VIRTIO_CCW_RING_ALIGN 4096
> @@ -145,15 +146,18 @@ static int ccw_io_helper(struct virtio_ccw_device
*vcdev,
>  }
> 
>  static inline long do_kvm_notify(struct subchannel_id schid,
> -				 unsigned long queue_index)
> +				 unsigned long queue_index,
> +				 long cookie)
>  {
>  	register unsigned long __nr asm("1") =
KVM_S390_VIRTIO_CCW_NOTIFY;
>  	register struct subchannel_id __schid asm("2") = schid;
>  	register unsigned long __index asm("3") = queue_index;
>  	register long __rc asm("2");
> +	register long __cookie asm("4") = cookie;
> 
>  	asm volatile ("diag 2,4,0x500\n"
> -		      : "=d" (__rc) : "d" (__nr), "d"
(__schid), "d" (__index)
> +		      : "=d" (__rc) : "d" (__nr), "d"
(__schid), "d" (__index),
> +		      "d"(__cookie)
>  		      : "memory", "cc");
>  	return __rc;
>  }
> @@ -166,7 +170,7 @@ static void virtio_ccw_kvm_notify(struct virtqueue *vq)
> 
>  	vcdev = to_vc_device(info->vq->vdev);
>  	ccw_device_get_schid(vcdev->cdev, &schid);
> -	do_kvm_notify(schid, virtqueue_get_queue_index(vq));
> +	info->cookie = do_kvm_notify(schid, virtqueue_get_queue_index(vq),
info->cookie);
>  }
> 
>  static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,
>