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

[Paolo Bonzini]

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