Re: [PATCH for-9.2 00/10] s390: Convert virtio-ccw, cpu to three-phase reset, and followup cleanup

[Peter Maydell]

On Fri, 30 Aug 2024 at 12:56, Nico Boehr <nrb@linux.ibm.com> wrote:
>
> Quoting Peter Maydell (2024-08-30 12:01:47)
> > I ran overnight with none of the patchset applied, and it never
> > failed (as expected). Running with just the first virtio-ccw
> > patch does fall over fairly quickly. So something's up with
> > that patch, which is curious because that's the one I thought
> > was a straightforward conversion without any complications :-)
> >
> > I'll investigate further today, I have the beginnings of a
> > theory about what might be happening...
>
> Thanks for taking the time, Peter! Let me know when you have insights.

I think I've found the problem, I'm just testing it to see if it
does properly fix the intermittent. The issue is that before we
can convert a class to three-phase reset we need to have
already converted all its parent classes. TYPE_VIRTIO_CCW_DEVICE
is a subclass of TYPE_CCW_DEVICE, but I forgot to convert
TYPE_CCW_DEVICE to three-phase reset. The result is that the
reset in the parent class was not happening at all (presumably
leaving the TYPE_CCW_DEVICE in an invalid/unexpected state on
reboot). The fix is to add an extra patch at the start of the
series. Once I've tested this I'll send out a v2 of the series,
maybe also adding the cleanup RTH suggested in one of the later
patches. (Ironically, if we do that cleanup then the bug would
also go away, because old-style DeviceClass:reset methods will
get called in exactly the same way as if they were registered
as phases.hold methods, rather than being specially handled...)


commit ff9bc4c8910f636f20e9b6e91d63dd003408ce10
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Fri Aug 30 12:50:03 2024 +0100

    hw/s390/ccw-device: Convert to three-phase reset

    Convert the TYPE_CCW_DEVICE to three-phase reset. This is a
    device class which is subclassed, so it needs to be three-phase
    before we can convert the subclass.

    Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index a7d682e5af9..14c24e38904 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -44,9 +44,9 @@ static Property ccw_device_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };

-static void ccw_device_reset(DeviceState *d)
+static void ccw_device_reset_hold(Object *obj, ResetType type)
 {
-    CcwDevice *ccw_dev = CCW_DEVICE(d);
+    CcwDevice *ccw_dev = CCW_DEVICE(obj);

     css_reset_sch(ccw_dev->sch);
 }
@@ -55,11 +55,12 @@ static void ccw_device_class_init(ObjectClass
*klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     CCWDeviceClass *k = CCW_DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);

     k->realize = ccw_device_realize;
     k->refill_ids = ccw_device_refill_ids;
     device_class_set_props(dc, ccw_device_properties);
-    dc->reset = ccw_device_reset;
+    rc->phases.hold = ccw_device_reset_hold;
     dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
 }

-- PMM