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

[Michael S. Tsirkin]

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?

This will solve the problem neatly without virtio specific hacks,
won't it?

> 
> 
> >>
> >>
> >>>
> >>>>>> ---
> >>>>>>   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