[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writi
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field |
Date: |
Wed, 12 Dec 2012 19:05:22 +0200 |
On Wed, Dec 12, 2012 at 05:37:15PM +0100, Paolo Bonzini wrote:
> 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,
I think we should improve the spec.
> 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).
That's kind of vague.
> 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.
I might be convinced if it's renamed qdev_reset_soft,
qbus_reset_hard.
As it is it, these look like internal interfaces
that one needs to poke at implementation to figure out.
> 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. :)
When we have a similar issue in another device it
will be easier to see how to do it right.
At the moment let's fix it locally.
> 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.
OK the right thing would be to to qdev_reset_all
at the virtio scsi level. The modeling
is wrong though and the devices are not the
children of the right device.
Sticking the reset in virtio-pci just propagates this bug.
> > 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
Fine bug call it from virtio_scsi_reset.
- [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- [Qemu-devel] [PATCH 2/2] virtio-s390: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/12
- Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/13
Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/12
- Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/12
- Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/12
- Message not available
- Message not available
- Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/13
- Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/16
- Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Paolo Bonzini, 2012/12/16
- Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field, Michael S. Tsirkin, 2012/12/16