Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-24 Thread Michael S. Tsirkin 
On Fri, Feb 22, 2013 at 08:22:08AM +0100, Cornelia Huck wrote:
 On Thu, 21 Feb 2013 22:42:41 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Thu, Feb 21, 2013 at 07:14:31PM +0100, Cornelia Huck wrote:
   On Thu, 21 Feb 2013 18:34:59 +0200
   Michael S. Tsirkin m...@redhat.com wrote:
   
On Thu, Feb 21, 2013 at 04:21:43PM +0100, Cornelia Huck wrote:
 On Thu, 21 Feb 2013 16:39:05 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
   As s390 doesn't use memory writes for virtio notifcations, create
   a special kind of ioeventfd instead that hooks up into diagnose
   0x500 (kvm hypercall) with function code 3 (virtio-ccw 
   notification).
   
   Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
  
  Do we really have to put virtio specific stuff into kvm?
  How about we add generic functionality to match GPRs
  on a hypercall and signal an eventfd?
 
 Worth a try implementing that.
 
  
  Also, it's a bit unfortunate that this doesn't use
  the io bus datastructure, long term the linked list handling
  might become a bottleneck, using shared code this could maybe
  benefit from performance optimizations there.
 
 The linked list stuff was more like an initial implementation that
 could be improved later.
 
  io bus data structure currently has the ability to match on
  two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
  Isn't this sufficient for your purposes?
  How about sticking subchannel id in address, vq in data match
  and using io bus?
 
 I can give that a try. (I must admit that I didn't look at the iobus
 stuff in detail.)
 
  
  BTW maybe we could do this for the user interface too,
  while I'm not 100% sure it's the cleanest thing to do
  (or will work), it would certainly minimize the patchset.
 
 You mean integrating with the generic interface and dropping the new
 ARCH flag?

Not sure about the flag but we could use the general structure
without an arch-specific format, if that's a good fit.

   So I have something that seems to do what I want. I'll see if I can
   morph it into something presentable tomorrow.
   
   diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
   index b58dd86..3c43e30 100644
   --- a/arch/s390/kvm/Kconfig
   +++ b/arch/s390/kvm/Kconfig
   @@ -22,6 +22,7 @@ config KVM
 select PREEMPT_NOTIFIERS
 select ANON_INODES
 select HAVE_KVM_CPU_RELAX_INTERCEPT
   + select HAVE_KVM_EVENTFD
 ---help---
   Support hosting paravirtualized guest machines using the SIE
   virtualization capability on the mainframe. This should work
   diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
   index 3975722..8fe9d65 100644
   --- a/arch/s390/kvm/Makefile
   +++ b/arch/s390/kvm/Makefile
   @@ -6,7 +6,7 @@
# it under the terms of the GNU General Public License (version 2 only)
# as published by the Free Software Foundation.

   -common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
   +common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)

ccflags-y := -Ivirt/kvm -Iarch/s390/kvm

   diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
   index a390687..7fc195e 100644
   --- a/arch/s390/kvm/diag.c
   +++ b/arch/s390/kvm/diag.c
   @@ -104,6 +104,20 @@ static int __diag_ipl_functions(struct kvm_vcpu 
   *vcpu)
 return -EREMOTE;
}

   +static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
   +{
   + int ret, idx;
   + u64 vq = vcpu-run-s.regs.gprs[3];
   +
   + idx = srcu_read_lock(vcpu-kvm-srcu);
   + ret = kvm_io_bus_write(vcpu-kvm, KVM_CSS_BUS,
   + vcpu-run-s.regs.gprs[2],
   + vcpu-run-s.regs.gprs[1],
   + vq);
  
  Hmm, do you really need three gprs? If not, we could
  just pass len == 8, which would be cleaner.
  Also might as well drop the vq variable, no?
 
 If I want to pass generic hypercalls, I need to pass the subcode (in
 gpr 1) in the len variable. If all we'll ever need is the virtio-ccw
 notify hypercall, gprs 2 and 3 are enough. Would perhaps make the
 common code less hacky, but we'd lose (unneeded?) flexibility.

I see. I'm fine with either way, but I note the code above
can overflow int len, which is likely not what you want.


  
   + srcu_read_unlock(vcpu-kvm-srcu, idx);
   + return ret;
   +}
   +
int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
{
 int code = (vcpu-arch.sie_block-ipb  0xfff)  16;
   @@ -118,6 +132,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 return __diag_time_slice_end_directed(vcpu);
 case 0x308:
 return __diag_ipl_functions(vcpu);
   + case 0x500:
   + return __diag_virtio_hypercall(vcpu);
 default:
 return -EOPNOTSUPP;
 }

[Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Cornelia Huck 
As s390 doesn't use memory writes for virtio notifcations, create
a special kind of ioeventfd instead that hooks up into diagnose
0x500 (kvm hypercall) with function code 3 (virtio-ccw notification).

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 arch/s390/include/asm/kvm_host.h |  23 ++
 arch/s390/kvm/Kconfig|   1 +
 arch/s390/kvm/Makefile   |   2 +-
 arch/s390/kvm/diag.c |  23 ++
 arch/s390/kvm/kvm-s390.c | 165 +++
 arch/s390/kvm/kvm-s390.h |   3 +
 6 files changed, 216 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 16bd5d1..8dad9dc 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -18,6 +18,7 @@
 #include linux/kvm_host.h
 #include asm/debug.h
 #include asm/cpu.h
+#include asm/schid.h
 
 #define KVM_MAX_VCPUS 64
 #define KVM_USER_MEM_SLOTS 32
@@ -262,8 +263,30 @@ struct kvm_arch{
debug_info_t *dbf;
struct kvm_s390_float_interrupt float_int;
struct gmap *gmap;
+   struct list_head sch_fds;
+   struct rw_semaphore sch_fds_sem;
int css_support;
 };
 
 extern int sie64a(struct kvm_s390_sie_block *, u64 *);
+#define __KVM_HAVE_ARCH_IOEVENTFD
+
+#define KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY 1
+
+struct kvm_s390_ioeventfd_data {
+   __u8 type;
+   union {
+   /* VIRTIO_CCW_NOTIFY */
+   struct {
+   __u64 vq;
+   struct subchannel_id schid;
+   } virtio_ccw_vq;
+   char padding[35];
+   };
+} __packed;
+
+struct kvm_arch_ioeventfd {
+   struct list_head entry;
+   struct kvm_s390_ioeventfd_data data;
+};
 #endif
diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index b58dd86..3c43e30 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -22,6 +22,7 @@ config KVM
select PREEMPT_NOTIFIERS
select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
+   select HAVE_KVM_EVENTFD
---help---
  Support hosting paravirtualized guest machines using the SIE
  virtualization capability on the mainframe. This should work
diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index 2441ffd..dbd8cc9 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -6,7 +6,7 @@
 # it under the terms of the GNU General Public License (version 2 only)
 # as published by the Free Software Foundation.
 
-common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
+common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)
 
 ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
 
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index a390687..51ea66f 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -104,6 +104,27 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
return -EREMOTE;
 }
 
+static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
+{
+   struct kvm_s390_ioeventfd_data data;
+   u32 tmp;
+
+   /* No channel I/O? Get out quickly. */
+   if (!vcpu-kvm-arch.css_support ||
+   (vcpu-run-s.regs.gprs[1] != 3))
+   return -EOPNOTSUPP;
+
+   /* subchannel id is in gpr 2, queue in gpr 3 */
+   tmp = vcpu-run-s.regs.gprs[2]  0x;
+   memcpy(data.virtio_ccw_vq.schid, tmp,
+  sizeof(data.virtio_ccw_vq.schid));
+   data.virtio_ccw_vq.vq = vcpu-run-s.regs.gprs[3];
+   data.type = KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY;
+
+   /* If signalling via eventfd fails, we want to drop to userspace. */
+   return kvm_s390_ioeventfd_signal(vcpu-kvm, data) ? -EOPNOTSUPP : 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
int code = (vcpu-arch.sie_block-ipb  0xfff)  16;
@@ -118,6 +139,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
return __diag_time_slice_end_directed(vcpu);
case 0x308:
return __diag_ipl_functions(vcpu);
+   case 0x500:
+   return __diag_virtio_hypercall(vcpu);
default:
return -EOPNOTSUPP;
}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 58a5f03..cd9eb0e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -15,6 +15,7 @@
 
 #include linux/compiler.h
 #include linux/err.h
+#include linux/eventfd.h
 #include linux/fs.h
 #include linux/hrtimer.h
 #include linux/init.h
@@ -143,6 +144,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_ONE_REG:
case KVM_CAP_ENABLE_CAP:
case KVM_CAP_S390_CSS_SUPPORT:
+   case KVM_CAP_IOEVENTFD:
r = 1;
break;
case KVM_CAP_NR_VCPUS:
@@ -237,6 +239,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
if (!kvm-arch.gmap)
goto out_nogmap;
}
+

Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Michael S. Tsirkin 
On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
 As s390 doesn't use memory writes for virtio notifcations, create
 a special kind of ioeventfd instead that hooks up into diagnose
 0x500 (kvm hypercall) with function code 3 (virtio-ccw notification).
 
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com

Do we really have to put virtio specific stuff into kvm?
How about we add generic functionality to match GPRs
on a hypercall and signal an eventfd?

Also, it's a bit unfortunate that this doesn't use
the io bus datastructure, long term the linked list handling
might become a bottleneck, using shared code this could maybe
benefit from performance optimizations there.
io bus data structure currently has the ability to match on
two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
Isn't this sufficient for your purposes?
How about sticking subchannel id in address, vq in data match
and using io bus?

BTW maybe we could do this for the user interface too,
while I'm not 100% sure it's the cleanest thing to do
(or will work), it would certainly minimize the patchset.

 ---
  arch/s390/include/asm/kvm_host.h |  23 ++
  arch/s390/kvm/Kconfig|   1 +
  arch/s390/kvm/Makefile   |   2 +-
  arch/s390/kvm/diag.c |  23 ++
  arch/s390/kvm/kvm-s390.c | 165 
 +++
  arch/s390/kvm/kvm-s390.h |   3 +
  6 files changed, 216 insertions(+), 1 deletion(-)
 
 diff --git a/arch/s390/include/asm/kvm_host.h 
 b/arch/s390/include/asm/kvm_host.h
 index 16bd5d1..8dad9dc 100644
 --- a/arch/s390/include/asm/kvm_host.h
 +++ b/arch/s390/include/asm/kvm_host.h
 @@ -18,6 +18,7 @@
  #include linux/kvm_host.h
  #include asm/debug.h
  #include asm/cpu.h
 +#include asm/schid.h
  
  #define KVM_MAX_VCPUS 64
  #define KVM_USER_MEM_SLOTS 32
 @@ -262,8 +263,30 @@ struct kvm_arch{
   debug_info_t *dbf;
   struct kvm_s390_float_interrupt float_int;
   struct gmap *gmap;
 + struct list_head sch_fds;
 + struct rw_semaphore sch_fds_sem;

Why sch_? Related to subchannel somehow?
Also you mean _ioeventfds really?
Might be a good idea to document locking here.

   int css_support;
  };
  
  extern int sie64a(struct kvm_s390_sie_block *, u64 *);
 +#define __KVM_HAVE_ARCH_IOEVENTFD
 +
 +#define KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY 1
 +
 +struct kvm_s390_ioeventfd_data {
 + __u8 type;
 + union {
 + /* VIRTIO_CCW_NOTIFY */
 + struct {
 + __u64 vq;
 + struct subchannel_id schid;
 + } virtio_ccw_vq;
 + char padding[35];
 + };
 +} __packed;
 +

Do you expect userspace to use this structure?
If yes this is the wrong header. If not why is it packed?

 +struct kvm_arch_ioeventfd {
 + struct list_head entry;
 + struct kvm_s390_ioeventfd_data data;

Let's not waste memory keeping padding in kernel datastructures.

 +};
  #endif
 diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
 index b58dd86..3c43e30 100644
 --- a/arch/s390/kvm/Kconfig
 +++ b/arch/s390/kvm/Kconfig
 @@ -22,6 +22,7 @@ config KVM
   select PREEMPT_NOTIFIERS
   select ANON_INODES
   select HAVE_KVM_CPU_RELAX_INTERCEPT
 + select HAVE_KVM_EVENTFD
   ---help---
 Support hosting paravirtualized guest machines using the SIE
 virtualization capability on the mainframe. This should work
 diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
 index 2441ffd..dbd8cc9 100644
 --- a/arch/s390/kvm/Makefile
 +++ b/arch/s390/kvm/Makefile
 @@ -6,7 +6,7 @@
  # it under the terms of the GNU General Public License (version 2 only)
  # as published by the Free Software Foundation.
  
 -common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
 +common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)
  
  ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
  
 diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
 index a390687..51ea66f 100644
 --- a/arch/s390/kvm/diag.c
 +++ b/arch/s390/kvm/diag.c
 @@ -104,6 +104,27 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
   return -EREMOTE;
  }
  
 +static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 +{
 + struct kvm_s390_ioeventfd_data data;
 + u32 tmp;
 +
 + /* No channel I/O? Get out quickly. */
 + if (!vcpu-kvm-arch.css_support ||
 + (vcpu-run-s.regs.gprs[1] != 3))
 + return -EOPNOTSUPP;
 +
 + /* subchannel id is in gpr 2, queue in gpr 3 */
 + tmp = vcpu-run-s.regs.gprs[2]  0x;
 + memcpy(data.virtio_ccw_vq.schid, tmp,
 +sizeof(data.virtio_ccw_vq.schid));
 + data.virtio_ccw_vq.vq = vcpu-run-s.regs.gprs[3];
 + data.type = KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY;
 +
 + /* If signalling via eventfd fails, we want to drop to userspace. */
 + return kvm_s390_ioeventfd_signal(vcpu-kvm, data) ? -EOPNOTSUPP : 0;
 +}
 +
  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
  {
   int

Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Cornelia Huck 
On Thu, 21 Feb 2013 16:39:05 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
  As s390 doesn't use memory writes for virtio notifcations, create
  a special kind of ioeventfd instead that hooks up into diagnose
  0x500 (kvm hypercall) with function code 3 (virtio-ccw notification).
  
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 
 Do we really have to put virtio specific stuff into kvm?
 How about we add generic functionality to match GPRs
 on a hypercall and signal an eventfd?

Worth a try implementing that.

 
 Also, it's a bit unfortunate that this doesn't use
 the io bus datastructure, long term the linked list handling
 might become a bottleneck, using shared code this could maybe
 benefit from performance optimizations there.

The linked list stuff was more like an initial implementation that
could be improved later.

 io bus data structure currently has the ability to match on
 two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
 Isn't this sufficient for your purposes?
 How about sticking subchannel id in address, vq in data match
 and using io bus?

I can give that a try. (I must admit that I didn't look at the iobus
stuff in detail.)

 
 BTW maybe we could do this for the user interface too,
 while I'm not 100% sure it's the cleanest thing to do
 (or will work), it would certainly minimize the patchset.

You mean integrating with the generic interface and dropping the new
ARCH flag?

 
  ---
   arch/s390/include/asm/kvm_host.h |  23 ++
   arch/s390/kvm/Kconfig|   1 +
   arch/s390/kvm/Makefile   |   2 +-
   arch/s390/kvm/diag.c |  23 ++
   arch/s390/kvm/kvm-s390.c | 165 
  +++
   arch/s390/kvm/kvm-s390.h |   3 +
   6 files changed, 216 insertions(+), 1 deletion(-)
  
  diff --git a/arch/s390/include/asm/kvm_host.h 
  b/arch/s390/include/asm/kvm_host.h
  index 16bd5d1..8dad9dc 100644
  --- a/arch/s390/include/asm/kvm_host.h
  +++ b/arch/s390/include/asm/kvm_host.h
  @@ -18,6 +18,7 @@
   #include linux/kvm_host.h
   #include asm/debug.h
   #include asm/cpu.h
  +#include asm/schid.h
   
   #define KVM_MAX_VCPUS 64
   #define KVM_USER_MEM_SLOTS 32
  @@ -262,8 +263,30 @@ struct kvm_arch{
  debug_info_t *dbf;
  struct kvm_s390_float_interrupt float_int;
  struct gmap *gmap;
  +   struct list_head sch_fds;
  +   struct rw_semaphore sch_fds_sem;
 
 Why sch_? Related to subchannel somehow?

Yes.

 Also you mean _ioeventfds really?

Probably, I don't have the irqfd stuff figured out yet.

 Might be a good idea to document locking here.

OK.

 
  int css_support;
   };
   
   extern int sie64a(struct kvm_s390_sie_block *, u64 *);
  +#define __KVM_HAVE_ARCH_IOEVENTFD
  +
  +#define KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY 1
  +
  +struct kvm_s390_ioeventfd_data {
  +   __u8 type;
  +   union {
  +   /* VIRTIO_CCW_NOTIFY */
  +   struct {
  +   __u64 vq;
  +   struct subchannel_id schid;
  +   } virtio_ccw_vq;
  +   char padding[35];
  +   };
  +} __packed;
  +
 
 Do you expect userspace to use this structure?
 If yes this is the wrong header. If not why is it packed?

Indeed, userspace is supposed to use this.

 
  +struct kvm_arch_ioeventfd {
  +   struct list_head entry;
  +   struct kvm_s390_ioeventfd_data data;
 
 Let's not waste memory keeping padding in kernel datastructures.
 
  +};
   #endif
  diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
  index b58dd86..3c43e30 100644
  --- a/arch/s390/kvm/Kconfig
  +++ b/arch/s390/kvm/Kconfig
  @@ -22,6 +22,7 @@ config KVM
  select PREEMPT_NOTIFIERS
  select ANON_INODES
  select HAVE_KVM_CPU_RELAX_INTERCEPT
  +   select HAVE_KVM_EVENTFD
  ---help---
Support hosting paravirtualized guest machines using the SIE
virtualization capability on the mainframe. This should work
  diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
  index 2441ffd..dbd8cc9 100644
  --- a/arch/s390/kvm/Makefile
  +++ b/arch/s390/kvm/Makefile
  @@ -6,7 +6,7 @@
   # it under the terms of the GNU General Public License (version 2 only)
   # as published by the Free Software Foundation.
   
  -common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
  +common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)
   
   ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
   
  diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
  index a390687..51ea66f 100644
  --- a/arch/s390/kvm/diag.c
  +++ b/arch/s390/kvm/diag.c
  @@ -104,6 +104,27 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
  return -EREMOTE;
   }
   
  +static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
  +{
  +   struct kvm_s390_ioeventfd_data data;
  +   u32 tmp;
  +
  +   /* No channel I/O? Get out quickly. */
  +   if (!vcpu-kvm-arch.css_support ||
  +   (vcpu-run-s.regs.gprs[1] != 3))
  +

Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Michael S. Tsirkin 
On Thu, Feb 21, 2013 at 04:21:43PM +0100, Cornelia Huck wrote:
 On Thu, 21 Feb 2013 16:39:05 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
   As s390 doesn't use memory writes for virtio notifcations, create
   a special kind of ioeventfd instead that hooks up into diagnose
   0x500 (kvm hypercall) with function code 3 (virtio-ccw notification).
   
   Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
  
  Do we really have to put virtio specific stuff into kvm?
  How about we add generic functionality to match GPRs
  on a hypercall and signal an eventfd?
 
 Worth a try implementing that.
 
  
  Also, it's a bit unfortunate that this doesn't use
  the io bus datastructure, long term the linked list handling
  might become a bottleneck, using shared code this could maybe
  benefit from performance optimizations there.
 
 The linked list stuff was more like an initial implementation that
 could be improved later.
 
  io bus data structure currently has the ability to match on
  two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
  Isn't this sufficient for your purposes?
  How about sticking subchannel id in address, vq in data match
  and using io bus?
 
 I can give that a try. (I must admit that I didn't look at the iobus
 stuff in detail.)
 
  
  BTW maybe we could do this for the user interface too,
  while I'm not 100% sure it's the cleanest thing to do
  (or will work), it would certainly minimize the patchset.
 
 You mean integrating with the generic interface and dropping the new
 ARCH flag?

Not sure about the flag but we could use the general structure
without an arch-specific format, if that's a good fit.

  
   ---
arch/s390/include/asm/kvm_host.h |  23 ++
arch/s390/kvm/Kconfig|   1 +
arch/s390/kvm/Makefile   |   2 +-
arch/s390/kvm/diag.c |  23 ++
arch/s390/kvm/kvm-s390.c | 165 
   +++
arch/s390/kvm/kvm-s390.h |   3 +
6 files changed, 216 insertions(+), 1 deletion(-)
   
   diff --git a/arch/s390/include/asm/kvm_host.h 
   b/arch/s390/include/asm/kvm_host.h
   index 16bd5d1..8dad9dc 100644
   --- a/arch/s390/include/asm/kvm_host.h
   +++ b/arch/s390/include/asm/kvm_host.h
   @@ -18,6 +18,7 @@
#include linux/kvm_host.h
#include asm/debug.h
#include asm/cpu.h
   +#include asm/schid.h

#define KVM_MAX_VCPUS 64
#define KVM_USER_MEM_SLOTS 32
   @@ -262,8 +263,30 @@ struct kvm_arch{
 debug_info_t *dbf;
 struct kvm_s390_float_interrupt float_int;
 struct gmap *gmap;
   + struct list_head sch_fds;
   + struct rw_semaphore sch_fds_sem;
  
  Why sch_? Related to subchannel somehow?
 
 Yes.
 
  Also you mean _ioeventfds really?
 
 Probably, I don't have the irqfd stuff figured out yet.
 
  Might be a good idea to document locking here.
 
 OK.
 
  
 int css_support;
};

extern int sie64a(struct kvm_s390_sie_block *, u64 *);
   +#define __KVM_HAVE_ARCH_IOEVENTFD
   +
   +#define KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY 1
   +
   +struct kvm_s390_ioeventfd_data {
   + __u8 type;
   + union {
   + /* VIRTIO_CCW_NOTIFY */
   + struct {
   + __u64 vq;
   + struct subchannel_id schid;
   + } virtio_ccw_vq;
   + char padding[35];
   + };
   +} __packed;
   +
  
  Do you expect userspace to use this structure?
  If yes this is the wrong header. If not why is it packed?
 
 Indeed, userspace is supposed to use this.
 
  
   +struct kvm_arch_ioeventfd {
   + struct list_head entry;
   + struct kvm_s390_ioeventfd_data data;
  
  Let's not waste memory keeping padding in kernel datastructures.
  
   +};
#endif
   diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
   index b58dd86..3c43e30 100644
   --- a/arch/s390/kvm/Kconfig
   +++ b/arch/s390/kvm/Kconfig
   @@ -22,6 +22,7 @@ config KVM
 select PREEMPT_NOTIFIERS
 select ANON_INODES
 select HAVE_KVM_CPU_RELAX_INTERCEPT
   + select HAVE_KVM_EVENTFD
 ---help---
   Support hosting paravirtualized guest machines using the SIE
   virtualization capability on the mainframe. This should work
   diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
   index 2441ffd..dbd8cc9 100644
   --- a/arch/s390/kvm/Makefile
   +++ b/arch/s390/kvm/Makefile
   @@ -6,7 +6,7 @@
# it under the terms of the GNU General Public License (version 2 only)
# as published by the Free Software Foundation.

   -common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
   +common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)

ccflags-y := -Ivirt/kvm -Iarch/s390/kvm

   diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
   index a390687..51ea66f 100644
   --- a/arch/s390/kvm/diag.c
   +++ b/arch/s390/kvm/diag.c
   @@ -104,6 +104,27 @@ static int __diag_ipl_functions(struct kvm_vcpu 
   *vcpu)

Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Cornelia Huck 
On Thu, 21 Feb 2013 18:34:59 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Feb 21, 2013 at 04:21:43PM +0100, Cornelia Huck wrote:
  On Thu, 21 Feb 2013 16:39:05 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
As s390 doesn't use memory writes for virtio notifcations, create
a special kind of ioeventfd instead that hooks up into diagnose
0x500 (kvm hypercall) with function code 3 (virtio-ccw notification).

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
   
   Do we really have to put virtio specific stuff into kvm?
   How about we add generic functionality to match GPRs
   on a hypercall and signal an eventfd?
  
  Worth a try implementing that.
  
   
   Also, it's a bit unfortunate that this doesn't use
   the io bus datastructure, long term the linked list handling
   might become a bottleneck, using shared code this could maybe
   benefit from performance optimizations there.
  
  The linked list stuff was more like an initial implementation that
  could be improved later.
  
   io bus data structure currently has the ability to match on
   two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
   Isn't this sufficient for your purposes?
   How about sticking subchannel id in address, vq in data match
   and using io bus?
  
  I can give that a try. (I must admit that I didn't look at the iobus
  stuff in detail.)
  
   
   BTW maybe we could do this for the user interface too,
   while I'm not 100% sure it's the cleanest thing to do
   (or will work), it would certainly minimize the patchset.
  
  You mean integrating with the generic interface and dropping the new
  ARCH flag?
 
 Not sure about the flag but we could use the general structure
 without an arch-specific format, if that's a good fit.
 
So I have something that seems to do what I want. I'll see if I can
morph it into something presentable tomorrow.

diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index b58dd86..3c43e30 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -22,6 +22,7 @@ config KVM
select PREEMPT_NOTIFIERS
select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
+   select HAVE_KVM_EVENTFD
---help---
  Support hosting paravirtualized guest machines using the SIE
  virtualization capability on the mainframe. This should work
diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index 3975722..8fe9d65 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -6,7 +6,7 @@
 # it under the terms of the GNU General Public License (version 2 only)
 # as published by the Free Software Foundation.
 
-common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
+common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)
 
 ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
 
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index a390687..7fc195e 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -104,6 +104,20 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
return -EREMOTE;
 }
 
+static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
+{
+   int ret, idx;
+   u64 vq = vcpu-run-s.regs.gprs[3];
+
+   idx = srcu_read_lock(vcpu-kvm-srcu);
+   ret = kvm_io_bus_write(vcpu-kvm, KVM_CSS_BUS,
+   vcpu-run-s.regs.gprs[2],
+   vcpu-run-s.regs.gprs[1],
+   vq);
+   srcu_read_unlock(vcpu-kvm-srcu, idx);
+   return ret;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
int code = (vcpu-arch.sie_block-ipb  0xfff)  16;
@@ -118,6 +132,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
return __diag_time_slice_end_directed(vcpu);
case 0x308:
return __diag_ipl_functions(vcpu);
+   case 0x500:
+   return __diag_virtio_hypercall(vcpu);
default:
return -EOPNOTSUPP;
}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f822d36..04d2454 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -142,6 +142,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_ONE_REG:
case KVM_CAP_ENABLE_CAP:
case KVM_CAP_S390_CSS_SUPPORT:
+   case KVM_CAP_IOEVENTFD:
r = 1;
break;
case KVM_CAP_NR_VCPUS:
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3b768ef..59be516 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -148,6 +148,7 @@ struct kvm_io_bus {
 enum kvm_bus {
KVM_MMIO_BUS,
KVM_PIO_BUS,
+   KVM_CSS_BUS,
KVM_NR_BUSES
 };
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 9a2db57..1df0766 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -448,12 +448,14 @@ enum {

Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Michael S. Tsirkin 
On Thu, Feb 21, 2013 at 07:14:31PM +0100, Cornelia Huck wrote:
 On Thu, 21 Feb 2013 18:34:59 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Thu, Feb 21, 2013 at 04:21:43PM +0100, Cornelia Huck wrote:
   On Thu, 21 Feb 2013 16:39:05 +0200
   Michael S. Tsirkin m...@redhat.com wrote:
   
On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
 As s390 doesn't use memory writes for virtio notifcations, create
 a special kind of ioeventfd instead that hooks up into diagnose
 0x500 (kvm hypercall) with function code 3 (virtio-ccw notification).
 
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com

Do we really have to put virtio specific stuff into kvm?
How about we add generic functionality to match GPRs
on a hypercall and signal an eventfd?
   
   Worth a try implementing that.
   

Also, it's a bit unfortunate that this doesn't use
the io bus datastructure, long term the linked list handling
might become a bottleneck, using shared code this could maybe
benefit from performance optimizations there.
   
   The linked list stuff was more like an initial implementation that
   could be improved later.
   
io bus data structure currently has the ability to match on
two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
Isn't this sufficient for your purposes?
How about sticking subchannel id in address, vq in data match
and using io bus?
   
   I can give that a try. (I must admit that I didn't look at the iobus
   stuff in detail.)
   

BTW maybe we could do this for the user interface too,
while I'm not 100% sure it's the cleanest thing to do
(or will work), it would certainly minimize the patchset.
   
   You mean integrating with the generic interface and dropping the new
   ARCH flag?
  
  Not sure about the flag but we could use the general structure
  without an arch-specific format, if that's a good fit.
  
 So I have something that seems to do what I want. I'll see if I can
 morph it into something presentable tomorrow.
 
 diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
 index b58dd86..3c43e30 100644
 --- a/arch/s390/kvm/Kconfig
 +++ b/arch/s390/kvm/Kconfig
 @@ -22,6 +22,7 @@ config KVM
   select PREEMPT_NOTIFIERS
   select ANON_INODES
   select HAVE_KVM_CPU_RELAX_INTERCEPT
 + select HAVE_KVM_EVENTFD
   ---help---
 Support hosting paravirtualized guest machines using the SIE
 virtualization capability on the mainframe. This should work
 diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
 index 3975722..8fe9d65 100644
 --- a/arch/s390/kvm/Makefile
 +++ b/arch/s390/kvm/Makefile
 @@ -6,7 +6,7 @@
  # it under the terms of the GNU General Public License (version 2 only)
  # as published by the Free Software Foundation.
  
 -common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
 +common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)
  
  ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
  
 diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
 index a390687..7fc195e 100644
 --- a/arch/s390/kvm/diag.c
 +++ b/arch/s390/kvm/diag.c
 @@ -104,6 +104,20 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
   return -EREMOTE;
  }
  
 +static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 +{
 + int ret, idx;
 + u64 vq = vcpu-run-s.regs.gprs[3];
 +
 + idx = srcu_read_lock(vcpu-kvm-srcu);
 + ret = kvm_io_bus_write(vcpu-kvm, KVM_CSS_BUS,
 + vcpu-run-s.regs.gprs[2],
 + vcpu-run-s.regs.gprs[1],
 + vq);

Hmm, do you really need three gprs? If not, we could
just pass len == 8, which would be cleaner.
Also might as well drop the vq variable, no?

 + srcu_read_unlock(vcpu-kvm-srcu, idx);
 + return ret;
 +}
 +
  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
  {
   int code = (vcpu-arch.sie_block-ipb  0xfff)  16;
 @@ -118,6 +132,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
   return __diag_time_slice_end_directed(vcpu);
   case 0x308:
   return __diag_ipl_functions(vcpu);
 + case 0x500:
 + return __diag_virtio_hypercall(vcpu);
   default:
   return -EOPNOTSUPP;
   }
 diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
 index f822d36..04d2454 100644
 --- a/arch/s390/kvm/kvm-s390.c
 +++ b/arch/s390/kvm/kvm-s390.c
 @@ -142,6 +142,7 @@ int kvm_dev_ioctl_check_extension(long ext)
   case KVM_CAP_ONE_REG:
   case KVM_CAP_ENABLE_CAP:
   case KVM_CAP_S390_CSS_SUPPORT:
 + case KVM_CAP_IOEVENTFD:
   r = 1;
   break;
   case KVM_CAP_NR_VCPUS:
 diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
 index 3b768ef..59be516 100644
 --- a/include/linux/kvm_host.h
 +++ b/include/linux/kvm_host.h
 @@ -148,6 +148,7 @@ struct kvm_io_bus {
  enum kvm_bus {
   KVM_MMIO_BUS,

Re: [Qemu-devel] [RFC PATCH 3/3] KVM: s390: Hook up ioeventfds.

2013-02-21 Thread Cornelia Huck 
On Thu, 21 Feb 2013 22:42:41 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Feb 21, 2013 at 07:14:31PM +0100, Cornelia Huck wrote:
  On Thu, 21 Feb 2013 18:34:59 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Thu, Feb 21, 2013 at 04:21:43PM +0100, Cornelia Huck wrote:
On Thu, 21 Feb 2013 16:39:05 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Feb 21, 2013 at 02:13:00PM +0100, Cornelia Huck wrote:
  As s390 doesn't use memory writes for virtio notifcations, create
  a special kind of ioeventfd instead that hooks up into diagnose
  0x500 (kvm hypercall) with function code 3 (virtio-ccw 
  notification).
  
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 
 Do we really have to put virtio specific stuff into kvm?
 How about we add generic functionality to match GPRs
 on a hypercall and signal an eventfd?

Worth a try implementing that.

 
 Also, it's a bit unfortunate that this doesn't use
 the io bus datastructure, long term the linked list handling
 might become a bottleneck, using shared code this could maybe
 benefit from performance optimizations there.

The linked list stuff was more like an initial implementation that
could be improved later.

 io bus data structure currently has the ability to match on
 two 64 bit fields (addr/datamatch) and a signed 32 bit one (length).
 Isn't this sufficient for your purposes?
 How about sticking subchannel id in address, vq in data match
 and using io bus?

I can give that a try. (I must admit that I didn't look at the iobus
stuff in detail.)

 
 BTW maybe we could do this for the user interface too,
 while I'm not 100% sure it's the cleanest thing to do
 (or will work), it would certainly minimize the patchset.

You mean integrating with the generic interface and dropping the new
ARCH flag?
   
   Not sure about the flag but we could use the general structure
   without an arch-specific format, if that's a good fit.
   
  So I have something that seems to do what I want. I'll see if I can
  morph it into something presentable tomorrow.
  
  diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
  index b58dd86..3c43e30 100644
  --- a/arch/s390/kvm/Kconfig
  +++ b/arch/s390/kvm/Kconfig
  @@ -22,6 +22,7 @@ config KVM
  select PREEMPT_NOTIFIERS
  select ANON_INODES
  select HAVE_KVM_CPU_RELAX_INTERCEPT
  +   select HAVE_KVM_EVENTFD
  ---help---
Support hosting paravirtualized guest machines using the SIE
virtualization capability on the mainframe. This should work
  diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
  index 3975722..8fe9d65 100644
  --- a/arch/s390/kvm/Makefile
  +++ b/arch/s390/kvm/Makefile
  @@ -6,7 +6,7 @@
   # it under the terms of the GNU General Public License (version 2 only)
   # as published by the Free Software Foundation.
   
  -common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
  +common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o eventfd.o)
   
   ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
   
  diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
  index a390687..7fc195e 100644
  --- a/arch/s390/kvm/diag.c
  +++ b/arch/s390/kvm/diag.c
  @@ -104,6 +104,20 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
  return -EREMOTE;
   }
   
  +static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
  +{
  +   int ret, idx;
  +   u64 vq = vcpu-run-s.regs.gprs[3];
  +
  +   idx = srcu_read_lock(vcpu-kvm-srcu);
  +   ret = kvm_io_bus_write(vcpu-kvm, KVM_CSS_BUS,
  +   vcpu-run-s.regs.gprs[2],
  +   vcpu-run-s.regs.gprs[1],
  +   vq);
 
 Hmm, do you really need three gprs? If not, we could
 just pass len == 8, which would be cleaner.
 Also might as well drop the vq variable, no?

If I want to pass generic hypercalls, I need to pass the subcode (in
gpr 1) in the len variable. If all we'll ever need is the virtio-ccw
notify hypercall, gprs 2 and 3 are enough. Would perhaps make the
common code less hacky, but we'd lose (unneeded?) flexibility.

 
  +   srcu_read_unlock(vcpu-kvm-srcu, idx);
  +   return ret;
  +}
  +
   int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
   {
  int code = (vcpu-arch.sie_block-ipb  0xfff)  16;
  @@ -118,6 +132,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
  return __diag_time_slice_end_directed(vcpu);
  case 0x308:
  return __diag_ipl_functions(vcpu);
  +   case 0x500:
  +   return __diag_virtio_hypercall(vcpu);
  default:
  return -EOPNOTSUPP;
  }
  diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
  index f822d36..04d2454 100644
  --- a/arch/s390/kvm/kvm-s390.c
  +++ b/arch/s390/kvm/kvm-s390.c
  @@ -142,6 +142,7 @@ int kvm_dev_ioctl_check_extension(long ext)
  case KVM_CAP_ONE_REG: