| Message ID | 50C8BF12.1070906@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 12, 2012 at 06:29:54PM +0100, Paolo Bonzini wrote: > > >> 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. I dislike this approach. A specific function call is easier to follow. > > 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 The virtio reset just resets virtio registers and stops DMA and interrupts. That is all. If it's not clear from spec we should make it clear. The fact that virtio scsi needs to do something special with qdev or whatever for this to happen is it's own business. E.g. -net does it differently it checks state before processing packets. Generic virtio core does not care.
Il 12/12/2012 22:32, Michael S. Tsirkin ha scritto: > > i.e. just implement a method on the bus to do the hard-reset, and let > > generic infrastructure call it. > > I dislike this approach. > A specific function call is easier to follow. "I dislike reading documentation, so I prefer to have ugly code than good documentation". The right way to fix it is to document the reset semantics, and use them. > > 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. > > > > The virtio reset just resets virtio registers and stops DMA > and interrupts. That is all. If it's not clear from spec > we should make it clear. > > The fact that virtio scsi needs to do something special > with qdev or whatever for this to happen is it's own business. Same for virtio-console. If you don't call qemu_chr_fe_close, the char device layer ends up calling chr_read in virtio-console.c which ultimately does DMA. It's how most qdev buses work. The bus callbacks includes a parent->child interface to submit request and a child->parent interface to DMA. If you do not reset the children, you do not reset DMA. Period. > E.g. -net does it differently it checks state > before processing packets. > Generic virtio core does not care. Generic qdev core cares. virtio is a qdev device and it should use services provided by the generic framework, as much as you hate it. Paolo
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;