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