From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiphP-00074d-9J for qemu-devel@nongnu.org; Wed, 12 Dec 2012 12:02:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TiphH-0006kS-7e for qemu-devel@nongnu.org; Wed, 12 Dec 2012 12:02:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiphH-0006kM-04 for qemu-devel@nongnu.org; Wed, 12 Dec 2012 12:02:15 -0500 Date: Wed, 12 Dec 2012 19:05:22 +0200 From: "Michael S. Tsirkin" Message-ID: <20121212170522.GA18597@redhat.com> References: <1355322396-32026-1-git-send-email-pbonzini@redhat.com> <1355322396-32026-2-git-send-email-pbonzini@redhat.com> <20121212144448.GE15555@redhat.com> <50C8A0A8.3000303@redhat.com> <20121212153333.GC16750@redhat.com> <50C8B2BB.50606@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50C8B2BB.50606@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-pci: reset all qbuses too when writing to the status field List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, agraf@suse.de 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.