Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/02/2012 01:07 PM, Alexander Graf wrote:
> On 02.02.2012, at 20:01, Anthony Liguori wrote:
>
> > On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
> > > On 02/02/2012 06:03 PM, Anthony Liguori wrote:
> > > > >
> > > >
> > > > Is this still needed with qom-upstream.14? I fixed a bug on .14 that
> > > > involved child properties that was making device-del sometimes fail.
> > >
> > > Not sure, I tried with .13 but, from the look of it, it should still be there.
> > > Regarding the .13->.14 diff:
> > >
> > > - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
> >
> > Ack.
> >
> > > - you need to check for the existence of the non-aliased name when accessing the
> > > alias table, because s390 does not have PCI.
> >
> > I don't think that's the right strategy as it means that s390 only works if we 
> > don't include the PCI objects in the build (regardless of whether it uses PCI). 
> >  This would be defeated if/when we move to having all device objects in a 
> > single shared library used by all of the qemu executables.
> >
> > I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it 
> > and I don't think there are tons of s390 users that will be affected.
>
> The reason for the aliases is to make -drive and -net work. If you have 
> alternatives to aliases there, I'm happy to go with them.

Um, but I see (in s390-virtio.c):


    for(i = 0; i < nb_nics; i++) {
        NICInfo *nd = &nd_table[i];
        DeviceState *dev;

        if (!nd->model) {
            nd->model = g_strdup("virtio");
        }

        if (strcmp(nd->model, "virtio")) {
            fprintf(stderr, "S390 only supports VirtIO nics\n");
            exit(1);
        }

        dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
        qdev_set_nic_properties(dev, nd);
        qdev_init_nofail(dev);
    }

    /* Create VirtIO disk drives */
    for(i = 0; i < MAX_BLK_DEVS; i++) {
        DriveInfo *dinfo;
        DeviceState *dev;

        dinfo = drive_get(IF_IDE, 0, i);
        if (!dinfo) {
            continue;
        }

        dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
        qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
        qdev_init_nofail(dev);
    }

So s390 totally ignores the -drive if parameter and will only accept virtio for 
-net.

From what I can tell, it's not an issue.  But if we need it, we can do:

diff --git a/arch_init.h b/arch_init.h
index 828256c..bfbd9e1 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -32,4 +32,9 @@ int tcg_available(void);
 int kvm_available(void);
 int xen_available(void);

+static inline int target_get_arch(void)
+{
+    return arch_type;
+}
+
 #endif
diff --git a/blockdev.c b/blockdev.c
index 7e4c548..caa9205 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     case IF_VIRTIO:
         /* add virtio block device */
         opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
-        qemu_opt_set(opts, "driver", "virtio-blk");
+        switch(target_get_arch()) {
+        case QEMU_ARCH_S390X:
+            qemu_opt_set(opts, "driver", "virtio-blk-s390");
+            break;
+        default:
+            qemu_opt_set(opts, "driver", "virtio-blk-pci");
+            break;
+        }
+
         qemu_opt_set(opts, "drive", dinfo->id);
         if (devaddr)
             qemu_opt_set(opts, "addr", devaddr);
diff --git a/blockdev.c b/blockdev.c
index 7e4c548..caa9205 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     case IF_VIRTIO:
         /* add virtio block device */
         opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
-        qemu_opt_set(opts, "driver", "virtio-blk");
+        switch(target_get_arch()) {
+        case QEMU_ARCH_S390X:
+            qemu_opt_set(opts, "driver", "virtio-blk-s390");
+            break;
+        default:
+            qemu_opt_set(opts, "driver", "virtio-blk-pci");
+            break;
+        }
+
         qemu_opt_set(opts, "drive", dinfo->id);
         if (devaddr)
             qemu_opt_set(opts, "addr", devaddr);


Can you confirm what we actually need here?

Regards,

Anthony Liguori

> Alex