From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpaCK-0002gm-FJ for qemu-devel@nongnu.org; Thu, 20 Jun 2013 04:26:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpaCF-0007RK-A4 for qemu-devel@nongnu.org; Thu, 20 Jun 2013 04:26:28 -0400 Message-ID: <51C2BCAA.1070208@greensocs.com> Date: Thu, 20 Jun 2013 10:26:18 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <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> <20130613075957.GA10967@redhat.com> <51BAB489.7040702@greensocs.com> <20130618152108.GA18977@redhat.com> In-Reply-To: <20130618152108.GA18977@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: "Michael S. Tsirkin" 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 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 wrot= e: >>>>>> 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@greenso= cs.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 for= ward >>>>>>>>>>>> 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 ar= e 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 w= ill >>>>>>>>> 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 try= the fix >>>>>> till now :) >>>>>> >>>>>> It works now if I run QEMU with "-device virtio-scsi-pci" and do t= his 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 whil= e >>>>>> spapr-vscsi is VIO device so if the guest kernel does not have vir= tio-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 is= also >>>>> gross - user asked for device0, the .0 part is qemu internals expos= ed to >>>>> users. >>>>> How about teaching qdev that if there's a single bus under a device= , >>>>> naming the device itself should be identical? >>>> Yes why not seems a good idea, but you'll pass it through bus=3D opt= ion? >>>>> 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? Ok, will try to look at it next week. What about the stable release? Wouldn't be safe to take this patch for the stable? Fred > >>>>>>>>>>>> --- >>>>>>>>>>>> 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, De= viceState >>>>>>>>>>>> *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 f= orward >>>>>>>>>>>> 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