[libvirt] [PATCH v5 10/13] qemu: Generate and use zPCI device in QEMU command line

Yi Min Zhao zyimin at linux.ibm.com
Mon Sep 17 05:51:33 UTC 2018



在 2018/9/11 下午10:31, Andrea Bolognani 写道:
> On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> [...]
>> +char *
>> +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
>> +{
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    virBufferAddLit(&buf, "zpci");
>> +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci.uid);
>> +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci.fid);
>> +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
>> +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci.uid);
> All of the above can be a single virBufferAsprintf() call.
Sure.
> [...]
>> +static int
>> +qemuAppendZPCIDevStr(virCommandPtr cmd,
>> +                     virDomainDeviceInfoPtr dev)
> I'd call this qemuCommandAddZPCIDevice() or something like that.
OK.
>
>> +{
>> +    char *devstr = NULL;
>> +
>> +    virCommandAddArg(cmd, "-device");
> Empty line here.
Yeah.
>
>> +    if (!(devstr = qemuBuildZPCIDevStr(dev)))
>> +        return -1;
> [...]
>> +static int
>> +qemuBuildExtensionCommandLine(virCommandPtr cmd,
>> +                              virDomainDeviceInfoPtr dev)
> Same comment about the naming as above.
OK.
>
>> +{
>> +    if (!virZPCIDeviceAddressIsEmpty(&dev->addr.pci.zpci))
>> +        return qemuAppendZPCIDevStr(cmd, dev);
>> +
>> +    return 0;
> I'd rather see this as
>
>    if (virZPCIDeviceAddressIsEmpty(&dev->addr.pci.zpci))
>        return 0;
>
>    return qemuAppendZPCIDevStr(cmd, dev);
>
> instead.
How about using switch? I think extension flag should be check and
then build command for each case although there's only zpci case.
>
> [...]
>> @@ -4327,6 +4390,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd,
>>           if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) {
>>               virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL);
>>           } else {
>> +            if (qemuBuildExtensionCommandLine(cmd, &sound->info) < 0)
>> +                return -1;
> Do the codecs (handled immediately below) require a zpci device
> as well? I figure they don't, but I also don't know much about
> sound devices :)
If its address is PCI, we should add a zpci for it. zPCI is not related 
to its functionality.
>
> [...]
>> @@ -9086,6 +9171,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>>   
>>       switch ((virDomainShmemModel)shmem->model) {
>>       case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
>> +        if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
>> +            return -1;
>> +
>>           devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps);
>>           break;
>>   
>> @@ -9104,6 +9192,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>>   
>>           ATTRIBUTE_FALLTHROUGH;
>>       case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
>> +        if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
>> +            return -1;
>> +
>>           devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps);
>>           break;
> This looks wrong: you should call qemuBuildExtensionCommandLine()
> later on, like
>
>    if (!devstr)
>        return -1;
>
>    if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
>      return -1;
>
>    virCommandAddArgList(cmd, "-device", devstr, NULL);
>
> instead.
>
Good catch. Thanks for your comments.

-- 
Yi Min




More information about the libvir-list mailing list