From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Un2RH-00018P-I4 for qemu-devel@nongnu.org; Thu, 13 Jun 2013 03:59:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Un2RE-0006oc-OX for qemu-devel@nongnu.org; Thu, 13 Jun 2013 03:59:23 -0400 Date: Thu, 13 Jun 2013 10:59:57 +0300 From: "Michael S. Tsirkin" Message-ID: <20130613075957.GA10967@redhat.com> References: <1371017717-5439-1-git-send-email-fred.konrad@greensocs.com> <20130612090324.GB8060@redhat.com> <51B839B0.8070104@ozlabs.ru> <20130612091623.GA8686@redhat.com> <51B859C5.3080707@ozlabs.ru> <51B96680.8050001@greensocs.com> <51B96AB1.9010900@ozlabs.ru> <20130613072328.GA10326@redhat.com> <51B97606.8060702@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <51B97606.8060702@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 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 wrote: > >>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 wro= te: > >>>>>>On 06/12/2013 07:03 PM, Michael S. Tsirkin wrote: > >>>>>>>On Wed, Jun 12, 2013 at 08:15:17AM +0200, fred.konrad@greensocs.= 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 forwar= d > >>>>>>>>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 are t= alking > >>>>>>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 the= n 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 try th= e 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 while > >>spapr-vscsi is VIO device so if the guest kernel does not have virtio= -scsi > >>support, I have to do what I described in the quote but this is a dif= ferent > >>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 is al= so > >gross - user asked for device0, the .0 part is qemu internals exposed = to > >users. > >How about teaching qdev that if there's a single bus under a device, > >naming the device itself should be identical? >=20 > Yes why not seems a good idea, but you'll pass it through bus=3D option= ? > >This will solve the problem neatly without virtio specific hacks, > >won't it? >=20 > 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. > > > >> > >>>> > >>>>>>>>--- > >>>>>>>> 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, Devic= eState > >>>>>>>>*adapter, > >>>>>>>> { > >>>>>>>> SCSIBus *scsibus; > >>>>>>>> SCSIDevice *scsidev; > >>>>>>>>+ VirtIOPCIProxy *virtio_proxy; > >>>>>>>> scsibus =3D (SCSIBus *) > >>>>>>>> object_dynamic_cast(OBJECT(QLIST_FIRST(&adapter->chi= ld_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 forw= ard > >>>>>>>>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