[Qemu-devel] [PATCH 0/5] Add S390 hotplug add support

[Qemu-devel] [PATCH 0/5] Add S390 hotplug add support

[Alexander Graf]

Hey guys,

this patch set adds support for hotplug add on S390. Apparently it's the first
non-x86 platform receiving so much love in Qemu, so I've stumbled over some
very basic #if defined(TARGET_I386) cases that just shouldn't be there.

I'm open for suggestions, comments, improvements, etc. on this set. The guest
patch for this is on the virtualization ML.

Alex

Alexander Graf (5):
  [S390] Add hotplug support
  [S390] Increase amount of virtio pages
  Compile device-hotplug on all targets
  Add generic drive hotplugging
  Expose drive_add on all architectures

 Makefile.target      |    5 ++++-
 hw/device-hotplug.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
 hw/s390-virtio-bus.c |    9 +++++++++
 hw/s390-virtio-bus.h |    7 ++++++-
 qemu-monitor.hx      |    2 --
 5 files changed, 62 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH 1/5] [S390] Add hotplug support

[Alexander Graf]

I just submitted a few patches that enable the s390 virtio bus to receive
a hotplug add event. This patch implements the qemu side of it, so that new
hotplug events can be submitted to the guest.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/s390-virtio-bus.c |    9 +++++++++
 hw/s390-virtio-bus.h |    5 +++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index fe6884d..3ded3f9 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -76,6 +76,9 @@ VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
     bus->dev_offs = bus->dev_page;
     bus->next_ring = bus->dev_page + TARGET_PAGE_SIZE;
 
+    /* Enable hotplugging */
+    _bus->allow_hotplug = 1;
+
     /* Allocate RAM for VirtIO device pages (descriptors, queues, rings) */
     *ram_size += S390_DEVICE_PAGES * TARGET_PAGE_SIZE;
 
@@ -103,6 +106,11 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
     dev->host_features = vdev->get_features(vdev, dev->host_features);
     s390_virtio_device_sync(dev);
 
+    if (dev->qdev.hotplugged) {
+        kvm_s390_virtio_irq(s390_cpu_addr2state(0), VIRTIO_PARAM_DEV_ADD,
+                            dev->dev_offs);
+    }
+
     return 0;
 }
 
@@ -365,6 +373,7 @@ static void s390_virtio_bus_register_withprop(VirtIOS390DeviceInfo *info)
 {
     info->qdev.init = s390_virtio_busdev_init;
     info->qdev.bus_info = &s390_virtio_bus_info;
+    info->qdev.unplug = qdev_simple_unplug_cb;
 
     assert(info->qdev.size >= sizeof(VirtIOS390Device));
     qdev_register(&info->qdev);
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index 333fea8..7a2f8dc 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -32,6 +32,11 @@
 #define VIRTIO_RING_LEN			(TARGET_PAGE_SIZE * 3)
 #define S390_DEVICE_PAGES		256
 
+#define VIRTIO_PARAM_MASK               0xff
+#define VIRTIO_PARAM_VRING_INTERRUPT    0x0
+#define VIRTIO_PARAM_CONFIG_CHANGED     0x1
+#define VIRTIO_PARAM_DEV_ADD            0x2
+
 typedef struct VirtIOS390Device {
     DeviceState qdev;
     ram_addr_t dev_offs;
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/5] [S390] Increase amount of virtio pages

[Alexander Graf]

While booting a recent guest, I realized that we're know spawning 2x 64 vrings.
Each vring occupies 3 * PAGE_SIZE size and we only reserved 256 * PAGE_SIZE
bytes where some additional meta info also has to fit in.

This is obviously too little. So let's increase the amount of reserved pages
to a reasonable 1024.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/s390-virtio-bus.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index 7a2f8dc..c2f8dde 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -30,7 +30,7 @@
 #define VIRTIO_VQCONFIG_LEN		24
 
 #define VIRTIO_RING_LEN			(TARGET_PAGE_SIZE * 3)
-#define S390_DEVICE_PAGES		256
+#define S390_DEVICE_PAGES		1024
 
 #define VIRTIO_PARAM_MASK               0xff
 #define VIRTIO_PARAM_VRING_INTERRUPT    0x0
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/5] Compile device-hotplug on all targets

[Alexander Graf]

All guest targets could potentially implement hotplugging. With the next
patches in this set I will also reflect this in the monitor interface.

So let's always compile it in. It shouldn't hurt.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Makefile.target |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index c8281e9..8d9ff57 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -193,12 +193,15 @@ obj-y += e1000.o
 # Inter-VM PCI shared memory
 obj-$(CONFIG_KVM) += ivshmem.o
 
+# Generic hotplugging
+obj-y += device-hotplug.o
+
 # Hardware support
 obj-i386-y += vga.o
 obj-i386-y += mc146818rtc.o i8259.o pc.o
 obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
 obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
-obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
+obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += debugcon.o multiboot.o
 obj-i386-y += pc_piix.o
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

The monitor command for hotplugging is in i386 specific code. This is just
plain wrong, as S390 just learned how to do hotplugging too and needs to
get drives for that.

So let's add a generic copy to generic code that handles drive_add in a
way that doesn't have pci dependencies.

I'm not fully happy with the patch as is. IMHO there should only be a
single target agnostic drive_hot_add function available. How we could
potentially fit IF_SCSI in there I don't know though.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/device-hotplug.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index c1a9a56..f311c7f 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -25,6 +25,9 @@
 #include "hw.h"
 #include "boards.h"
 #include "net.h"
+#include "qemu-config.h"
+#include "sysemu.h"
+#include "monitor.h"
 
 DriveInfo *add_init_drive(const char *optstr)
 {
@@ -44,3 +47,43 @@ DriveInfo *add_init_drive(const char *optstr)
 
     return dinfo;
 }
+
+#if !defined(TARGET_I386)
+
+/*
+ * This version of drive_hot_add does not know anything about PCI specifics.
+ * It is used as fallback on architectures that don't implement pci-hotplug.
+ */
+void drive_hot_add(Monitor *mon, const QDict *qdict)
+{
+    int type;
+    DriveInfo *dinfo = NULL;
+    const char *opts = qdict_get_str(qdict, "opts");
+
+    dinfo = add_init_drive(opts);
+    if (!dinfo)
+        goto err;
+    if (dinfo->devaddr) {
+        monitor_printf(mon, "Parameter addr not supported\n");
+        goto err;
+    }
+    type = dinfo->type;
+
+    switch (type) {
+    case IF_NONE:
+        monitor_printf(mon, "OK\n");
+        break;
+    default:
+        monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
+        goto err;
+    }
+    return;
+
+err:
+    if (dinfo)
+        drive_uninit(dinfo);
+    return;
+}
+
+#endif /* !defined(TARGET_I386) */
+
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/5] Expose drive_add on all architectures

[Alexander Graf]

All architectures can now use drive_add on the monitor. This of course
does not mean that there is hotplug support for the specific platform,
so in order to actually make use of the new drives you still need to
have a hotplug capable device.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 qemu-monitor.hx |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 2af3de6..892fe9a 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1101,7 +1101,6 @@ Example:
 
 EQMP
 
-#if defined(TARGET_I386)
     {
         .name       = "drive_add",
         .args_type  = "pci_addr:s,opts:s",
@@ -1113,7 +1112,6 @@ EQMP
         .help       = "add drive to PCI storage controller",
         .mhandler.cmd = drive_hot_add,
     },
-#endif
 
 STEXI
 @item drive_add
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Anthony Liguori]

On 08/23/2010 05:02 PM, Alexander Graf wrote:
> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
>
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies.
>
> I'm not fully happy with the patch as is. IMHO there should only be a
> single target agnostic drive_hot_add function available. How we could
> potentially fit IF_SCSI in there I don't know though.
>
> Signed-off-by: Alexander Graf<agraf@suse.de>
>    

I think you really want device_add plus a blockdev_add.

Regards,

Anthony Liguori

> ---
>   hw/device-hotplug.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index c1a9a56..f311c7f 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -25,6 +25,9 @@
>   #include "hw.h"
>   #include "boards.h"
>   #include "net.h"
> +#include "qemu-config.h"
> +#include "sysemu.h"
> +#include "monitor.h"
>
>   DriveInfo *add_init_drive(const char *optstr)
>   {
> @@ -44,3 +47,43 @@ DriveInfo *add_init_drive(const char *optstr)
>
>       return dinfo;
>   }
> +
> +#if !defined(TARGET_I386)
> +
> +/*
> + * This version of drive_hot_add does not know anything about PCI specifics.
> + * It is used as fallback on architectures that don't implement pci-hotplug.
> + */
> +void drive_hot_add(Monitor *mon, const QDict *qdict)
> +{
> +    int type;
> +    DriveInfo *dinfo = NULL;
> +    const char *opts = qdict_get_str(qdict, "opts");
> +
> +    dinfo = add_init_drive(opts);
> +    if (!dinfo)
> +        goto err;
> +    if (dinfo->devaddr) {
> +        monitor_printf(mon, "Parameter addr not supported\n");
> +        goto err;
> +    }
> +    type = dinfo->type;
> +
> +    switch (type) {
> +    case IF_NONE:
> +        monitor_printf(mon, "OK\n");
> +        break;
> +    default:
> +        monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
> +        goto err;
> +    }
> +    return;
> +
> +err:
> +    if (dinfo)
> +        drive_uninit(dinfo);
> +    return;
> +}
> +
> +#endif /* !defined(TARGET_I386) */
> +
>    

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

On 24.08.2010, at 00:21, Anthony Liguori wrote:

> On 08/23/2010 05:02 PM, Alexander Graf wrote:
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>> 
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies.
>> 
>> I'm not fully happy with the patch as is. IMHO there should only be a
>> single target agnostic drive_hot_add function available. How we could
>> potentially fit IF_SCSI in there I don't know though.
>> 
>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>   
> 
> I think you really want device_add plus a blockdev_add.

Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.

What is blockdev_add supposed to be? drive_add without IF_SCSI?


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

On 24.08.2010, at 00:23, Alexander Graf wrote:

> 
> On 24.08.2010, at 00:21, Anthony Liguori wrote:
> 
>> On 08/23/2010 05:02 PM, Alexander Graf wrote:
>>> The monitor command for hotplugging is in i386 specific code. This is just
>>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>>> get drives for that.
>>> 
>>> So let's add a generic copy to generic code that handles drive_add in a
>>> way that doesn't have pci dependencies.
>>> 
>>> I'm not fully happy with the patch as is. IMHO there should only be a
>>> single target agnostic drive_hot_add function available. How we could
>>> potentially fit IF_SCSI in there I don't know though.
>>> 
>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>> 
>> 
>> I think you really want device_add plus a blockdev_add.
> 
> Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.
> 
> What is blockdev_add supposed to be? drive_add without IF_SCSI?

To be a bit more precise on how things work with this set:

(qemu) drive_add 0 id=my_disk,if=none,file=/dev/null
OK
(qemu) device_add virtio-blk-s390,drive=my_disk,id=new_disk

gives me a working new virtio disk in the VM that's mapped to /dev/null :).


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Anthony Liguori]

On 08/23/2010 05:45 PM, Alexander Graf wrote:
>> Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.
>>
>> What is blockdev_add supposed to be? drive_add without IF_SCSI?

drive_add without if= and without the PCI-isms.

>> To be a bit more precise on how things work with this set:
>>
>> (qemu) drive_add 0 id=my_disk,if=none,file=/dev/null
>> OK
>>      

In theory, something like:

(qemu) blockdev_add id=my_disk,file=/dev/null

Regards,

Anthony Liguori

>> (qemu) device_add virtio-blk-s390,drive=my_disk,id=new_disk
>>
>> gives me a working new virtio disk in the VM that's mapped to /dev/null :).
>>
>>
>> Alex
>>      

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

On 24.08.2010, at 00:50, Anthony Liguori wrote:

> On 08/23/2010 05:45 PM, Alexander Graf wrote:
>>> Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.
>>> 
>>> What is blockdev_add supposed to be? drive_add without IF_SCSI?
> 
> drive_add without if= and without the PCI-isms.
> 
>>> To be a bit more precise on how things work with this set:
>>> 
>>> (qemu) drive_add 0 id=my_disk,if=none,file=/dev/null
>>> OK
>>>     
> 
> In theory, something like:
> 
> (qemu) blockdev_add id=my_disk,file=/dev/null

So why add yet another command when we could use the existing drive_add? Wouldn't it make more sense to deprecate if=!none, move forward from there, make if=none the default and around 0.16 or so drop if= from drive_add?


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Daniel P. Berrange]

On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
> 
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies.
> 
> I'm not fully happy with the patch as is. IMHO there should only be a
> single target agnostic drive_hot_add function available. How we could
> potentially fit IF_SCSI in there I don't know though.

I'm not sure that this patch is actually neccessary. Via a undocumented,
sick, dirty hack, you can already use the current drive_add command
without a PCI address, for both virtio + scsi. In fact not using the
PCI address with drive_add is the preferred approach in the new qdev
world even on x86

The key is that you should use  if=none for all cases. Here are two
examples of how libvirt does it currently:

VirtIO:

  drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
  device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'

SCSI:

  drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
  device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1

The 'dummy' value there can be absolutely anything you want.
It is totaly ignored when QEMU sees if=none in 2nd arg.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

Daniel P. Berrange wrote:
> On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
>   
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>>
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies.
>>
>> I'm not fully happy with the patch as is. IMHO there should only be a
>> single target agnostic drive_hot_add function available. How we could
>> potentially fit IF_SCSI in there I don't know though.
>>     
>
> I'm not sure that this patch is actually neccessary. Via a undocumented,
> sick, dirty hack, you can already use the current drive_add command
> without a PCI address, for both virtio + scsi. In fact not using the
> PCI address with drive_add is the preferred approach in the new qdev
> world even on x86
>   

It is certainly necessary since the current code is in a big fat #if
defined(TARGET_I386) block :).

> The key is that you should use  if=none for all cases. Here are two
> examples of how libvirt does it currently:
>
> VirtIO:
>
>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>
> SCSI:
>
>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>
> The 'dummy' value there can be absolutely anything you want.
> It is totaly ignored when QEMU sees if=none in 2nd arg.
>   

I'd be all for removing the pci-hotplug.c version of drive_add then. But
I think the IF_SCSI option there is to append a drive to an existing
SCSI bus, no?


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Daniel P. Berrange]

On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
> >   
> >> The monitor command for hotplugging is in i386 specific code. This is just
> >> plain wrong, as S390 just learned how to do hotplugging too and needs to
> >> get drives for that.
> >>
> >> So let's add a generic copy to generic code that handles drive_add in a
> >> way that doesn't have pci dependencies.
> >>
> >> I'm not fully happy with the patch as is. IMHO there should only be a
> >> single target agnostic drive_hot_add function available. How we could
> >> potentially fit IF_SCSI in there I don't know though.
> >>     
> >
> > I'm not sure that this patch is actually neccessary. Via a undocumented,
> > sick, dirty hack, you can already use the current drive_add command
> > without a PCI address, for both virtio + scsi. In fact not using the
> > PCI address with drive_add is the preferred approach in the new qdev
> > world even on x86
> >   
> 
> It is certainly necessary since the current code is in a big fat #if
> defined(TARGET_I386) block :).

True, true, killing the #ifdef is needed :-)

> > The key is that you should use  if=none for all cases. Here are two
> > examples of how libvirt does it currently:
> >
> > VirtIO:
> >
> >   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
> >   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
> >
> > SCSI:
> >
> >   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
> >   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >
> > The 'dummy' value there can be absolutely anything you want.
> > It is totaly ignored when QEMU sees if=none in 2nd arg.
> >   
> 
> I'd be all for removing the pci-hotplug.c version of drive_add then. But
> I think the IF_SCSI option there is to append a drive to an existing
> SCSI bus, no?

Actually this SCSI example I give above is appending a drive to an existing
bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
approach can cope with all, modulo bugs that appear periodically with code
that mistakenly checks for a particular IF_XXX constant.

If you wanted to also create a new SCSI bus, before creating the drive on
it, you'd need to run three commands in total:

  device_add lsi,id=scsi0,bus=pci.0,addr=0x7
  drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
  device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

Daniel P. Berrange wrote:
> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
>>>   
>>>       
>>>> The monitor command for hotplugging is in i386 specific code. This is just
>>>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>>>> get drives for that.
>>>>
>>>> So let's add a generic copy to generic code that handles drive_add in a
>>>> way that doesn't have pci dependencies.
>>>>
>>>> I'm not fully happy with the patch as is. IMHO there should only be a
>>>> single target agnostic drive_hot_add function available. How we could
>>>> potentially fit IF_SCSI in there I don't know though.
>>>>     
>>>>         
>>> I'm not sure that this patch is actually neccessary. Via a undocumented,
>>> sick, dirty hack, you can already use the current drive_add command
>>> without a PCI address, for both virtio + scsi. In fact not using the
>>> PCI address with drive_add is the preferred approach in the new qdev
>>> world even on x86
>>>   
>>>       
>> It is certainly necessary since the current code is in a big fat #if
>> defined(TARGET_I386) block :).
>>     
>
> True, true, killing the #ifdef is needed :-)
>   

And moving it out of the pci specific file. Yeah :). Basically my
proposal was to take the patches as is and phase out the pci-hotplug.c
version.

>   
>>> The key is that you should use  if=none for all cases. Here are two
>>> examples of how libvirt does it currently:
>>>
>>> VirtIO:
>>>
>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>>>
>>> SCSI:
>>>
>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>
>>> The 'dummy' value there can be absolutely anything you want.
>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>>>   
>>>       
>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>> I think the IF_SCSI option there is to append a drive to an existing
>> SCSI bus, no?
>>     
>
> Actually this SCSI example I give above is appending a drive to an existing
> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
> approach can cope with all, modulo bugs that appear periodically with code
> that mistakenly checks for a particular IF_XXX constant.
>
> If you wanted to also create a new SCSI bus, before creating the drive on
> it, you'd need to run three commands in total:
>
>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>   

Nice - so we can just deprecate if=!none?


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Daniel P. Berrange]

On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
> >   
> >> Daniel P. Berrange wrote:
> >>     
> >>> On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
> >>>   
> >>>       
> >>>> The monitor command for hotplugging is in i386 specific code. This is just
> >>>> plain wrong, as S390 just learned how to do hotplugging too and needs to
> >>>> get drives for that.
> >>>>
> >>>> So let's add a generic copy to generic code that handles drive_add in a
> >>>> way that doesn't have pci dependencies.
> >>>>
> >>>> I'm not fully happy with the patch as is. IMHO there should only be a
> >>>> single target agnostic drive_hot_add function available. How we could
> >>>> potentially fit IF_SCSI in there I don't know though.
> >>>>     
> >>>>         
> >>> I'm not sure that this patch is actually neccessary. Via a undocumented,
> >>> sick, dirty hack, you can already use the current drive_add command
> >>> without a PCI address, for both virtio + scsi. In fact not using the
> >>> PCI address with drive_add is the preferred approach in the new qdev
> >>> world even on x86
> >>>   
> >>>       
> >> It is certainly necessary since the current code is in a big fat #if
> >> defined(TARGET_I386) block :).
> >>     
> >
> > True, true, killing the #ifdef is needed :-)
> >   
> 
> And moving it out of the pci specific file. Yeah :). Basically my
> proposal was to take the patches as is and phase out the pci-hotplug.c
> version.
> 
> >   
> >>> The key is that you should use  if=none for all cases. Here are two
> >>> examples of how libvirt does it currently:
> >>>
> >>> VirtIO:
> >>>
> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
> >>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
> >>>
> >>> SCSI:
> >>>
> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
> >>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >>>
> >>> The 'dummy' value there can be absolutely anything you want.
> >>> It is totaly ignored when QEMU sees if=none in 2nd arg.
> >>>   
> >>>       
> >> I'd be all for removing the pci-hotplug.c version of drive_add then. But
> >> I think the IF_SCSI option there is to append a drive to an existing
> >> SCSI bus, no?
> >>     
> >
> > Actually this SCSI example I give above is appending a drive to an existing
> > bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
> > remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
> > approach can cope with all, modulo bugs that appear periodically with code
> > that mistakenly checks for a particular IF_XXX constant.
> >
> > If you wanted to also create a new SCSI bus, before creating the drive on
> > it, you'd need to run three commands in total:
> >
> >   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
> >   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
> >   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >   
> 
> Nice - so we can just deprecate if=!none?

In theory yes, but its not nice to tell users to switch everything over to
use if=none, if we're going to deprecate that too in the next release when
blockdev appears. Might as well just deprecate entire of drive_add/-drive
at once.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

Daniel P. Berrange wrote:
> On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>>>       
>>>>> The key is that you should use  if=none for all cases. Here are two
>>>>> examples of how libvirt does it currently:
>>>>>
>>>>> VirtIO:
>>>>>
>>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>>>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>>>>>
>>>>> SCSI:
>>>>>
>>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>>>
>>>>> The 'dummy' value there can be absolutely anything you want.
>>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>>>>>   
>>>>>       
>>>>>           
>>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>>>> I think the IF_SCSI option there is to append a drive to an existing
>>>> SCSI bus, no?
>>>>     
>>>>         
>>> Actually this SCSI example I give above is appending a drive to an existing
>>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>>> approach can cope with all, modulo bugs that appear periodically with code
>>> that mistakenly checks for a particular IF_XXX constant.
>>>
>>> If you wanted to also create a new SCSI bus, before creating the drive on
>>> it, you'd need to run three commands in total:
>>>
>>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>   
>>>       
>> Nice - so we can just deprecate if=!none?
>>     
>
> In theory yes, but its not nice to tell users to switch everything over to
> use if=none, if we're going to deprecate that too in the next release when
> blockdev appears. Might as well just deprecate entire of drive_add/-drive
> at once.
>   

I guess I still fail to see the reason for blockdev when we force
drive_add to if=none...


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Daniel P. Berrange]

On Tue, Aug 24, 2010 at 03:46:14PM +0200, Alexander Graf wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
> >   
> >> Daniel P. Berrange wrote:
> >>     
> >>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
> >>>       
> >>>>> The key is that you should use  if=none for all cases. Here are two
> >>>>> examples of how libvirt does it currently:
> >>>>>
> >>>>> VirtIO:
> >>>>>
> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
> >>>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
> >>>>>
> >>>>> SCSI:
> >>>>>
> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
> >>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >>>>>
> >>>>> The 'dummy' value there can be absolutely anything you want.
> >>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
> >>>>>   
> >>>>>       
> >>>>>           
> >>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
> >>>> I think the IF_SCSI option there is to append a drive to an existing
> >>>> SCSI bus, no?
> >>>>     
> >>>>         
> >>> Actually this SCSI example I give above is appending a drive to an existing
> >>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
> >>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
> >>> approach can cope with all, modulo bugs that appear periodically with code
> >>> that mistakenly checks for a particular IF_XXX constant.
> >>>
> >>> If you wanted to also create a new SCSI bus, before creating the drive on
> >>> it, you'd need to run three commands in total:
> >>>
> >>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
> >>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >>>   
> >>>       
> >> Nice - so we can just deprecate if=!none?
> >>     
> >
> > In theory yes, but its not nice to tell users to switch everything over to
> > use if=none, if we're going to deprecate that too in the next release when
> > blockdev appears. Might as well just deprecate entire of drive_add/-drive
> > at once.
> >   
> 
> I guess I still fail to see the reason for blockdev when we force
> drive_add to if=none...

Markus can no doubt explain better than me, but off the top of my head
 
 - 'drive' has guest properties that should be against the device
   not the drive which is for host properties (eg serial=, if=)
 - 'file' is mangled to include protocol/format which means that 
   it can't be unambigously parsed. (eg filenames containing :)

Fixing those, particularly the latter, would breaks back-compat so we
really need a new arg with sensible definition. This is what blockdev
is intended todo (as well as a internal code cleanup)

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Anthony Liguori]

On 08/24/2010 08:44 AM, Daniel P. Berrange wrote:
>
>>> Actually this SCSI example I give above is appending a drive to an existing
>>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>>> approach can cope with all, modulo bugs that appear periodically with code
>>> that mistakenly checks for a particular IF_XXX constant.
>>>
>>> If you wanted to also create a new SCSI bus, before creating the drive on
>>> it, you'd need to run three commands in total:
>>>
>>>    device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>>>    drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>>>    device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>
>>>        
>> Nice - so we can just deprecate if=!none?
>>      
> In theory yes, but its not nice to tell users to switch everything over to
> use if=none, if we're going to deprecate that too in the next release when
> blockdev appears. Might as well just deprecate entire of drive_add/-drive
> at once.
>    

I think what Alex is really asking is can we have 'blockdev_add 
var0=val0,var1=val1[,...]' implemented as 'drive_add dummy 
if=none,var0=val0,var1=val1[,...]'.  I don't know the answer to why that 
isn't possible or desirable.

Regards,

Anthony Liguori

> Regards,
> Daniel
>    

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

On 24.08.2010, at 20:35, Anthony Liguori wrote:

> On 08/24/2010 08:44 AM, Daniel P. Berrange wrote:
>> 
>>>> Actually this SCSI example I give above is appending a drive to an existing
>>>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>>>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>>>> approach can cope with all, modulo bugs that appear periodically with code
>>>> that mistakenly checks for a particular IF_XXX constant.
>>>> 
>>>> If you wanted to also create a new SCSI bus, before creating the drive on
>>>> it, you'd need to run three commands in total:
>>>> 
>>>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>> 
>>>>       
>>> Nice - so we can just deprecate if=!none?
>>>     
>> In theory yes, but its not nice to tell users to switch everything over to
>> use if=none, if we're going to deprecate that too in the next release when
>> blockdev appears. Might as well just deprecate entire of drive_add/-drive
>> at once.
>>   
> 
> I think what Alex is really asking is can we have 'blockdev_add var0=val0,var1=val1[,...]' implemented as 'drive_add dummy if=none,var0=val0,var1=val1[,...]'.  I don't know the answer to why that isn't possible or desirable.

Uh, I was really asking for why we need something not drive_add and instead have yet another iteration of drive parameter. But oh well.

I guess we deferred a bit from the original thread. What's the preferred way to go here? Remove the IF_SCSI case completely or keep two copies of the drive_add function around?


Alex

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Aug 24, 2010 at 03:46:14PM +0200, Alexander Graf wrote:
>> Daniel P. Berrange wrote:
>> > On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
>> >   
>> >> Daniel P. Berrange wrote:
>> >>     
>> >>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>> >>>       
>> >>>>> The key is that you should use  if=none for all cases. Here are two
>> >>>>> examples of how libvirt does it currently:
>> >>>>>
>> >>>>> VirtIO:
>> >>>>>
>> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>> >>>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>> >>>>>
>> >>>>> SCSI:
>> >>>>>
>> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>> >>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>> >>>>>
>> >>>>> The 'dummy' value there can be absolutely anything you want.
>> >>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>> >>>>>   
>> >>>>>       
>> >>>>>           
>> >>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>> >>>> I think the IF_SCSI option there is to append a drive to an existing
>> >>>> SCSI bus, no?
>> >>>>     
>> >>>>         
>> >>> Actually this SCSI example I give above is appending a drive to an existing
>> >>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>> >>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>> >>> approach can cope with all, modulo bugs that appear periodically with code
>> >>> that mistakenly checks for a particular IF_XXX constant.
>> >>>
>> >>> If you wanted to also create a new SCSI bus, before creating the drive on
>> >>> it, you'd need to run three commands in total:
>> >>>
>> >>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>> >>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>> >>>   
>> >>>       
>> >> Nice - so we can just deprecate if=!none?
>> >>     
>> >
>> > In theory yes, but its not nice to tell users to switch everything over to
>> > use if=none, if we're going to deprecate that too in the next release when
>> > blockdev appears. Might as well just deprecate entire of drive_add/-drive
>> > at once.
>> >   
>> 
>> I guess I still fail to see the reason for blockdev when we force
>> drive_add to if=none...
>
> Markus can no doubt explain better than me, but off the top of my head
>  
>  - 'drive' has guest properties that should be against the device
>    not the drive which is for host properties (eg serial=, if=)
>  - 'file' is mangled to include protocol/format which means that 
>    it can't be unambigously parsed. (eg filenames containing :)
>
> Fixing those, particularly the latter, would breaks back-compat so we
> really need a new arg with sensible definition. This is what blockdev
> is intended todo (as well as a internal code cleanup)

Yes, -drive and drive_add are a tangled mess.  I decided to start with a
clean slate, because getting it right is difficult enough without the
historical baggage.

Perhaps the end result can be hammered into the drive_add mold somehow,
by adding a few new options and deprecating a few old ones.  Let's
decide when we get there.

The goal is clean separation of host and guest properties.  device_add
is for guest properties, blockdev_add is for host properties.  Just like
netdev_add, only for block instead of network.

We also want sufficient flexibility (or at least extensibility) to be
able to specify any useful host block driver configuration.  And all
that without the syntactic embarrassments that plague drive_add, such as
':' in filenames.

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
>
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies.
>
> I'm not fully happy with the patch as is. IMHO there should only be a
> single target agnostic drive_hot_add function available. How we could
> potentially fit IF_SCSI in there I don't know though.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Yes, we need a target-agnostic command to add host block devices.  All
we have now is x86's drive_add, which isn't target-agnostic, because its
normal function is to add both host and guest part, and the latter is
entangled with PCI.

Your patch creates a copy of x86's drive_add with the guest bits
stripped from the code, so it's usable for non-PCI targets.  Guest bits
remain in the syntax (first argument, useless baggage with your
command).  Ugly.

I'm working on a clean solution for this problem.  If you must have a
solution right away, and ugly is fine, then your patch should do for
now.

In my opinion, pci_add, pci_del and drive_add all need to go.  They're
insufficiently general, and they fail to separate host and guest parts.

Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Alexander Graf]

On 27.08.2010, at 11:53, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>> 
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies.
>> 
>> I'm not fully happy with the patch as is. IMHO there should only be a
>> single target agnostic drive_hot_add function available. How we could
>> potentially fit IF_SCSI in there I don't know though.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> Yes, we need a target-agnostic command to add host block devices.  All
> we have now is x86's drive_add, which isn't target-agnostic, because its
> normal function is to add both host and guest part, and the latter is
> entangled with PCI.
> 
> Your patch creates a copy of x86's drive_add with the guest bits
> stripped from the code, so it's usable for non-PCI targets.  Guest bits
> remain in the syntax (first argument, useless baggage with your
> command).  Ugly.
> 
> I'm working on a clean solution for this problem.  If you must have a
> solution right away, and ugly is fine, then your patch should do for
> now.

Yes. IMHO it'd be useful to just broaden the scope of drive_add for now and remove it later in favor of your improved blockdev_add framework. So I guess the code duplication doesn't really hurt either, since this will all go away anyways.

> In my opinion, pci_add, pci_del and drive_add all need to go.  They're
> insufficiently general, and they fail to separate host and guest parts.

Sure :). Please make sure to always keep non-PCI capably systems in mind on your redesign!


Alex