| Message ID | 1371017717-5439-1-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
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? > --- > 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); > } > > /* > -- > 1.8.1.4
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". >> --- >> 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); >> } >> >> /* >> -- >> 1.8.1.4
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. > > >> --- > >> 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); > >> } > >> > >> /* > >> -- > >> 1.8.1.4 > > > -- > Alexey
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 :) > > >> >>>> --- >>>> 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); >>>> } >>>> >>>> /*
On Wed, Jun 12, 2013 at 09:21:41PM +1000, 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 see. So you pass the device id of a patent as a bus option? So the real problem is that there's no documentation and what is there in hmp-commands.hx, is wrong. > > > > > > >> > >>>> --- > >>>> 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
On 06/12/2013 09:52 PM, Michael S. Tsirkin wrote: > On Wed, Jun 12, 2013 at 09:21:41PM +1000, 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 see. So you pass the device id of a patent as a bus option? Yes. Furthermore I can add many drive+scsi-disk to the same virtio-scsi-pci device/bus this way. > So the real problem is that there's no documentation > and what is there in hmp-commands.hx, is wrong. Yes, confusing. > >> >>> >>> >>>> >>>>>> --- >>>>>> 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); >>>>>> } >>>>>> >>>>>> /*
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? > > >> >>>>> --- >>>>> 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); >>>>> } >>>>> >>>>> /* >
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. >> >> >>> >>>>>> --- >>>>>> 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); >>>>>> } >>>>>> /* >> >
On 13/06/2013 08:46, 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. Understood, thanks 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); >>>>>>> } >>>>>>> /* >
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? 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
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? > >> >>>> >>>>>>>> --- >>>>>>>> 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
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. > > > >> > >>>> > >>>>>>>>--- > >>>>>>>> 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
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
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, 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 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
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 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 > 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, 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
On Thu, Jun 20, 2013 at 10:26:18AM +0200, Frederic Konrad wrote: > 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 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 > >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 Yes. My ACK is for stable. > > > >>>>>>>>>>>>--- > >>>>>>>>>>>> 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
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); } /*