[RFC PATCH 0/3] kvm: Make ioeventfd usable on s390.

[RFC PATCH 0/3] kvm: Make ioeventfd usable on s390.

[Cornelia Huck]

This patch series aims at making ioeventfds usable on s390.

For "normal" architectures, ioeventfds work by registering
notifications that trap on write operations on memory, making
it possible to handle those writes in the kernel. This won't
work on s390, however.

We want a mechanism that enables trapping of operations equivalent
to the memory writes on other architectures - for the use case
we care about, the diagnose 500 hypercall that performs
virtio notifications. (There's room for other types as well.)

Patch 1 is a preparation patch for kvm/s390.

Patch 2 tries to split out the parts of the ioeventfd mechanism
that need to be implemented differently on special architectures
(i. e. s390). If __KVM_HAVE_ARCH_IOEVENTFD is set, the architecture
will provide a set of kvm_arch_ioeventfd_* functions to be used
instead of the kvm_arch_ioeventfd_* functions implementing the
generic mechanism. Extra payload for the KVM_IOEVENTFD ioctl
can be provided in the existing data fields (renamed to data).
A new flag, KVM_IOEVENTFD_FLAG_ARCH, is used to indicate that an
architecture-specific implementation is used that makes use of
the extra data fields.

Patch 3 implements the kvm_arch_ioeventfd_* functions for s390
and hooks up ioeventfds with diagnose 500.

There's also a companion qemu patch series that makes use of this
for virtio-ccw.

(Patches are against kvm-next.)

Cornelia Huck (3):
  KVM: s390: Move out initialization code.
  KVM: Generalize ioeventfds.
  KVM: s390: Hook up ioeventfds.

 Documentation/virtual/kvm/api.txt |   5 +-
 arch/s390/include/asm/kvm_host.h  |  23 +++++
 arch/s390/kvm/Kconfig             |   1 +
 arch/s390/kvm/Makefile            |   6 +-
 arch/s390/kvm/diag.c              |  23 +++++
 arch/s390/kvm/init.c              |  52 +++++++++++
 arch/s390/kvm/kvm-s390.c          | 181 +++++++++++++++++++++++++++++++++-----
 arch/s390/kvm/kvm-s390.h          |   3 +
 include/linux/kvm_host.h          |  13 +++
 include/uapi/linux/kvm.h          |   4 +-
 virt/kvm/eventfd.c                | 181 ++++++++++++++++++++++++++------------
 11 files changed, 410 insertions(+), 82 deletions(-)
 create mode 100644 arch/s390/kvm/init.c

-- 
1.7.12.4

[RFC PATCH 1/3] KVM: s390: Move out initialization code.

[Cornelia Huck]

kvm-s390's module initialization code needs to live in a separate
module (kvm-s390.ko) if we want to include eventfd (which has its
own module init func).

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 arch/s390/kvm/Makefile   |  4 +++-
 arch/s390/kvm/init.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c | 38 ++++-------------------------------
 3 files changed, 59 insertions(+), 35 deletions(-)
 create mode 100644 arch/s390/kvm/init.c

diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index 3975722..2441ffd 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -11,4 +11,6 @@ common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
 ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
 
 kvm-objs := $(common-objs) kvm-s390.o intercept.o interrupt.o priv.o sigp.o diag.o
-obj-$(CONFIG_KVM) += kvm.o
+kvm_s390-objs := init.o
+
+obj-$(CONFIG_KVM) += kvm.o kvm_s390.o
diff --git a/arch/s390/kvm/init.c b/arch/s390/kvm/init.c
new file mode 100644
index 0000000..dc4028a
--- /dev/null
+++ b/arch/s390/kvm/init.c
@@ -0,0 +1,52 @@
+/*
+ * kvm on s390 module initialization
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2 only)
+ * as published by the Free Software Foundation.
+ *
+ *    Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
+ */
+
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <linux/module.h>
+#include "kvm-s390.h"
+
+extern unsigned long long *facilities;
+
+static int __init kvm_s390_init(void)
+{
+	int ret;
+	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
+	if (ret)
+		return ret;
+
+	/*
+	 * guests can ask for up to 255+1 double words, we need a full page
+	 * to hold the maximum amount of facilities. On the other hand, we
+	 * only set facilities that are known to work in KVM.
+	 */
+	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
+	if (!facilities) {
+		kvm_exit();
+		return -ENOMEM;
+	}
+	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
+	facilities[0] &= 0xff00fff3f47c0000ULL;
+	facilities[1] &= 0x001c000000000000ULL;
+	return 0;
+}
+
+static void __exit kvm_s390_exit(void)
+{
+	free_page((unsigned long) facilities);
+	kvm_exit();
+}
+
+module_init(kvm_s390_init);
+module_exit(kvm_s390_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f822d36..58a5f03 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -36,6 +36,9 @@
 #include "trace.h"
 #include "trace-s390.h"
 
+unsigned long long *facilities;
+EXPORT_SYMBOL_GPL(facilities);
+
 #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
 
 struct kvm_stats_debugfs_item debugfs_entries[] = {
@@ -83,8 +86,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ NULL }
 };
 
-static unsigned long long *facilities;
-
 /* Section: not file related */
 int kvm_arch_hardware_enable(void *garbage)
 {
@@ -823,6 +824,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 		return -EFAULT;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(kvm_s390_vcpu_store_status);
 
 static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 				     struct kvm_enable_cap *cap)
@@ -1026,35 +1028,3 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 				   struct kvm_memory_slot *slot)
 {
 }
-
-static int __init kvm_s390_init(void)
-{
-	int ret;
-	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
-	if (ret)
-		return ret;
-
-	/*
-	 * guests can ask for up to 255+1 double words, we need a full page
-	 * to hold the maximum amount of facilities. On the other hand, we
-	 * only set facilities that are known to work in KVM.
-	 */
-	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
-	if (!facilities) {
-		kvm_exit();
-		return -ENOMEM;
-	}
-	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
-	facilities[0] &= 0xff00fff3f47c0000ULL;
-	facilities[1] &= 0x001c000000000000ULL;
-	return 0;
-}
-
-static void __exit kvm_s390_exit(void)
-{
-	free_page((unsigned long) facilities);
-	kvm_exit();
-}
-
-module_init(kvm_s390_init);
-module_exit(kvm_s390_exit);
-- 
1.7.12.4

Re: [RFC PATCH 1/3] KVM: s390: Move out initialization code.

[Michael S. Tsirkin]

On Thu, Feb 21, 2013 at 02:12:58PM +0100, Cornelia Huck wrote:
> kvm-s390's module initialization code needs to live in a separate
> module (kvm-s390.ko) if we want to include eventfd (which has its
> own module init func).
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

I don't get this explanation.
What's the problem this solves?
Could you clarify please?

> ---
>  arch/s390/kvm/Makefile   |  4 +++-
>  arch/s390/kvm/init.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c | 38 ++++-------------------------------
>  3 files changed, 59 insertions(+), 35 deletions(-)
>  create mode 100644 arch/s390/kvm/init.c
> 
> diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
> index 3975722..2441ffd 100644
> --- a/arch/s390/kvm/Makefile
> +++ b/arch/s390/kvm/Makefile
> @@ -11,4 +11,6 @@ common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
>  ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
>  
>  kvm-objs := $(common-objs) kvm-s390.o intercept.o interrupt.o priv.o sigp.o diag.o
> -obj-$(CONFIG_KVM) += kvm.o
> +kvm_s390-objs := init.o
> +
> +obj-$(CONFIG_KVM) += kvm.o kvm_s390.o
> diff --git a/arch/s390/kvm/init.c b/arch/s390/kvm/init.c
> new file mode 100644
> index 0000000..dc4028a
> --- /dev/null
> +++ b/arch/s390/kvm/init.c
> @@ -0,0 +1,52 @@
> +/*
> + * kvm on s390 module initialization
> + *
> + * Copyright IBM Corp. 2013
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2 only)
> + * as published by the Free Software Foundation.
> + *
> + *    Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
> + */
> +
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <linux/module.h>
> +#include "kvm-s390.h"
> +
> +extern unsigned long long *facilities;
> +
> +static int __init kvm_s390_init(void)
> +{
> +	int ret;
> +	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * guests can ask for up to 255+1 double words, we need a full page
> +	 * to hold the maximum amount of facilities. On the other hand, we
> +	 * only set facilities that are known to work in KVM.
> +	 */
> +	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
> +	if (!facilities) {
> +		kvm_exit();
> +		return -ENOMEM;
> +	}
> +	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
> +	facilities[0] &= 0xff00fff3f47c0000ULL;
> +	facilities[1] &= 0x001c000000000000ULL;
> +	return 0;
> +}
> +
> +static void __exit kvm_s390_exit(void)
> +{
> +	free_page((unsigned long) facilities);
> +	kvm_exit();
> +}
> +
> +module_init(kvm_s390_init);
> +module_exit(kvm_s390_exit);
> +
> +MODULE_LICENSE("GPL");

GPL v2?

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index f822d36..58a5f03 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -36,6 +36,9 @@
>  #include "trace.h"
>  #include "trace-s390.h"
>  
> +unsigned long long *facilities;
> +EXPORT_SYMBOL_GPL(facilities);
> +
>  #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
>  
>  struct kvm_stats_debugfs_item debugfs_entries[] = {
> @@ -83,8 +86,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	{ NULL }
>  };
>  
> -static unsigned long long *facilities;
> -
>  /* Section: not file related */
>  int kvm_arch_hardware_enable(void *garbage)
>  {
> @@ -823,6 +824,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
>  		return -EFAULT;
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(kvm_s390_vcpu_store_status);
>  
>  static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  				     struct kvm_enable_cap *cap)
> @@ -1026,35 +1028,3 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>  				   struct kvm_memory_slot *slot)
>  {
>  }
> -
> -static int __init kvm_s390_init(void)
> -{
> -	int ret;
> -	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * guests can ask for up to 255+1 double words, we need a full page
> -	 * to hold the maximum amount of facilities. On the other hand, we
> -	 * only set facilities that are known to work in KVM.
> -	 */
> -	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
> -	if (!facilities) {
> -		kvm_exit();
> -		return -ENOMEM;
> -	}
> -	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
> -	facilities[0] &= 0xff00fff3f47c0000ULL;
> -	facilities[1] &= 0x001c000000000000ULL;
> -	return 0;
> -}
> -
> -static void __exit kvm_s390_exit(void)
> -{
> -	free_page((unsigned long) facilities);
> -	kvm_exit();
> -}
> -
> -module_init(kvm_s390_init);
> -module_exit(kvm_s390_exit);
> -- 
> 1.7.12.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/3] KVM: s390: Move out initialization code.

[Cornelia Huck]

On Thu, 21 Feb 2013 15:43:55 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 21, 2013 at 02:12:58PM +0100, Cornelia Huck wrote:
> > kvm-s390's module initialization code needs to live in a separate
> > module (kvm-s390.ko) if we want to include eventfd (which has its
> > own module init func).
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> I don't get this explanation.
> What's the problem this solves?
> Could you clarify please?

On s390, we currently build a single 'kvm' module, with a module_init
function. eventfd has its own module_init function, and we can't have
two of them in the same module. I just moved our specific module
initialization into a new 'kvm_s390' module.

> 
> > ---
> >  arch/s390/kvm/Makefile   |  4 +++-
> >  arch/s390/kvm/init.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  arch/s390/kvm/kvm-s390.c | 38 ++++-------------------------------
> >  3 files changed, 59 insertions(+), 35 deletions(-)
> >  create mode 100644 arch/s390/kvm/init.c
> > 
> > diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
> > index 3975722..2441ffd 100644
> > --- a/arch/s390/kvm/Makefile
> > +++ b/arch/s390/kvm/Makefile
> > @@ -11,4 +11,6 @@ common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
> >  ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
> >  
> >  kvm-objs := $(common-objs) kvm-s390.o intercept.o interrupt.o priv.o sigp.o diag.o
> > -obj-$(CONFIG_KVM) += kvm.o
> > +kvm_s390-objs := init.o
> > +
> > +obj-$(CONFIG_KVM) += kvm.o kvm_s390.o
> > diff --git a/arch/s390/kvm/init.c b/arch/s390/kvm/init.c
> > new file mode 100644
> > index 0000000..dc4028a
> > --- /dev/null
> > +++ b/arch/s390/kvm/init.c
> > @@ -0,0 +1,52 @@
> > +/*
> > + * kvm on s390 module initialization
> > + *
> > + * Copyright IBM Corp. 2013
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License (version 2 only)
> > + * as published by the Free Software Foundation.
> > + *
> > + *    Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
> > + */
> > +
> > +#include <linux/kvm.h>
> > +#include <linux/kvm_host.h>
> > +#include <linux/module.h>
> > +#include "kvm-s390.h"
> > +
> > +extern unsigned long long *facilities;
> > +
> > +static int __init kvm_s390_init(void)
> > +{
> > +	int ret;
> > +	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * guests can ask for up to 255+1 double words, we need a full page
> > +	 * to hold the maximum amount of facilities. On the other hand, we
> > +	 * only set facilities that are known to work in KVM.
> > +	 */
> > +	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
> > +	if (!facilities) {
> > +		kvm_exit();
> > +		return -ENOMEM;
> > +	}
> > +	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
> > +	facilities[0] &= 0xff00fff3f47c0000ULL;
> > +	facilities[1] &= 0x001c000000000000ULL;
> > +	return 0;
> > +}
> > +
> > +static void __exit kvm_s390_exit(void)
> > +{
> > +	free_page((unsigned long) facilities);
> > +	kvm_exit();
> > +}
> > +
> > +module_init(kvm_s390_init);
> > +module_exit(kvm_s390_exit);
> > +
> > +MODULE_LICENSE("GPL");
> 
> GPL v2?
> 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index f822d36..58a5f03 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -36,6 +36,9 @@
> >  #include "trace.h"
> >  #include "trace-s390.h"
> >  
> > +unsigned long long *facilities;
> > +EXPORT_SYMBOL_GPL(facilities);
> > +
> >  #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
> >  
> >  struct kvm_stats_debugfs_item debugfs_entries[] = {
> > @@ -83,8 +86,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
> >  	{ NULL }
> >  };
> >  
> > -static unsigned long long *facilities;
> > -
> >  /* Section: not file related */
> >  int kvm_arch_hardware_enable(void *garbage)
> >  {
> > @@ -823,6 +824,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
> >  		return -EFAULT;
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(kvm_s390_vcpu_store_status);
> >  
> >  static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> >  				     struct kvm_enable_cap *cap)
> > @@ -1026,35 +1028,3 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> >  				   struct kvm_memory_slot *slot)
> >  {
> >  }
> > -
> > -static int __init kvm_s390_init(void)
> > -{
> > -	int ret;
> > -	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> > -	if (ret)
> > -		return ret;
> > -
> > -	/*
> > -	 * guests can ask for up to 255+1 double words, we need a full page
> > -	 * to hold the maximum amount of facilities. On the other hand, we
> > -	 * only set facilities that are known to work in KVM.
> > -	 */
> > -	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
> > -	if (!facilities) {
> > -		kvm_exit();
> > -		return -ENOMEM;
> > -	}
> > -	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
> > -	facilities[0] &= 0xff00fff3f47c0000ULL;
> > -	facilities[1] &= 0x001c000000000000ULL;
> > -	return 0;
> > -}
> > -
> > -static void __exit kvm_s390_exit(void)
> > -{
> > -	free_page((unsigned long) facilities);
> > -	kvm_exit();
> > -}
> > -
> > -module_init(kvm_s390_init);
> > -module_exit(kvm_s390_exit);
> > -- 
> > 1.7.12.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH 1/3] KVM: s390: Move out initialization code.

[Michael S. Tsirkin]

On Thu, Feb 21, 2013 at 03:07:32PM +0100, Cornelia Huck wrote:
> On Thu, 21 Feb 2013 15:43:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Feb 21, 2013 at 02:12:58PM +0100, Cornelia Huck wrote:
> > > kvm-s390's module initialization code needs to live in a separate
> > > module (kvm-s390.ko) if we want to include eventfd (which has its
> > > own module init func).
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> > I don't get this explanation.
> > What's the problem this solves?
> > Could you clarify please?
> 
> On s390, we currently build a single 'kvm' module, with a module_init
> function. eventfd has its own module_init function, and we can't have
> two of them in the same module. I just moved our specific module
> initialization into a new 'kvm_s390' module.


You mean this?

virt/kvm/eventfd.c:static int __init irqfd_module_init(void)
virt/kvm/eventfd.c:module_init(irqfd_module_init);

I see. Won't it be easier to just call irqfd_module_init
from kvm_init?

> > 
> > > ---
> > >  arch/s390/kvm/Makefile   |  4 +++-
> > >  arch/s390/kvm/init.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  arch/s390/kvm/kvm-s390.c | 38 ++++-------------------------------
> > >  3 files changed, 59 insertions(+), 35 deletions(-)
> > >  create mode 100644 arch/s390/kvm/init.c
> > > 
> > > diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
> > > index 3975722..2441ffd 100644
> > > --- a/arch/s390/kvm/Makefile
> > > +++ b/arch/s390/kvm/Makefile
> > > @@ -11,4 +11,6 @@ common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o)
> > >  ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
> > >  
> > >  kvm-objs := $(common-objs) kvm-s390.o intercept.o interrupt.o priv.o sigp.o diag.o
> > > -obj-$(CONFIG_KVM) += kvm.o
> > > +kvm_s390-objs := init.o
> > > +
> > > +obj-$(CONFIG_KVM) += kvm.o kvm_s390.o
> > > diff --git a/arch/s390/kvm/init.c b/arch/s390/kvm/init.c
> > > new file mode 100644
> > > index 0000000..dc4028a
> > > --- /dev/null
> > > +++ b/arch/s390/kvm/init.c
> > > @@ -0,0 +1,52 @@
> > > +/*
> > > + * kvm on s390 module initialization
> > > + *
> > > + * Copyright IBM Corp. 2013
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License (version 2 only)
> > > + * as published by the Free Software Foundation.
> > > + *
> > > + *    Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
> > > + */
> > > +
> > > +#include <linux/kvm.h>
> > > +#include <linux/kvm_host.h>
> > > +#include <linux/module.h>
> > > +#include "kvm-s390.h"
> > > +
> > > +extern unsigned long long *facilities;
> > > +
> > > +static int __init kvm_s390_init(void)
> > > +{
> > > +	int ret;
> > > +	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * guests can ask for up to 255+1 double words, we need a full page
> > > +	 * to hold the maximum amount of facilities. On the other hand, we
> > > +	 * only set facilities that are known to work in KVM.
> > > +	 */
> > > +	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
> > > +	if (!facilities) {
> > > +		kvm_exit();
> > > +		return -ENOMEM;
> > > +	}
> > > +	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
> > > +	facilities[0] &= 0xff00fff3f47c0000ULL;
> > > +	facilities[1] &= 0x001c000000000000ULL;
> > > +	return 0;
> > > +}
> > > +
> > > +static void __exit kvm_s390_exit(void)
> > > +{
> > > +	free_page((unsigned long) facilities);
> > > +	kvm_exit();
> > > +}
> > > +
> > > +module_init(kvm_s390_init);
> > > +module_exit(kvm_s390_exit);
> > > +
> > > +MODULE_LICENSE("GPL");
> > 
> > GPL v2?
> > 
> > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > > index f822d36..58a5f03 100644
> > > --- a/arch/s390/kvm/kvm-s390.c
> > > +++ b/arch/s390/kvm/kvm-s390.c
> > > @@ -36,6 +36,9 @@
> > >  #include "trace.h"
> > >  #include "trace-s390.h"
> > >  
> > > +unsigned long long *facilities;
> > > +EXPORT_SYMBOL_GPL(facilities);
> > > +
> > >  #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
> > >  
> > >  struct kvm_stats_debugfs_item debugfs_entries[] = {
> > > @@ -83,8 +86,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
> > >  	{ NULL }
> > >  };
> > >  
> > > -static unsigned long long *facilities;
> > > -
> > >  /* Section: not file related */
> > >  int kvm_arch_hardware_enable(void *garbage)
> > >  {
> > > @@ -823,6 +824,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
> > >  		return -EFAULT;
> > >  	return 0;
> > >  }
> > > +EXPORT_SYMBOL_GPL(kvm_s390_vcpu_store_status);
> > >  
> > >  static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> > >  				     struct kvm_enable_cap *cap)
> > > @@ -1026,35 +1028,3 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> > >  				   struct kvm_memory_slot *slot)
> > >  {
> > >  }
> > > -
> > > -static int __init kvm_s390_init(void)
> > > -{
> > > -	int ret;
> > > -	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	/*
> > > -	 * guests can ask for up to 255+1 double words, we need a full page
> > > -	 * to hold the maximum amount of facilities. On the other hand, we
> > > -	 * only set facilities that are known to work in KVM.
> > > -	 */
> > > -	facilities = (unsigned long long *) get_zeroed_page(GFP_KERNEL|GFP_DMA);
> > > -	if (!facilities) {
> > > -		kvm_exit();
> > > -		return -ENOMEM;
> > > -	}
> > > -	memcpy(facilities, S390_lowcore.stfle_fac_list, 16);
> > > -	facilities[0] &= 0xff00fff3f47c0000ULL;
> > > -	facilities[1] &= 0x001c000000000000ULL;
> > > -	return 0;
> > > -}
> > > -
> > > -static void __exit kvm_s390_exit(void)
> > > -{
> > > -	free_page((unsigned long) facilities);
> > > -	kvm_exit();
> > > -}
> > > -
> > > -module_init(kvm_s390_init);
> > > -module_exit(kvm_s390_exit);
> > > -- 
> > > 1.7.12.4
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [RFC PATCH 1/3] KVM: s390: Move out initialization code.

[Cornelia Huck]

On Thu, 21 Feb 2013 16:18:34 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 21, 2013 at 03:07:32PM +0100, Cornelia Huck wrote:
> > On Thu, 21 Feb 2013 15:43:55 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Feb 21, 2013 at 02:12:58PM +0100, Cornelia Huck wrote:
> > > > kvm-s390's module initialization code needs to live in a separate
> > > > module (kvm-s390.ko) if we want to include eventfd (which has its
> > > > own module init func).
> > > > 
> > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > 
> > > I don't get this explanation.
> > > What's the problem this solves?
> > > Could you clarify please?
> > 
> > On s390, we currently build a single 'kvm' module, with a module_init
> > function. eventfd has its own module_init function, and we can't have
> > two of them in the same module. I just moved our specific module
> > initialization into a new 'kvm_s390' module.
> 
> 
> You mean this?
> 
> virt/kvm/eventfd.c:static int __init irqfd_module_init(void)
> virt/kvm/eventfd.c:module_init(irqfd_module_init);
> 
> I see. Won't it be easier to just call irqfd_module_init
> from kvm_init?
> 
It does make it clearer what happens, and eliminates the need for a new
module on s390. I'll do that in the next round:

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d7965fb..55a6de1 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -423,6 +423,19 @@ void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
 int __must_check vcpu_load(struct kvm_vcpu *vcpu);
 void vcpu_put(struct kvm_vcpu *vcpu);
 
+#ifdef __KVM_HAVE_IOAPIC
+int kvm_irqfd_init(void);
+void kvm_irqfd_exit(void);
+#else
+static inline int kvm_irqfd_init(void)
+{
+	return 0;
+}
+
+static inline void kvm_irqfd_exit(void)
+{
+}
+#endif
 int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 		  struct module *module);
 void kvm_exit(void);
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 63fe454..c631c4b 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -544,7 +544,7 @@ void kvm_irq_routing_update(struct kvm *kvm,
  * aggregated from all vm* instances. We need our own isolated single-thread
  * queue to prevent deadlock against flushing the normal work-queue.
  */
-static int __init irqfd_module_init(void)
+int kvm_irqfd_init(void)
 {
 	irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
 	if (!irqfd_cleanup_wq)
@@ -553,13 +553,10 @@ static int __init irqfd_module_init(void)
 	return 0;
 }
 
-static void __exit irqfd_module_exit(void)
+void kvm_irqfd_exit(void)
 {
 	destroy_workqueue(irqfd_cleanup_wq);
 }
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);
 #endif
 
 /*
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index adc68fe..7c188a3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2920,6 +2920,9 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 	int r;
 	int cpu;
 
+	r = kvm_irqfd_init();
+	if (r)
+		goto out_irqfd;
 	r = kvm_arch_init(opaque);
 	if (r)
 		goto out_fail;
@@ -3000,6 +3003,8 @@ out_free_0a:
 out_free_0:
 	kvm_arch_exit();
 out_fail:
+	kvm_irqfd_exit();
+out_irqfd:
 	return r;
 }
 EXPORT_SYMBOL_GPL(kvm_init);
@@ -3016,6 +3021,7 @@ void kvm_exit(void)
 	on_each_cpu(hardware_disable_nolock, NULL, 1);
 	kvm_arch_hardware_unsetup();
 	kvm_arch_exit();
+	kvm_irqfd_exit();
 	free_cpumask_var(cpus_hardware_enabled);
 }
 EXPORT_SYMBOL_GPL(kvm_exit);

[RFC PATCH 2/3] KVM: Generalize ioeventfds.

[Cornelia Huck]

Currently, ioeventfds are designed to work on architectures that
can trap I/O memory writes. This won't work for architectures like
s390, however; therefore provide a way for architectures to override
this with an architecture-specific implementation.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |   5 +-
 include/linux/kvm_host.h          |  13 +++
 include/uapi/linux/kvm.h          |   4 +-
 virt/kvm/eventfd.c                | 181 ++++++++++++++++++++++++++------------
 4 files changed, 146 insertions(+), 57 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index c2534c3..13c038c 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1465,7 +1465,7 @@ struct kvm_ioeventfd {
 	__u32 len;         /* 1, 2, 4, or 8 bytes    */
 	__s32 fd;
 	__u32 flags;
-	__u8  pad[36];
+	__u8  data[36];    /* for architecture-specific data */
 };
 
 The following flags are defined:
@@ -1473,10 +1473,13 @@ The following flags are defined:
 #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
 #define KVM_IOEVENTFD_FLAG_PIO       (1 << kvm_ioeventfd_flag_nr_pio)
 #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
+#define KVM_IOEVENTFD_FLAG_ARCH      (1 << kvm_ioeventfd_flag_nr_arch)
 
 If datamatch flag is set, the event will be signaled only if the written value
 to the registered address is equal to datamatch in struct kvm_ioeventfd.
 
+If the arch flag is set, the eventfd will use the data field. If the arch flag
+is not set, the data field is not valid.
 
 4.60 KVM_DIRTY_TLB
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 722cae7..d7965fb 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -905,8 +905,21 @@ static inline void kvm_free_irq_routing(struct kvm *kvm) {}
 
 #ifdef CONFIG_HAVE_KVM_EVENTFD
 
+struct kvm_arch_ioeventfd;
 void kvm_eventfd_init(struct kvm *kvm);
 int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args);
+struct eventfd_ctx *kvm_ioeventfd_get_eventfd(struct kvm_arch_ioeventfd *arch);
+int kvm_arch_ioeventfd_check(struct kvm_ioeventfd *args);
+void kvm_arch_ioeventfd_init(struct kvm_arch_ioeventfd *arch,
+			     struct kvm_ioeventfd *args);
+int kvm_arch_ioeventfd_activate(struct kvm *kvm,
+				struct kvm_arch_ioeventfd *arch,
+				struct kvm_ioeventfd *args);
+bool kvm_arch_ioeventfd_match(struct kvm_arch_ioeventfd *arch,
+			      struct kvm_arch_ioeventfd *to_match);
+bool kvm_arch_ioeventfd_match_and_release(struct kvm *kvm,
+					  struct kvm_arch_ioeventfd *arch,
+					  struct kvm_ioeventfd *args);
 
 #ifdef CONFIG_HAVE_KVM_IRQCHIP
 int kvm_irqfd(struct kvm *kvm, struct kvm_irqfd *args);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 9a2db57..dfd444b 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -448,12 +448,14 @@ enum {
 	kvm_ioeventfd_flag_nr_datamatch,
 	kvm_ioeventfd_flag_nr_pio,
 	kvm_ioeventfd_flag_nr_deassign,
+	kvm_ioeventfd_flag_nr_arch,
 	kvm_ioeventfd_flag_nr_max,
 };
 
 #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
 #define KVM_IOEVENTFD_FLAG_PIO       (1 << kvm_ioeventfd_flag_nr_pio)
 #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
+#define KVM_IOEVENTFD_FLAG_ARCH      (1 << kvm_ioeventfd_flag_nr_arch)
 
 #define KVM_IOEVENTFD_VALID_FLAG_MASK  ((1 << kvm_ioeventfd_flag_nr_max) - 1)
 
@@ -463,7 +465,7 @@ struct kvm_ioeventfd {
 	__u32 len;         /* 1, 2, 4, or 8 bytes    */
 	__s32 fd;
 	__u32 flags;
-	__u8  pad[36];
+	__u8  data[36];    /* for architecture-specific data */
 };
 
 /* for KVM_ENABLE_CAP */
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index b6eea5c..63fe454 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -568,23 +568,37 @@ module_exit(irqfd_module_exit);
  *
  * userspace can register a PIO/MMIO address with an eventfd for receiving
  * notification when the memory has been touched.
+ *
+ * Architectures that use a notification mechanism different from memory
+ * writes may override this with architecture-specific callbacks.
  * --------------------------------------------------------------------
  */
-
-struct _ioeventfd {
-	struct list_head     list;
+#ifndef __KVM_HAVE_ARCH_IOEVENTFD
+struct kvm_arch_ioeventfd {
 	u64                  addr;
 	int                  length;
-	struct eventfd_ctx  *eventfd;
 	u64                  datamatch;
 	struct kvm_io_device dev;
 	bool                 wildcard;
 };
+#endif
+
+struct _ioeventfd {
+	struct list_head     list;
+	struct eventfd_ctx  *eventfd;
+	struct kvm_arch_ioeventfd arch;
+};
 
-static inline struct _ioeventfd *
-to_ioeventfd(struct kvm_io_device *dev)
+static inline struct _ioeventfd *to_ioeventfd(struct kvm_arch_ioeventfd *arch)
 {
-	return container_of(dev, struct _ioeventfd, dev);
+	return container_of(arch, struct _ioeventfd, arch);
+}
+
+struct eventfd_ctx *kvm_ioeventfd_get_eventfd(struct kvm_arch_ioeventfd *arch)
+{
+	struct _ioeventfd *p = to_ioeventfd(arch);
+
+	return p->eventfd;
 }
 
 static void
@@ -595,16 +609,18 @@ ioeventfd_release(struct _ioeventfd *p)
 	kfree(p);
 }
 
+#ifndef __KVM_HAVE_ARCH_IOEVENTFD
 static bool
-ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
+ioeventfd_in_range(struct kvm_arch_ioeventfd *arch, gpa_t addr, int len,
+		   const void *val)
 {
 	u64 _val;
 
-	if (!(addr == p->addr && len == p->length))
+	if (!(addr == arch->addr && len == arch->length))
 		/* address-range must be precise for a hit */
 		return false;
 
-	if (p->wildcard)
+	if (arch->wildcard)
 		/* all else equal, wildcard is always a hit */
 		return true;
 
@@ -629,7 +645,13 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
 		return false;
 	}
 
-	return _val == p->datamatch ? true : false;
+	return _val == arch->datamatch ? true : false;
+}
+
+static inline struct kvm_arch_ioeventfd *
+to_arch_ioeventfd(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_arch_ioeventfd, dev);
 }
 
 /* MMIO/PIO writes trigger an event if the addr/val match */
@@ -637,12 +659,12 @@ static int
 ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
 		const void *val)
 {
-	struct _ioeventfd *p = to_ioeventfd(this);
+	struct kvm_arch_ioeventfd *arch = to_arch_ioeventfd(this);
 
-	if (!ioeventfd_in_range(p, addr, len, val))
+	if (!ioeventfd_in_range(arch, addr, len, val))
 		return -EOPNOTSUPP;
 
-	eventfd_signal(p->eventfd, 1);
+	eventfd_signal(kvm_ioeventfd_get_eventfd(arch), 1);
 	return 0;
 }
 
@@ -653,7 +675,7 @@ ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
 static void
 ioeventfd_destructor(struct kvm_io_device *this)
 {
-	struct _ioeventfd *p = to_ioeventfd(this);
+	struct _ioeventfd *p = to_ioeventfd(to_arch_ioeventfd(this));
 
 	ioeventfd_release(p);
 }
@@ -663,6 +685,87 @@ static const struct kvm_io_device_ops ioeventfd_ops = {
 	.destructor = ioeventfd_destructor,
 };
 
+int kvm_arch_ioeventfd_check(struct kvm_ioeventfd *args)
+{
+	if (args->flags & KVM_IOEVENTFD_FLAG_ARCH)
+		return -EINVAL;
+
+	/* must be natural-word sized */
+	switch (args->len) {
+	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;
+
+	return 0;
+}
+
+void kvm_arch_ioeventfd_init(struct kvm_arch_ioeventfd *arch,
+			     struct kvm_ioeventfd *args)
+{
+	arch->addr    = args->addr;
+	arch->length  = args->len;
+
+	/* The datamatch feature is optional, otherwise this is a wildcard */
+	if (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH)
+		arch->datamatch = args->datamatch;
+	else
+		arch->wildcard = true;
+}
+
+int kvm_arch_ioeventfd_activate(struct kvm *kvm,
+				struct kvm_arch_ioeventfd *arch,
+				struct kvm_ioeventfd *args)
+{
+	int pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
+	enum kvm_bus bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
+
+	kvm_iodevice_init(&arch->dev, &ioeventfd_ops);
+
+	return kvm_io_bus_register_dev(kvm, bus_idx, arch->addr, arch->length,
+				       &arch->dev);
+}
+
+bool kvm_arch_ioeventfd_match(struct kvm_arch_ioeventfd *arch,
+			      struct kvm_arch_ioeventfd *to_match)
+{
+	if (to_match->addr == arch->addr &&
+	    to_match->length == arch->length &&
+	    (to_match->wildcard || arch->wildcard ||
+	     to_match->datamatch == arch->datamatch))
+			return true;
+	return false;
+}
+
+bool kvm_arch_ioeventfd_match_and_release(struct kvm *kvm,
+					  struct kvm_arch_ioeventfd *arch,
+					  struct kvm_ioeventfd *args)
+{
+	int pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
+	enum kvm_bus bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
+	bool wildcard = !(args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH);
+
+	if (arch->addr != args->addr  ||
+	    arch->length != args->len ||
+	    arch->wildcard != wildcard)
+		return false;
+
+	if (!arch->wildcard && arch->datamatch != args->datamatch)
+		return false;
+
+	kvm_io_bus_unregister_dev(kvm, bus_idx, &arch->dev);
+	return true;
+}
+#endif
+
 /* assumes kvm->slots_lock held */
 static bool
 ioeventfd_check_collision(struct kvm *kvm, struct _ioeventfd *p)
@@ -670,9 +773,7 @@ ioeventfd_check_collision(struct kvm *kvm, struct _ioeventfd *p)
 	struct _ioeventfd *_p;
 
 	list_for_each_entry(_p, &kvm->ioeventfds, list)
-		if (_p->addr == p->addr && _p->length == p->length &&
-		    (_p->wildcard || p->wildcard ||
-		     _p->datamatch == p->datamatch))
+		if (kvm_arch_ioeventfd_match(&p->arch, &_p->arch))
 			return true;
 
 	return false;
@@ -681,26 +782,13 @@ ioeventfd_check_collision(struct kvm *kvm, struct _ioeventfd *p)
 static int
 kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
-	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
-	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
 	struct _ioeventfd        *p;
 	struct eventfd_ctx       *eventfd;
 	int                       ret;
 
-	/* must be natural-word sized */
-	switch (args->len) {
-	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;
+	ret = kvm_arch_ioeventfd_check(args);
+	if (ret)
+		return ret;
 
 	/* check for extra flags that we don't understand */
 	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
@@ -717,15 +805,9 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 	}
 
 	INIT_LIST_HEAD(&p->list);
-	p->addr    = args->addr;
-	p->length  = args->len;
 	p->eventfd = eventfd;
 
-	/* The datamatch feature is optional, otherwise this is a wildcard */
-	if (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH)
-		p->datamatch = args->datamatch;
-	else
-		p->wildcard = true;
+	kvm_arch_ioeventfd_init(&p->arch, args);
 
 	mutex_lock(&kvm->slots_lock);
 
@@ -735,10 +817,7 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 		goto unlock_fail;
 	}
 
-	kvm_iodevice_init(&p->dev, &ioeventfd_ops);
-
-	ret = kvm_io_bus_register_dev(kvm, bus_idx, p->addr, p->length,
-				      &p->dev);
+	ret = kvm_arch_ioeventfd_activate(kvm, &p->arch, args);
 	if (ret < 0)
 		goto unlock_fail;
 
@@ -761,8 +840,6 @@ fail:
 static int
 kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
-	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
-	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
 	struct _ioeventfd        *p, *tmp;
 	struct eventfd_ctx       *eventfd;
 	int                       ret = -ENOENT;
@@ -774,18 +851,12 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 	mutex_lock(&kvm->slots_lock);
 
 	list_for_each_entry_safe(p, tmp, &kvm->ioeventfds, list) {
-		bool wildcard = !(args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH);
-
-		if (p->eventfd != eventfd  ||
-		    p->addr != args->addr  ||
-		    p->length != args->len ||
-		    p->wildcard != wildcard)
+		if (p->eventfd != eventfd)
 			continue;
 
-		if (!p->wildcard && p->datamatch != args->datamatch)
+		if (!kvm_arch_ioeventfd_match_and_release(kvm, &p->arch, args))
 			continue;
 
-		kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
 		ioeventfd_release(p);
 		ret = 0;
 		break;
-- 
1.7.12.4

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

[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.huck@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] & 0xffffffff;
+	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 & 0xfff0000) >> 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;
 	}
+	INIT_LIST_HEAD(&kvm->arch.sch_fds);
+	init_rwsem(&kvm->arch.sch_fds_sem);
 
 	kvm->arch.css_support = 0;
 
@@ -1028,3 +1032,164 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 				   struct kvm_memory_slot *slot)
 {
 }
+static void kvm_s390_ioeventfd_add(struct kvm *kvm,
+				   struct kvm_arch_ioeventfd *arch)
+{
+	switch (arch->data.type) {
+	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
+		down_write(&kvm->arch.sch_fds_sem);
+		list_add_tail(&arch->entry, &kvm->arch.sch_fds);
+		up_write(&kvm->arch.sch_fds_sem);
+		break;
+	default:
+		pr_warn("Trying to add ioeventfd type %x\n", arch->data.type);
+	}
+}
+
+static void kvm_s390_ioeventfd_remove(struct kvm *kvm,
+				      struct kvm_arch_ioeventfd *arch)
+{
+	switch (arch->data.type) {
+	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
+		down_write(&kvm->arch.sch_fds_sem);
+		list_del(&arch->entry);
+		up_write(&kvm->arch.sch_fds_sem);
+		break;
+	default:
+		pr_warn("Trying to remove ioeventfd type %x\n",
+			arch->data.type);
+	}
+}
+
+int kvm_s390_ioeventfd_signal(struct kvm *kvm,
+			      struct kvm_s390_ioeventfd_data *data)
+{
+	struct kvm_arch_ioeventfd *arch, match;
+	int ret;
+
+	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
+		return -ENOENT;
+	down_read(&kvm->arch.sch_fds_sem);
+	if (list_empty(&kvm->arch.sch_fds)) {
+		ret = -ENOENT;
+		goto out_unlock;
+	}
+	memcpy(&match.data, data, sizeof(match.data));
+	list_for_each_entry(arch, &kvm->arch.sch_fds, entry) {
+		if (!kvm_arch_ioeventfd_match(arch, &match))
+			continue;
+		ret = eventfd_signal(kvm_ioeventfd_get_eventfd(arch), 1);
+		goto out_unlock;
+	}
+	ret = -ENOENT;
+out_unlock:
+	if (ret > 0)
+		ret = 0;
+	up_read(&kvm->arch.sch_fds_sem);
+	return ret;
+}
+
+int kvm_arch_ioeventfd_check(struct kvm_ioeventfd *args)
+{
+	struct kvm_s390_ioeventfd_data *data;
+
+	if (!(args->flags & KVM_IOEVENTFD_FLAG_ARCH))
+		return -EINVAL;
+	if (args->flags & (KVM_IOEVENTFD_FLAG_DATAMATCH |
+			   KVM_IOEVENTFD_FLAG_PIO))
+		return -EINVAL;
+
+	data = (struct kvm_s390_ioeventfd_data *) args->data;
+	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
+		return -EINVAL;
+	if (((data->virtio_ccw_vq.schid.m == 1) &&
+	     (data->virtio_ccw_vq.schid.cssid != 0xfe)) ||
+	    ((data->virtio_ccw_vq.schid.m == 0) &&
+	     (data->virtio_ccw_vq.schid.cssid != 0)))
+		return -EINVAL;
+	if (data->virtio_ccw_vq.schid.one != 1)
+		return -EINVAL;
+	if (data->virtio_ccw_vq.vq > 128)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_check);
+
+void kvm_arch_ioeventfd_init(struct kvm_arch_ioeventfd *arch,
+			     struct kvm_ioeventfd *args)
+{
+	INIT_LIST_HEAD(&arch->entry);
+	memcpy(&arch->data, &args->data, sizeof(arch->data));
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_init);
+
+int kvm_arch_ioeventfd_activate(struct kvm *kvm,
+				struct kvm_arch_ioeventfd *arch,
+				struct kvm_ioeventfd *args)
+{
+	int ret;
+
+	switch (arch->data.type) {
+	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
+		/* Fail if channel subsystem support is not active. */
+		if (!kvm->arch.css_support)
+			ret = -EINVAL;
+		else {
+			kvm_s390_ioeventfd_add(kvm, arch);
+			ret = 0;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_activate);
+
+bool kvm_arch_ioeventfd_match(struct kvm_arch_ioeventfd *arch,
+			      struct kvm_arch_ioeventfd *to_match)
+{
+	if (arch->data.type != to_match->data.type)
+		return false;
+
+	switch (arch->data.type) {
+	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
+		if (memcmp(&arch->data.virtio_ccw_vq.schid,
+			   &to_match->data.virtio_ccw_vq.schid,
+			   sizeof(arch->data.virtio_ccw_vq.schid)))
+			return false;
+		if (arch->data.virtio_ccw_vq.vq !=
+		    to_match->data.virtio_ccw_vq.vq)
+			return false;
+		return true;
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match);
+
+bool kvm_arch_ioeventfd_match_and_release(struct kvm *kvm,
+					  struct kvm_arch_ioeventfd *arch,
+					  struct kvm_ioeventfd *args)
+{
+	struct kvm_s390_ioeventfd_data *data;
+
+	data = (struct kvm_s390_ioeventfd_data *)args->data;
+	if (arch->data.type != data->type)
+		return false;
+
+	switch (arch->data.type) {
+	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
+		if (memcmp(&arch->data.virtio_ccw_vq.schid,
+			   &data->virtio_ccw_vq.schid,
+			   sizeof(arch->data.virtio_ccw_vq.schid)))
+			return false;
+		if (arch->data.virtio_ccw_vq.vq != data->virtio_ccw_vq.vq)
+			return false;
+		kvm_s390_ioeventfd_remove(kvm, arch);
+		return true;
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match_and_release);
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 4d89d64..9794906 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -136,4 +136,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu,
 /* implemented in diag.c */
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu);
 
+int kvm_s390_ioeventfd_signal(struct kvm *kvm,
+			      struct kvm_s390_ioeventfd_data *data);
+
 #endif
-- 
1.7.12.4

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

[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.huck@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] & 0xffffffff;
> +	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 & 0xfff0000) >> 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;
>  	}
> +	INIT_LIST_HEAD(&kvm->arch.sch_fds);
> +	init_rwsem(&kvm->arch.sch_fds_sem);
>  
>  	kvm->arch.css_support = 0;
>  
> @@ -1028,3 +1032,164 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>  				   struct kvm_memory_slot *slot)
>  {
>  }
> +static void kvm_s390_ioeventfd_add(struct kvm *kvm,
> +				   struct kvm_arch_ioeventfd *arch)
> +{
> +	switch (arch->data.type) {
> +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> +		down_write(&kvm->arch.sch_fds_sem);
> +		list_add_tail(&arch->entry, &kvm->arch.sch_fds);
> +		up_write(&kvm->arch.sch_fds_sem);
> +		break;
> +	default:
> +		pr_warn("Trying to add ioeventfd type %x\n", arch->data.type);
> +	}
> +}
> +
> +static void kvm_s390_ioeventfd_remove(struct kvm *kvm,
> +				      struct kvm_arch_ioeventfd *arch)
> +{
> +	switch (arch->data.type) {
> +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> +		down_write(&kvm->arch.sch_fds_sem);
> +		list_del(&arch->entry);
> +		up_write(&kvm->arch.sch_fds_sem);
> +		break;
> +	default:
> +		pr_warn("Trying to remove ioeventfd type %x\n",
> +			arch->data.type);
> +	}
> +}
> +
> +int kvm_s390_ioeventfd_signal(struct kvm *kvm,
> +			      struct kvm_s390_ioeventfd_data *data)
> +{
> +	struct kvm_arch_ioeventfd *arch, match;
> +	int ret;
> +
> +	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
> +		return -ENOENT;
> +	down_read(&kvm->arch.sch_fds_sem);
> +	if (list_empty(&kvm->arch.sch_fds)) {
> +		ret = -ENOENT;
> +		goto out_unlock;
> +	}
> +	memcpy(&match.data, data, sizeof(match.data));
> +	list_for_each_entry(arch, &kvm->arch.sch_fds, entry) {
> +		if (!kvm_arch_ioeventfd_match(arch, &match))
> +			continue;
> +		ret = eventfd_signal(kvm_ioeventfd_get_eventfd(arch), 1);
> +		goto out_unlock;
> +	}
> +	ret = -ENOENT;
> +out_unlock:
> +	if (ret > 0)
> +		ret = 0;
> +	up_read(&kvm->arch.sch_fds_sem);
> +	return ret;
> +}
> +
> +int kvm_arch_ioeventfd_check(struct kvm_ioeventfd *args)
> +{
> +	struct kvm_s390_ioeventfd_data *data;
> +
> +	if (!(args->flags & KVM_IOEVENTFD_FLAG_ARCH))
> +		return -EINVAL;
> +	if (args->flags & (KVM_IOEVENTFD_FLAG_DATAMATCH |
> +			   KVM_IOEVENTFD_FLAG_PIO))
> +		return -EINVAL;
> +
> +	data = (struct kvm_s390_ioeventfd_data *) args->data;
> +	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
> +		return -EINVAL;
> +	if (((data->virtio_ccw_vq.schid.m == 1) &&
> +	     (data->virtio_ccw_vq.schid.cssid != 0xfe)) ||
> +	    ((data->virtio_ccw_vq.schid.m == 0) &&
> +	     (data->virtio_ccw_vq.schid.cssid != 0)))
> +		return -EINVAL;
> +	if (data->virtio_ccw_vq.schid.one != 1)
> +		return -EINVAL;
> +	if (data->virtio_ccw_vq.vq > 128)

Why 128?
Generally maybe add a comment documenting the
constants used.

> +		return -EINVAL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_check);
> +
> +void kvm_arch_ioeventfd_init(struct kvm_arch_ioeventfd *arch,
> +			     struct kvm_ioeventfd *args)
> +{
> +	INIT_LIST_HEAD(&arch->entry);
> +	memcpy(&arch->data, &args->data, sizeof(arch->data));
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_init);
> +
> +int kvm_arch_ioeventfd_activate(struct kvm *kvm,
> +				struct kvm_arch_ioeventfd *arch,
> +				struct kvm_ioeventfd *args)
> +{
> +	int ret;
> +
> +	switch (arch->data.type) {
> +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> +		/* Fail if channel subsystem support is not active. */
> +		if (!kvm->arch.css_support)
> +			ret = -EINVAL;
> +		else {
> +			kvm_s390_ioeventfd_add(kvm, arch);
> +			ret = 0;
> +		}
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_activate);
> +
> +bool kvm_arch_ioeventfd_match(struct kvm_arch_ioeventfd *arch,
> +			      struct kvm_arch_ioeventfd *to_match)
> +{
> +	if (arch->data.type != to_match->data.type)
> +		return false;
> +
> +	switch (arch->data.type) {
> +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> +		if (memcmp(&arch->data.virtio_ccw_vq.schid,
> +			   &to_match->data.virtio_ccw_vq.schid,
> +			   sizeof(arch->data.virtio_ccw_vq.schid)))
> +			return false;
> +		if (arch->data.virtio_ccw_vq.vq !=
> +		    to_match->data.virtio_ccw_vq.vq)
> +			return false;
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match);
> +
> +bool kvm_arch_ioeventfd_match_and_release(struct kvm *kvm,
> +					  struct kvm_arch_ioeventfd *arch,
> +					  struct kvm_ioeventfd *args)
> +{
> +	struct kvm_s390_ioeventfd_data *data;
> +
> +	data = (struct kvm_s390_ioeventfd_data *)args->data;
> +	if (arch->data.type != data->type)
> +		return false;
> +
> +	switch (arch->data.type) {
> +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> +		if (memcmp(&arch->data.virtio_ccw_vq.schid,
> +			   &data->virtio_ccw_vq.schid,
> +			   sizeof(arch->data.virtio_ccw_vq.schid)))
> +			return false;
> +		if (arch->data.virtio_ccw_vq.vq != data->virtio_ccw_vq.vq)
> +			return false;
> +		kvm_s390_ioeventfd_remove(kvm, arch);
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match_and_release);
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 4d89d64..9794906 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -136,4 +136,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu,
>  /* implemented in diag.c */
>  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu);
>  
> +int kvm_s390_ioeventfd_signal(struct kvm *kvm,
> +			      struct kvm_s390_ioeventfd_data *data);
> +
>  #endif
> -- 
> 1.7.12.4
> 

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

[Cornelia Huck]

On Thu, 21 Feb 2013 16:39:05 +0200
"Michael S. Tsirkin" <mst@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.huck@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))
> > +		return -EOPNOTSUPP;
> > +
> > +	/* subchannel id is in gpr 2, queue in gpr 3 */
> > +	tmp = vcpu->run->s.regs.gprs[2] & 0xffffffff;
> > +	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 & 0xfff0000) >> 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;
> >  	}
> > +	INIT_LIST_HEAD(&kvm->arch.sch_fds);
> > +	init_rwsem(&kvm->arch.sch_fds_sem);
> >  
> >  	kvm->arch.css_support = 0;
> >  
> > @@ -1028,3 +1032,164 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> >  				   struct kvm_memory_slot *slot)
> >  {
> >  }
> > +static void kvm_s390_ioeventfd_add(struct kvm *kvm,
> > +				   struct kvm_arch_ioeventfd *arch)
> > +{
> > +	switch (arch->data.type) {
> > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > +		down_write(&kvm->arch.sch_fds_sem);
> > +		list_add_tail(&arch->entry, &kvm->arch.sch_fds);
> > +		up_write(&kvm->arch.sch_fds_sem);
> > +		break;
> > +	default:
> > +		pr_warn("Trying to add ioeventfd type %x\n", arch->data.type);
> > +	}
> > +}
> > +
> > +static void kvm_s390_ioeventfd_remove(struct kvm *kvm,
> > +				      struct kvm_arch_ioeventfd *arch)
> > +{
> > +	switch (arch->data.type) {
> > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > +		down_write(&kvm->arch.sch_fds_sem);
> > +		list_del(&arch->entry);
> > +		up_write(&kvm->arch.sch_fds_sem);
> > +		break;
> > +	default:
> > +		pr_warn("Trying to remove ioeventfd type %x\n",
> > +			arch->data.type);
> > +	}
> > +}
> > +
> > +int kvm_s390_ioeventfd_signal(struct kvm *kvm,
> > +			      struct kvm_s390_ioeventfd_data *data)
> > +{
> > +	struct kvm_arch_ioeventfd *arch, match;
> > +	int ret;
> > +
> > +	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
> > +		return -ENOENT;
> > +	down_read(&kvm->arch.sch_fds_sem);
> > +	if (list_empty(&kvm->arch.sch_fds)) {
> > +		ret = -ENOENT;
> > +		goto out_unlock;
> > +	}
> > +	memcpy(&match.data, data, sizeof(match.data));
> > +	list_for_each_entry(arch, &kvm->arch.sch_fds, entry) {
> > +		if (!kvm_arch_ioeventfd_match(arch, &match))
> > +			continue;
> > +		ret = eventfd_signal(kvm_ioeventfd_get_eventfd(arch), 1);
> > +		goto out_unlock;
> > +	}
> > +	ret = -ENOENT;
> > +out_unlock:
> > +	if (ret > 0)
> > +		ret = 0;
> > +	up_read(&kvm->arch.sch_fds_sem);
> > +	return ret;
> > +}
> > +
> > +int kvm_arch_ioeventfd_check(struct kvm_ioeventfd *args)
> > +{
> > +	struct kvm_s390_ioeventfd_data *data;
> > +
> > +	if (!(args->flags & KVM_IOEVENTFD_FLAG_ARCH))
> > +		return -EINVAL;
> > +	if (args->flags & (KVM_IOEVENTFD_FLAG_DATAMATCH |
> > +			   KVM_IOEVENTFD_FLAG_PIO))
> > +		return -EINVAL;
> > +
> > +	data = (struct kvm_s390_ioeventfd_data *) args->data;
> > +	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
> > +		return -EINVAL;
> > +	if (((data->virtio_ccw_vq.schid.m == 1) &&
> > +	     (data->virtio_ccw_vq.schid.cssid != 0xfe)) ||
> > +	    ((data->virtio_ccw_vq.schid.m == 0) &&
> > +	     (data->virtio_ccw_vq.schid.cssid != 0)))
> > +		return -EINVAL;
> > +	if (data->virtio_ccw_vq.schid.one != 1)
> > +		return -EINVAL;
> > +	if (data->virtio_ccw_vq.vq > 128)
> 
> Why 128?
> Generally maybe add a comment documenting the
> constants used.

OK.

> 
> > +		return -EINVAL;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_check);
> > +
> > +void kvm_arch_ioeventfd_init(struct kvm_arch_ioeventfd *arch,
> > +			     struct kvm_ioeventfd *args)
> > +{
> > +	INIT_LIST_HEAD(&arch->entry);
> > +	memcpy(&arch->data, &args->data, sizeof(arch->data));
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_init);
> > +
> > +int kvm_arch_ioeventfd_activate(struct kvm *kvm,
> > +				struct kvm_arch_ioeventfd *arch,
> > +				struct kvm_ioeventfd *args)
> > +{
> > +	int ret;
> > +
> > +	switch (arch->data.type) {
> > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > +		/* Fail if channel subsystem support is not active. */
> > +		if (!kvm->arch.css_support)
> > +			ret = -EINVAL;
> > +		else {
> > +			kvm_s390_ioeventfd_add(kvm, arch);
> > +			ret = 0;
> > +		}
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_activate);
> > +
> > +bool kvm_arch_ioeventfd_match(struct kvm_arch_ioeventfd *arch,
> > +			      struct kvm_arch_ioeventfd *to_match)
> > +{
> > +	if (arch->data.type != to_match->data.type)
> > +		return false;
> > +
> > +	switch (arch->data.type) {
> > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > +		if (memcmp(&arch->data.virtio_ccw_vq.schid,
> > +			   &to_match->data.virtio_ccw_vq.schid,
> > +			   sizeof(arch->data.virtio_ccw_vq.schid)))
> > +			return false;
> > +		if (arch->data.virtio_ccw_vq.vq !=
> > +		    to_match->data.virtio_ccw_vq.vq)
> > +			return false;
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match);
> > +
> > +bool kvm_arch_ioeventfd_match_and_release(struct kvm *kvm,
> > +					  struct kvm_arch_ioeventfd *arch,
> > +					  struct kvm_ioeventfd *args)
> > +{
> > +	struct kvm_s390_ioeventfd_data *data;
> > +
> > +	data = (struct kvm_s390_ioeventfd_data *)args->data;
> > +	if (arch->data.type != data->type)
> > +		return false;
> > +
> > +	switch (arch->data.type) {
> > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > +		if (memcmp(&arch->data.virtio_ccw_vq.schid,
> > +			   &data->virtio_ccw_vq.schid,
> > +			   sizeof(arch->data.virtio_ccw_vq.schid)))
> > +			return false;
> > +		if (arch->data.virtio_ccw_vq.vq != data->virtio_ccw_vq.vq)
> > +			return false;
> > +		kvm_s390_ioeventfd_remove(kvm, arch);
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match_and_release);
> > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> > index 4d89d64..9794906 100644
> > --- a/arch/s390/kvm/kvm-s390.h
> > +++ b/arch/s390/kvm/kvm-s390.h
> > @@ -136,4 +136,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu,
> >  /* implemented in diag.c */
> >  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu);
> >  
> > +int kvm_s390_ioeventfd_signal(struct kvm *kvm,
> > +			      struct kvm_s390_ioeventfd_data *data);
> > +
> >  #endif
> > -- 
> > 1.7.12.4
> > 
> 

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

[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" <mst@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.huck@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)
> > >  	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] & 0xffffffff;
> > > +	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 & 0xfff0000) >> 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;
> > >  	}
> > > +	INIT_LIST_HEAD(&kvm->arch.sch_fds);
> > > +	init_rwsem(&kvm->arch.sch_fds_sem);
> > >  
> > >  	kvm->arch.css_support = 0;
> > >  
> > > @@ -1028,3 +1032,164 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> > >  				   struct kvm_memory_slot *slot)
> > >  {
> > >  }
> > > +static void kvm_s390_ioeventfd_add(struct kvm *kvm,
> > > +				   struct kvm_arch_ioeventfd *arch)
> > > +{
> > > +	switch (arch->data.type) {
> > > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > > +		down_write(&kvm->arch.sch_fds_sem);
> > > +		list_add_tail(&arch->entry, &kvm->arch.sch_fds);
> > > +		up_write(&kvm->arch.sch_fds_sem);
> > > +		break;
> > > +	default:
> > > +		pr_warn("Trying to add ioeventfd type %x\n", arch->data.type);
> > > +	}
> > > +}
> > > +
> > > +static void kvm_s390_ioeventfd_remove(struct kvm *kvm,
> > > +				      struct kvm_arch_ioeventfd *arch)
> > > +{
> > > +	switch (arch->data.type) {
> > > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > > +		down_write(&kvm->arch.sch_fds_sem);
> > > +		list_del(&arch->entry);
> > > +		up_write(&kvm->arch.sch_fds_sem);
> > > +		break;
> > > +	default:
> > > +		pr_warn("Trying to remove ioeventfd type %x\n",
> > > +			arch->data.type);
> > > +	}
> > > +}
> > > +
> > > +int kvm_s390_ioeventfd_signal(struct kvm *kvm,
> > > +			      struct kvm_s390_ioeventfd_data *data)
> > > +{
> > > +	struct kvm_arch_ioeventfd *arch, match;
> > > +	int ret;
> > > +
> > > +	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
> > > +		return -ENOENT;
> > > +	down_read(&kvm->arch.sch_fds_sem);
> > > +	if (list_empty(&kvm->arch.sch_fds)) {
> > > +		ret = -ENOENT;
> > > +		goto out_unlock;
> > > +	}
> > > +	memcpy(&match.data, data, sizeof(match.data));
> > > +	list_for_each_entry(arch, &kvm->arch.sch_fds, entry) {
> > > +		if (!kvm_arch_ioeventfd_match(arch, &match))
> > > +			continue;
> > > +		ret = eventfd_signal(kvm_ioeventfd_get_eventfd(arch), 1);
> > > +		goto out_unlock;
> > > +	}
> > > +	ret = -ENOENT;
> > > +out_unlock:
> > > +	if (ret > 0)
> > > +		ret = 0;
> > > +	up_read(&kvm->arch.sch_fds_sem);
> > > +	return ret;
> > > +}
> > > +
> > > +int kvm_arch_ioeventfd_check(struct kvm_ioeventfd *args)
> > > +{
> > > +	struct kvm_s390_ioeventfd_data *data;
> > > +
> > > +	if (!(args->flags & KVM_IOEVENTFD_FLAG_ARCH))
> > > +		return -EINVAL;
> > > +	if (args->flags & (KVM_IOEVENTFD_FLAG_DATAMATCH |
> > > +			   KVM_IOEVENTFD_FLAG_PIO))
> > > +		return -EINVAL;
> > > +
> > > +	data = (struct kvm_s390_ioeventfd_data *) args->data;
> > > +	if (data->type != KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY)
> > > +		return -EINVAL;
> > > +	if (((data->virtio_ccw_vq.schid.m == 1) &&
> > > +	     (data->virtio_ccw_vq.schid.cssid != 0xfe)) ||
> > > +	    ((data->virtio_ccw_vq.schid.m == 0) &&
> > > +	     (data->virtio_ccw_vq.schid.cssid != 0)))
> > > +		return -EINVAL;
> > > +	if (data->virtio_ccw_vq.schid.one != 1)
> > > +		return -EINVAL;
> > > +	if (data->virtio_ccw_vq.vq > 128)
> > 
> > Why 128?
> > Generally maybe add a comment documenting the
> > constants used.
> 
> OK.
> 
> > 
> > > +		return -EINVAL;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_check);
> > > +
> > > +void kvm_arch_ioeventfd_init(struct kvm_arch_ioeventfd *arch,
> > > +			     struct kvm_ioeventfd *args)
> > > +{
> > > +	INIT_LIST_HEAD(&arch->entry);
> > > +	memcpy(&arch->data, &args->data, sizeof(arch->data));
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_init);
> > > +
> > > +int kvm_arch_ioeventfd_activate(struct kvm *kvm,
> > > +				struct kvm_arch_ioeventfd *arch,
> > > +				struct kvm_ioeventfd *args)
> > > +{
> > > +	int ret;
> > > +
> > > +	switch (arch->data.type) {
> > > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > > +		/* Fail if channel subsystem support is not active. */
> > > +		if (!kvm->arch.css_support)
> > > +			ret = -EINVAL;
> > > +		else {
> > > +			kvm_s390_ioeventfd_add(kvm, arch);
> > > +			ret = 0;
> > > +		}
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +	}
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_activate);
> > > +
> > > +bool kvm_arch_ioeventfd_match(struct kvm_arch_ioeventfd *arch,
> > > +			      struct kvm_arch_ioeventfd *to_match)
> > > +{
> > > +	if (arch->data.type != to_match->data.type)
> > > +		return false;
> > > +
> > > +	switch (arch->data.type) {
> > > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > > +		if (memcmp(&arch->data.virtio_ccw_vq.schid,
> > > +			   &to_match->data.virtio_ccw_vq.schid,
> > > +			   sizeof(arch->data.virtio_ccw_vq.schid)))
> > > +			return false;
> > > +		if (arch->data.virtio_ccw_vq.vq !=
> > > +		    to_match->data.virtio_ccw_vq.vq)
> > > +			return false;
> > > +		return true;
> > > +	default:
> > > +		return false;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match);
> > > +
> > > +bool kvm_arch_ioeventfd_match_and_release(struct kvm *kvm,
> > > +					  struct kvm_arch_ioeventfd *arch,
> > > +					  struct kvm_ioeventfd *args)
> > > +{
> > > +	struct kvm_s390_ioeventfd_data *data;
> > > +
> > > +	data = (struct kvm_s390_ioeventfd_data *)args->data;
> > > +	if (arch->data.type != data->type)
> > > +		return false;
> > > +
> > > +	switch (arch->data.type) {
> > > +	case KVM_S390_IOEVENTFD_VIRTIO_CCW_NOTIFY:
> > > +		if (memcmp(&arch->data.virtio_ccw_vq.schid,
> > > +			   &data->virtio_ccw_vq.schid,
> > > +			   sizeof(arch->data.virtio_ccw_vq.schid)))
> > > +			return false;
> > > +		if (arch->data.virtio_ccw_vq.vq != data->virtio_ccw_vq.vq)
> > > +			return false;
> > > +		kvm_s390_ioeventfd_remove(kvm, arch);
> > > +		return true;
> > > +	default:
> > > +		return false;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_arch_ioeventfd_match_and_release);
> > > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> > > index 4d89d64..9794906 100644
> > > --- a/arch/s390/kvm/kvm-s390.h
> > > +++ b/arch/s390/kvm/kvm-s390.h
> > > @@ -136,4 +136,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu,
> > >  /* implemented in diag.c */
> > >  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu);
> > >  
> > > +int kvm_s390_ioeventfd_signal(struct kvm *kvm,
> > > +			      struct kvm_s390_ioeventfd_data *data);
> > > +
> > >  #endif
> > > -- 
> > > 1.7.12.4
> > > 
> > 

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

[Cornelia Huck]

On Thu, 21 Feb 2013 18:34:59 +0200
"Michael S. Tsirkin" <mst@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" <mst@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.huck@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 & 0xfff0000) >> 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 {
 	kvm_ioeventfd_flag_nr_datamatch,
 	kvm_ioeventfd_flag_nr_pio,
 	kvm_ioeventfd_flag_nr_deassign,
+	kvm_ioeventfd_flag_nr_css,
 	kvm_ioeventfd_flag_nr_max,
 };
 
 #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
 #define KVM_IOEVENTFD_FLAG_PIO       (1 << kvm_ioeventfd_flag_nr_pio)
 #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
+#define KVM_IOEVENTFD_FLAG_CSS       (1 << kvm_ioeventfd_flag_nr_css)
 
 #define KVM_IOEVENTFD_VALID_FLAG_MASK  ((1 << kvm_ioeventfd_flag_nr_max) - 1)
 
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index f0ced1a..57a06ff 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -576,6 +576,7 @@ struct _ioeventfd {
 	u64                  datamatch;
 	struct kvm_io_device dev;
 	bool                 wildcard;
+	bool                 is_css;
 };
 
 static inline struct _ioeventfd *
@@ -607,24 +608,27 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
 
 	/* otherwise, we have to actually compare the data */
 
-	BUG_ON(!IS_ALIGNED((unsigned long)val, len));
-
-	switch (len) {
-	case 1:
-		_val = *(u8 *)val;
-		break;
-	case 2:
-		_val = *(u16 *)val;
-		break;
-	case 4:
-		_val = *(u32 *)val;
-		break;
-	case 8:
+	if (!p->is_css) {
+		BUG_ON(!IS_ALIGNED((unsigned long)val, len));
+
+		switch (len) {
+		case 1:
+			_val = *(u8 *)val;
+			break;
+		case 2:
+			_val = *(u16 *)val;
+			break;
+		case 4:
+			_val = *(u32 *)val;
+			break;
+		case 8:
+			_val = *(u64 *)val;
+			break;
+		default:
+			return false;
+		}
+	} else
 		_val = *(u64 *)val;
-		break;
-	default:
-		return false;
-	}
 
 	return _val == p->datamatch ? true : false;
 }
@@ -679,25 +683,29 @@ static int
 kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
-	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
+	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
+	enum kvm_bus              bus_idx;
 	struct _ioeventfd        *p;
 	struct eventfd_ctx       *eventfd;
 	int                       ret;
 
-	/* must be natural-word sized */
-	switch (args->len) {
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-		break;
-	default:
-		return -EINVAL;
-	}
+	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
+	if (!css) {
+		/* must be natural-word sized */
+		switch (args->len) {
+		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 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)
@@ -717,6 +725,7 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 	p->addr    = args->addr;
 	p->length  = args->len;
 	p->eventfd = eventfd;
+	p->is_css  = css;
 
 	/* The datamatch feature is optional, otherwise this is a wildcard */
 	if (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH)
@@ -759,11 +768,13 @@ static int
 kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
-	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
+	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
+	enum kvm_bus              bus_idx;
 	struct _ioeventfd        *p, *tmp;
 	struct eventfd_ctx       *eventfd;
 	int                       ret = -ENOENT;
 
+	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
 	eventfd = eventfd_ctx_fdget(args->fd);
 	if (IS_ERR(eventfd))
 		return PTR_ERR(eventfd);

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

[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" <mst@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" <mst@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.huck@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 & 0xfff0000) >> 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 {
>  	kvm_ioeventfd_flag_nr_datamatch,
>  	kvm_ioeventfd_flag_nr_pio,
>  	kvm_ioeventfd_flag_nr_deassign,
> +	kvm_ioeventfd_flag_nr_css,
>  	kvm_ioeventfd_flag_nr_max,
>  };
>  
>  #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
>  #define KVM_IOEVENTFD_FLAG_PIO       (1 << kvm_ioeventfd_flag_nr_pio)
>  #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
> +#define KVM_IOEVENTFD_FLAG_CSS       (1 << kvm_ioeventfd_flag_nr_css)
>  
>  #define KVM_IOEVENTFD_VALID_FLAG_MASK  ((1 << kvm_ioeventfd_flag_nr_max) - 1)
>  
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index f0ced1a..57a06ff 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -576,6 +576,7 @@ struct _ioeventfd {
>  	u64                  datamatch;
>  	struct kvm_io_device dev;
>  	bool                 wildcard;
> +	bool                 is_css;
>  };
>  
>  static inline struct _ioeventfd *
> @@ -607,24 +608,27 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
>  
>  	/* otherwise, we have to actually compare the data */
>  
> -	BUG_ON(!IS_ALIGNED((unsigned long)val, len));
> -
> -	switch (len) {
> -	case 1:
> -		_val = *(u8 *)val;
> -		break;
> -	case 2:
> -		_val = *(u16 *)val;
> -		break;
> -	case 4:
> -		_val = *(u32 *)val;
> -		break;
> -	case 8:
> +	if (!p->is_css) {
> +		BUG_ON(!IS_ALIGNED((unsigned long)val, len));
> +
> +		switch (len) {
> +		case 1:
> +			_val = *(u8 *)val;
> +			break;
> +		case 2:
> +			_val = *(u16 *)val;
> +			break;
> +		case 4:
> +			_val = *(u32 *)val;
> +			break;
> +		case 8:
> +			_val = *(u64 *)val;
> +			break;
> +		default:
> +			return false;
> +		}
> +	} else
>  		_val = *(u64 *)val;
> -		break;
> -	default:
> -		return false;
> -	}
>  
>  	return _val == p->datamatch ? true : false;
>  }
> @@ -679,25 +683,29 @@ static int
>  kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  {
>  	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
> -	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
> +	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
> +	enum kvm_bus              bus_idx;
>  	struct _ioeventfd        *p;
>  	struct eventfd_ctx       *eventfd;
>  	int                       ret;
>  
> -	/* must be natural-word sized */
> -	switch (args->len) {
> -	case 1:
> -	case 2:
> -	case 4:
> -	case 8:
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
> +	if (!css) {
> +		/* must be natural-word sized */
> +		switch (args->len) {
> +		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 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)
> @@ -717,6 +725,7 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  	p->addr    = args->addr;
>  	p->length  = args->len;
>  	p->eventfd = eventfd;
> +	p->is_css  = css;
>  
>  	/* The datamatch feature is optional, otherwise this is a wildcard */
>  	if (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH)
> @@ -759,11 +768,13 @@ static int
>  kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  {
>  	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
> -	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
> +	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
> +	enum kvm_bus              bus_idx;
>  	struct _ioeventfd        *p, *tmp;
>  	struct eventfd_ctx       *eventfd;
>  	int                       ret = -ENOENT;
>  
> +	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
>  	eventfd = eventfd_ctx_fdget(args->fd);
>  	if (IS_ERR(eventfd))
>  		return PTR_ERR(eventfd);

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

[Cornelia Huck]

On Thu, 21 Feb 2013 22:42:41 +0200
"Michael S. Tsirkin" <mst@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" <mst@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" <mst@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.huck@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 & 0xfff0000) >> 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 {
> >  	kvm_ioeventfd_flag_nr_datamatch,
> >  	kvm_ioeventfd_flag_nr_pio,
> >  	kvm_ioeventfd_flag_nr_deassign,
> > +	kvm_ioeventfd_flag_nr_css,
> >  	kvm_ioeventfd_flag_nr_max,
> >  };
> >  
> >  #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
> >  #define KVM_IOEVENTFD_FLAG_PIO       (1 << kvm_ioeventfd_flag_nr_pio)
> >  #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
> > +#define KVM_IOEVENTFD_FLAG_CSS       (1 << kvm_ioeventfd_flag_nr_css)
> >  
> >  #define KVM_IOEVENTFD_VALID_FLAG_MASK  ((1 << kvm_ioeventfd_flag_nr_max) - 1)
> >  
> > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > index f0ced1a..57a06ff 100644
> > --- a/virt/kvm/eventfd.c
> > +++ b/virt/kvm/eventfd.c
> > @@ -576,6 +576,7 @@ struct _ioeventfd {
> >  	u64                  datamatch;
> >  	struct kvm_io_device dev;
> >  	bool                 wildcard;
> > +	bool                 is_css;
> >  };
> >  
> >  static inline struct _ioeventfd *
> > @@ -607,24 +608,27 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
> >  
> >  	/* otherwise, we have to actually compare the data */
> >  
> > -	BUG_ON(!IS_ALIGNED((unsigned long)val, len));
> > -
> > -	switch (len) {
> > -	case 1:
> > -		_val = *(u8 *)val;
> > -		break;
> > -	case 2:
> > -		_val = *(u16 *)val;
> > -		break;
> > -	case 4:
> > -		_val = *(u32 *)val;
> > -		break;
> > -	case 8:
> > +	if (!p->is_css) {
> > +		BUG_ON(!IS_ALIGNED((unsigned long)val, len));
> > +
> > +		switch (len) {
> > +		case 1:
> > +			_val = *(u8 *)val;
> > +			break;
> > +		case 2:
> > +			_val = *(u16 *)val;
> > +			break;
> > +		case 4:
> > +			_val = *(u32 *)val;
> > +			break;
> > +		case 8:
> > +			_val = *(u64 *)val;
> > +			break;
> > +		default:
> > +			return false;
> > +		}
> > +	} else
> >  		_val = *(u64 *)val;
> > -		break;
> > -	default:
> > -		return false;
> > -	}
> >  
> >  	return _val == p->datamatch ? true : false;
> >  }
> > @@ -679,25 +683,29 @@ static int
> >  kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> >  {
> >  	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
> > -	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
> > +	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
> > +	enum kvm_bus              bus_idx;
> >  	struct _ioeventfd        *p;
> >  	struct eventfd_ctx       *eventfd;
> >  	int                       ret;
> >  
> > -	/* must be natural-word sized */
> > -	switch (args->len) {
> > -	case 1:
> > -	case 2:
> > -	case 4:
> > -	case 8:
> > -		break;
> > -	default:
> > -		return -EINVAL;
> > -	}
> > +	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
> > +	if (!css) {
> > +		/* must be natural-word sized */
> > +		switch (args->len) {
> > +		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 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)
> > @@ -717,6 +725,7 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> >  	p->addr    = args->addr;
> >  	p->length  = args->len;
> >  	p->eventfd = eventfd;
> > +	p->is_css  = css;
> >  
> >  	/* The datamatch feature is optional, otherwise this is a wildcard */
> >  	if (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH)
> > @@ -759,11 +768,13 @@ static int
> >  kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> >  {
> >  	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
> > -	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
> > +	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
> > +	enum kvm_bus              bus_idx;
> >  	struct _ioeventfd        *p, *tmp;
> >  	struct eventfd_ctx       *eventfd;
> >  	int                       ret = -ENOENT;
> >  
> > +	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
> >  	eventfd = eventfd_ctx_fdget(args->fd);
> >  	if (IS_ERR(eventfd))
> >  		return PTR_ERR(eventfd);
> 

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

[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" <mst@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" <mst@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" <mst@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.huck@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 & 0xfff0000) >> 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 {
> > >  	kvm_ioeventfd_flag_nr_datamatch,
> > >  	kvm_ioeventfd_flag_nr_pio,
> > >  	kvm_ioeventfd_flag_nr_deassign,
> > > +	kvm_ioeventfd_flag_nr_css,
> > >  	kvm_ioeventfd_flag_nr_max,
> > >  };
> > >  
> > >  #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
> > >  #define KVM_IOEVENTFD_FLAG_PIO       (1 << kvm_ioeventfd_flag_nr_pio)
> > >  #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
> > > +#define KVM_IOEVENTFD_FLAG_CSS       (1 << kvm_ioeventfd_flag_nr_css)
> > >  
> > >  #define KVM_IOEVENTFD_VALID_FLAG_MASK  ((1 << kvm_ioeventfd_flag_nr_max) - 1)
> > >  
> > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > > index f0ced1a..57a06ff 100644
> > > --- a/virt/kvm/eventfd.c
> > > +++ b/virt/kvm/eventfd.c
> > > @@ -576,6 +576,7 @@ struct _ioeventfd {
> > >  	u64                  datamatch;
> > >  	struct kvm_io_device dev;
> > >  	bool                 wildcard;
> > > +	bool                 is_css;
> > >  };
> > >  
> > >  static inline struct _ioeventfd *
> > > @@ -607,24 +608,27 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
> > >  
> > >  	/* otherwise, we have to actually compare the data */
> > >  
> > > -	BUG_ON(!IS_ALIGNED((unsigned long)val, len));
> > > -
> > > -	switch (len) {
> > > -	case 1:
> > > -		_val = *(u8 *)val;
> > > -		break;
> > > -	case 2:
> > > -		_val = *(u16 *)val;
> > > -		break;
> > > -	case 4:
> > > -		_val = *(u32 *)val;
> > > -		break;
> > > -	case 8:
> > > +	if (!p->is_css) {
> > > +		BUG_ON(!IS_ALIGNED((unsigned long)val, len));
> > > +
> > > +		switch (len) {
> > > +		case 1:
> > > +			_val = *(u8 *)val;
> > > +			break;
> > > +		case 2:
> > > +			_val = *(u16 *)val;
> > > +			break;
> > > +		case 4:
> > > +			_val = *(u32 *)val;
> > > +			break;
> > > +		case 8:
> > > +			_val = *(u64 *)val;
> > > +			break;
> > > +		default:
> > > +			return false;
> > > +		}
> > > +	} else
> > >  		_val = *(u64 *)val;
> > > -		break;
> > > -	default:
> > > -		return false;
> > > -	}
> > >  
> > >  	return _val == p->datamatch ? true : false;
> > >  }
> > > @@ -679,25 +683,29 @@ static int
> > >  kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> > >  {
> > >  	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
> > > -	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
> > > +	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
> > > +	enum kvm_bus              bus_idx;
> > >  	struct _ioeventfd        *p;
> > >  	struct eventfd_ctx       *eventfd;
> > >  	int                       ret;
> > >  
> > > -	/* must be natural-word sized */
> > > -	switch (args->len) {
> > > -	case 1:
> > > -	case 2:
> > > -	case 4:
> > > -	case 8:
> > > -		break;
> > > -	default:
> > > -		return -EINVAL;
> > > -	}
> > > +	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
> > > +	if (!css) {
> > > +		/* must be natural-word sized */
> > > +		switch (args->len) {
> > > +		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 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)
> > > @@ -717,6 +725,7 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> > >  	p->addr    = args->addr;
> > >  	p->length  = args->len;
> > >  	p->eventfd = eventfd;
> > > +	p->is_css  = css;
> > >  
> > >  	/* The datamatch feature is optional, otherwise this is a wildcard */
> > >  	if (args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH)
> > > @@ -759,11 +768,13 @@ static int
> > >  kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> > >  {
> > >  	int                       pio = args->flags & KVM_IOEVENTFD_FLAG_PIO;
> > > -	enum kvm_bus              bus_idx = pio ? KVM_PIO_BUS : KVM_MMIO_BUS;
> > > +	int                       css = args->flags & KVM_IOEVENTFD_FLAG_CSS;
> > > +	enum kvm_bus              bus_idx;
> > >  	struct _ioeventfd        *p, *tmp;
> > >  	struct eventfd_ctx       *eventfd;
> > >  	int                       ret = -ENOENT;
> > >  
> > > +	bus_idx = pio ? KVM_PIO_BUS : css ? KVM_CSS_BUS: KVM_MMIO_BUS;
> > >  	eventfd = eventfd_ctx_fdget(args->fd);
> > >  	if (IS_ERR(eventfd))
> > >  		return PTR_ERR(eventfd);
> >