Patchwork [1/2] virtio-pci: reset all qbuses too when writing to the status field

login
register
mail settings
Submitter Paolo Bonzini
Date Dec. 12, 2012, 2:26 p.m.
Message ID <1355322396-32026-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/205541/
State New
Headers show

Comments

Paolo Bonzini - Dec. 12, 2012, 2:26 p.m.
virtio-pci devices do not perform a full reset when zero is written
to the status field.  While PCI-specific status is initialized, the
reset does not propagate down the qdev bus hierarchy.  Because of
this, a virtio reset does not cancel in-flight I/O for virtio-scsi
(where the cancellation is handled automatically by the SCSI
devices underneath virtio-scsi-pci).

The patch calls qdev_reset_all, which calls virtio_pci_reset,
instead of basically inlining the contents of the latter.

Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-pci.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)
Michael S. Tsirkin - Dec. 12, 2012, 2:44 p.m.
On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote:
> virtio-pci devices do not perform a full reset when zero is written
> to the status field.  While PCI-specific status is initialized, the
> reset does not propagate down the qdev bus hierarchy.  Because of
> this, a virtio reset does not cancel in-flight I/O for virtio-scsi
> (where the cancellation is handled automatically by the SCSI
> devices underneath virtio-scsi-pci).
> 
> The patch calls qdev_reset_all, which calls virtio_pci_reset,
> instead of basically inlining the contents of the latter.
> 
> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This is a device specific register, IMO it should reset
very specific things not what happens to be on the bus.
For example qdev resets the PCI header: will or
will not this reset it? It should not but no easy way to figure out.

Can't the required code just go into the virtio-scsi
reset callback?

> ---
>  hw/virtio-pci.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 71f4fb5..a1685f1 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>      case VIRTIO_PCI_QUEUE_PFN:
>          pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
>          if (pa == 0) {
> -            virtio_pci_stop_ioeventfd(proxy);
> -            virtio_reset(proxy->vdev);
> -            msix_unuse_all_vectors(&proxy->pci_dev);
> -        }
> -        else
> +            qdev_reset_all(&proxy->pci_dev.qdev);
> +        } else {
>              virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
> +        }
>          break;
>      case VIRTIO_PCI_QUEUE_SEL:
>          if (val < VIRTIO_PCI_QUEUE_MAX)
> @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          }
>          break;
>      case VIRTIO_PCI_STATUS:
> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> -            virtio_pci_stop_ioeventfd(proxy);
> -        }
> -
>          virtio_set_status(vdev, val & 0xFF);
>  
> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> -            virtio_pci_start_ioeventfd(proxy);
> -        }
> -
>          if (vdev->status == 0) {
> -            virtio_reset(proxy->vdev);
> -            msix_unuse_all_vectors(&proxy->pci_dev);
> +            qdev_reset_all(&proxy->pci_dev.qdev);
> +        } else {
> +            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +                virtio_pci_stop_ioeventfd(proxy);
> +            } else {
> +                virtio_pci_start_ioeventfd(proxy);
> +            }
>          }
>  
>          /* Linux before 2.6.34 sets the device as OK without enabling
> -- 
> 1.8.0.1
>
Paolo Bonzini - Dec. 12, 2012, 3:20 p.m.
Il 12/12/2012 15:44, Michael S. Tsirkin ha scritto:
> On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote:
>> virtio-pci devices do not perform a full reset when zero is written
>> to the status field.  While PCI-specific status is initialized, the
>> reset does not propagate down the qdev bus hierarchy.  Because of
>> this, a virtio reset does not cancel in-flight I/O for virtio-scsi
>> (where the cancellation is handled automatically by the SCSI
>> devices underneath virtio-scsi-pci).
>>
>> The patch calls qdev_reset_all, which calls virtio_pci_reset,
>> instead of basically inlining the contents of the latter.
>>
>> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This is a device specific register, IMO it should reset
> very specific things not what happens to be on the bus.
> For example qdev resets the PCI header: will or
> will not this reset it?

qdev does not reset the PCI header.  Only pci_device_reset (called when
resetting the whole bus and also by FLR) does.

> It should not but no easy way to figure out.

qdev_reset_all is not FLR.  If you don't like direct usage of
qdev_reset_all, let's add a pci_device_soft_reset function that just
calls qdev_reset_all.  This way it is more self-documenting.

Other virtio implementations may not have an equivalent of FLR on their
bus and do a hard-reset when 0 is written to the status field.  The
virtio spec is silent on this---they can do it if they want.

> Can't the required code just go into the virtio-scsi
> reset callback?

Of course yes, but it'd be different from all other SCSI adapters then.
 They don't expect that they need to do anything to reset devices on
their SCSI bus.

Paolo

>> ---
>>  hw/virtio-pci.c | 25 ++++++++++---------------
>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 71f4fb5..a1685f1 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>      case VIRTIO_PCI_QUEUE_PFN:
>>          pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
>>          if (pa == 0) {
>> -            virtio_pci_stop_ioeventfd(proxy);
>> -            virtio_reset(proxy->vdev);
>> -            msix_unuse_all_vectors(&proxy->pci_dev);
>> -        }
>> -        else
>> +            qdev_reset_all(&proxy->pci_dev.qdev);
>> +        } else {
>>              virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
>> +        }
>>          break;
>>      case VIRTIO_PCI_QUEUE_SEL:
>>          if (val < VIRTIO_PCI_QUEUE_MAX)
>> @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>          }
>>          break;
>>      case VIRTIO_PCI_STATUS:
>> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> -            virtio_pci_stop_ioeventfd(proxy);
>> -        }
>> -
>>          virtio_set_status(vdev, val & 0xFF);
>>  
>> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
>> -            virtio_pci_start_ioeventfd(proxy);
>> -        }
>> -
>>          if (vdev->status == 0) {
>> -            virtio_reset(proxy->vdev);
>> -            msix_unuse_all_vectors(&proxy->pci_dev);
>> +            qdev_reset_all(&proxy->pci_dev.qdev);
>> +        } else {
>> +            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> +                virtio_pci_stop_ioeventfd(proxy);
>> +            } else {
>> +                virtio_pci_start_ioeventfd(proxy);
>> +            }
>>          }
>>  
>>          /* Linux before 2.6.34 sets the device as OK without enabling
>> -- 
>> 1.8.0.1
>>
Michael S. Tsirkin - Dec. 12, 2012, 3:33 p.m.
On Wed, Dec 12, 2012 at 04:20:08PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 15:44, Michael S. Tsirkin ha scritto:
> > On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote:
> >> virtio-pci devices do not perform a full reset when zero is written
> >> to the status field.  While PCI-specific status is initialized, the
> >> reset does not propagate down the qdev bus hierarchy.  Because of
> >> this, a virtio reset does not cancel in-flight I/O for virtio-scsi
> >> (where the cancellation is handled automatically by the SCSI
> >> devices underneath virtio-scsi-pci).
> >>
> >> The patch calls qdev_reset_all, which calls virtio_pci_reset,
> >> instead of basically inlining the contents of the latter.
> >>
> >> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > This is a device specific register, IMO it should reset
> > very specific things not what happens to be on the bus.
> > For example qdev resets the PCI header: will or
> > will not this reset it?
> 
> qdev does not reset the PCI header.  Only pci_device_reset (called when
> resetting the whole bus and also by FLR) does.

Point is it's a simple register, the easier it is to understand
what happens on this write the better.

> > It should not but no easy way to figure out.
> 
> qdev_reset_all is not FLR.  If you don't like direct usage of
> qdev_reset_all, let's add a pci_device_soft_reset function that just
> calls qdev_reset_all.  This way it is more self-documenting.
> 
> Other virtio implementations may not have an equivalent of FLR on their
> bus and do a hard-reset when 0 is written to the status field.  The
> virtio spec is silent on this---they can do it if they want.

guests expect sane behaviour, losing bios-assigned registers
on a device specific write wouldn't be sane.


> > Can't the required code just go into the virtio-scsi
> > reset callback?
> 
> Of course yes, but it'd be different from all other SCSI adapters then.
> They don't expect that they need to do anything to reset devices on
> their SCSI bus.
> 
> Paolo

On virtio level, it's a device specific register, there's nothing
standard about it.
If you are talking about some scsi thing here it belongs in
virtio scsi, but apparently same applies to virtio-blk which
really has no block bus.

> >> ---
> >>  hw/virtio-pci.c | 25 ++++++++++---------------
> >>  1 file changed, 10 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >> index 71f4fb5..a1685f1 100644
> >> --- a/hw/virtio-pci.c
> >> +++ b/hw/virtio-pci.c
> >> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>      case VIRTIO_PCI_QUEUE_PFN:
> >>          pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
> >>          if (pa == 0) {
> >> -            virtio_pci_stop_ioeventfd(proxy);
> >> -            virtio_reset(proxy->vdev);
> >> -            msix_unuse_all_vectors(&proxy->pci_dev);
> >> -        }
> >> -        else
> >> +            qdev_reset_all(&proxy->pci_dev.qdev);
> >> +        } else {
> >>              virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
> >> +        }
> >>          break;
> >>      case VIRTIO_PCI_QUEUE_SEL:
> >>          if (val < VIRTIO_PCI_QUEUE_MAX)
> >> @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>          }
> >>          break;
> >>      case VIRTIO_PCI_STATUS:
> >> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> >> -            virtio_pci_stop_ioeventfd(proxy);
> >> -        }
> >> -
> >>          virtio_set_status(vdev, val & 0xFF);
> >>  
> >> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> >> -            virtio_pci_start_ioeventfd(proxy);
> >> -        }
> >> -
> >>          if (vdev->status == 0) {
> >> -            virtio_reset(proxy->vdev);
> >> -            msix_unuse_all_vectors(&proxy->pci_dev);
> >> +            qdev_reset_all(&proxy->pci_dev.qdev);
> >> +        } else {
> >> +            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> >> +                virtio_pci_stop_ioeventfd(proxy);
> >> +            } else {
> >> +                virtio_pci_start_ioeventfd(proxy);
> >> +            }
> >>          }
> >>  
> >>          /* Linux before 2.6.34 sets the device as OK without enabling
> >> -- 
> >> 1.8.0.1
> >>
Paolo Bonzini - Dec. 12, 2012, 4:37 p.m.
Il 12/12/2012 16:33, Michael S. Tsirkin ha scritto:
>>> This is a device specific register, IMO it should reset
>>> very specific things not what happens to be on the bus.
>>> For example qdev resets the PCI header: will or
>>> will not this reset it?
>>
>> qdev does not reset the PCI header.  Only pci_device_reset (called when
>> resetting the whole bus and also by FLR) does.
> 
> Point is it's a simple register, the easier it is to understand
> what happens on this write the better.

The easiest way to understand what happens is to have a definition in
the spec.  There is none, so to me the simplest definition of resetting
the device is "soft-reset the device", and in qdev a soft-reset is
qdev_reset_all (see below).

Whether it involves function pointers or not, I don't care.

>>> Can't the required code just go into the virtio-scsi
>>> reset callback?
>>
>> Of course yes, but it'd be different from all other SCSI adapters then.
>> They don't expect that they need to do anything to reset devices on
>> their SCSI bus.
> 
> On virtio level, it's a device specific register, there's nothing 
> standard about it. If you are talking about some scsi thing here it
> belongs in virtio scsi

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.  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. :)

In the virtio case you have (logically, not at the qdev level):

           virtio-pci
                |
           virtio-scsi
          /     |     \
        disk   disk   disk

Writing zero to the status register does a soft reset of the virtio-pci
device.  This includes a hard reset of the virtio-scsi device, and a
hard reset of the disks.  That can be easily modeled by
qdev_reset_all(pci_dev).

What we actually have at the qdev level is:

           virtio-pci  ---> virtio-scsi (via ->vdev pointer)
          /     |    \
       disk   disk   disk

The soft reset of the virtio-scsi device is done by virtio_reset instead
of walking the qdev bus, but even in this case the soft reset is
perfectly described by qdev_reset_all.

Hence, writing zero to the status register should use qdev_reset_all.

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

Paolo
Michael S. Tsirkin - Dec. 12, 2012, 5:05 p.m.
On Wed, Dec 12, 2012 at 05:37:15PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 16:33, Michael S. Tsirkin ha scritto:
> >>> This is a device specific register, IMO it should reset
> >>> very specific things not what happens to be on the bus.
> >>> For example qdev resets the PCI header: will or
> >>> will not this reset it?
> >>
> >> qdev does not reset the PCI header.  Only pci_device_reset (called when
> >> resetting the whole bus and also by FLR) does.
> > 
> > Point is it's a simple register, the easier it is to understand
> > what happens on this write the better.
> 
> The easiest way to understand what happens is to have a definition in
> the spec.  There is none,

I think we should improve the spec.

> so to me the simplest definition of resetting
> the device is "soft-reset the device", and in qdev a soft-reset is
> qdev_reset_all (see below).

That's kind of vague.

> Whether it involves function pointers or not, I don't care.
> 
> >>> Can't the required code just go into the virtio-scsi
> >>> reset callback?
> >>
> >> Of course yes, but it'd be different from all other SCSI adapters then.
> >> They don't expect that they need to do anything to reset devices on
> >> their SCSI bus.
> > 
> > On virtio level, it's a device specific register, there's nothing 
> > standard about it. If you are talking about some scsi thing here it
> > belongs in virtio scsi
> 
> 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.
As it is it, these look like internal interfaces
that one needs to poke at implementation to figure out.

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

> In the virtio case you have (logically, not at the qdev level):
> 
>            virtio-pci
>                 |
>            virtio-scsi
>           /     |     \
>         disk   disk   disk
> 
> Writing zero to the status register does a soft reset of the virtio-pci
> device.  This includes a hard reset of the virtio-scsi device, and a
> hard reset of the disks.  That can be easily modeled by
> qdev_reset_all(pci_dev).
> 
> What we actually have at the qdev level is:
> 
>            virtio-pci  ---> virtio-scsi (via ->vdev pointer)
>           /     |    \
>        disk   disk   disk
> 
> The soft reset of the virtio-scsi device is done by virtio_reset instead
> of walking the qdev bus, but even in this case the soft reset is
> perfectly described by qdev_reset_all.
> 
> Hence, writing zero to the status register should use qdev_reset_all.

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.

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

Fine bug call it from virtio_scsi_reset.

Patch

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71f4fb5..a1685f1 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -268,12 +268,10 @@  static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case VIRTIO_PCI_QUEUE_PFN:
         pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
         if (pa == 0) {
-            virtio_pci_stop_ioeventfd(proxy);
-            virtio_reset(proxy->vdev);
-            msix_unuse_all_vectors(&proxy->pci_dev);
-        }
-        else
+            qdev_reset_all(&proxy->pci_dev.qdev);
+        } else {
             virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
+        }
         break;
     case VIRTIO_PCI_QUEUE_SEL:
         if (val < VIRTIO_PCI_QUEUE_MAX)
@@ -285,19 +283,16 @@  static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         break;
     case VIRTIO_PCI_STATUS:
-        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
-            virtio_pci_stop_ioeventfd(proxy);
-        }
-
         virtio_set_status(vdev, val & 0xFF);
 
-        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
-            virtio_pci_start_ioeventfd(proxy);
-        }
-
         if (vdev->status == 0) {
-            virtio_reset(proxy->vdev);
-            msix_unuse_all_vectors(&proxy->pci_dev);
+            qdev_reset_all(&proxy->pci_dev.qdev);
+        } else {
+            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
+                virtio_pci_stop_ioeventfd(proxy);
+            } else {
+                virtio_pci_start_ioeventfd(proxy);
+            }
         }
 
         /* Linux before 2.6.34 sets the device as OK without enabling