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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/12/2012 17:48, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 05:24:48PM +0100, Paolo Bonzini wrote:
>> virtio-pci devices do not perform a full reset when zero is written
>> to the status byte.  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).
>>
>> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I would still prefer this logic to reside in virtio.c instead of being
> duplicated in each bus.
> My idea was to simply call qdev_reset_all on the binding from virtio.c
> but other ideas wellcome.

I think you're confusing "in the common superclass of all virtio
transports" vs "in the common superclass of all virtio devices".
virtio.c only implements a common superclass of all virtio devices; in
fact, there is no common superclass of all virtio transports, and it is
not possible without multiple inheritance or stuff like traits (you're
already inheriting from PCIDevice for virtio-*-pci).

Such common superclass, if it existed, would abstract stuff like "write
zero to the status register" and would call qdev_reset_all.  But again,
we don't have this concept.

Paolo

> 
> 
>> ---
>>  hw/virtio-pci.c | 28 ++++++++++------------------
>>  1 file changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index a7c75fe..2cf5282 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_reset(proxy->vdev);
>> -            virtio_pci_stop_ioeventfd(proxy);
>> -            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,22 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>          }
>>          break;
>>      case VIRTIO_PCI_STATUS:
>> -        if (vdev->status == 0) {
>> -            virtio_reset(proxy->vdev);
>> -        }
>> -
>> -        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) {
>> -            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.2
>>