Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field

[Paolo Bonzini <pbonzini@redhat.com>]

>> No, it's a generic thing.  Other virtio devices may have a bus, and they
>> should reset it just as well.  virtio-serial's guest_reset function
>> closes each port from its own reset callback manually (since commit
>> 4399722, virtio-serial-bus: Unset guest_connected at reset and driver
>> reset, 2012-04-24).  That shouldn't even exist.  The ports should close
>> themselves when a reset signal reaches them, and the propagation of the
>> reset should happen automatically just like IMO it should for virtio-scsi.
>>
>> Let's not hide the basic concepts behind "it's a SCSI thing".  The
>> concept here is that of a soft and hard reset, and these is the definition:
>>
>> - you soft-reset a device by writing to a register which is in the spec
>> of the device (e.g., write zero to the status register);
>>
>> - you hard-reset a device by writing to a register which is in the spec
>> of the bus (e.g., FLR);
>>
>> - hard reset is a superset of soft reset;
>>
>> - soft-resetting device X will also do a hard reset of all devices below X.
>>
>> For example, a soft-reset of a PCI-to-PCI bridge does a hard-reset of
>> all devices below (qdev_reset_all calls pcibus_reset calls
>> pci_device_reset).
>>
>> In QEMU, qdev_reset_all is a soft reset of a device.  qbus_reset_all_fn
>> is a hard reset of all devices below a particular device.
> 
> I might be convinced if it's renamed qdev_reset_soft,
> qbus_reset_hard.

Let's rename it.

> As it is it, these look like internal interfaces
> that one needs to poke at implementation to figure out.

Point taken.

>> There is no
>> generic qdev function for a hard reset of a particular device
>> (pci_device_reset is it for the PCI case, just to complete the
>> nomenclature with something you're familiar with).
>>
>> These are all generic concepts.  Nothing virtio-specific, nothing
>> SCSI-specific.  It's not open for questioning. :)
> 
> When we have a similar issue in another device it
> will be easier to see how to do it right.
> At the moment let's fix it locally.

We have a similar issue in virtio-serial.c, and with my patch you can
do this:

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 155da58..1564482 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -532,12 +532,13 @@ static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
     memcpy(&config, config_data, sizeof(config));
 }
 
-static void guest_reset(VirtIOSerial *vser)
+static int virtser_bus_reset(BusState *qbus)
 {
+    VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, qbus);
     VirtIOSerialPort *port;
     VirtIOSerialPortClass *vsc;
 
-    QTAILQ_FOREACH(port, &vser->ports, next) {
+    QTAILQ_FOREACH(port, &bus->vser->ports, next) {
         vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
         if (port->guest_connected) {
             port->guest_connected = false;
@@ -546,6 +547,7 @@ static void guest_reset(VirtIOSerial *vser)
                 vsc->guest_close(port);
         }
     }
+    return 0;
 }
 
 static void set_status(VirtIODevice *vdev, uint8_t status)
@@ -566,17 +568,6 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
          */
         port->guest_connected = true;
     }
-    if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
-        guest_reset(vser);
-    }
-}
-
-static void vser_reset(VirtIODevice *vdev)
-{
-    VirtIOSerial *vser;
-
-    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
-    guest_reset(vser);
 }
 
 static void virtio_serial_save(QEMUFile *f, void *opaque)
@@ -766,6 +757,7 @@ static void virtser_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
     k->print_dev = virtser_bus_dev_print;
+    k->reset = virtser_bus_reset;
 }
 
 static const TypeInfo virtser_bus_info = {
@@ -985,7 +977,6 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
     vser->vdev.get_config = get_config;
     vser->vdev.set_config = set_config;
     vser->vdev.set_status = set_status;
-    vser->vdev.reset = vser_reset;
 
     vser->qdev = dev;
 

i.e. just implement a method on the bus to do the hard-reset, and let
generic infrastructure call it.

> OK the right thing would be to to qdev_reset_all
> at the virtio scsi level. The modeling
> is wrong though and the devices are not the
> children of the right device.
> 
> Sticking the reset in virtio-pci just propagates this bug.

Sticking a bus reset in virtio_pci_reset would.  But that's not
what my patch does.

>>> but apparently same applies to virtio-blk which
>>> really has no block bus.
>>
>> virtio-blk has no bus, so it does the reset from its own virtio_reset
>> callback.
>>
>> virtio-scsi needs to ask the SCSIDevices to reset themselves.  Whatever
>> virtio-blk does in its reset callback, the SCSIDevices will do---only in
>> a "regular" qdev reset rather than a special-purpose reset callback.
>>
>> I can of course iterate on all the devices, but it is pointless when
>> there is already a perfectly good callback to do a soft reset, and it's
>> called qdev_reset_all.
> 
> Fine bug call it from virtio_scsi_reset.

No, because that would also reset the virtio-pci bits (ioeventfd etc.) which
virtio-scsi has no business with.  What I could do is to call qbus_reset_all,
but it makes no sense to me when there is a generic solution.

Paolo