[Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Cornelia Huck]

On Friday, Thomas noticed some problems with 3270 devices. One result
was "s390x/css: disabled subchannels cannot be status pending", but
a reboot did not cure the previous broken status. Turns out that 3270
devices are missing a reset handler.

This series cleans up reset handling a bit and makes sure that the base
ccw device class provides a subchannel reset handler. I'm currently
not sure what we should do with vfio-ccw, so the behaviour there is
left unchanged.

Cornelia Huck (2):
  virtio-ccw: common reset handler
  s390x/ccw: make sure all ccw devices are properly reset

 hw/s390x/ccw-device.c |  8 ++++++++
 hw/s390x/virtio-ccw.c | 20 ++++++--------------
 hw/s390x/virtio-ccw.h |  1 +
 3 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler

[Cornelia Huck]

All the different virtio ccw devices use the same reset handler,
so let's move setting it into the base virtio ccw device class.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/virtio-ccw.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e51fbefd23..40a33302a7 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1345,7 +1345,6 @@ static void virtio_ccw_net_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_net_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_net_properties;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
 }
@@ -1373,7 +1372,6 @@ static void virtio_ccw_blk_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_blk_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_blk_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
@@ -1401,7 +1399,6 @@ static void virtio_ccw_serial_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_serial_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_serial_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
@@ -1429,7 +1426,6 @@ static void virtio_ccw_balloon_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_balloon_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_balloon_properties;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
@@ -1457,7 +1453,6 @@ static void virtio_ccw_scsi_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_scsi_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_scsi_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
@@ -1484,7 +1479,6 @@ static void vhost_ccw_scsi_class_init(ObjectClass *klass, void *data)
 
     k->realize = vhost_ccw_scsi_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = vhost_ccw_scsi_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
@@ -1521,7 +1515,6 @@ static void virtio_ccw_rng_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_rng_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_rng_properties;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
@@ -1559,7 +1552,6 @@ static void virtio_ccw_crypto_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_crypto_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_crypto_properties;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
@@ -1597,7 +1589,6 @@ static void virtio_ccw_gpu_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_gpu_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_gpu_properties;
     dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
@@ -1626,7 +1617,6 @@ static void virtio_ccw_input_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_ccw_input_realize;
     k->unrealize = virtio_ccw_unrealize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_input_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
@@ -1730,6 +1720,7 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
     dc->realize = virtio_ccw_busdev_realize;
     dc->unrealize = virtio_ccw_busdev_unrealize;
     dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
+    dc->reset = virtio_ccw_reset;
 }
 
 static const TypeInfo virtio_ccw_device_info = {
@@ -1806,7 +1797,6 @@ static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data)
 
     k->unrealize = virtio_ccw_unrealize;
     k->realize = virtio_ccw_9p_realize;
-    dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_9p_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
@@ -1856,7 +1846,6 @@ static void vhost_vsock_ccw_class_init(ObjectClass *klass, void *data)
     k->unrealize = virtio_ccw_unrealize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->props = vhost_vsock_ccw_properties;
-    dc->reset = virtio_ccw_reset;
 }
 
 static void vhost_vsock_ccw_instance_init(Object *obj)
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset

[Cornelia Huck]

Thomas reported that the subchannel for a  3270 device that ended up
in a broken state (status pending even though not enabled) did not
get out of that state even after a reboot (which involves a subsytem
reset). The reason for this is that the 3270 device did not define
a reset handler.

Let's fix this by introducing a base reset handler (set up for all
ccw devices) that resets the subchannel and have virtio-ccw call
its virtio-specific reset procedure in addition to that.

Reported-by: Thomas Huth <thuth@redhat.com>
Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/ccw-device.c | 8 ++++++++
 hw/s390x/virtio-ccw.c | 9 ++++++---
 hw/s390x/virtio-ccw.h | 1 +
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index f9bfa154d6..7cd73df4aa 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -40,6 +40,13 @@ static Property ccw_device_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void ccw_device_reset(DeviceState *d)
+{
+    CcwDevice *ccw_dev = CCW_DEVICE(d);
+
+    css_reset_sch(ccw_dev->sch);
+}
+
 static void ccw_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -48,6 +55,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
     k->realize = ccw_device_realize;
     k->refill_ids = ccw_device_refill_ids;
     dc->props = ccw_device_properties;
+    dc->reset = ccw_device_reset;
 }
 
 const VMStateDescription vmstate_ccw_dev = {
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 40a33302a7..22df33b509 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1058,10 +1058,12 @@ static void virtio_ccw_reset(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
     VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
-    CcwDevice *ccw_dev = CCW_DEVICE(d);
+    VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
 
     virtio_ccw_reset_virtio(dev, vdev);
-    css_reset_sch(ccw_dev->sch);
+    if (vdc->parent_reset) {
+        vdc->parent_reset(d);
+    }
 }
 
 static void virtio_ccw_vmstate_change(DeviceState *d, bool running)
@@ -1715,12 +1717,13 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     CCWDeviceClass *k = CCW_DEVICE_CLASS(dc);
+    VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_CLASS(klass);
 
     k->unplug = virtio_ccw_busdev_unplug;
     dc->realize = virtio_ccw_busdev_realize;
     dc->unrealize = virtio_ccw_busdev_unrealize;
     dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
-    dc->reset = virtio_ccw_reset;
+    device_class_set_parent_reset(dc, virtio_ccw_reset, &vdc->parent_reset);
 }
 
 static const TypeInfo virtio_ccw_device_info = {
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 2fc513001e..3453aa1f98 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -77,6 +77,7 @@ typedef struct VirtIOCCWDeviceClass {
     CCWDeviceClass parent_class;
     void (*realize)(VirtioCcwDevice *dev, Error **errp);
     void (*unrealize)(VirtioCcwDevice *dev, Error **errp);
+    void (*parent_reset)(DeviceState *dev);
 } VirtIOCCWDeviceClass;
 
 /* Performance improves when virtqueue kick processing is decoupled from the
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler

[Thomas Huth]

On 07.05.2018 17:51, Cornelia Huck wrote:
> All the different virtio ccw devices use the same reset handler,
> so let's move setting it into the base virtio ccw device class.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler

[David Hildenbrand]

On 07.05.2018 17:51, Cornelia Huck wrote:
> All the different virtio ccw devices use the same reset handler,
> so let's move setting it into the base virtio ccw device class.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index e51fbefd23..40a33302a7 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1345,7 +1345,6 @@ static void virtio_ccw_net_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_net_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_net_properties;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>  }
> @@ -1373,7 +1372,6 @@ static void virtio_ccw_blk_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_blk_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_blk_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
> @@ -1401,7 +1399,6 @@ static void virtio_ccw_serial_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_serial_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_serial_properties;
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
> @@ -1429,7 +1426,6 @@ static void virtio_ccw_balloon_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_balloon_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_balloon_properties;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
> @@ -1457,7 +1453,6 @@ static void virtio_ccw_scsi_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_scsi_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_scsi_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
> @@ -1484,7 +1479,6 @@ static void vhost_ccw_scsi_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = vhost_ccw_scsi_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = vhost_ccw_scsi_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
> @@ -1521,7 +1515,6 @@ static void virtio_ccw_rng_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_rng_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_rng_properties;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
> @@ -1559,7 +1552,6 @@ static void virtio_ccw_crypto_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_crypto_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_crypto_properties;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
> @@ -1597,7 +1589,6 @@ static void virtio_ccw_gpu_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_gpu_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_gpu_properties;
>      dc->hotpluggable = false;
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> @@ -1626,7 +1617,6 @@ static void virtio_ccw_input_class_init(ObjectClass *klass, void *data)
>  
>      k->realize = virtio_ccw_input_realize;
>      k->unrealize = virtio_ccw_unrealize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_input_properties;
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
> @@ -1730,6 +1720,7 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
>      dc->realize = virtio_ccw_busdev_realize;
>      dc->unrealize = virtio_ccw_busdev_unrealize;
>      dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> +    dc->reset = virtio_ccw_reset;
>  }
>  
>  static const TypeInfo virtio_ccw_device_info = {
> @@ -1806,7 +1797,6 @@ static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data)
>  
>      k->unrealize = virtio_ccw_unrealize;
>      k->realize = virtio_ccw_9p_realize;
> -    dc->reset = virtio_ccw_reset;
>      dc->props = virtio_ccw_9p_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
> @@ -1856,7 +1846,6 @@ static void vhost_vsock_ccw_class_init(ObjectClass *klass, void *data)
>      k->unrealize = virtio_ccw_unrealize;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      dc->props = vhost_vsock_ccw_properties;
> -    dc->reset = virtio_ccw_reset;
>  }
>  
>  static void vhost_vsock_ccw_instance_init(Object *obj)
> 

Indeed, this makes sense and I also don't expect device specific proxy
device reset code

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset

[Thomas Huth]

On 07.05.2018 17:51, Cornelia Huck wrote:
> Thomas reported that the subchannel for a  3270 device that ended up
> in a broken state (status pending even though not enabled) did not
> get out of that state even after a reboot (which involves a subsytem
> reset). The reason for this is that the 3270 device did not define
> a reset handler.
> 
> Let's fix this by introducing a base reset handler (set up for all
> ccw devices) that resets the subchannel and have virtio-ccw call
> its virtio-specific reset procedure in addition to that.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/ccw-device.c | 8 ++++++++
>  hw/s390x/virtio-ccw.c | 9 ++++++---
>  hw/s390x/virtio-ccw.h | 1 +
>  3 files changed, 15 insertions(+), 3 deletions(-)

Looks good!

Reviewed-by: Thomas Huth <thuth@redhat.com>

I also checked that the broken 3270 device is now operational after a
reboot again with your two patches, so:

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset

[Christian Borntraeger]

ACK.

cc stable?



On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> Thomas reported that the subchannel for a  3270 device that ended up
> in a broken state (status pending even though not enabled) did not
> get out of that state even after a reboot (which involves a subsytem
> reset). The reason for this is that the 3270 device did not define
> a reset handler.
> 
> Let's fix this by introducing a base reset handler (set up for all
> ccw devices) that resets the subchannel and have virtio-ccw call
> its virtio-specific reset procedure in addition to that.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/ccw-device.c | 8 ++++++++
>  hw/s390x/virtio-ccw.c | 9 ++++++---
>  hw/s390x/virtio-ccw.h | 1 +
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index f9bfa154d6..7cd73df4aa 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -40,6 +40,13 @@ static Property ccw_device_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void ccw_device_reset(DeviceState *d)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(d);
> +
> +    css_reset_sch(ccw_dev->sch);
> +}
> +
>  static void ccw_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -48,6 +55,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>      k->realize = ccw_device_realize;
>      k->refill_ids = ccw_device_refill_ids;
>      dc->props = ccw_device_properties;
> +    dc->reset = ccw_device_reset;
>  }
>  
>  const VMStateDescription vmstate_ccw_dev = {
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 40a33302a7..22df33b509 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1058,10 +1058,12 @@ static void virtio_ccw_reset(DeviceState *d)
>  {
>      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>      VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> -    CcwDevice *ccw_dev = CCW_DEVICE(d);
> +    VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
>  
>      virtio_ccw_reset_virtio(dev, vdev);
> -    css_reset_sch(ccw_dev->sch);
> +    if (vdc->parent_reset) {
> +        vdc->parent_reset(d);
> +    }
>  }
>  
>  static void virtio_ccw_vmstate_change(DeviceState *d, bool running)
> @@ -1715,12 +1717,13 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      CCWDeviceClass *k = CCW_DEVICE_CLASS(dc);
> +    VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_CLASS(klass);
>  
>      k->unplug = virtio_ccw_busdev_unplug;
>      dc->realize = virtio_ccw_busdev_realize;
>      dc->unrealize = virtio_ccw_busdev_unrealize;
>      dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> -    dc->reset = virtio_ccw_reset;
> +    device_class_set_parent_reset(dc, virtio_ccw_reset, &vdc->parent_reset);
>  }
>  
>  static const TypeInfo virtio_ccw_device_info = {
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index 2fc513001e..3453aa1f98 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -77,6 +77,7 @@ typedef struct VirtIOCCWDeviceClass {
>      CCWDeviceClass parent_class;
>      void (*realize)(VirtioCcwDevice *dev, Error **errp);
>      void (*unrealize)(VirtioCcwDevice *dev, Error **errp);
> +    void (*parent_reset)(DeviceState *dev);
>  } VirtIOCCWDeviceClass;
>  
>  /* Performance improves when virtqueue kick processing is decoupled from the
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset

[Cornelia Huck]

On Tue, 8 May 2018 09:17:08 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> ACK.
> 
> cc stable?

Probably. We'd also need patch 1 for that.

> 
> 
> 
> On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> > Thomas reported that the subchannel for a  3270 device that ended up
> > in a broken state (status pending even though not enabled) did not
> > get out of that state even after a reboot (which involves a subsytem
> > reset). The reason for this is that the 3270 device did not define
> > a reset handler.
> > 
> > Let's fix this by introducing a base reset handler (set up for all
> > ccw devices) that resets the subchannel and have virtio-ccw call
> > its virtio-specific reset procedure in addition to that.
> > 
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/ccw-device.c | 8 ++++++++
> >  hw/s390x/virtio-ccw.c | 9 ++++++---
> >  hw/s390x/virtio-ccw.h | 1 +
> >  3 files changed, 15 insertions(+), 3 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Cornelia Huck]

On Mon,  7 May 2018 17:51:28 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Friday, Thomas noticed some problems with 3270 devices. One result
> was "s390x/css: disabled subchannels cannot be status pending", but
> a reboot did not cure the previous broken status. Turns out that 3270
> devices are missing a reset handler.
> 
> This series cleans up reset handling a bit and makes sure that the base
> ccw device class provides a subchannel reset handler. I'm currently
> not sure what we should do with vfio-ccw, so the behaviour there is
> left unchanged.
> 
> Cornelia Huck (2):
>   virtio-ccw: common reset handler
>   s390x/ccw: make sure all ccw devices are properly reset
> 
>  hw/s390x/ccw-device.c |  8 ++++++++
>  hw/s390x/virtio-ccw.c | 20 ++++++--------------
>  hw/s390x/virtio-ccw.h |  1 +
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 

Queued to s390-next (with cc:stable added).

Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Halil Pasic]

On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> On Friday, Thomas noticed some problems with 3270 devices. One result
> was "s390x/css: disabled subchannels cannot be status pending", but
> a reboot did not cure the previous broken status. Turns out that 3270
> devices are missing a reset handler.
> 
> This series cleans up reset handling a bit and makes sure that the base
> ccw device class provides a subchannel reset handler. I'm currently
> not sure what we should do with vfio-ccw, so the behaviour there is
> left unchanged.

Had a look, and LGTM (will tag separately) modulo what follows here. I'm
a bit concerned about vfio-ccw not being made compliant to this new
he reset of CCWDeviceClass is taking care of resetting the subchannel data
structures. This feels like introducing a common abstraction
to me, but then some things bother me.

In particular the the pim, the lpm and the pam set in css_reset_sch
seems to suit only devices that use the virtual chp. That
is it ain't suits any CCWDevice instance.

Do you plan to tackle vfio-ccw reset?

> 
> Cornelia Huck (2):
>    virtio-ccw: common reset handler
>    s390x/ccw: make sure all ccw devices are properly reset
> 
>   hw/s390x/ccw-device.c |  8 ++++++++
>   hw/s390x/virtio-ccw.c | 20 ++++++--------------
>   hw/s390x/virtio-ccw.h |  1 +
>   3 files changed, 15 insertions(+), 14 deletions(-)
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/2] virtio-ccw: common reset handler

[Halil Pasic]

On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> All the different virtio ccw devices use the same reset handler,
> so let's move setting it into the base virtio ccw device class.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>


> ---
>   hw/s390x/virtio-ccw.c | 13 +------------

[..]

Re: [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset

[Halil Pasic]

On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> Thomas reported that the subchannel for a  3270 device that ended up
> in a broken state (status pending even though not enabled) did not
> get out of that state even after a reboot (which involves a subsytem
> reset). The reason for this is that the 3270 device did not define
> a reset handler.
> 
> Let's fix this by introducing a base reset handler (set up for all
> ccw devices) that resets the subchannel and have virtio-ccw call
> its virtio-specific reset procedure in addition to that.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

The 'all' from the subject is actually 'all except maybe vfio-ccw'
AFAIU.

[..]

Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Cornelia Huck]

On Tue, 8 May 2018 15:29:50 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> > On Friday, Thomas noticed some problems with 3270 devices. One result
> > was "s390x/css: disabled subchannels cannot be status pending", but
> > a reboot did not cure the previous broken status. Turns out that 3270
> > devices are missing a reset handler.
> > 
> > This series cleans up reset handling a bit and makes sure that the base
> > ccw device class provides a subchannel reset handler. I'm currently
> > not sure what we should do with vfio-ccw, so the behaviour there is
> > left unchanged.  
> 
> Had a look, and LGTM (will tag separately) modulo what follows here. I'm
> a bit concerned about vfio-ccw not being made compliant to this new
> he reset of CCWDeviceClass is taking care of resetting the subchannel data
> structures. This feels like introducing a common abstraction
> to me, but then some things bother me.

We are having a common abstraction that can be overwritten by any
specialized implementation - and this is what vfio-ccw is doing,
therefore nothing changes for it.

> In particular the the pim, the lpm and the pam set in css_reset_sch
> seems to suit only devices that use the virtual chp. That
> is it ain't suits any CCWDevice instance.

Yes, we need to revisit this code and split out what makes sense and
what doesn't. For now, we only have virtual devices and vfio-ccw, so
we're fine. It even might make sense to keep them separate in the
future, as having a virtual device and one only mirroring some state in
QEMU sound like they want to be handled differently.

> 
> Do you plan to tackle vfio-ccw reset?

It's on my to-do list (which is sadly quite crowded...)

> 
> > 
> > Cornelia Huck (2):
> >    virtio-ccw: common reset handler
> >    s390x/ccw: make sure all ccw devices are properly reset
> > 
> >   hw/s390x/ccw-device.c |  8 ++++++++
> >   hw/s390x/virtio-ccw.c | 20 ++++++--------------
> >   hw/s390x/virtio-ccw.h |  1 +
> >   3 files changed, 15 insertions(+), 14 deletions(-)
> >   
> 

Re: [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset

[Cornelia Huck]

On Tue, 8 May 2018 15:42:17 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> > Thomas reported that the subchannel for a  3270 device that ended up
> > in a broken state (status pending even though not enabled) did not
> > get out of that state even after a reboot (which involves a subsytem
> > reset). The reason for this is that the 3270 device did not define
> > a reset handler.
> > 
> > Let's fix this by introducing a base reset handler (set up for all
> > ccw devices) that resets the subchannel and have virtio-ccw call
> > its virtio-specific reset procedure in addition to that.
> > 
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
> 
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> 
> The 'all' from the subject is actually 'all except maybe vfio-ccw'
> AFAIU.

Yeah, but as it overwrites the default handler, it's on its own :)

Thanks for your review!

Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Halil Pasic]

On 05/08/2018 03:55 PM, Cornelia Huck wrote:
> On Tue, 8 May 2018 15:29:50 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On 05/07/2018 05:51 PM, Cornelia Huck wrote:
>>> On Friday, Thomas noticed some problems with 3270 devices. One result
>>> was "s390x/css: disabled subchannels cannot be status pending", but
>>> a reboot did not cure the previous broken status. Turns out that 3270
>>> devices are missing a reset handler.
>>>
>>> This series cleans up reset handling a bit and makes sure that the base
>>> ccw device class provides a subchannel reset handler. I'm currently
>>> not sure what we should do with vfio-ccw, so the behaviour there is
>>> left unchanged.
>>
>> Had a look, and LGTM (will tag separately) modulo what follows here. I'm
>> a bit concerned about vfio-ccw not being made compliant to this new
>> he reset of CCWDeviceClass is taking care of resetting the subchannel data
>> structures. This feels like introducing a common abstraction
>> to me, but then some things bother me.
> 
> We are having a common abstraction that can be overwritten by any
> specialized implementation - and this is what vfio-ccw is doing,
> therefore nothing changes for it.
> 

Sorry my OO background is making me overthink things. I was on
the separation of concerns line: base class takes care of its
own class invariants and the the derived of its own. Considering
what your statements below, I think we are in agreement.

>> In particular the the pim, the lpm and the pam set in css_reset_sch
>> seems to suit only devices that use the virtual chp. That
>> is it ain't suits any CCWDevice instance.
> 
> Yes, we need to revisit this code and split out what makes sense and
> what doesn't. For now, we only have virtual devices and vfio-ccw, so
> we're fine. It even might make sense to keep them separate in the
> future, as having a virtual device and one only mirroring some state in
> QEMU sound like they want to be handled differently.
> 

I agree. The last sentence probably means that resetting the in QEMU
state may not be sufficient.

Another to me somewhat strange thing I noticed is this disabled_cb
used by virtio. It's is I guess the way it it is (specified in
the OASIS spec and everything), but I don't really understand how
this aligns with what the PoP says about MSCH. I mean AFAIU
MSCH does not trigger any communication between the channel subsystem
and the CU and or the device. I read the PoP as nothing is supposed
to change expect the things specified where MSCH is described. I guess
it is just another strange thing we have to live with -- for historical
reasons.

Regards,
Halil

Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices

[Cornelia Huck]

On Tue, 8 May 2018 16:24:08 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/08/2018 03:55 PM, Cornelia Huck wrote:
> > On Tue, 8 May 2018 15:29:50 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:

> >> In particular the the pim, the lpm and the pam set in css_reset_sch
> >> seems to suit only devices that use the virtual chp. That
> >> is it ain't suits any CCWDevice instance.  
> > 
> > Yes, we need to revisit this code and split out what makes sense and
> > what doesn't. For now, we only have virtual devices and vfio-ccw, so
> > we're fine. It even might make sense to keep them separate in the
> > future, as having a virtual device and one only mirroring some state in
> > QEMU sound like they want to be handled differently.
> >   
> 
> I agree. The last sentence probably means that resetting the in QEMU
> state may not be sufficient.

We currently have vfio-ccw's reset handler calling into the kernel and
triggering an disable/enable. What's missing is resetting QEMU's
internal state (or rather, syncing it up with the hardware state).
Needs some thinking.

> Another to me somewhat strange thing I noticed is this disabled_cb
> used by virtio. It's is I guess the way it it is (specified in
> the OASIS spec and everything), but I don't really understand how
> this aligns with what the PoP says about MSCH. I mean AFAIU
> MSCH does not trigger any communication between the channel subsystem
> and the CU and or the device. I read the PoP as nothing is supposed
> to change expect the things specified where MSCH is described. I guess
> it is just another strange thing we have to live with -- for historical
> reasons.

OTOH, I'd expect to have to setup things again if I disabled and
afterwards enabled a subchannel again. We should be able to overwrite
any old state after doing that. This was the best way to get virtio to
start with a clean slate again -- we don't want virtio reset to clean
the revision.