Re: [libvirt] [PATCH v5 10/13] qemu: Generate and use zPCI device in QEMU command line
在 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