s390x: reset handling for ccw devices

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 <[email protected]>

---
 hw/s390x/virtio-ccw.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

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 <[email protected]>

---
 hw/s390x/virtio-ccw.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

hw/s390x/virtio-ccw.c +1 -11

index e51fbefd23..40a33302a7 100644
@@ -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);
 }

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 <[email protected]>

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

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 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(-)

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 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(-)

hw/s390x/ccw-device.c +8 -0

index f9bfa154d6..7cd73df4aa 100644
@@ -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 = {

hw/s390x/virtio-ccw.c +6 -3

index 40a33302a7..22df33b509 100644
@@ -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 = {

hw/s390x/virtio-ccw.h +1 -0

index 2fc513001e..3453aa1f98 100644
@@ -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

ACK.

cc stable?

Quoted 18 lines

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 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(-)

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

Quoted 10 lines · 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.

Quoted 3 lines

Reported-by Thomas Huth <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

Reviewed-by Halil Pasic <[email protected]>

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

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(-)

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(-)

Quoted 9 lines · 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.

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?

Quoted 11 lines

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(-)

Quoted 11 lines · On Tue, 8 May 2018 15:29:50 +0200 Halil Pasic <[email protected]> wrote:

Quoted 9 lines · 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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines · Cornelia Huck

Quoted 3 lines · On Tue, 8 May 2018 15:29:50 +0200 Halil Pasic <[email protected]> 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.

Quoted 6 lines

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.