Re: [Qemu-devel] [RESEND PATCH] virtio-scsi: forward scsibus for virtio-scsi-pci.

[Michael S. Tsirkin]

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 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 wrote:
> >>>>>>>>On 06/12/2013 07:03 PM, Michael S. Tsirkin wrote:
> >>>>>>>>>On Wed, Jun 12, 2013 at 08:15:17AM +0200, address@hidden
> >>>>>>>>>wrote:
> >>>>>>>>>>From: KONRAD Frederic <address@hidden>
> >>>>>>>>>>
> >>>>>>>>>>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.
> >>>>>>>>>>
> >>>>>>>>>>Cc: address@hidden
> >>>>>>>>>>Reported-by: Alexey Kardashevskiy <address@hidden>
> >>>>>>>>>>Reviewed-by: Andreas Färber <address@hidden>
> >>>>>>>>>>Signed-off-by: KONRAD Frederic <address@hidden>
> >>>>>>>>>Acked-by: Michael S. Tsirkin <address@hidden>
> >>>>>>>>>
> >>>>>>>>>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 
> >>>>>>>>talking
> >>>>>>>>about "drive_add"?) is ignored when "if=none".
> >>>>>>>>
> >>>>>>>Then documentation in hmp-commands.hx is wrong, isn't it?
> >>>>>>>Add that to the list.
> >>>>>>>
> >>>>>>>if=none 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=device0" and then do in
> >>>>>>console:
> >>>>>>drive_add auto file=virtimg/fc18guest,if=none,id=bar1
> >>>>>>device_add scsi-disk,bus=device0.0,drive=bar1
> >>>>>>
> >>>>>>Pretty hot plug :)
> >>>>>I thought you use drive_add 0 if=scsi?
> >>>>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 this in
> >>>>qemu console:
> >>>>drive_add 0 file=virtimg/fc18guest
> >>>>
> >>>>No extra parameters or anything, cool, thanks, and :)
> >>>>
> >>>>Tested-by: Alexey Kardashevskiy <address@hidden>
> >>>>
> >>>>
> >>>>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 
> >>>>different
> >>>>story.
> >>>Okay.  How about:
> >>>- document that pci_addr is optional in hmp
> >>>- if no pci_addr assume if=none
> >>>- add drive_add to qmp without the pci_addr and if options
> >>>
> >>>We are left with the bus=device0.0 syntax for device_add which is also
> >>>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?
> >>Yes why not seems a good idea, but you'll pass it through bus= option?
> >>>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 = (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?

> >
> >>>>>>>>>>---
> >>>>>>>>>>   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);
> >>>>>>>>>>       }
> >>>>>>>>>>         /*
> >>>>-- 
> >>>>Alexey