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 <bry...@daemoninthecloset.org>
> >> Cc: Michael S. Tsirkin <m...@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonz...@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
> >>
