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.kon...@greensocs.com
> >>>>>>>>>>>wrote:
> >>>>>>>>>>>>From: KONRAD Frederic <fred.kon...@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-sta...@nongnu.org
> >>>>>>>>>>>>Reported-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> >>>>>>>>>>>>Reviewed-by: Andreas Färber <afaer...@suse.de>
> >>>>>>>>>>>>Signed-off-by: KONRAD Frederic <fred.kon...@greensocs.com>
> >>>>>>>>>>>Acked-by: Michael S. Tsirkin <m...@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 <a...@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
