From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tke7i-00032U-BP for qemu-devel@nongnu.org; Mon, 17 Dec 2012 12:05:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tke7f-0001x4-6M for qemu-devel@nongnu.org; Mon, 17 Dec 2012 12:05:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60303) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tke7e-0001wn-VG for qemu-devel@nongnu.org; Mon, 17 Dec 2012 12:04:59 -0500 Date: Mon, 17 Dec 2012 19:08:08 +0200 From: "Michael S. Tsirkin" Message-ID: <20121217170808.GA29953@redhat.com> References: <1355761490-10073-1-git-send-email-pbonzini@redhat.com> <1355761490-10073-14-git-send-email-pbonzini@redhat.com> <20121217164838.GA29085@redhat.com> <50CF4E2E.90605@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50CF4E2E.90605@redhat.com> Subject: Re: [Qemu-devel] [PATCH 13/15] 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: aliguori@us.ibm.com, qemu-devel@nongnu.org On Mon, Dec 17, 2012 at 05:54:06PM +0100, Paolo Bonzini wrote: > 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 > >> Cc: Michael S. Tsirkin > >> Signed-off-by: Paolo Bonzini > > > > 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 There's some misunderstanding here. This is not about classes and stuff. Common code should be in a single place so everyone can reuse it through function calls, not duplicated in all transports. And yes we do have examples of this if you are asking for examples; examples are functions like virtio_reset, virtio_set_status, etc. > > > > > >> --- > >> 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 > >>