On Tue, 2018-08-07 at 17:10 +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->zpci_uid);
> + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
> + virBufferAsprintf(&buf, ",target=%s", dev->alias);
> + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
Just making sure: is the uid a good choice for naming the zpci
device? I would perhaps expect the fid to be in there as well,
but since both have to be unique (right?) I guess it doesn't
really matter...
> +
> + if (virBufferCheckError(&buf) < 0) {
> + virBufferFreeAndReset(&buf);
> + return NULL;
> + }
> +
> + return virBufferContentAndReset(&buf);
> +}
> +
> +int
> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
Based on the name, this is a predicate: it should return a
boolean value rather than an int.
> +{
> + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> + info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> + return 1;
> + }
> +
> + if (info->addr.pci.zpci) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("This QEMU doesn't support zpci devices"));
> + return -1;
> + }
Not sure about this second check. I guess the logic is supposed to
be that, when passed a "proper" zPCI device, the function would
have returned with the first check, and if we find a zPCI address
after that it's an error. Makes sense, but it's kinda obfuscated
and I'm not sure the error message is accurate: can it really only
be a problem of the QEMU binary not supporting the zPCI feature,
or could we have gotten here because the user is trying to assing
zPCI addresses to devices that don't support them?
Either way, calling a predicate should never result in an error
being reported, so you need to move this check somewhere else.
[...]
> +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
> @@ -0,0 +1,18 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory>219100</memory>
> + <os>
> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
> + </os>
> + <devices>
> + <emulator>/usr/bin/qemu-system-s390x</emulator>
> + <hostdev mode='subsystem' type='pci'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
> + </source>
> + <address type='pci'/>
> + </hostdev>
> + </devices>
> +</domain>
This test case would have been a great addition to the previous
commit, where you actually introduced the ability to automatically
allocate zPCI addresses...
Another thing that I forgot to ask earlier and this is as good a
time as any: once zPCI support has been merged, will users have
to opt-in to it using
<address type='pci'/>
or will they get zPCI devices by default? And if so, will it still
be possible for them to get CCW devices instead if wanted?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
