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

[Paolo Bonzini]

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 <address@hidden>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
> 
> 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
>>