* [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices @ 2018-05-07 15:51 Cornelia Huck 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-07 15:51 UTC (permalink / raw) To: Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, David Hildenbrand, Halil Pasic, qemu-s390x, qemu-devel, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler 2018-05-07 15:51 [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck @ 2018-05-07 15:51 ` Cornelia Huck 2018-05-07 19:21 ` Thomas Huth ` (2 more replies) 2018-05-07 15:51 ` [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset Cornelia Huck ` (2 subsequent siblings) 3 siblings, 3 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-07 15:51 UTC (permalink / raw) To: Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, David Hildenbrand, Halil Pasic, qemu-s390x, qemu-devel, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck @ 2018-05-07 19:21 ` Thomas Huth 2018-05-07 19:44 ` David Hildenbrand 2018-05-08 13:31 ` [Qemu-devel] [qemu-s390x] " Halil Pasic 2 siblings, 0 replies; 16+ messages in thread From: Thomas Huth @ 2018-05-07 19:21 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger Cc: Richard Henderson, David Hildenbrand, Halil Pasic, qemu-s390x, qemu-devel 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck 2018-05-07 19:21 ` Thomas Huth @ 2018-05-07 19:44 ` David Hildenbrand 2018-05-08 13:31 ` [Qemu-devel] [qemu-s390x] " Halil Pasic 2 siblings, 0 replies; 16+ messages in thread From: David Hildenbrand @ 2018-05-07 19:44 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, Halil Pasic, qemu-s390x, qemu-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/2] virtio-ccw: common reset handler 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck 2018-05-07 19:21 ` Thomas Huth 2018-05-07 19:44 ` David Hildenbrand @ 2018-05-08 13:31 ` Halil Pasic 2 siblings, 0 replies; 16+ messages in thread From: Halil Pasic @ 2018-05-08 13:31 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger, Thomas Huth Cc: David Hildenbrand, qemu-devel, Alexander Graf, qemu-s390x, Richard Henderson 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 +------------ [..] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset 2018-05-07 15:51 [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck @ 2018-05-07 15:51 ` Cornelia Huck 2018-05-08 5:05 ` [Qemu-devel] [qemu-s390x] " Thomas Huth ` (2 more replies) 2018-05-08 12:55 ` [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck 2018-05-08 13:29 ` Halil Pasic 3 siblings, 3 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-07 15:51 UTC (permalink / raw) To: Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, David Hildenbrand, Halil Pasic, qemu-s390x, qemu-devel, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset 2018-05-07 15:51 ` [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset Cornelia Huck @ 2018-05-08 5:05 ` Thomas Huth 2018-05-08 7:17 ` Christian Borntraeger 2018-05-08 13:42 ` [Qemu-devel] " Halil Pasic 2 siblings, 0 replies; 16+ messages in thread From: Thomas Huth @ 2018-05-08 5:05 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger Cc: David Hildenbrand, qemu-devel, Alexander Graf, Halil Pasic, qemu-s390x, Richard Henderson 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset 2018-05-07 15:51 ` [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset Cornelia Huck 2018-05-08 5:05 ` [Qemu-devel] [qemu-s390x] " Thomas Huth @ 2018-05-08 7:17 ` Christian Borntraeger 2018-05-08 7:38 ` Cornelia Huck 2018-05-08 13:42 ` [Qemu-devel] " Halil Pasic 2 siblings, 1 reply; 16+ messages in thread From: Christian Borntraeger @ 2018-05-08 7:17 UTC (permalink / raw) To: Cornelia Huck, Thomas Huth Cc: David Hildenbrand, qemu-devel, Alexander Graf, Halil Pasic, qemu-s390x, Richard Henderson 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset 2018-05-08 7:17 ` Christian Borntraeger @ 2018-05-08 7:38 ` Cornelia Huck 0 siblings, 0 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-08 7:38 UTC (permalink / raw) To: Christian Borntraeger Cc: Thomas Huth, David Hildenbrand, qemu-devel, Alexander Graf, Halil Pasic, qemu-s390x, Richard Henderson 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(-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset 2018-05-07 15:51 ` [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset Cornelia Huck 2018-05-08 5:05 ` [Qemu-devel] [qemu-s390x] " Thomas Huth 2018-05-08 7:17 ` Christian Borntraeger @ 2018-05-08 13:42 ` Halil Pasic 2018-05-08 14:01 ` Cornelia Huck 2 siblings, 1 reply; 16+ messages in thread From: Halil Pasic @ 2018-05-08 13:42 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, David Hildenbrand, qemu-s390x, qemu-devel 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. [..] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset 2018-05-08 13:42 ` [Qemu-devel] " Halil Pasic @ 2018-05-08 14:01 ` Cornelia Huck 0 siblings, 0 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-08 14:01 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Thomas Huth, Alexander Graf, Richard Henderson, David Hildenbrand, qemu-s390x, qemu-devel 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! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices 2018-05-07 15:51 [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck 2018-05-07 15:51 ` [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset Cornelia Huck @ 2018-05-08 12:55 ` Cornelia Huck 2018-05-08 13:29 ` Halil Pasic 3 siblings, 0 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-08 12:55 UTC (permalink / raw) To: Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, David Hildenbrand, Halil Pasic, qemu-s390x, qemu-devel 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). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices 2018-05-07 15:51 [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck ` (2 preceding siblings ...) 2018-05-08 12:55 ` [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck @ 2018-05-08 13:29 ` Halil Pasic 2018-05-08 13:55 ` Cornelia Huck 3 siblings, 1 reply; 16+ messages in thread From: Halil Pasic @ 2018-05-08 13:29 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger, Thomas Huth Cc: Alexander Graf, Richard Henderson, David Hildenbrand, qemu-s390x, qemu-devel 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(-) > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices 2018-05-08 13:29 ` Halil Pasic @ 2018-05-08 13:55 ` Cornelia Huck 2018-05-08 14:24 ` Halil Pasic 0 siblings, 1 reply; 16+ messages in thread From: Cornelia Huck @ 2018-05-08 13:55 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Thomas Huth, Alexander Graf, Richard Henderson, David Hildenbrand, qemu-s390x, qemu-devel 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(-) > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices 2018-05-08 13:55 ` Cornelia Huck @ 2018-05-08 14:24 ` Halil Pasic 2018-05-08 15:22 ` Cornelia Huck 0 siblings, 1 reply; 16+ messages in thread From: Halil Pasic @ 2018-05-08 14:24 UTC (permalink / raw) To: Cornelia Huck Cc: Christian Borntraeger, Thomas Huth, Alexander Graf, Richard Henderson, David Hildenbrand, qemu-s390x, qemu-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices 2018-05-08 14:24 ` Halil Pasic @ 2018-05-08 15:22 ` Cornelia Huck 0 siblings, 0 replies; 16+ messages in thread From: Cornelia Huck @ 2018-05-08 15:22 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Thomas Huth, Alexander Graf, Richard Henderson, David Hildenbrand, qemu-s390x, qemu-devel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-05-08 15:22 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-07 15:51 [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck 2018-05-07 15:51 ` [Qemu-devel] [PATCH 1/2] virtio-ccw: common reset handler Cornelia Huck 2018-05-07 19:21 ` Thomas Huth 2018-05-07 19:44 ` David Hildenbrand 2018-05-08 13:31 ` [Qemu-devel] [qemu-s390x] " Halil Pasic 2018-05-07 15:51 ` [Qemu-devel] [PATCH 2/2] s390x/ccw: make sure all ccw devices are properly reset Cornelia Huck 2018-05-08 5:05 ` [Qemu-devel] [qemu-s390x] " Thomas Huth 2018-05-08 7:17 ` Christian Borntraeger 2018-05-08 7:38 ` Cornelia Huck 2018-05-08 13:42 ` [Qemu-devel] " Halil Pasic 2018-05-08 14:01 ` Cornelia Huck 2018-05-08 12:55 ` [Qemu-devel] [PATCH 0/2] s390x: reset handling for ccw devices Cornelia Huck 2018-05-08 13:29 ` Halil Pasic 2018-05-08 13:55 ` Cornelia Huck 2018-05-08 14:24 ` Halil Pasic 2018-05-08 15:22 ` Cornelia Huck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.