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

["Michael S. Tsirkin" <mst@redhat.com>]

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