From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UmJ3r-0001Ai-6B for qemu-devel@nongnu.org; Tue, 11 Jun 2013 03:32:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UmJ3p-0001gI-PT for qemu-devel@nongnu.org; Tue, 11 Jun 2013 03:32:11 -0400 Message-ID: <51B6D275.8020602@greensocs.com> Date: Tue, 11 Jun 2013 09:32:05 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <1370857984-17584-1-git-send-email-fred.konrad@greensocs.com> <20130610170040.GA31383@redhat.com> <51B6C727.9050604@greensocs.com> <20130611072112.GD31474@redhat.com> In-Reply-To: <20130611072112.GD31474@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [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: aliguori@us.ibm.com, aik@ozlabs.ru, mark.burton@greensocs.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org, pbonzini@redhat.com, =?ISO-8859-1?Q?Andreas_F=E4rber?= , david@gibson.dropbear.id.au On 11/06/2013 09:21, Michael S. Tsirkin wrote: > On Tue, Jun 11, 2013 at 08:43:51AM +0200, Frederic Konrad wrote: >> On 10/06/2013 19:00, Michael S. Tsirkin wrote: >>> On Mon, Jun 10, 2013 at 11:53:04AM +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 forward scsi-hot-add >>>> to the virtio-scsi-device plugged on the virtio-bus. >>>> >>>> Reported-by: Alexey Kardashevskiy >>>> Signed-off-by: KONRAD Frederic >>>> --- >>>> 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, DeviceState *adapter, >>>> { >>>> SCSIBus *scsibus; >>>> SCSIDevice *scsidev; >>>> + VirtIOPCIProxy *virtio_proxy; >>>> scsibus = (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 = VIRTIO_PCI(adapter); >>>> + adapter = DEVICE(virtio_proxy->bus.vdev); >>>> + scsibus = (SCSIBus *) >>>> + object_dynamic_cast(OBJECT(QLIST_FIRST(&adapter->child_bus)), >>>> + TYPE_SCSI_BUS); >>>> + assert(scsibus); >>>> } >>>> /* >>> By the way I really wonder. pci-hotplug.c was supposed to >>> be legacy interface. >>> Is this the only way to add scsi disks? >>> And are other ways broken, too? >> Here you can add scsi disks to a given device. >> I think the other ways add scsi disks to a given bus? >> >> Do you see any other? >> >> Thanks, >> Fred > I don't know, that's why I'm asking. > > If it is, that's crazy. There's no reason for > it to be tied to pci at all. > > The commands in pci-hotplug.c are legacy - they don't support things > like multi root systems and there's no sane way to make them do it. > > Your patch probably makes sense for stable, but for 1.6, > let's fix this properly. Ok, what do you propose to fix that properly? If we want to keep the behaviour of drive_add command, I think we have two solutions: A/ Change the scsi_hot_add to check if it is a virtio-scsi-pci device and take virtio-scsi-device's scsi bus. B/ Make somehow virtio-scsi-pci having a scsi-bus (and the same in virtio-scsi-device). I chose A because it seems a lot easier. For the moment B seems not possible, but would it be a cleaner solution? What do you think? Fred > >>>> -- >>>> 1.8.1.4