[Qemu-devel] [PATCH 0/2] virtio-scsi: abort in-flight I/O when the device is reset

[Qemu-devel] [PATCH 0/2] virtio-scsi: abort in-flight I/O when the device is reset

[Paolo Bonzini]

This fixes the virtio-scsi problem by forcing a reset of the SCSI
bus from virtio_reset.  The reset will happen twice on machine
initialization (and this is why I preferred the other approach,
using qdev_reset_all in virtio code), but that's harmless.

Paolo Bonzini (2):
  qdev: add qbus_reset_all
  virtio-scsi: abort in-flight I/O when the device is reset

 hw/qdev-core.h   | 12 ++++++++++++
 hw/qdev.c        |  7 ++++++-
 hw/virtio-scsi.c |  4 ++++
 3 files changed, 22 insertions(+), 1 deletion(-)

-- 
1.8.1

[Qemu-devel] [PATCH 1/2] qdev: add qbus_reset_all

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h | 12 ++++++++++++
 hw/qdev.c      |  7 ++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fdf14ec..853bd08 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -182,6 +182,18 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
 void qdev_reset_all(DeviceState *dev);
+
+/**
+ * @qbus_reset_all:
+ * @bus: Bus to be reset.
+ *
+ * Reset @bus and perform a bus-level ("hard") reset of all devices connected
+ * to it, including recursive processing of all buses below @bus itself.  A
+ * hard reset means that qbus_reset_all will reset all state of the device.
+ * For PCI devices, for example, this will include the base address registers
+ * or configuration space.
+ */
+void qbus_reset_all(BusState *bus);
 void qbus_reset_all_fn(void *opaque);
 
 void qbus_free(BusState *bus);
diff --git a/hw/qdev.c b/hw/qdev.c
index f2c2484..e2f957e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -228,10 +228,15 @@ void qdev_reset_all(DeviceState *dev)
     qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
 }
 
+void qbus_reset_all(BusState *bus)
+{
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+}
+
 void qbus_reset_all_fn(void *opaque)
 {
     BusState *bus = opaque;
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+    qbus_reset_all(bus);
 }
 
 /* can be used as ->unplug() callback for the simple cases */
-- 
1.8.1

Re: [Qemu-devel] [PATCH 1/2] qdev: add qbus_reset_all

[Andreas Färber]

Am 10.01.2013 15:49, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev-core.h | 12 ++++++++++++
>  hw/qdev.c      |  7 ++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index fdf14ec..853bd08 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -182,6 +182,18 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
>  int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
>                         qbus_walkerfn *busfn, void *opaque);
>  void qdev_reset_all(DeviceState *dev);
> +
> +/**
> + * @qbus_reset_all:

Just " * qbus_reset_all:" please.

> + * @bus: Bus to be reset.
> + *
> + * Reset @bus and perform a bus-level ("hard") reset of all devices connected
> + * to it, including recursive processing of all buses below @bus itself.  A
> + * hard reset means that qbus_reset_all will reset all state of the device.

qbus_reset_all()?

> + * For PCI devices, for example, this will include the base address registers

#PCIDevice?

> + * or configuration space.
> + */
> +void qbus_reset_all(BusState *bus);

Otherwise looks trivial and okay.

Andreas

>  void qbus_reset_all_fn(void *opaque);
>  
>  void qbus_free(BusState *bus);
> diff --git a/hw/qdev.c b/hw/qdev.c
> index f2c2484..e2f957e 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -228,10 +228,15 @@ void qdev_reset_all(DeviceState *dev)
>      qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
> +void qbus_reset_all(BusState *bus)
> +{
> +    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
> +}
> +
>  void qbus_reset_all_fn(void *opaque)
>  {
>      BusState *bus = opaque;
> -    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
> +    qbus_reset_all(bus);
>  }
>  
>  /* can be used as ->unplug() callback for the simple cases */

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/2] qdev: add qbus_reset_all

[Andreas Färber]

Am 10.01.2013 19:34, schrieb Andreas Färber:
> Am 10.01.2013 15:49, schrieb Paolo Bonzini:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/qdev-core.h | 12 ++++++++++++
>>  hw/qdev.c      |  7 ++++++-
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
>> index fdf14ec..853bd08 100644
>> --- a/hw/qdev-core.h
>> +++ b/hw/qdev-core.h
>> @@ -182,6 +182,18 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
>>  int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
>>                         qbus_walkerfn *busfn, void *opaque);
>>  void qdev_reset_all(DeviceState *dev);
>> +
>> +/**
>> + * @qbus_reset_all:
> 
> Just " * qbus_reset_all:" please.

Patch was applied nontheless, please fix in a follow-up.

Andreas

>> + * @bus: Bus to be reset.
>> + *
>> + * Reset @bus and perform a bus-level ("hard") reset of all devices connected
>> + * to it, including recursive processing of all buses below @bus itself.  A
>> + * hard reset means that qbus_reset_all will reset all state of the device.
> 
> qbus_reset_all()?
> 
>> + * For PCI devices, for example, this will include the base address registers
> 
> #PCIDevice?
> 
>> + * or configuration space.
>> + */
>> +void qbus_reset_all(BusState *bus);
> 
> Otherwise looks trivial and okay.
> 
> Andreas
> 
>>  void qbus_reset_all_fn(void *opaque);
>>  
>>  void qbus_free(BusState *bus);
[snip]

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 2/2] virtio-scsi: abort in-flight I/O when the device is reset

[Paolo Bonzini]

When the device is reset, the SCSI bus should also be reset so
that in-flight I/O is cancelled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-scsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
index bfe1860..0715865 100644
--- a/hw/virtio-scsi.c
+++ b/hw/virtio-scsi.c
@@ -565,6 +565,10 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
 {
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
 
+    s->resetting++;
+    qbus_reset_all(&s->bus.qbus);
+    s->resetting--;
+
     s->sense_size = VIRTIO_SCSI_SENSE_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_SIZE;
     s->events_dropped = false;
-- 
1.8.1

Re: [Qemu-devel] [PATCH 0/2] virtio-scsi: abort in-flight I/O when the device is reset

[Michael S. Tsirkin]

On Thu, Jan 10, 2013 at 03:49:06PM +0100, Paolo Bonzini wrote:
> This fixes the virtio-scsi problem by forcing a reset of the SCSI
> bus from virtio_reset.  The reset will happen twice on machine
> initialization (and this is why I preferred the other approach,
> using qdev_reset_all in virtio code), but that's harmless.

Now I finally see what you worry about. It's the double reset.
Yes a bit ugly but harmless.

> Paolo Bonzini (2):
>   qdev: add qbus_reset_all
>   virtio-scsi: abort in-flight I/O when the device is reset
> 
>  hw/qdev-core.h   | 12 ++++++++++++
>  hw/qdev.c        |  7 ++++++-
>  hw/virtio-scsi.c |  4 ++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> -- 
> 1.8.1

Re: [Qemu-devel] [PATCH 0/2] virtio-scsi: abort in-flight I/O when the device is reset

[Anthony Liguori]

Thanks, applied.

Regards,

Anthony Liguori