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... Either uid or fid, the value must be unique. But uid is defined by user. Of course, we also give the permission to define fid. But uid is more appropriate. + + 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. 0 means it's not zpci. 1 means it's zpci. -1 means error.
+{ + 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? Yes, it's because user could define zpci address in xml but
在 2018/8/17 上午12:31, Andrea Bolognani 写道: there's no zpci support.
Either way, calling a predicate should never result in an error being reported, so you need to move this check somewhere else.
Acutally I have found out a proper place to add this check for a long time. So this is the best place to add, at least I think for now.
[...]
+++ 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?