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

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/03/2012 10:57 AM, Alexander Graf wrote:
>
>
> On 03.02.2012, at 17:37, Anthony Liguori<anthony@codemonkey.ws>  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.

Right, you're just pointing out though that the current code is dumb.  I full 
heartedly agree with you :-)

If you added PCI to the build for s390, aliases wouldn't do what you expect anymore.

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


Okay, I'm going to go with this change as this is the only place that it seems 
to exist.

Regards,

Anthony Liguori

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