From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpaMG-0000JW-I9 for qemu-devel@nongnu.org; Thu, 20 Jun 2013 04:36:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpaM9-0002SD-ME for qemu-devel@nongnu.org; Thu, 20 Jun 2013 04:36:44 -0400 Date: Thu, 20 Jun 2013 11:37:18 +0300 From: "Michael S. Tsirkin" Message-ID: <20130620083718.GB23909@redhat.com> References: <20130612091623.GA8686@redhat.com> <51B859C5.3080707@ozlabs.ru> <51B96680.8050001@greensocs.com> <51B96AB1.9010900@ozlabs.ru> <20130613072328.GA10326@redhat.com> <51B97606.8060702@greensocs.com> <20130613075957.GA10967@redhat.com> <51BAB489.7040702@greensocs.com> <20130618152108.GA18977@redhat.com> <51C2BCAA.1070208@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <51C2BCAA.1070208@greensocs.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RESEND PATCH] virtio-scsi: forward scsibus for virtio-scsi-pci. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Frederic Konrad Cc: kwolf@redhat.com, aliguori@us.ibm.com, Alexey Kardashevskiy , mark.burton@greensocs.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org, pbonzini@redhat.com On Thu, Jun 20, 2013 at 10:26:18AM +0200, Frederic Konrad wrote: > On 18/06/2013 17:21, Michael S. Tsirkin wrote: > >On Fri, Jun 14, 2013 at 08:13:29AM +0200, Frederic Konrad wrote: > >>On 13/06/2013 09:59, Michael S. Tsirkin wrote: > >>>On Thu, Jun 13, 2013 at 09:34:30AM +0200, Frederic Konrad wrote: > >>>>On 13/06/2013 09:23, Michael S. Tsirkin wrote: > >>>>>On Thu, Jun 13, 2013 at 04:46:09PM +1000, Alexey Kardashevskiy wro= te: > >>>>>>On 06/13/2013 04:28 PM, Frederic Konrad wrote: > >>>>>>>On 12/06/2013 13:21, Alexey Kardashevskiy wrote: > >>>>>>>>On 06/12/2013 07:16 PM, Michael S. Tsirkin wrote: > >>>>>>>>>On Wed, Jun 12, 2013 at 07:04:48PM +1000, Alexey Kardashevskiy= wrote: > >>>>>>>>>>On 06/12/2013 07:03 PM, Michael S. Tsirkin wrote: > >>>>>>>>>>>On Wed, Jun 12, 2013 at 08:15:17AM +0200, fred.konrad@greens= ocs.com > >>>>>>>>>>>wrote: > >>>>>>>>>>>>From: KONRAD Frederic > >>>>>>>>>>>> > >>>>>>>>>>>>This fix a bug with scsi hotplug on virtio-scsi-pci: > >>>>>>>>>>>> > >>>>>>>>>>>>As virtio-scsi-pci doesn't have any scsi bus, we need to fo= rward > >>>>>>>>>>>>scsi-hot-add > >>>>>>>>>>>>to the virtio-scsi-device plugged on the virtio-bus. > >>>>>>>>>>>> > >>>>>>>>>>>>Cc: qemu-stable@nongnu.org > >>>>>>>>>>>>Reported-by: Alexey Kardashevskiy > >>>>>>>>>>>>Reviewed-by: Andreas F=E4rber > >>>>>>>>>>>>Signed-off-by: KONRAD Frederic > >>>>>>>>>>>Acked-by: Michael S. Tsirkin > >>>>>>>>>>> > >>>>>>>>>>>Note: we don't seem to have any decent way to > >>>>>>>>>>>add disks to devices: no QMP interface, > >>>>>>>>>>>pci address is required instead of using an id ... > >>>>>>>>>>> > >>>>>>>>>>>Anyone can be bothered to fix this? > >>>>>>>>>>Actually PCI address is not always required, this field (we a= re talking > >>>>>>>>>>about "drive_add"?) is ignored when "if=3Dnone". > >>>>>>>>>> > >>>>>>>>>Then documentation in hmp-commands.hx is wrong, isn't it? > >>>>>>>>>Add that to the list. > >>>>>>>>> > >>>>>>>>>if=3Dnone can't be actually used to hot-add > >>>>>>>>>a disk to a device, can it? It creates a disc and assumes you = will > >>>>>>>>>use it by a device created later. > >>>>>>>>Yep. I run QEMU with -device "virtio-scsi-pci,id=3Ddevice0" and= then do in > >>>>>>>>console: > >>>>>>>>drive_add auto file=3Dvirtimg/fc18guest,if=3Dnone,id=3Dbar1 > >>>>>>>>device_add scsi-disk,bus=3Ddevice0.0,drive=3Dbar1 > >>>>>>>> > >>>>>>>>Pretty hot plug :) > >>>>>>>I thought you use drive_add 0 if=3Dscsi? > >>>>>>That's the other option, I posted a bug but I did not actually tr= y the fix > >>>>>>till now :) > >>>>>> > >>>>>>It works now if I run QEMU with "-device virtio-scsi-pci" and do = this in > >>>>>>qemu console: > >>>>>>drive_add 0 file=3Dvirtimg/fc18guest > >>>>>> > >>>>>>No extra parameters or anything, cool, thanks, and :) > >>>>>> > >>>>>>Tested-by: Alexey Kardashevskiy > >>>>>> > >>>>>> > >>>>>>The only problem with it that it still wants PCI SCSI adapter whi= le > >>>>>>spapr-vscsi is VIO device so if the guest kernel does not have vi= rtio-scsi > >>>>>>support, I have to do what I described in the quote but this is a= different > >>>>>>story. > >>>>>Okay. How about: > >>>>>- document that pci_addr is optional in hmp > >>>>>- if no pci_addr assume if=3Dnone > >>>>>- add drive_add to qmp without the pci_addr and if options > >>>>> > >>>>>We are left with the bus=3Ddevice0.0 syntax for device_add which i= s also > >>>>>gross - user asked for device0, the .0 part is qemu internals expo= sed to > >>>>>users. > >>>>>How about teaching qdev that if there's a single bus under a devic= e, > >>>>>naming the device itself should be identical? > >>>>Yes why not seems a good idea, but you'll pass it through bus=3D op= tion? > >>>>>This will solve the problem neatly without virtio specific hacks, > >>>>>won't it? > >>>>The issue here is command line back-compatibility for pci_addr, > >>>>which won't be solved with > >>>>the "single bus" idea? > >>>Why not? This code: > >>> scsibus =3D (SCSIBus *) > >>> object_dynamic_cast(OBJECT(QLIST_FIRST(&adapter->child_bus)= ), > >>> TYPE_SCSI_BUS); > >>>should be replaced with code from qdev that we'll write > >>>that goes down the chain as long as there's 1 device > >>>on each bus, looking for a device of the appropriate type. > >>Ok, understood what you mean :). > >> > >>Why not if everybody is happy with it. > >> > >>Fred > >Ok so - want to try implementing this? >=20 > Ok, will try to look at it next week. >=20 > What about the stable release? > Wouldn't be safe to take this patch for the stable? >=20 > Fred Yes. My ACK is for stable. > > > >>>>>>>>>>>>--- > >>>>>>>>>>>> hw/pci/pci-hotplug.c | 19 +++++++++++++++++-- > >>>>>>>>>>>> 1 file changed, 17 insertions(+), 2 deletions(-) > >>>>>>>>>>>> > >>>>>>>>>>>>diff --git a/hw/pci/pci-hotplug.c b/hw/pci/pci-hotplug.c > >>>>>>>>>>>>index 12287d1..c708752 100644 > >>>>>>>>>>>>--- a/hw/pci/pci-hotplug.c > >>>>>>>>>>>>+++ b/hw/pci/pci-hotplug.c > >>>>>>>>>>>>@@ -30,6 +30,8 @@ > >>>>>>>>>>>> #include "monitor/monitor.h" > >>>>>>>>>>>> #include "hw/scsi/scsi.h" > >>>>>>>>>>>> #include "hw/virtio/virtio-blk.h" > >>>>>>>>>>>>+#include "hw/virtio/virtio-scsi.h" > >>>>>>>>>>>>+#include "hw/virtio/virtio-pci.h" > >>>>>>>>>>>> #include "qemu/config-file.h" > >>>>>>>>>>>> #include "sysemu/blockdev.h" > >>>>>>>>>>>> #include "qapi/error.h" > >>>>>>>>>>>>@@ -79,13 +81,26 @@ static int scsi_hot_add(Monitor *mon, D= eviceState > >>>>>>>>>>>>*adapter, > >>>>>>>>>>>> { > >>>>>>>>>>>> SCSIBus *scsibus; > >>>>>>>>>>>> SCSIDevice *scsidev; > >>>>>>>>>>>>+ VirtIOPCIProxy *virtio_proxy; > >>>>>>>>>>>> scsibus =3D (SCSIBus *) > >>>>>>>>>>>> object_dynamic_cast(OBJECT(QLIST_FIRST(&adapter-= >child_bus)), > >>>>>>>>>>>> TYPE_SCSI_BUS); > >>>>>>>>>>>> if (!scsibus) { > >>>>>>>>>>>>- error_report("Device is not a SCSI adapter"); > >>>>>>>>>>>>- return -1; > >>>>>>>>>>>>+ /* > >>>>>>>>>>>>+ * Check if the adapter is a virtio-scsi-pci, and = forward > >>>>>>>>>>>>scsi_hot_add > >>>>>>>>>>>>+ * to the virtio-scsi-device. > >>>>>>>>>>>>+ */ > >>>>>>>>>>>>+ if (!object_dynamic_cast(OBJECT(adapter), > >>>>>>>>>>>>TYPE_VIRTIO_SCSI_PCI)) { > >>>>>>>>>>>>+ error_report("Device is not a SCSI adapter"); > >>>>>>>>>>>>+ return -1; > >>>>>>>>>>>>+ } > >>>>>>>>>>>>+ virtio_proxy =3D VIRTIO_PCI(adapter); > >>>>>>>>>>>>+ adapter =3D DEVICE(virtio_proxy->bus.vdev); > >>>>>>>>>>>>+ scsibus =3D (SCSIBus *) > >>>>>>>>>>>>+ > >>>>>>>>>>>>object_dynamic_cast(OBJECT(QLIST_FIRST(&adapter->child_bus)= ), > >>>>>>>>>>>>+ TYPE_SCSI_BUS); > >>>>>>>>>>>>+ assert(scsibus); > >>>>>>>>>>>> } > >>>>>>>>>>>> /* > >>>>>>--=20 > >>>>>>Alexey