[PATCH 1/2] S390: take a full byte as ext_param indicator

[PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Currenty the ext_param field only distinguishes between "config change" and
"vring interrupt". We can do a lot more with it though, so let's enable a
full byte of possible values and constants to #defines while at it.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/s390/kvm/kvm_virtio.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 4e298bc..ada7e2c 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -27,6 +27,9 @@
 #include <asm/s390_ext.h>
 
 #define VIRTIO_SUBCODE_64 0x0D00
+#define VIRTIO_PARAM_MASK		0xff
+#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
+#define VIRTIO_PARAM_CONFIG_CHANGED	0x1
 
 /*
  * The pointer to our (page) of device descriptions.
@@ -334,7 +337,7 @@ static void kvm_extint_handler(u16 code)
 {
 	struct virtqueue *vq;
 	u16 subcode;
-	int config_changed;
+	u32 param;
 
 	subcode = S390_lowcore.cpu_addr;
 	if ((subcode & 0xff00) != VIRTIO_SUBCODE_64)
@@ -343,18 +346,25 @@ static void kvm_extint_handler(u16 code)
 	/* The LSB might be overloaded, we have to mask it */
 	vq = (struct virtqueue *)(S390_lowcore.ext_params2 & ~1UL);
 
-	/* We use the LSB of extparam, to decide, if this interrupt is a config
-	 * change or a "standard" interrupt */
-	config_changed = S390_lowcore.ext_params & 1;
+	/* We use ext_params to decide what this interrupt means */
+	param = S390_lowcore.ext_params & VIRTIO_PARAM_MASK;
 
-	if (config_changed) {
+	switch (param) {
+	case VIRTIO_PARAM_CONFIG_CHANGED:
+	{
 		struct virtio_driver *drv;
 		drv = container_of(vq->vdev->dev.driver,
 				   struct virtio_driver, driver);
 		if (drv->config_changed)
 			drv->config_changed(vq->vdev);
-	} else
+
+		break;
+	}
+	case VIRTIO_PARAM_VRING_INTERRUPT:
+	default:
 		vring_interrupt(0, vq);
+		break;
+	}
 }
 
 /*
-- 
1.6.0.2

[PATCH 2/2] S390: Add virtio hotplug add support

[Alexander Graf]

The one big missing feature in s390-virtio was hotplugging. This is no more.
This patch implements hotplug add support, so you can on the fly add new devices
in the guest.

Keep in mind that this needs a patch for qemu to actually leverage the
functionality.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/s390/kvm/kvm_virtio.c |   48 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index ada7e2c..35383a3 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -30,11 +30,13 @@
 #define VIRTIO_PARAM_MASK		0xff
 #define VIRTIO_PARAM_VRING_INTERRUPT	0x0
 #define VIRTIO_PARAM_CONFIG_CHANGED	0x1
+#define VIRTIO_PARAM_DEV_ADD		0x2
 
 /*
  * The pointer to our (page) of device descriptions.
  */
 static void *kvm_devices;
+struct work_struct hotplug_work;
 
 struct kvm_device {
 	struct virtio_device vdev;
@@ -331,6 +333,47 @@ static void scan_devices(void)
 }
 
 /*
+ * match for a kvm device with a specific desc pointer
+ */
+static int match_desc(struct device *dev, void *data)
+{
+	if ((ulong)to_kvmdev(dev_to_virtio(dev))->desc == (ulong)data)
+		return 1;
+
+	return 0;
+}
+
+/*
+ * hotplug_device tries to find changes in the device page.
+ */
+static void hotplug_devices(struct work_struct *dummy)
+{
+	unsigned int i;
+	struct kvm_device_desc *d;
+	struct device *dev;
+
+	for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
+		d = kvm_devices + i;
+
+		/* end of list */
+		if (d->type == 0)
+			break;
+
+		/* device already exists */
+		dev = device_find_child(kvm_root, d, match_desc);
+		if (dev) {
+			/* XXX check for hotplug remove */
+			put_device(dev);
+			continue;
+		}
+
+		/* new device */
+		printk(KERN_INFO "Adding new virtio device %p\n", d);
+		add_kvm_device(d, i);
+	}
+}
+
+/*
  * we emulate the request_irq behaviour on top of s390 extints
  */
 static void kvm_extint_handler(u16 code)
@@ -360,6 +403,9 @@ static void kvm_extint_handler(u16 code)
 
 		break;
 	}
+	case VIRTIO_PARAM_DEV_ADD:
+		schedule_work(&hotplug_work);
+		break;
 	case VIRTIO_PARAM_VRING_INTERRUPT:
 	default:
 		vring_interrupt(0, vq);
@@ -393,6 +439,8 @@ static int __init kvm_devices_init(void)
 
 	kvm_devices = (void *) real_memory_size;
 
+	INIT_WORK(&hotplug_work, hotplug_devices);
+
 	ctl_set_bit(0, 9);
 	register_external_interrupt(0x2603, kvm_extint_handler);
 
-- 
1.6.0.2

Re: [PATCH 2/2] S390: Add virtio hotplug add support

[Christian Borntraeger]

Am 23.08.2010 23:31, schrieb Alexander Graf:
> The one big missing feature in s390-virtio was hotplugging. This is no more.
> This patch implements hotplug add support, so you can on the fly add new devices
> in the guest.

Nice :-)


> Keep in mind that this needs a patch for qemu to actually leverage the
> functionality.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
With the minor nits below fixed:
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

>  #define VIRTIO_PARAM_MASK		0xff
>  #define VIRTIO_PARAM_VRING_INTERRUPT	0x0
>  #define VIRTIO_PARAM_CONFIG_CHANGED	0x1
> +#define VIRTIO_PARAM_DEV_ADD		0x2

See the other patch. This becomes an interface and should go into a header file.
> 
>  /*
>   * The pointer to our (page) of device descriptions.
>   */
>  static void *kvm_devices;
> +struct work_struct hotplug_work;
> 
>  struct kvm_device {
>  	struct virtio_device vdev;
> @@ -331,6 +333,47 @@ static void scan_devices(void)
>  }
> 
>  /*
> + * match for a kvm device with a specific desc pointer
> + */
> +static int match_desc(struct device *dev, void *data)
> +{
> +	if ((ulong)to_kvmdev(dev_to_virtio(dev))->desc == (ulong)data)

ulong doesnt look like kernel coding style.

Re: [PATCH 2/2] S390: Add virtio hotplug add support

[Christian Borntraeger]

Am 23.08.2010 23:31, schrieb Alexander Graf:
> The one big missing feature in s390-virtio was hotplugging. This is no more.
> This patch implements hotplug add support, so you can on the fly add new devices
> in the guest.

Nice :-)


> Keep in mind that this needs a patch for qemu to actually leverage the
> functionality.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
With the minor nits below fixed:
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

>  #define VIRTIO_PARAM_MASK		0xff
>  #define VIRTIO_PARAM_VRING_INTERRUPT	0x0
>  #define VIRTIO_PARAM_CONFIG_CHANGED	0x1
> +#define VIRTIO_PARAM_DEV_ADD		0x2

See the other patch. This becomes an interface and should go into a header file.
> 
>  /*
>   * The pointer to our (page) of device descriptions.
>   */
>  static void *kvm_devices;
> +struct work_struct hotplug_work;
> 
>  struct kvm_device {
>  	struct virtio_device vdev;
> @@ -331,6 +333,47 @@ static void scan_devices(void)
>  }
> 
>  /*
> + * match for a kvm device with a specific desc pointer
> + */
> +static int match_desc(struct device *dev, void *data)
> +{
> +	if ((ulong)to_kvmdev(dev_to_virtio(dev))->desc == (ulong)data)

ulong doesnt look like kernel coding style.

[PATCH 2/2] S390: Add virtio hotplug add support

[Alexander Graf]

The one big missing feature in s390-virtio was hotplugging. This is no more.
This patch implements hotplug add support, so you can on the fly add new devices
in the guest.

Keep in mind that this needs a patch for qemu to actually leverage the
functionality.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/s390/kvm/kvm_virtio.c |   48 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index ada7e2c..35383a3 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -30,11 +30,13 @@
 #define VIRTIO_PARAM_MASK		0xff
 #define VIRTIO_PARAM_VRING_INTERRUPT	0x0
 #define VIRTIO_PARAM_CONFIG_CHANGED	0x1
+#define VIRTIO_PARAM_DEV_ADD		0x2
 
 /*
  * The pointer to our (page) of device descriptions.
  */
 static void *kvm_devices;
+struct work_struct hotplug_work;
 
 struct kvm_device {
 	struct virtio_device vdev;
@@ -331,6 +333,47 @@ static void scan_devices(void)
 }
 
 /*
+ * match for a kvm device with a specific desc pointer
+ */
+static int match_desc(struct device *dev, void *data)
+{
+	if ((ulong)to_kvmdev(dev_to_virtio(dev))->desc == (ulong)data)
+		return 1;
+
+	return 0;
+}
+
+/*
+ * hotplug_device tries to find changes in the device page.
+ */
+static void hotplug_devices(struct work_struct *dummy)
+{
+	unsigned int i;
+	struct kvm_device_desc *d;
+	struct device *dev;
+
+	for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
+		d = kvm_devices + i;
+
+		/* end of list */
+		if (d->type == 0)
+			break;
+
+		/* device already exists */
+		dev = device_find_child(kvm_root, d, match_desc);
+		if (dev) {
+			/* XXX check for hotplug remove */
+			put_device(dev);
+			continue;
+		}
+
+		/* new device */
+		printk(KERN_INFO "Adding new virtio device %p\n", d);
+		add_kvm_device(d, i);
+	}
+}
+
+/*
  * we emulate the request_irq behaviour on top of s390 extints
  */
 static void kvm_extint_handler(u16 code)
@@ -360,6 +403,9 @@ static void kvm_extint_handler(u16 code)
 
 		break;
 	}
+	case VIRTIO_PARAM_DEV_ADD:
+		schedule_work(&hotplug_work);
+		break;
 	case VIRTIO_PARAM_VRING_INTERRUPT:
 	default:
 		vring_interrupt(0, vq);
@@ -393,6 +439,8 @@ static int __init kvm_devices_init(void)
 
 	kvm_devices = (void *) real_memory_size;
 
+	INIT_WORK(&hotplug_work, hotplug_devices);
+
 	ctl_set_bit(0, 9);
 	register_external_interrupt(0x2603, kvm_extint_handler);
 
-- 
1.6.0.2

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Christian Borntraeger]

Am 23.08.2010 23:31, schrieb Alexander Graf:
> Currenty the ext_param field only distinguishes between "config change" and
> "vring interrupt". We can do a lot more with it though, so let's enable a
> full byte of possible values and constants to #defines while at it.

Makes a lot of sense.
[...]

>  #define VIRTIO_SUBCODE_64 0x0D00
> +#define VIRTIO_PARAM_MASK		0xff
> +#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
> +#define VIRTIO_PARAM_CONFIG_CHANGED	0x1

Maybe this should be exported in a header, something like 
arch/s390/include/asm/kvm_virtio.h? In that case this file
must be added to Kbuild for make headers_install.

Christian

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Christian Borntraeger]

Am 23.08.2010 23:31, schrieb Alexander Graf:
> Currenty the ext_param field only distinguishes between "config change" and
> "vring interrupt". We can do a lot more with it though, so let's enable a
> full byte of possible values and constants to #defines while at it.

Makes a lot of sense.
[...]

>  #define VIRTIO_SUBCODE_64 0x0D00
> +#define VIRTIO_PARAM_MASK		0xff
> +#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
> +#define VIRTIO_PARAM_CONFIG_CHANGED	0x1

Maybe this should be exported in a header, something like 
arch/s390/include/asm/kvm_virtio.h? In that case this file
must be added to Kbuild for make headers_install.

Christian

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Christian Borntraeger wrote:
> Am 23.08.2010 23:31, schrieb Alexander Graf:
>   
>> Currenty the ext_param field only distinguishes between "config change" and
>> "vring interrupt". We can do a lot more with it though, so let's enable a
>> full byte of possible values and constants to #defines while at it.
>>     
>
> Makes a lot of sense.
> [...]
>
>   
>>  #define VIRTIO_SUBCODE_64 0x0D00
>> +#define VIRTIO_PARAM_MASK		0xff
>> +#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
>> +#define VIRTIO_PARAM_CONFIG_CHANGED	0x1
>>     
>
> Maybe this should be exported in a header, something like 
> arch/s390/include/asm/kvm_virtio.h? In that case this file
> must be added to Kbuild for make headers_install.
>   

While that thought sounds good at first, it's really no use for anyone,
right? I mean - in qemu we need to define the defines manually anyways
because we need to potentially be able to build the s390x target on
non-s390x, possibly on non-Linux.


Alex

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Christian Borntraeger wrote:
> Am 23.08.2010 23:31, schrieb Alexander Graf:
>   
>> Currenty the ext_param field only distinguishes between "config change" and
>> "vring interrupt". We can do a lot more with it though, so let's enable a
>> full byte of possible values and constants to #defines while at it.
>>     
>
> Makes a lot of sense.
> [...]
>
>   
>>  #define VIRTIO_SUBCODE_64 0x0D00
>> +#define VIRTIO_PARAM_MASK		0xff
>> +#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
>> +#define VIRTIO_PARAM_CONFIG_CHANGED	0x1
>>     
>
> Maybe this should be exported in a header, something like 
> arch/s390/include/asm/kvm_virtio.h? In that case this file
> must be added to Kbuild for make headers_install.
>   

While that thought sounds good at first, it's really no use for anyone,
right? I mean - in qemu we need to define the defines manually anyways
because we need to potentially be able to build the s390x target on
non-s390x, possibly on non-Linux.


Alex

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Christian Borntraeger]

Am 24.08.2010 14:06, schrieb Alexander Graf:
>>>  #define VIRTIO_SUBCODE_64 0x0D00
>>> +#define VIRTIO_PARAM_MASK		0xff
>>> +#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
>>> +#define VIRTIO_PARAM_CONFIG_CHANGED	0x1
>>>     
>>
>> Maybe this should be exported in a header, something like 
>> arch/s390/include/asm/kvm_virtio.h? In that case this file
>> must be added to Kbuild for make headers_install.
>>   
> 
> While that thought sounds good at first, it's really no use for anyone,
> right? I mean - in qemu we need to define the defines manually anyways
> because we need to potentially be able to build the s390x target on
> non-s390x, possibly on non-Linux.

I have no strong opinion on that, but I think its more a matter of where
to put an interface description. A header file seems just the right place.
I will let you (or Rusty) decide.

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Christian Borntraeger]

Am 24.08.2010 14:06, schrieb Alexander Graf:
>>>  #define VIRTIO_SUBCODE_64 0x0D00
>>> +#define VIRTIO_PARAM_MASK		0xff
>>> +#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
>>> +#define VIRTIO_PARAM_CONFIG_CHANGED	0x1
>>>     
>>
>> Maybe this should be exported in a header, something like 
>> arch/s390/include/asm/kvm_virtio.h? In that case this file
>> must be added to Kbuild for make headers_install.
>>   
> 
> While that thought sounds good at first, it's really no use for anyone,
> right? I mean - in qemu we need to define the defines manually anyways
> because we need to potentially be able to build the s390x target on
> non-s390x, possibly on non-Linux.

I have no strong opinion on that, but I think its more a matter of where
to put an interface description. A header file seems just the right place.
I will let you (or Rusty) decide.

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Avi Kivity]

  On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>
> I have no strong opinion on that, but I think its more a matter of where
> to put an interface description. A header file seems just the right place.
> I will let you (or Rusty) decide.

First of all we need a virtio/s390 specification, like we have a 
virtio/pci spec.

Second, I agree it should be in an exported header file, even if qemu 
doesn't make use of it.  It's an interface and should be exported.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Avi Kivity]

  On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>
> I have no strong opinion on that, but I think its more a matter of where
> to put an interface description. A header file seems just the right place.
> I will let you (or Rusty) decide.

First of all we need a virtio/s390 specification, like we have a 
virtio/pci spec.

Second, I agree it should be in an exported header file, even if qemu 
doesn't make use of it.  It's an interface and should be exported.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Avi Kivity wrote:
>  On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>>
>> I have no strong opinion on that, but I think its more a matter of where
>> to put an interface description. A header file seems just the right
>> place.
>> I will let you (or Rusty) decide.
>
> First of all we need a virtio/s390 specification, like we have a
> virtio/pci spec.

Sure, go ahead and write one :). All the bits are open.
No seriously, I've wanted to write one for quite a while but this is not
the right patch set for this.

> Second, I agree it should be in an exported header file, even if qemu
> doesn't make use of it.  It's an interface and should be exported.

Hrm. *shrug* if you think it makes sense. I'm reasonably indifferent
either way, but I don't see value-add in it.


Alex

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Avi Kivity wrote:
>  On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>>
>> I have no strong opinion on that, but I think its more a matter of where
>> to put an interface description. A header file seems just the right
>> place.
>> I will let you (or Rusty) decide.
>
> First of all we need a virtio/s390 specification, like we have a
> virtio/pci spec.

Sure, go ahead and write one :). All the bits are open.
No seriously, I've wanted to write one for quite a while but this is not
the right patch set for this.

> Second, I agree it should be in an exported header file, even if qemu
> doesn't make use of it.  It's an interface and should be exported.

Hrm. *shrug* if you think it makes sense. I'm reasonably indifferent
either way, but I don't see value-add in it.


Alex

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Avi Kivity]

  On 08/24/2010 03:25 PM, Alexander Graf wrote:
> Avi Kivity wrote:
>>   On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>>> I have no strong opinion on that, but I think its more a matter of where
>>> to put an interface description. A header file seems just the right
>>> place.
>>> I will let you (or Rusty) decide.
>> First of all we need a virtio/s390 specification, like we have a
>> virtio/pci spec.
> Sure, go ahead and write one :). All the bits are open.
> No seriously, I've wanted to write one for quite a while but this is not
> the right patch set for this.

Perhaps we should freeze virtio/s390 development until someone feels 
sufficiently motivated.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Avi Kivity]

  On 08/24/2010 03:25 PM, Alexander Graf wrote:
> Avi Kivity wrote:
>>   On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>>> I have no strong opinion on that, but I think its more a matter of where
>>> to put an interface description. A header file seems just the right
>>> place.
>>> I will let you (or Rusty) decide.
>> First of all we need a virtio/s390 specification, like we have a
>> virtio/pci spec.
> Sure, go ahead and write one :). All the bits are open.
> No seriously, I've wanted to write one for quite a while but this is not
> the right patch set for this.

Perhaps we should freeze virtio/s390 development until someone feels 
sufficiently motivated.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Avi Kivity wrote:
>  On 08/24/2010 03:25 PM, Alexander Graf wrote:
>> Avi Kivity wrote:
>>>   On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>>>> I have no strong opinion on that, but I think its more a matter of
>>>> where
>>>> to put an interface description. A header file seems just the right
>>>> place.
>>>> I will let you (or Rusty) decide.
>>> First of all we need a virtio/s390 specification, like we have a
>>> virtio/pci spec.
>> Sure, go ahead and write one :). All the bits are open.
>> No seriously, I've wanted to write one for quite a while but this is not
>> the right patch set for this.
>
> Perhaps we should freeze virtio/s390 development until someone feels
> sufficiently motivated.

Sure, go ahead. I don't think that'll help anyone but if it makes you
feel good...


Alex

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Avi Kivity wrote:
>  On 08/24/2010 03:25 PM, Alexander Graf wrote:
>> Avi Kivity wrote:
>>>   On 08/24/2010 03:14 PM, Christian Borntraeger wrote:
>>>> I have no strong opinion on that, but I think its more a matter of
>>>> where
>>>> to put an interface description. A header file seems just the right
>>>> place.
>>>> I will let you (or Rusty) decide.
>>> First of all we need a virtio/s390 specification, like we have a
>>> virtio/pci spec.
>> Sure, go ahead and write one :). All the bits are open.
>> No seriously, I've wanted to write one for quite a while but this is not
>> the right patch set for this.
>
> Perhaps we should freeze virtio/s390 development until someone feels
> sufficiently motivated.

Sure, go ahead. I don't think that'll help anyone but if it makes you
feel good...


Alex

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Avi Kivity]

  On 08/24/2010 03:32 PM, Alexander Graf wrote:
>
>> Perhaps we should freeze virtio/s390 development until someone feels
>> sufficiently motivated.
> Sure, go ahead. I don't think that'll help anyone but if it makes you
> feel good...

I don't maintain virtio or the virtio-s390 interface, so I can't freeze 
it, but I think we should do so.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Avi Kivity]

  On 08/24/2010 03:32 PM, Alexander Graf wrote:
>
>> Perhaps we should freeze virtio/s390 development until someone feels
>> sufficiently motivated.
> Sure, go ahead. I don't think that'll help anyone but if it makes you
> feel good...

I don't maintain virtio or the virtio-s390 interface, so I can't freeze 
it, but I think we should do so.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Christian Borntraeger]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

Am 24.08.2010 14:22, schrieb Avi Kivity:
> First of all we need a virtio/s390 specification, like we have a
> virtio/pci spec.

Here is something that I started a year ago but never finished.

Christian





[-- Attachment #2: virtio.txt --]
[-- Type: text/plain, Size: 9520 bytes --]

guest/host interface for s390/virtio devices

            KVM_DEVICE_DESCRIPTOR PAGE
                0        8        16       24    31
              +--------+--------+--------+--------+
            0 | type   | num_vq | featlen| conflen|
              +--------+--------+--------+--------+
            1 | status |                          |
              +--------+                          +------>+
            2 |                 config            |       |
              +                                   +       |
                            ...                           |
                                                          |
    +-----------------------------------------------------+
    |
    |       CONFIG ARRAY
    |           0        8        16       24    31
    +-------->+--------+--------+--------+--------+
            0 |                                   |
              +                                   +
            1 |                                   |
              +                                   +
            2 |                                   |
              +    virtqueue 0                    +------>+
            3 |                                   |       |
              +                                   +       |
            4 |                                   |       |
              +                                   +       |
            5 |                                   |       |
              +--------+--------+--------+--------+       |
              |///////////////////////////////////+       |
              |///////////////////////////////////+       |
              +--------+--------+--------+--------+       |
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +    virtqueue num_vq - 1           +------>+
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +--------+--------+--------+--------+       |
     num_vq*6 | feature bits  (featlen * 2 Bytes) |       |
              +                 +--------+--------+       |
              |                 |                 |       |
              +--------+--------+                 +       |
              | config space (conflen Bytes)      |       |
              +                          +--------+       |
              |                          |                |
              +--------+--------+--------+                |
                                                          |
                                                          |
    +-----------------------------------------------------+
    |
    |       VIRTQUEUE
    |          0        8        16       24    31
    |-------->+--------+--------+--------+--------+
            0 |     interrupt token ...           |
              +                                   +
            1 |     ... set by guest              |
              +--------+--------+--------+--------+
            2 |     virtio ring address           |
              +                                   +
            3 |     ... set by host               |
              +--------+--------+--------+--------+
            4 | number of elems |/////////////////+
              +--------+--------+/////////////////+
            5 |///////////////////////////////////+
              +--------+--------+--------+--------+



            COMPLETE KVM_DEVICE_DESCRIPTOR PAGE
                0        8        16       24    31
              +--------+--------+--------+--------+
            0 | type   | num_vq | featlen| conflen|
              +--------+--------+--------+--------+
            1 | status |  interrupt token         |
              +--------+                          +
            2 | virtqueue 0(set by guest)         |
              +        +--------+--------+--------+
            3 |        | virtio ring address      |
              +--------+                          +
            4 | virtqueue 0 (set by host)         |
              +        +--------+--------+--------+
            5 |        | number of descr |////////|
              +--------+--------+--------+////////+
            6 |///////////////////////////////////|
              +////////+--------+--------+--------+
            7 |////////|   interrupt token        |
              +--------+                          +
            8 | virtqueue 1(set by guest)         |
              +        +--------+--------+--------+
            9 |        | virtio ring address      |
              +--------+                          +
           10 | virtqueue 1 (set by host)         |
              +        +--------+--------+--------+
           11 |        | number of descr |////////|
              +--------+--------+--------+////////+
           12 |///////////////////////////////////|
              +////////+--------+--------+--------+
           13 |////////|
              +--------+      .....

            ? |///////////////////////////////////|
              +////////+--------+--------+--------+
            ? |////////| feature bits             |
              +--------+ (size multiple  +--------+
              |          of 2 bytes)     |        |
              +--------+--------+--------+        +
              | config space (size of any length) |
              +                 +++++++++++++++++++
              |                 |
              +--------+--------+




Host->Guest notification:
-------------------------

via external interrupt with
- real storage 0x86-0x87 : 0x2603  --> host interrupt
- real storage 0x84: 0xd --> VIRTIO_SUBCODE_64
- real storage 0x11B8-0x11bf: the interrupt token (is a pointer to a struct virtqueue -> set by guest)
- real storage 0x83:
   0: normal interrupt
   1: config change

Guest->Host notification:
-------------------------
via  diagnose call:

diag 2,4,0x500\n"

 The calling convention is similar to the
 * s390 ABI, so we use R2-R6 for parameters 1-5. In addition we use R1
 * as hypercall number and R7 as parameter 6. The return value is
 * written to R2. We use the diagnose instru

hypercall numbers are
#define KVM_S390_VIRTIO_NOTIFY          0
#define KVM_S390_VIRTIO_RESET           1
#define KVM_S390_VIRTIO_SET_STATUS      2



Virtio Ring: (3Pages)
---------------------

                0        8        16       24    31
              +--------+--------+--------+--------+\
            0 |  Desriptor 0:                     | \
              +  guest physical address           +  \
            1 |                                   |   \
              +--------+--------+--------+--------+     one descriptor
            2 | Descriptor 0: Length              |   /
              +--------+--------+--------+--------+  /
            3 |   D0: Flags     |   D0: Next      | /
              +--------+--------+--------+--------+/
              |                                   |
                              ...
              |                                   |
              +--------+--------+--------+--------+
            0 |       Descriptor num -1           |
              +  - guest physical address 64bit   +
            1 |  - Length 32bit                   |
              +  - flags 16 bit                   +
            2 |  - next 16 bit                    |
              +                                   +
            3 |                                   |
              +--------+--------+--------+--------+
              |///////////////////////////////////|
                                ...
              |///////////////////////////////////|
              +--------+--------+--------+--------+
         4096 |   avail_flags   |    avail_idx    |
              +--------+--------+--------+--------+
         4097 |  available 0    |   available 1   |
              +--------+--------+--------+--------+
              |                                   |
                               ...
              |                                   |
              +--------+--------+--------+--------+
              | available num-2 | available num-1 |
              +--------+--------+--------+--------+
              |///////////////////////////////////|
                                ...
              |///////////////////////////////////|
              +--------+--------+--------+--------+
         8192 |   used_flags    | used_idx        |
              +--------+--------+--------+--------+
              | used_elem_id 0                    |
              +--------+--------+--------+--------+
              | used_elem_len 0                   |
              +--------+--------+--------+--------+
              |                                   |
                               ...
              |                                   |
              +--------+--------+--------+--------+
              | used_elem_id num-1                |
              +--------+--------+--------+--------+
              | used_elem_len num-1               |
              +--------+--------+--------+--------+



[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] S390: take a full byte as ext_param indicator

[Christian Borntraeger]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

Am 24.08.2010 14:22, schrieb Avi Kivity:
> First of all we need a virtio/s390 specification, like we have a
> virtio/pci spec.

Here is something that I started a year ago but never finished.

Christian





[-- Attachment #2: virtio.txt --]
[-- Type: text/plain, Size: 9520 bytes --]

guest/host interface for s390/virtio devices

            KVM_DEVICE_DESCRIPTOR PAGE
                0        8        16       24    31
              +--------+--------+--------+--------+
            0 | type   | num_vq | featlen| conflen|
              +--------+--------+--------+--------+
            1 | status |                          |
              +--------+                          +------>+
            2 |                 config            |       |
              +                                   +       |
                            ...                           |
                                                          |
    +-----------------------------------------------------+
    |
    |       CONFIG ARRAY
    |           0        8        16       24    31
    +-------->+--------+--------+--------+--------+
            0 |                                   |
              +                                   +
            1 |                                   |
              +                                   +
            2 |                                   |
              +    virtqueue 0                    +------>+
            3 |                                   |       |
              +                                   +       |
            4 |                                   |       |
              +                                   +       |
            5 |                                   |       |
              +--------+--------+--------+--------+       |
              |///////////////////////////////////+       |
              |///////////////////////////////////+       |
              +--------+--------+--------+--------+       |
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +    virtqueue num_vq - 1           +------>+
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +                                   +       |
              |                                   |       |
              +--------+--------+--------+--------+       |
     num_vq*6 | feature bits  (featlen * 2 Bytes) |       |
              +                 +--------+--------+       |
              |                 |                 |       |
              +--------+--------+                 +       |
              | config space (conflen Bytes)      |       |
              +                          +--------+       |
              |                          |                |
              +--------+--------+--------+                |
                                                          |
                                                          |
    +-----------------------------------------------------+
    |
    |       VIRTQUEUE
    |          0        8        16       24    31
    |-------->+--------+--------+--------+--------+
            0 |     interrupt token ...           |
              +                                   +
            1 |     ... set by guest              |
              +--------+--------+--------+--------+
            2 |     virtio ring address           |
              +                                   +
            3 |     ... set by host               |
              +--------+--------+--------+--------+
            4 | number of elems |/////////////////+
              +--------+--------+/////////////////+
            5 |///////////////////////////////////+
              +--------+--------+--------+--------+



            COMPLETE KVM_DEVICE_DESCRIPTOR PAGE
                0        8        16       24    31
              +--------+--------+--------+--------+
            0 | type   | num_vq | featlen| conflen|
              +--------+--------+--------+--------+
            1 | status |  interrupt token         |
              +--------+                          +
            2 | virtqueue 0(set by guest)         |
              +        +--------+--------+--------+
            3 |        | virtio ring address      |
              +--------+                          +
            4 | virtqueue 0 (set by host)         |
              +        +--------+--------+--------+
            5 |        | number of descr |////////|
              +--------+--------+--------+////////+
            6 |///////////////////////////////////|
              +////////+--------+--------+--------+
            7 |////////|   interrupt token        |
              +--------+                          +
            8 | virtqueue 1(set by guest)         |
              +        +--------+--------+--------+
            9 |        | virtio ring address      |
              +--------+                          +
           10 | virtqueue 1 (set by host)         |
              +        +--------+--------+--------+
           11 |        | number of descr |////////|
              +--------+--------+--------+////////+
           12 |///////////////////////////////////|
              +////////+--------+--------+--------+
           13 |////////|
              +--------+      .....

            ? |///////////////////////////////////|
              +////////+--------+--------+--------+
            ? |////////| feature bits             |
              +--------+ (size multiple  +--------+
              |          of 2 bytes)     |        |
              +--------+--------+--------+        +
              | config space (size of any length) |
              +                 +++++++++++++++++++
              |                 |
              +--------+--------+




Host->Guest notification:
-------------------------

via external interrupt with
- real storage 0x86-0x87 : 0x2603  --> host interrupt
- real storage 0x84: 0xd --> VIRTIO_SUBCODE_64
- real storage 0x11B8-0x11bf: the interrupt token (is a pointer to a struct virtqueue -> set by guest)
- real storage 0x83:
   0: normal interrupt
   1: config change

Guest->Host notification:
-------------------------
via  diagnose call:

diag 2,4,0x500\n"

 The calling convention is similar to the
 * s390 ABI, so we use R2-R6 for parameters 1-5. In addition we use R1
 * as hypercall number and R7 as parameter 6. The return value is
 * written to R2. We use the diagnose instru

hypercall numbers are
#define KVM_S390_VIRTIO_NOTIFY          0
#define KVM_S390_VIRTIO_RESET           1
#define KVM_S390_VIRTIO_SET_STATUS      2



Virtio Ring: (3Pages)
---------------------

                0        8        16       24    31
              +--------+--------+--------+--------+\
            0 |  Desriptor 0:                     | \
              +  guest physical address           +  \
            1 |                                   |   \
              +--------+--------+--------+--------+     one descriptor
            2 | Descriptor 0: Length              |   /
              +--------+--------+--------+--------+  /
            3 |   D0: Flags     |   D0: Next      | /
              +--------+--------+--------+--------+/
              |                                   |
                              ...
              |                                   |
              +--------+--------+--------+--------+
            0 |       Descriptor num -1           |
              +  - guest physical address 64bit   +
            1 |  - Length 32bit                   |
              +  - flags 16 bit                   +
            2 |  - next 16 bit                    |
              +                                   +
            3 |                                   |
              +--------+--------+--------+--------+
              |///////////////////////////////////|
                                ...
              |///////////////////////////////////|
              +--------+--------+--------+--------+
         4096 |   avail_flags   |    avail_idx    |
              +--------+--------+--------+--------+
         4097 |  available 0    |   available 1   |
              +--------+--------+--------+--------+
              |                                   |
                               ...
              |                                   |
              +--------+--------+--------+--------+
              | available num-2 | available num-1 |
              +--------+--------+--------+--------+
              |///////////////////////////////////|
                                ...
              |///////////////////////////////////|
              +--------+--------+--------+--------+
         8192 |   used_flags    | used_idx        |
              +--------+--------+--------+--------+
              | used_elem_id 0                    |
              +--------+--------+--------+--------+
              | used_elem_len 0                   |
              +--------+--------+--------+--------+
              |                                   |
                               ...
              |                                   |
              +--------+--------+--------+--------+
              | used_elem_id num-1                |
              +--------+--------+--------+--------+
              | used_elem_len num-1               |
              +--------+--------+--------+--------+

[PATCH 1/2] S390: take a full byte as ext_param indicator

[Alexander Graf]

Currenty the ext_param field only distinguishes between "config change" and
"vring interrupt". We can do a lot more with it though, so let's enable a
full byte of possible values and constants to #defines while at it.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/s390/kvm/kvm_virtio.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 4e298bc..ada7e2c 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -27,6 +27,9 @@
 #include <asm/s390_ext.h>
 
 #define VIRTIO_SUBCODE_64 0x0D00
+#define VIRTIO_PARAM_MASK		0xff
+#define VIRTIO_PARAM_VRING_INTERRUPT	0x0
+#define VIRTIO_PARAM_CONFIG_CHANGED	0x1
 
 /*
  * The pointer to our (page) of device descriptions.
@@ -334,7 +337,7 @@ static void kvm_extint_handler(u16 code)
 {
 	struct virtqueue *vq;
 	u16 subcode;
-	int config_changed;
+	u32 param;
 
 	subcode = S390_lowcore.cpu_addr;
 	if ((subcode & 0xff00) != VIRTIO_SUBCODE_64)
@@ -343,18 +346,25 @@ static void kvm_extint_handler(u16 code)
 	/* The LSB might be overloaded, we have to mask it */
 	vq = (struct virtqueue *)(S390_lowcore.ext_params2 & ~1UL);
 
-	/* We use the LSB of extparam, to decide, if this interrupt is a config
-	 * change or a "standard" interrupt */
-	config_changed = S390_lowcore.ext_params & 1;
+	/* We use ext_params to decide what this interrupt means */
+	param = S390_lowcore.ext_params & VIRTIO_PARAM_MASK;
 
-	if (config_changed) {
+	switch (param) {
+	case VIRTIO_PARAM_CONFIG_CHANGED:
+	{
 		struct virtio_driver *drv;
 		drv = container_of(vq->vdev->dev.driver,
 				   struct virtio_driver, driver);
 		if (drv->config_changed)
 			drv->config_changed(vq->vdev);
-	} else
+
+		break;
+	}
+	case VIRTIO_PARAM_VRING_INTERRUPT:
+	default:
 		vring_interrupt(0, vq);
+		break;
+	}
 }
 
 /*
-- 
1.6.0.2