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

[Alexander Graf]

On 03.02.2012, at 17:37, Anthony Liguori <address@hidden> wrote:

> 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

Nope, since virtio drives aren't handled through the IF_ legacy stuff but 
through qden instantiation. We only fake virtio disks for -hda here (which 
should be replaced by a default_virtio option in the machine config).

> parameter and will only accept virtio for -net.

It only supports virtio at all, yes. No MMIO there ;).

> 
> 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;

We could also have a machine type field that could be PCI or S390, right? I 
somehow don't like globals.

And just because it's s390 doesn't tell us anything. Maybe someone clever will 
find a way to expose PCI in a later machine type and use that for all devices?

The same thing goes for arm and their mmio virtio too btw.

> +}
> +
> #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?

Every time you grep for virtio-xxx-pci in the code without an s390 branch it's 
wrong. Pretty simple, eh? :)

Alex

> 
> Regards,
> 
> Anthony Liguori
> 
>> 
>> 
>> Alex
>> 
>> 
>