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

[Frederic Konrad <fred.konrad@greensocs.com>]

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, fred.konrad@greensocs.com
>>>>>>>>> wrote:
>>>>>>>>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>>>>>>>>
>>>>>>>>>> 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: qemu-stable@nongnu.org
>>>>>>>>>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>>>>>>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>>>
>>>>>>>>> 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 <aik@ozlabs.ru>
>>>>
>>>>
>>>> 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
>
>>>>>>>>>> ---
>>>>>>>>>>    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