[libvirt] [PATCH v5 00/13] PCI passthrough support on s390

Abstract ======== The PCI representation in QEMU has recently been extended for S390 allowing configuration of zPCI attributes like uid (user-defined identifier) and fid (PCI function identifier). The details can be found here: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html To support the new zPCI feature of the S390 platform, two new XML attributes, @uid and @fid, are introduced for device addresses of type 'pci', i.e.: <hostdev mode='subsystem' type='pci'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> </hostdev> uid and fid are optional attributes. If they are defined by the user, unique values within the guest domain must be used. If they are not specified and the architecture requires them, they are automatically generated with non-conflicting values. Current implementation is the most seamless one for the user as it unites the address specific data of a PCI device on one XML element. It could accommodate both specifying our special parameters (uid and fid) and re-using standard statements (domain, bus, slot and function) for PCI devices. User can still specify bus/slot/function for the virtualized PCI devices in the XML. Thus uid/fid act as an extension to the PCI address and are stored in a new structure 'virZPCIDeviceAddress' which is a member of common PCI Address structure. Additionally, two hashtables are used for assignment and reservation of uid/fid. In support of extending the PCI address, a new PCI address extension flag is introduced. This extension flag allows is not only dedicated for the S390 platform but also other architectures needing certain extensions to PCI address space. Code Base ========= commit in master: 2f6ff0da5b qemu: Don't overwrite stats in qemuDomainBlocksStatsGather Change Log ========== v4->v5: 1. Update the version number. 2. Fixup code style error. 3. Separate qemu code into single patch. 4. Rebase the patches to the new code of master branch. v3->v4: 1. Update docs. 2. Format code style. 3. Optimize zPCI support check. 4. Move the check of zPCI defined in xml but unsupported by Qemu to qemuDomainDeviceDefValidate(). 5. Change zpci address member of PCI address struct from pointer to instance. 6. Modify zpci address definition principle. Currently the user must either define both of uid and fid or not. v2->v3: 1. Revise code style. 2. Update test cases. 3. Introduce qemuDomainCollectPCIAddressExtension() to collect PCI extension addresses. 4. Introduce virDeviceInfoPCIAddressExtensionPresent() to check if zPCI address exists. 5. Optimize zPCI address check logic. 6. Optimize passed parameters of zPCI addr alloc/release/reserve functions. 7. Report enum range error in qemuDomainDeviceSupportZPCI(). 8. Update commit messages. v1->v2: 1. Separate test commit and merge testcases into corresponding commits that introduce the functionalities firstly. 2. Spare some checks for zpci device. 3. Add vsock and controller support. 4. Add uin32 type schema. 5. Rename zpciuid and zpcifid to zpci_uid and zpci_fid. 6. Always return multibus support on S390. Yi Min Zhao (13): conf: Add definitions for 'uid' and 'fid' PCI address attributes qemu: Introduce zPCI capability conf: Introduce a new PCI address extension flag qemu: Enable PCI multi bus for S390 guests qemu: Auto add pci-root for s390/s390x guests conf: Introduce address caching for PCI extensions conf: Introduce parser, formatter for uid and fid qemu: Add zPCI address definition check conf: Allocate/release 'uid' and 'fid' in PCI address qemu: Generate and use zPCI device in QEMU command line qemu: Add hotpluging support for PCI devices on S390 guests docs: Add 'uid' and 'fid' information news: Update news for PCI address extension attributes cfg.mk | 1 + docs/formatdomain.html.in | 9 +- docs/news.xml | 10 + docs/schemas/basictypes.rng | 23 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 76 +++++ src/conf/device_conf.h | 4 + src/conf/domain_addr.c | 330 +++++++++++++++++++++ src/conf/domain_addr.h | 29 ++ src/conf/domain_conf.c | 6 + src/libvirt_private.syms | 6 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 95 ++++++ src/qemu/qemu_command.h | 2 + src/qemu/qemu_domain.c | 27 ++ src/qemu/qemu_domain_address.c | 205 ++++++++++++- src/qemu/qemu_hotplug.c | 151 +++++++++- src/util/virpci.h | 11 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 26 ++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 ++ .../hostdev-vfio-zpci-autogenerate.args | 25 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 ++ .../hostdev-vfio-zpci-boundaries.args | 29 ++ .../hostdev-vfio-zpci-boundaries.xml | 26 ++ .../hostdev-vfio-zpci-multidomain-many.args | 39 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 +++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 25 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++ tests/qemuxml2argvtest.c | 20 ++ tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 ++ .../hostdev-vfio-zpci-autogenerate.xml | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 42 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++ tests/qemuxml2xmltest.c | 17 ++ 42 files changed, 1523 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml -- Yi Min

Add zPCI definitions in preparation of extending the PCI address with parameters uid (user-defined identifier) and fid (PCI function identifier). Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- cfg.mk | 1 + src/util/virpci.h | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/cfg.mk b/cfg.mk index 609ae869c2..1116feb299 100644 --- a/cfg.mk +++ b/cfg.mk @@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name: # Insist on correct types for [pug]id. sc_correct_id_types: @prohibit='\<(int|long) *[pug]id\>' \ + exclude='exempt from syntax-check' \ halt='use pid_t for pid, uid_t for uid, gid_t for gid' \ $(_sc_search_regexp) diff --git a/src/util/virpci.h b/src/util/virpci.h index 2ac87694df..b7bcfa6d9f 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -37,12 +37,20 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr; +typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; +struct _virZPCIDeviceAddress { + unsigned int uid; /* exempt from syntax-check */ + unsigned int fid; +}; + struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ + virZPCIDeviceAddress zpci; }; typedef enum { -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
Add zPCI definitions in preparation of extending the PCI address with parameters uid (user-defined identifier) and fid (PCI function identifier).
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
[...]
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; +struct _virZPCIDeviceAddress { + unsigned int uid; /* exempt from syntax-check */ + unsigned int fid; +}; + struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ + virZPCIDeviceAddress zpci; };
As mentioned during an earlier review, virPCIDeviceAddress should itself include a virDomainPCIAddressExtensionFlags so that it would be possible to know whether zpci should be taken into account without having to look at other structs. Perhaps virPCIDeviceAddressExtensionFlags would be a more appropriate name then? An aside. I see you've carried over Jano's R-b from v2; given that the patch has changed substantially since then, I don't think it's fair to assume he'd stand behind the current incarnation just as he did originally. IMHO you should just drop R-bs when posting a new version, unless the patch is unchanged or the changes made are trivial. -- Andrea Bolognani / Red Hat / Virtualization

Let's introduce zPCI capability. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + 8 files changed, 9 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a075677421..62d56db9de 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -508,6 +508,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 315 */ "vfio-pci.display", "blockdev", + "zpci", ); @@ -1148,6 +1149,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "vhost-vsock-device", QEMU_CAPS_DEVICE_VHOST_VSOCK }, { "mch", QEMU_CAPS_DEVICE_MCH }, { "sev-guest", QEMU_CAPS_SEV_GUEST }, + { "zpci", QEMU_CAPS_DEVICE_ZPCI }, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3d3a978759..83b21b3763 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -492,6 +492,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 315 */ QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */ QEMU_CAPS_BLOCKDEV, /* -blockdev and blockdev-add are supported */ + QEMU_CAPS_DEVICE_ZPCI, /* -device zpci */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml index e9ccc63402..efb8c56354 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -118,6 +118,7 @@ <flag name='blockdev-del'/> <flag name='vhost-vsock'/> <flag name='egl-headless'/> + <flag name='zpci'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>307493</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index ec8330211c..cd3b0188c2 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -125,6 +125,7 @@ <flag name='vhost-vsock'/> <flag name='tpm-emulator'/> <flag name='egl-headless'/> + <flag name='zpci'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>346345</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index b8e46a970a..2f2e7da663 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -133,6 +133,7 @@ <flag name='tpm-emulator'/> <flag name='egl-headless'/> <flag name='vfio-pci.display'/> + <flag name='zpci'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>375593</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml index e8939667b3..06d451daec 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml @@ -105,6 +105,7 @@ <flag name='nbd-tls'/> <flag name='virtual-css-bridge'/> <flag name='sdl-gl'/> + <flag name='zpci'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>220386</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml index d91182ee84..d2c71c8163 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -108,6 +108,7 @@ <flag name='virtual-css-bridge'/> <flag name='sdl-gl'/> <flag name='vhost-vsock'/> + <flag name='zpci'/> <version>2007093</version> <kvmVersion>0</kvmVersion> <microcodeVersion>245800</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index e336cb1950..57251525b2 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -112,6 +112,7 @@ <flag name='sdl-gl'/> <flag name='blockdev-del'/> <flag name='vhost-vsock'/> + <flag name='zpci'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>269219</microcodeVersion> -- Yi Min

This patch introduces a new attribute PCI address extension flag to deal with the extension PCI attributes such as 'uid' and 'fid' on the S390 platform. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/device_conf.h | 1 + src/conf/domain_addr.h | 5 ++ src/qemu/qemu_domain_address.c | 142 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 146 insertions(+), 2 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index ff7d6c9d5f..11baf0c1e3 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -164,6 +164,7 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + int pciAddressExtFlags; /* enum virDomainPCIAddressExtensionFlags */ char *loadparm; /* PCI devices will only be automatically placed on a PCI bus diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5ad9d8ef3d..5219d2f208 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -29,6 +29,11 @@ # define VIR_PCI_ADDRESS_SLOT_LAST 31 # define VIR_PCI_ADDRESS_FUNCTION_LAST 7 +typedef enum { + VIR_PCI_ADDRESS_EXTENSION_NONE = 0, /* no extension */ + VIR_PCI_ADDRESS_EXTENSION_ZPCI = 1 << 0, /* zpci support */ +} virDomainPCIAddressExtensionFlags; + typedef enum { VIR_PCI_CONNECT_HOTPLUGGABLE = 1 << 0, /* is hotplug needed/supported */ diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index dda14adeb3..6fbe4a9644 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -509,6 +509,64 @@ qemuDomainAssignVirtioMMIOAddresses(virDomainDefPtr def, } +static bool +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { + case VIR_DOMAIN_DEVICE_CHR: + return false; + + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + break; + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: + default: + virReportEnumRangeError(virDomainDeviceType, device->type); + return false; + } + + return true; +} + + +static virDomainPCIAddressExtensionFlags +qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCapsPtr qemuCaps, + virDomainDeviceDefPtr dev) +{ + virDomainPCIAddressExtensionFlags extFlags = 0; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + qemuDomainDeviceSupportZPCI(dev)) { + extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI; + } + + return extFlags; +} + + /** * qemuDomainDeviceCalculatePCIConnectFlags: * @@ -991,6 +1049,56 @@ qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def, } +/** + * qemuDomainFillDevicePCIExtensionFlagsIter: + * + * @def: the entire DomainDef + * @dev: The device to be checked + * @info: virDomainDeviceInfo within the device + * @opaque: qemu capabilities + * + * Sets the pciAddressExtFlags for a single device's info. Has properly + * formatted arguments to be called by virDomainDeviceInfoIterate(). + * + * Always returns 0 - there is no failure. + */ +static int +qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virQEMUCapsPtr qemuCaps = opaque; + + info->pciAddressExtFlags + = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); + + return 0; +} + + +/** + * qemuDomainFillAllPCIExtensionFlags: + * + * @def: the entire DomainDef + * @qemuCaps: as you'd expect + * + * Set the info->pciAddressExtFlags for all devices in the domain. + * + * Returns 0 on success or -1 on failure (the only possibility of + * failure would be some internal problem with + * virDomainDeviceInfoIterate()) + */ +static int +qemuDomainFillAllPCIExtensionFlags(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + return virDomainDeviceInfoIterate(def, + qemuDomainFillDevicePCIExtensionFlagsIter, + qemuCaps); +} + + /** * qemuDomainFindUnusedIsolationGroupIter: * @def: domain definition @@ -1265,6 +1373,29 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def, } +/** + * qemuDomainFillDevicePCIExtensionFlags: + * + * @dev: The device to be checked + * @qemuCaps: as you'd expect + * + * Set the info->pciAddressExtFlags for a single device. + * + * No return value. + */ +static void +qemuDomainFillDevicePCIExtensionFlags(virDomainDeviceDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev); + + if (info) { + info->pciAddressExtFlags + = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); + } +} + + static int qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) @@ -2400,6 +2531,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainFillAllPCIConnectFlags(def, qemuCaps, driver) < 0) goto cleanup; + if (qemuDomainFillAllPCIExtensionFlags(def, qemuCaps) < 0) + goto cleanup; + if (qemuDomainSetupIsolationGroups(def) < 0) goto cleanup; @@ -2435,7 +2569,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, */ virDomainDeviceInfo info = { .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCI_DEVICE) + VIR_PCI_CONNECT_TYPE_PCI_DEVICE), + .pciAddressExtFlags = VIR_PCI_ADDRESS_EXTENSION_NONE }; bool buses_reserved = true; @@ -2472,7 +2607,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, qemuDomainHasPCIeRoot(def)) { virDomainDeviceInfo info = { .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCIE_DEVICE) + VIR_PCI_CONNECT_TYPE_PCIE_DEVICE), + .pciAddressExtFlags = VIR_PCI_ADDRESS_EXTENSION_NONE }; /* if there isn't an empty pcie-root-port, this will @@ -2989,6 +3125,8 @@ qemuDomainEnsurePCIAddress(virDomainObjPtr obj, qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps, driver); + qemuDomainFillDevicePCIExtensionFlags(dev, priv->qemuCaps); + return virDomainPCIAddressEnsureAddr(priv->pciaddrs, info, info->pciConnectFlags); } -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
@@ -164,6 +164,7 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + int pciAddressExtFlags; /* enum virDomainPCIAddressExtensionFlags */
There's a comment right above this that explains how pciConnectFlags is only used during address assignment: you should amend it to mention pciAddressExtFlags too. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/11 下午4:37, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
@@ -164,6 +164,7 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + int pciAddressExtFlags; /* enum virDomainPCIAddressExtensionFlags */ There's a comment right above this that explains how pciConnectFlags is only used during address assignment: you should amend it to mention pciAddressExtFlags too.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
As your comment on the 1st patch, if we have virPCIDeviceAddress include a extFlag, why not remove this one? -- Yi Min

On Wed, 2018-09-12 at 13:27 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午4:37, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
@@ -164,6 +164,7 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + int pciAddressExtFlags; /* enum virDomainPCIAddressExtensionFlags */
There's a comment right above this that explains how pciConnectFlags is only used during address assignment: you should amend it to mention pciAddressExtFlags too.
As your comment on the 1st patch, if we have virPCIDeviceAddress include a extFlag, why not remove this one?
Sure, if you can get away with it that's perfect! :) However, I'm not entirely convinced you can avoid duplicating the information, because I believe there will be at least some parts of the address allocation algorithm where you'll need to access the flags but haven't figured out you're going to assign a PCI address to the device yet, which makes accessing the flags in virPCIDeviceAddress not feasible. I could very well be wrong, though: I haven't actually looked into it in detail. -- Andrea Bolognani / Red Hat / Virtualization

QEMU on s390 supports PCI multibus since forever. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 62d56db9de..c307a8d571 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1793,6 +1793,10 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps, return false; } + /* S390 supports PCI-multibus. */ + if (ARCH_IS_S390(def->os.arch)) + return true; + /* If ARM 'virt' supports PCI, it supports multibus. * No extra conditions here for simplicity. */ -- Yi Min

The pci-root depends on zpci capability. So autogenerate pci-root if zpci exists. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f161cf6c84..aebd58c49c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3289,6 +3289,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; + addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break; case VIR_ARCH_SPARC: -- Yi Min

This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 9 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 ++++ 4 files changed, 102 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index b62fd53c66..363e5b21dc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -27,11 +27,23 @@ #include "virlog.h" #include "virstring.h" #include "domain_addr.h" +#include "virhashcode.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN VIR_LOG_INIT("conf.domain_addr"); +static void +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) +{ + if (!addrs || !addrs->zpciIds) + return; + + virHashFree(addrs->zpciIds->uids); + virHashFree(addrs->zpciIds->fids); + VIR_FREE(addrs->zpciIds); +} + virDomainPCIConnectFlags virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) { @@ -741,6 +753,78 @@ virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, addrs->buses[addr->bus].slot[addr->slot].functions &= ~(1 << addr->function); } + +static uint32_t +virZPCIAddrCode(const void *name, + uint32_t seed) +{ + unsigned int value = *((unsigned int *)name); + return virHashCodeGen(&value, sizeof(value), seed); +} + + +static bool +virZPCIAddrEqual(const void *namea, + const void *nameb) +{ + return *((unsigned int *)namea) == *((unsigned int *)nameb); +} + + +static void * +virZPCIAddrCopy(const void *name) +{ + unsigned int *copy; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + *copy = *((unsigned int *)name); + return (void *)copy; +} + + +static void +virZPCIAddrKeyFree(void *name) +{ + VIR_FREE(name); +} + + +int +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + if (addrs->zpciIds) + return 0; + + if (VIR_ALLOC(addrs->zpciIds) < 0) + return -1; + + if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL, + virZPCIAddrCode, + virZPCIAddrEqual, + virZPCIAddrCopy, + virZPCIAddrKeyFree))) + goto error; + + if (!(addrs->zpciIds->fids = virHashCreateFull(10, NULL, + virZPCIAddrCode, + virZPCIAddrEqual, + virZPCIAddrCopy, + virZPCIAddrKeyFree))) + goto error; + } + + return 0; + + error: + virDomainPCIAddressSetExtensionFree(addrs); + return -1; +} + + virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses) { @@ -767,6 +851,7 @@ virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs) if (!addrs) return; + virDomainPCIAddressSetExtensionFree(addrs); VIR_FREE(addrs->buses); VIR_FREE(addrs); } diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5219d2f208..d1ec869da4 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -116,6 +116,11 @@ typedef struct { } virDomainPCIAddressBus; typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr; +typedef struct { + virHashTablePtr uids, fids; +} virDomainZPCIAddressIds; +typedef virDomainZPCIAddressIds *virDomainZPCIAddressIdsPtr; + struct _virDomainPCIAddressSet { virDomainPCIAddressBus *buses; size_t nbuses; @@ -125,6 +130,7 @@ struct _virDomainPCIAddressSet { bool areMultipleRootsSupported; /* If true, the guest can use the pcie-to-pci-bridge controller */ bool isPCIeToPCIBridgeSupported; + virDomainZPCIAddressIdsPtr zpciIds; }; typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; @@ -132,6 +138,9 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1); +int virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virDomainPCIAddressExtensionFlags extFlags); + virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses); void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 954ab4b66c..7ee524ead0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -119,6 +119,7 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; +virDomainPCIAddressSetExtensionAlloc; virDomainPCIAddressSetFree; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 6fbe4a9644..162014e089 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1514,6 +1514,13 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, addrs->dryRun = dryRun; + /* create zpci address set for s390 domain */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + virDomainPCIAddressSetExtensionAlloc(addrs, + VIR_PCI_ADDRESS_EXTENSION_ZPCI)) { + goto error; + } + /* pSeries domains support multiple pci-root controllers */ if (qemuDomainIsPSeries(def)) addrs->areMultipleRootsSupported = true; -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 9 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 ++++ 4 files changed, 102 insertions(+)
Okay, I've spent some time actually digging into the hash table implementation this time around :) [...]
+static uint32_t +virZPCIAddrCode(const void *name, + uint32_t seed) +{ + unsigned int value = *((unsigned int *)name); + return virHashCodeGen(&value, sizeof(value), seed); +} + + +static bool +virZPCIAddrEqual(const void *namea, + const void *nameb) +{ + return *((unsigned int *)namea) == *((unsigned int *)nameb); +} + + +static void * +virZPCIAddrCopy(const void *name) +{ + unsigned int *copy; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + *copy = *((unsigned int *)name); + return (void *)copy; +} + + +static void +virZPCIAddrKeyFree(void *name) +{ + VIR_FREE(name); +}
This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful. vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer. That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? [...]
+int +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + if (addrs->zpciIds) + return 0;
This check doesn't look necessary. [...]
+typedef struct { + virHashTablePtr uids, fids; +} virDomainZPCIAddressIds;
One member per line, please. [...]
+ /* create zpci address set for s390 domain */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + virDomainPCIAddressSetExtensionAlloc(addrs, + VIR_PCI_ADDRESS_EXTENSION_ZPCI)) { + goto error; + }
You should check for virDomainPCIAddressSetExtensionAlloc() < 0 here. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/11 下午7:34, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 9 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 ++++ 4 files changed, 102 insertions(+) Okay, I've spent some time actually digging into the hash table implementation this time around :)
[...]
+static uint32_t +virZPCIAddrCode(const void *name, + uint32_t seed) +{ + unsigned int value = *((unsigned int *)name); + return virHashCodeGen(&value, sizeof(value), seed); +} + + +static bool +virZPCIAddrEqual(const void *namea, + const void *nameb) +{ + return *((unsigned int *)namea) == *((unsigned int *)nameb); +} + + +static void * +virZPCIAddrCopy(const void *name) +{ + unsigned int *copy; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + *copy = *((unsigned int *)name); + return (void *)copy; +} + + +static void +virZPCIAddrKeyFree(void *name) +{ + VIR_FREE(name); +} This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer.
That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later.
[...]
+int +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + if (addrs->zpciIds) + return 0; This check doesn't look necessary. All right.
[...]
+typedef struct { + virHashTablePtr uids, fids; +} virDomainZPCIAddressIds; One member per line, please. ok
[...]
+ /* create zpci address set for s390 domain */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + virDomainPCIAddressSetExtensionAlloc(addrs, + VIR_PCI_ADDRESS_EXTENSION_ZPCI)) { + goto error; + } You should check for
virDomainPCIAddressSetExtensionAlloc() < 0
here.
Thanks for your comments. -- Yi Min

在 2018/9/12 下午3:35, Yi Min Zhao 写道:
+static void * +virZPCIAddrCopy(const void *name) +{ + unsigned int *copy; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + *copy = *((unsigned int *)name); + return (void *)copy; +} + + +static void +virZPCIAddrKeyFree(void *name) +{ + VIR_FREE(name); +} This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer.
That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later. I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer.
-- Yi Min

On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote:
在 2018/9/12 下午3:35, Yi Min Zhao 写道:
This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer.
That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all?
I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later.
I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer.
Right. Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... I guess forbidding users from using UINT32_MAX as a fid is not an option, right? -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/12 下午6:37, Andrea Bolognani 写道:
On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote:
在 2018/9/12 下午3:35, Yi Min Zhao 写道:
This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer.
That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later. I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer. Right.
Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... Yes. Just one value makes all things complex.
I guess forbidding users from using UINT32_MAX as a fid is not an option, right?
Actually as my understanding, it's just a value to identify the pci function. IMO, it's not a big deal to decrease usable FID values. After all, UID set is smaller than FID set. The maximum number of pci devices is limited by UID. Anyway, I have to discuss this with my colleagues internally. I will tell you our discussion result first time. -- Yi Min

在 2018/9/12 下午6:37, Andrea Bolognani 写道:
On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote:
在 2018/9/12 下午3:35, Yi Min Zhao 写道:
This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer.
That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later. I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer. Right.
Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue...
I guess forbidding users from using UINT32_MAX as a fid is not an option, right?
We have discussed this. FID 0~UINT32_MAX are allowed in s390. We shouldn't forbid users. So we have to keep the hashtable code as it is. -- Yi Min

On Thu, 2018-09-13 at 15:58 +0800, Yi Min Zhao wrote:
在 2018/9/12 下午6:37, Andrea Bolognani 写道:
Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue...
I guess forbidding users from using UINT32_MAX as a fid is not an option, right?
We have discussed this. FID 0~UINT32_MAX are allowed in s390. We shouldn't forbid users. So we have to keep the hashtable code as it is.
That's a bit unfortunate, but okay. Just ignore the comments about it and keep the current implementation then. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/13 下午4:23, Andrea Bolognani 写道:
On Thu, 2018-09-13 at 15:58 +0800, Yi Min Zhao wrote:
在 2018/9/12 下午6:37, Andrea Bolognani 写道:
Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue...
I guess forbidding users from using UINT32_MAX as a fid is not an option, right? We have discussed this. FID 0~UINT32_MAX are allowed in s390. We shouldn't forbid users. So we have to keep the hashtable code as it is. That's a bit unfortunate, but okay. Just ignore the comments about it and keep the current implementation then.
Thanks for your understanding. -- Yi Min

This patch introduces new XML parser/formatter functions. Uid is 16-bit and non-zero. Fid is 32-bit. They are added as two new attributes of PCI address, and parsed/formatted along with PCI address parser/formatter. The related test is also added. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- docs/schemas/basictypes.rng | 23 ++++++++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 69 ++++++++++++++++++++++ src/conf/device_conf.h | 2 + src/conf/domain_addr.c | 3 + src/conf/domain_conf.c | 6 ++ src/libvirt_private.syms | 1 + src/util/virpci.h | 3 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 25 ++++++++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 ++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 23 ++++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++++++ tests/qemuxml2argvtest.c | 7 +++ tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 +++++++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++++++++++ tests/qemuxml2xmltest.c | 6 ++ 16 files changed, 264 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 14a3670e5c..3defb56316 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -65,6 +65,17 @@ </data> </choice> </define> + <define name="uint32"> + <choice> + <data type="string"> + <param name="pattern">(0x)?[0-9a-fA-F]{1,8}</param> + </data> + <data type="unsignedInt"> + <param name="minInclusive">0</param> + <param name="maxInclusive">4294967295</param> + </data> + </choice> + </define> <define name="UUID"> <choice> @@ -111,6 +122,18 @@ </attribute> </optional> </define> + <define name="zpciaddress"> + <optional> + <attribute name="uid"> + <ref name="uint16"/> + </attribute> + </optional> + <optional> + <attribute name="fid"> + <ref name="uint32"/> + </attribute> + </optional> + </define> <!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" --> <!-- The lowest bit of the 1st byte is the "multicast" bit. a --> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3796eb4b5e..2ccf777d20 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5220,6 +5220,7 @@ <value>pci</value> </attribute> <ref name="pciaddress"/> + <ref name="zpciaddress"/> </group> <group> <attribute name="type"> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 7a8f84e036..5ae990afdb 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -32,6 +32,65 @@ #define VIR_FROM_THIS VIR_FROM_DEVICE +static int +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) +{ + /* We don't need to check fid because fid covers + * all range of uint32 type. + */ + if (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || + zpci->uid == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%x', " + "must be > 0x0 and <= 0x%x"), + zpci->uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); + return 0; + } + + return 1; +} + +static int +virZPCIDeviceAddressParseXML(xmlNodePtr node, + virPCIDeviceAddressPtr addr) +{ + char *uid, *fid; + int ret = -1; + virZPCIDeviceAddress def = { 0 }; + + uid = virXMLPropString(node, "uid"); + fid = virXMLPropString(node, "fid"); + + if (uid) { + if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'uid' attribute")); + goto cleanup; + } + } + + if (fid) { + if (virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'fid' attribute")); + goto cleanup; + } + } + + if (!virZPCIDeviceAddressIsEmpty(&def) && + !virZPCIDeviceAddressIsValid(&def)) + goto cleanup; + + addr->zpci = def; + ret = 0; + + cleanup: + VIR_FREE(uid); + VIR_FREE(fid); + return ret; +} + int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src) @@ -213,6 +272,13 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info) } +bool +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +{ + return !(addr->uid || addr->fid); +} + + int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr) @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup; + if (virZPCIDeviceAddressParseXML(node, addr) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 11baf0c1e3..7e98b4ace0 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -194,6 +194,8 @@ bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); bool virDeviceInfoPCIAddressIsWanted(const virDomainDeviceInfo *info); bool virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info); +bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr); + int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr); diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 363e5b21dc..62932e4e62 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, function) < 0) return -1; + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + addr.zpci = dev->addr.pci.zpci; + if (virDomainPCIAddressReserveAddrInternal(addrs, &addr, flags, dev->isolationGroup, false) < 0) return -1; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 38cac07913..85234bc8d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6514,6 +6514,12 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.pci.slot, info->addr.pci.function); } + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + virBufferAsprintf(buf, " uid='0x%.4x'", + info->addr.pci.zpci.uid); + virBufferAsprintf(buf, " fid='0x%.8x'", + info->addr.pci.zpci.fid); + } if (info->addr.pci.multi) { virBufferAsprintf(buf, " multifunction='%s'", virTristateSwitchTypeToString(info->addr.pci.multi)); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7ee524ead0..12f051468e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -104,6 +104,7 @@ virPCIDeviceAddressFormat; virPCIDeviceAddressIsEmpty; virPCIDeviceAddressIsValid; virPCIDeviceAddressParseXML; +virZPCIDeviceAddressIsEmpty; # conf/domain_addr.h diff --git a/src/util/virpci.h b/src/util/virpci.h index b7bcfa6d9f..e2ffe11625 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr; +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX + typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; struct _virZPCIDeviceAddress { diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args new file mode 100644 index 0000000000..8ac435cb3e --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk0,\ +id=virtio-disk0,bootindex=1 \ +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000 diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml new file mode 100644 index 0000000000..7887b97b2c --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </disk> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args new file mode 100644 index 0000000000..d6b1520c47 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args @@ -0,0 +1,23 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x8 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml new file mode 100644 index 0000000000..cde333e220 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml @@ -0,0 +1,19 @@ +<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> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9bfe864597..e96d00cec7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1058,6 +1058,10 @@ mymain(void) QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); + DO_TEST("disk-virtio-s390-zpci", + QEMU_CAPS_DEVICE_ZPCI, + QEMU_CAPS_CCW, + QEMU_CAPS_VIRTIO_S390); DO_TEST("disk-order", QEMU_CAPS_VIRTIO_BLK_SCSI); DO_TEST("disk-virtio-queues", QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES); @@ -1651,6 +1655,9 @@ mymain(void) DO_TEST_PARSE_ERROR("hostdev-mdev-display-missing-graphics", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_VFIO_PCI_DISPLAY); + DO_TEST("hostdev-vfio-zpci", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); diff --git a/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml new file mode 100644 index 0000000000..39b5acdf3b --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </disk> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml new file mode 100644 index 0000000000..1f48c44e30 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </hostdev> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 43fd4e5f0f..47f3b9431b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -379,6 +379,9 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-ioeventfd", QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-s390-zpci", + QEMU_CAPS_DEVICE_ZPCI, + QEMU_CAPS_CCW); DO_TEST("disk-scsi-megasas", QEMU_CAPS_SCSI_MEGASAS); DO_TEST("disk-scsi-mptsas1068", @@ -460,6 +463,9 @@ mymain(void) DO_TEST("hostdev-usb-address", NONE); DO_TEST("hostdev-pci-address", NONE); DO_TEST("hostdev-vfio", NONE); + DO_TEST("hostdev-vfio-zpci", + QEMU_CAPS_DEVICE_ZPCI, + QEMU_CAPS_CCW); DO_TEST("hostdev-mdev-precreated", NONE); DO_TEST("hostdev-mdev-display", QEMU_CAPS_VFIO_PCI_DISPLAY); DO_TEST("pci-rom", NONE); -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
+static int +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
This is a predicate, so it should return a bool. [...]
+ virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%x', " + "must be > 0x0 and <= 0x%x"), + zpci->uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
You should always use "0x%.4x" when printing uids. [...]
+static int +virZPCIDeviceAddressParseXML(xmlNodePtr node, + virPCIDeviceAddressPtr addr) +{ + char *uid, *fid; + int ret = -1; + virZPCIDeviceAddress def = { 0 };
One variable per line, please. Also structs are usually declared first and 'ret' is usually last, but that's admittedly not very important :) [...]
+ if (uid) { + if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'uid' attribute")); + goto cleanup; + } + }
You can convert the above to if (uid && virStrToLong_uip(...) < 0) { ... } and do the same with fid. [...]
+bool +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +{ + return !(addr->uid || addr->fid); +}
This function belongs in virpci, besides the struct definition and the very much related virPCIDeviceAddressIsEmpty(). [...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup;
+ if (virZPCIDeviceAddressParseXML(node, addr) < 0) + goto cleanup;
I'm not super convinced about this. On one hand, it makes it so callers can't possibly forget to parse the zPCI part, and that's literally embedded in the PCI part now; on the other hand, we have functions to verify separately whether the PCI and zPCI parts are empty, which is pretty weird. I guess we can leave as it is for now, but the semantics are muddy enough that I can pretty much guarantee someone will have to clean them up at some point down the line. [...]
@@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, function) < 0) return -1;
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set of zPCI ids, the same way it's allocating a new PCI address right above, or do nothing at all because zPCI address allocation is handled by later calling virDomainZPCIAddressReserveNextAddr()? This is basically another manifestation of the issue I mentioned above: we don't seem to have a well-defined and strictly adhered to idea of how the PCI part and zPCI part should relate to each other, so they're either considered separate entities or part of the same entity depending on which APIs you're looking at. [...]
+ if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + virBufferAsprintf(buf, " uid='0x%.4x'", + info->addr.pci.zpci.uid); + virBufferAsprintf(buf, " fid='0x%.8x'", + info->addr.pci.zpci.fid);
You only need a single call to virBufferAsprintf() here. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/11 下午8:05, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
+static int +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) This is a predicate, so it should return a bool. OK.
[...]
+ virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%x', " + "must be > 0x0 and <= 0x%x"), + zpci->uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); You should always use "0x%.4x" when printing uids. ditto
[...]
+static int +virZPCIDeviceAddressParseXML(xmlNodePtr node, + virPCIDeviceAddressPtr addr) +{ + char *uid, *fid; + int ret = -1; + virZPCIDeviceAddress def = { 0 }; One variable per line, please.
Also structs are usually declared first and 'ret' is usually last, but that's admittedly not very important :) ditto. I'm very glad that you could give me so many valuable comments even if it's just about code style.
[...]
+ if (uid) { + if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'uid' attribute")); + goto cleanup; + } + } You can convert the above to
if (uid && virStrToLong_uip(...) < 0) { ... }
and do the same with fid. OK.
+bool +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +{ + return !(addr->uid || addr->fid); +} This function belongs in virpci, besides the struct definition and
[...] the very much related virPCIDeviceAddressIsEmpty(). I'm not very clear with your comment. Do you mean I should move it to other place?
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup;
+ if (virZPCIDeviceAddressParseXML(node, addr) < 0) + goto cleanup; I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse the zPCI part, and that's literally embedded in the PCI part now; on the other hand, we have functions to verify separately whether the PCI and zPCI parts are empty, which is pretty weird. It's weird indeed. But if we merge zPCI part check into PCI code. We might have to merge other code. But PCI address has extension only on S390 at least now.
I guess we can leave as it is for now, but the semantics are muddy enough that I can pretty much guarantee someone will have to clean them up at some point down the line. OK.
[...]
@@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, function) < 0) return -1;
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + addr.zpci = dev->addr.pci.zpci; I'm confused by this part. Shouldn't this either request a new set of zPCI ids, the same way it's allocating a new PCI address right above, or do nothing at all because zPCI address allocation is handled by later calling virDomainZPCIAddressReserveNextAddr()? Here, we want to store the defined zPCI address which has been reserved. If we don't do this, we might lose zPCI address and do reservation again but the reserved one are still existing in zPCI set.
For this problem, I once thought about adding extension address into DeviceInfo next to PCI address embraced in a struct. Then here is not a problem.
This is basically another manifestation of the issue I mentioned above: we don't seem to have a well-defined and strictly adhered to idea of how the PCI part and zPCI part should relate to each other, so they're either considered separate entities or part of the same entity depending on which APIs you're looking at.
I think the above idea I thought about before is like what you said?
[...]
+ if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + virBufferAsprintf(buf, " uid='0x%.4x'", + info->addr.pci.zpci.uid); + virBufferAsprintf(buf, " fid='0x%.8x'", + info->addr.pci.zpci.fid); You only need a single call to virBufferAsprintf() here.
OK. -- Yi Min

On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午8:05, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
+bool +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +{ + return !(addr->uid || addr->fid); +}
This function belongs in virpci, besides the struct definition and the very much related virPCIDeviceAddressIsEmpty().
I'm not very clear with your comment. Do you mean I should move it to other place?
Yeah, the function and its definition should be in src/util/virpci.c and src/util/virpci.h respectively. I realize now that virPCIDeviceAddressIsValid() and virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I swear that I posted patches moving them over... My bad, I'll do that right away. Sorry for the confusion.
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup;
+ if (virZPCIDeviceAddressParseXML(node, addr) < 0) + goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse the zPCI part, and that's literally embedded in the PCI part now; on the other hand, we have functions to verify separately whether the PCI and zPCI parts are empty, which is pretty weird.
It's weird indeed. But if we merge zPCI part check into PCI code. We might have to merge other code. But PCI address has extension only on S390 at least now.
That's not necessarily a problem, at least in principle. See all the code dealing with "isolation groups", for example: while the concept is only ever really used for pSeries guests, it's implemented in a way that's designed to stay out of the way in all other cases, and the result is that you won't have to worry about it except when needed. The zPCI code should ideally behave similarly.
I guess we can leave as it is for now, but the semantics are muddy enough that I can pretty much guarantee someone will have to clean them up at some point down the line.
OK.
[...]
@@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, function) < 0) return -1;
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set of zPCI ids, the same way it's allocating a new PCI address right above, or do nothing at all because zPCI address allocation is handled by later calling virDomainZPCIAddressReserveNextAddr()?
Here, we want to store the defined zPCI address which has been reserved. If we don't do this, we might lose zPCI address and do reservation again but the reserved one are still existing in zPCI set.
From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only
I can't picture the exact scenario right now but I assume something like that can happen if you have <address type='pci' uid='0x0001' fid='0x00000001'/> in which case the zPCI part has already been provided by the user but we still need to allocate the PCI part ourselves. Speaking of which, I wonder if something like <address type='pci'> <zpci uid='0x0001' fid='0x00000001'/> </address> would be a more appropriate syntax: not only it clearly shows that the uid and fid attributes are connected to zPCI, but if we ever introduce additional PCI address extensions they could be similarly be represented as childs of <address> instead of adding even more attributes to the existing element. Either way, this kind of ad-hoc messing with the zPCI part seems to me like clear evidence of the fact that we need to step back and rethink the way the various pieces of the puzzle fit together. part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments. Can you try and see whether this kind of API would work?
For this problem, I once thought about adding extension address into DeviceInfo next to PCI address embraced in a struct. Then here is not a problem.
That could almost work, seeing how virDomainDeviceInfoFormat() doesn't use virPCIDeviceAddressFormat() to format the PCI address[1], but at least in theory you should be able to take a virPCIDeviceAddress and turn it into a string without having to peek into other objects such as the parent virDomainDeviceInfo, so that approach wouldn't be very clean at all.
This is basically another manifestation of the issue I mentioned above: we don't seem to have a well-defined and strictly adhered to idea of how the PCI part and zPCI part should relate to each other, so they're either considered separate entities or part of the same entity depending on which APIs you're looking at.
I think the above idea I thought about before is like what you said?
Do you mean the idea of moving the zPCI part out of the PCI address? If so, I've already replied above; if not, please be more specific :) [1] Which is arguably an issue that should be resolved as well... There are a few places where virPCIDeviceAddressFormat() *is* used, and those won't ever format the zPCI stuff. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/13 下午9:58, Andrea Bolognani 写道:
On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午8:05, Andrea Bolognani 写道:
+bool +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +{ + return !(addr->uid || addr->fid); +} This function belongs in virpci, besides the struct definition and
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...] the very much related virPCIDeviceAddressIsEmpty(). I'm not very clear with your comment. Do you mean I should move it to other place? Yeah, the function and its definition should be in src/util/virpci.c and src/util/virpci.h respectively.
I realize now that virPCIDeviceAddressIsValid() and virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I swear that I posted patches moving them over... My bad, I'll do that right away.
Sorry for the confusion. Got it.
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup;
+ if (virZPCIDeviceAddressParseXML(node, addr) < 0) + goto cleanup; I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse the zPCI part, and that's literally embedded in the PCI part now; on the other hand, we have functions to verify separately whether the PCI and zPCI parts are empty, which is pretty weird. It's weird indeed. But if we merge zPCI part check into PCI code. We might have to merge other code. But PCI address has extension only on S390 at least now. That's not necessarily a problem, at least in principle.
See all the code dealing with "isolation groups", for example: while the concept is only ever really used for pSeries guests, it's implemented in a way that's designed to stay out of the way in all other cases, and the result is that you won't have to worry about it except when needed.
The zPCI code should ideally behave similarly.
@@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, function) < 0) return -1;
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + addr.zpci = dev->addr.pci.zpci; I'm confused by this part. Shouldn't this either request a new set of zPCI ids, the same way it's allocating a new PCI address right above, or do nothing at all because zPCI address allocation is handled by later calling virDomainZPCIAddressReserveNextAddr()? Here, we want to store the defined zPCI address which has been reserved. If we don't do this, we might lose zPCI address and do reservation again but the reserved one are still existing in zPCI set. I can't picture the exact scenario right now but I assume something
I guess we can leave as it is for now, but the semantics are muddy enough that I can pretty much guarantee someone will have to clean them up at some point down the line. OK. [...] like that can happen if you have
<address type='pci' uid='0x0001' fid='0x00000001'/>
in which case the zPCI part has already been provided by the user but we still need to allocate the PCI part ourselves.
Speaking of which, I wonder if something like
<address type='pci'> <zpci uid='0x0001' fid='0x00000001'/> </address>
would be a more appropriate syntax: not only it clearly shows that the uid and fid attributes are connected to zPCI, but if we ever introduce additional PCI address extensions they could be similarly be represented as childs of <address> instead of adding even more attributes to the existing element.
Either way, this kind of ad-hoc messing with the zPCI part seems to me like clear evidence of the fact that we need to step back and rethink the way the various pieces of the puzzle fit together.
From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments.
Can you try and see whether this kind of API would work? I did a simple test. It worked. Do you prefer this way?
For this problem, I once thought about adding extension address into DeviceInfo next to PCI address embraced in a struct. Then here is not a problem. That could almost work, seeing how virDomainDeviceInfoFormat() doesn't use virPCIDeviceAddressFormat() to format the PCI address[1], but at least in theory you should be able to take a virPCIDeviceAddress and turn it into a string without having to peek into other objects such as the parent virDomainDeviceInfo, so that approach wouldn't be very clean at all. Right. That's why I finally gave up it.
This is basically another manifestation of the issue I mentioned above: we don't seem to have a well-defined and strictly adhered to idea of how the PCI part and zPCI part should relate to each other, so they're either considered separate entities or part of the same entity depending on which APIs you're looking at. I think the above idea I thought about before is like what you said? Do you mean the idea of moving the zPCI part out of the PCI address? If so, I've already replied above; if not, please be more specific :) Yes.
[1] Which is arguably an issue that should be resolved as well... There are a few places where virPCIDeviceAddressFormat() *is* used, and those won't ever format the zPCI stuff.
-- Yi Min

On Wed, 2018-09-19 at 16:59 +0800, Yi Min Zhao wrote:
在 2018/9/13 下午9:58, Andrea Bolognani 写道:
I realize now that virPCIDeviceAddressIsValid() and virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I swear that I posted patches moving them over... My bad, I'll do that right away.
Sorry for the confusion.
Got it.
The patches mentioned above have been posted and merged in the meantime :)
From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments.
Can you try and see whether this kind of API would work?
I did a simple test. It worked. Do you prefer this way?
Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/20 下午4:28, Andrea Bolognani 写道:
On Wed, 2018-09-19 at 16:59 +0800, Yi Min Zhao wrote:
在 2018/9/13 下午9:58, Andrea Bolognani 写道:
I realize now that virPCIDeviceAddressIsValid() and virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I swear that I posted patches moving them over... My bad, I'll do that right away.
Sorry for the confusion. Got it. The patches mentioned above have been posted and merged in the meantime :)
From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments.
Can you try and see whether this kind of API would work? I did a simple test. It worked. Do you prefer this way? Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now.
So do you mean we use this kind of API in next version for review? -- Yi Min

On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote:
From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments.
Can you try and see whether this kind of API would work?
I did a simple test. It worked. Do you prefer this way?
Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now.
So do you mean we use this kind of API in next version for review?
Yes, please :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/25 下午4:02, Andrea Bolognani 写道:
On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote:
From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments.
Can you try and see whether this kind of API would work? I did a simple test. It worked. Do you prefer this way? Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now. So do you mean we use this kind of API in next version for review? Yes, please :)
I'm not sure how big the change is. So I might take more time to prepare the new version. So that we can't catch up with the new release. Thanks for your kindness and comments! -- Yi Min

On Tue, 2018-09-25 at 17:16 +0800, Yi Min Zhao wrote:
So do you mean we use this kind of API in next version for review?
Yes, please :)
I'm not sure how big the change is. So I might take more time to prepare the new version. So that we can't catch up with the new release. Thanks for your kindness and comments!
I think it's going to be a fairly sizeable change, but as we're getting awfully close to freeze anyway I would have advised against merging zPCI support this late in the cycle anyway, so timing-wise I'd say it probably won't change much. -- Andrea Bolognani / Red Hat / Virtualization

We should ensure that the Qemu should support zPCI when zPCI address is defined in XML. Otherwise the error should be reported. So this patch introduces the validation of zPCI address definition for qemuDomainDeviceDefValidate(). Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index aebd58c49c..7d7cd3cfdc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5728,6 +5728,27 @@ qemuDomainDeviceDefValidateGraphics(const virDomainGraphicsDef *graphics, } +static int +qemuDomainZPCIAddressDefValidate(virDomainDeviceDef *dev, + virQEMUCapsPtr qemuCaps) +{ + virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev); + + if (!info || (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) + return 0; + + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("This QEMU binary doesn't support zPCI.")); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -5741,6 +5762,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, def->emulator))) return -1; + ret = qemuDomainZPCIAddressDefValidate((virDomainDeviceDef *)dev, qemuCaps); + if (ret < 0) + goto out; + switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_NET: ret = qemuDomainDeviceDefValidateNetwork(dev->data.net); @@ -5813,6 +5838,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; } + out: virObjectUnref(qemuCaps); return ret; } -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
We should ensure that the Qemu should support zPCI when zPCI address is
s/Qemu/QEMU/ [...]
+static int +qemuDomainZPCIAddressDefValidate(virDomainDeviceDef *dev, + virQEMUCapsPtr qemuCaps)
The second argument is not aligned properly. I'd also change the name: qemuDomainDeviceDefValidateZPCIAddress() perhaps? It's quite a mouthful, but also more accurate. [...]
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("This QEMU binary doesn't support zPCI."));
No full stop at the end of the error message, please. [...]
@@ -5741,6 +5762,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, def->emulator))) return -1;
+ ret = qemuDomainZPCIAddressDefValidate((virDomainDeviceDef *)dev, qemuCaps);
This cast is kinds gross. I realize virDomainDeviceGetInfo() requires the pointer to be non-const, but from a semantics point of view qemuDomainZPCIAddressDefValidate() should be okay with being passed a const pointer - it's only validating the device, not changing it, after all. Please move the cast into the function instead of requiring the caller to perform it. We should also have a generic qemuDomainDeviceDefValidateAddress() wrapper that calls the zPCI-specific one only for PCI devices, and call that one from here. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/11 下午8:44, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
We should ensure that the Qemu should support zPCI when zPCI address is s/Qemu/QEMU/ Sure.
[...]
+static int +qemuDomainZPCIAddressDefValidate(virDomainDeviceDef *dev, + virQEMUCapsPtr qemuCaps) The second argument is not aligned properly.
I'd also change the name: qemuDomainDeviceDefValidateZPCIAddress() perhaps? It's quite a mouthful, but also more accurate. ok.
[...]
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("This QEMU binary doesn't support zPCI.")); No full stop at the end of the error message, please. Could you please explain more?
[...]
@@ -5741,6 +5762,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, def->emulator))) return -1;
+ ret = qemuDomainZPCIAddressDefValidate((virDomainDeviceDef *)dev, qemuCaps); This cast is kinds gross. I realize virDomainDeviceGetInfo() requires the pointer to be non-const, but from a semantics point of view qemuDomainZPCIAddressDefValidate() should be okay with being passed a const pointer - it's only validating the device, not changing it, after all.
Please move the cast into the function instead of requiring the caller to perform it.
We should also have a generic qemuDomainDeviceDefValidateAddress() wrapper that calls the zPCI-specific one only for PCI devices, and call that one from here.
Got it. -- Yi Min

On Thu, 2018-09-13 at 18:08 +0800, Yi Min Zhao wrote:
[...]
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("This QEMU binary doesn't support zPCI."));
No full stop at the end of the error message, please.
Could you please explain more?
Just remove the full stop, like virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This QEMU binary doesn't support zPCI")); -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/13 下午7:47, Andrea Bolognani 写道:
Just remove the full stop, like
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This QEMU binary doesn't support zPCI")); Ah....Got it.
-- Yi Min

This patch adds new functions for reservation, assignment and release to handle the uid/fid. If the uid/fid is defined in the domain XML, they will be reserved directly in collecting phase. If any of them is not defined, we will find out an available value for it from zPCI address hashtable, and reserve it. For hotplug case, there might be or not zPCI definition. So allocate and reserve uid/fid for undefined case. Assign if needed and reserve uid/fid for defined case. If the user define zPCI extension address but zPCI capability doesn't exist, an error will be reported. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/device_conf.c | 7 ++ src/conf/device_conf.h | 1 + src/conf/domain_addr.c | 242 +++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 15 +++ src/libvirt_private.syms | 4 + src/qemu/qemu_domain_address.c | 56 +++++++++- 6 files changed, 324 insertions(+), 1 deletion(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 5ae990afdb..692d4c31ea 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -278,6 +278,13 @@ virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) return !(addr->uid || addr->fid); } +bool +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci); +} + int virPCIDeviceAddressParseXML(xmlNodePtr node, diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7e98b4ace0..c5629dc26b 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -195,6 +195,7 @@ bool virDeviceInfoPCIAddressIsWanted(const virDomainDeviceInfo *info); bool virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info); bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr); +bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info); int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr); diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 62932e4e62..1c3248f04b 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -33,6 +33,152 @@ VIR_LOG_INIT("conf.domain_addr"); +static int +virDomainZPCIAddressReserveId(virHashTablePtr set, + unsigned int id, + const char *name) +{ + if (virHashLookup(set, &id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zPCI %s %u is already reserved"), + name, id); + return -1; + } + + if (virHashAddEntry(set, &id, (void*)1) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to reserve %s %u"), + name, id); + return -1; + } + + return 0; +} + + +static int +virDomainZPCIAddressReserveUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + return virDomainZPCIAddressReserveId(set, addr->uid, "uid"); +} + + +static int +virDomainZPCIAddressReserveFid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + return virDomainZPCIAddressReserveId(set, addr->fid, "fid"); +} + + +static int +virDomainZPCIAddressAssignId(virHashTablePtr set, + unsigned int *id, + unsigned int min, + unsigned int max, + const char *name) +{ + while (virHashLookup(set, &min)) { + if (min == max) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("There is no more free %s."), + name); + return -1; + } + ++min; + } + *id = min; + + return 0; +} + + +static int +virDomainZPCIAddressAssignUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + return virDomainZPCIAddressAssignId(set, &addr->uid, 1, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid"); +} + + +static int +virDomainZPCIAddressAssignFid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + return virDomainZPCIAddressAssignId(set, &addr->fid, 0, + VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid"); +} + + +static void +virDomainZPCIAddressReleaseUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (virHashRemoveEntry(set, &addr->uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release uid %u failed"), addr->uid); + } + + addr->uid = 0; +} + + +static void +virDomainZPCIAddressReleaseFid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (virHashRemoveEntry(set, &addr->fid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release fid %u failed"), addr->fid); + } + + addr->fid = 0; +} + + +static void +virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr)) + return; + + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + + virDomainZPCIAddressReleaseFid(zpciIds->fids, addr); +} + + +static int +virDomainZPCIAddressReserveNextUid(virHashTablePtr uids, + virZPCIDeviceAddressPtr zpci) +{ + if (virDomainZPCIAddressAssignUid(uids, zpci) < 0) + return -1; + + if (virDomainZPCIAddressReserveUid(uids, zpci) < 0) + return -1; + + return 0; +} + + +static int +virDomainZPCIAddressReserveNextFid(virHashTablePtr fids, + virZPCIDeviceAddressPtr zpci) +{ + if (virDomainZPCIAddressAssignFid(fids, zpci) < 0) + return -1; + + if (virDomainZPCIAddressReserveFid(fids, zpci) < 0) + return -1; + + return 0; +} + + static void virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) { @@ -44,6 +190,90 @@ virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) VIR_FREE(addrs->zpciIds); } + +static int +virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0) + return -1; + + if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + } + + return 0; +} + + +static int +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0) + return -1; + + if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + } + + return 0; +} + + +int +virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + /* Reserve uid/fid to ZPCI device which has defined uid/fid + * in the domain. + */ + return virDomainZPCIAddressReserveAddr(addrs->zpciIds, &addr->zpci); + } + + return 0; +} + + +int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) { + virZPCIDeviceAddress zpci = { 0 }; + + if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0) + return -1; + + if (!addrs->dryRun) + dev->zpci = zpci; + } + + return 0; +} + +static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{ + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + virZPCIDeviceAddressPtr zpci = &dev->addr.pci.zpci; + + if (virZPCIDeviceAddressIsEmpty(zpci)) + return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci); + else + return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci); + } + + return 0; +} + virDomainPCIConnectFlags virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) { @@ -740,12 +970,24 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, ret = virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1); } + ret = virDomainPCIAddressExtensionEnsureAddr(addrs, dev); + cleanup: VIR_FREE(addrStr); return ret; } +void +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + int extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)) + virDomainZPCIAddressReleaseIds(addrs->zpciIds, &addr->zpci); +} + + void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index d1ec869da4..a1f71f15f4 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -164,6 +164,16 @@ bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + virDomainPCIAddressExtensionFlags extFlags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + virDomainPCIAddressExtensionFlags extFlags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int virDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr, virDomainPCIConnectFlags flags, @@ -185,6 +195,11 @@ void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + int extFlags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + void virDomainPCIAddressSetAllMulti(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 12f051468e..f6c512710d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -93,6 +93,7 @@ virCPUModeTypeToString; # conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceInfoAddressIsEqual; @@ -115,6 +116,9 @@ virDomainPCIAddressAsString; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr; virDomainPCIAddressReleaseAddr; virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 162014e089..e9e0970567 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1405,6 +1405,22 @@ qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, } +static int +qemuDomainAssignPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainPCIAddressSetPtr addrs = opaque; + virPCIDeviceAddressPtr addr = &info->addr.pci; + virDomainPCIAddressExtensionFlags extFlags = info->pciAddressExtFlags; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return virDomainPCIAddressExtensionReserveNextAddr(addrs, addr, extFlags); + + return 0; +} + static int qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceDefPtr device, @@ -1498,6 +1514,29 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, return ret; } +static int +qemuDomainCollectPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainPCIAddressSetPtr addrs = opaque; + virPCIDeviceAddressPtr addr = &info->addr.pci; + + if (!virDeviceInfoPCIAddressExtensionIsPresent(info) || + ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && + (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) { + /* If a hostdev has a parent, its info will be a part of the + * parent, and will have its address collected during the scan + * of the parent's device type. + */ + return 0; + } + + return virDomainPCIAddressExtensionReserveAddr(addrs, addr, + info->pciAddressExtFlags); +} + static virDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -1592,6 +1631,12 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, addrs) < 0) goto error; + if (virDomainDeviceInfoIterate(def, + qemuDomainCollectPCIAddressExtension, + addrs) < 0) { + goto error; + } + return addrs; error: @@ -2595,6 +2640,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; + if (virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) < 0) + goto cleanup; + /* Only for *new* domains with pcie-root (and no other * manually specified PCI controllers in the definition): If, * after assigning addresses/reserving slots for all devices, @@ -2689,6 +2737,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; + if (virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) < 0) + goto cleanup; + /* set multi attribute for devices at function 0 of * any slot that has multiple functions in use */ @@ -3148,8 +3199,11 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (!devstr) devstr = info->alias; - if (virDeviceInfoPCIAddressIsPresent(info)) + if (virDeviceInfoPCIAddressIsPresent(info)) { virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci); + virDomainPCIAddressExtensionReleaseAddr(priv->pciaddrs, &info->addr.pci, + info->pciAddressExtFlags); + } if (virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) VIR_WARN("Unable to release USB address on %s", NULLSTR(devstr)); -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
If the user define zPCI extension address but zPCI capability doesn't exist, an error will be reported.
You're (no longer) checking for the capability here, so the commit message should be updated accordingly. [...]
+bool +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
The second line is not aligned properly. Actually, you'll probably want to restructure this a bit so that it only looks into info->addr.pci.zpci if the zPCI extension flag is present, after which the comment above will likely no longer apply. [...]
+static int +virDomainZPCIAddressReserveId(virHashTablePtr set, + unsigned int id, + const char *name) +{ + if (virHashLookup(set, &id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zPCI %s %u is already reserved"), + name, id);
Please format the id as octal for easier debugging when something goes wrong. Same below. [...]
+static void +virDomainZPCIAddressReleaseUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (virHashRemoveEntry(set, &addr->uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release uid %u failed"), addr->uid); + }
You should have a generic virDomainZPCIAddressReleaseId() function that you call from ReleaseUid() and ReleaseFid(), just like you have for reserving them. Additionally, it looks like failure to release an id is not a fatal error since processing continue, so you should use VIR_WARN() or something like that instead of virReportError() when that happens. [...]
+int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the next address regardless of what's currently stored in the device info, no? [...]
+void +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + int extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))
You don't need two sets of parentheses here :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/11 下午9:59, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
If the user define zPCI extension address but zPCI capability doesn't exist, an error will be reported. You're (no longer) checking for the capability here, so the commit message should be updated accordingly. Good catch!
[...]
+bool +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci); The second line is not aligned properly.
Actually, you'll probably want to restructure this a bit so that it only looks into info->addr.pci.zpci if the zPCI extension flag is present, after which the comment above will likely no longer apply.
[...] OK.
+static int +virDomainZPCIAddressReserveId(virHashTablePtr set, + unsigned int id, + const char *name) +{ + if (virHashLookup(set, &id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zPCI %s %u is already reserved"), + name, id); Please format the id as octal for easier debugging when something goes wrong. Same below. OK.
+static void +virDomainZPCIAddressReleaseUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (virHashRemoveEntry(set, &addr->uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release uid %u failed"), addr->uid); + } You should have a generic virDomainZPCIAddressReleaseId() function
[...] that you call from ReleaseUid() and ReleaseFid(), just like you have for reserving them. Actually there's the function ***ReleaseId() like you said. Don't you see it?
Additionally, it looks like failure to release an id is not a fatal error since processing continue, so you should use VIR_WARN() or something like that instead of virReportError() when that happens. Sure.
[...]
+int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) { You shouldn't need the second check: just go ahead and reserve the next address regardless of what's currently stored in the device info, no? I think it's hard to do it as what you said. We process assigned zpci addresses firstly. And then reserve next address for empty zpci addresses. If we don't check this, we might reserve an address for a reserved one.
[...]
+void +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + int extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)) You don't need two sets of parentheses here :)
yes. typo. -- Yi Min

On Mon, 2018-09-17 at 13:43 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午9:59, Andrea Bolognani 写道:
+static void +virDomainZPCIAddressReleaseUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (virHashRemoveEntry(set, &addr->uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release uid %u failed"), addr->uid); + }
You should have a generic virDomainZPCIAddressReleaseId() function that you call from ReleaseUid() and ReleaseFid(), just like you have for reserving them.
Actually there's the function ***ReleaseId() like you said. Don't you see it?
No such function exists; there is a virDomainZPCIAddressReleaseIds() but that doesn't do what I had in mind, which is along the lines of static void virDomainZPCIAddressReleaseId(virHashTablePtr set, unsigned int *id, const char *name) { if (virHashRemoveEntry(set, id) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Release %s %u failed"), name, *id); } *id = 0; } static void virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { virDomainZPCIAddressReleaseId(set, &addr->uid, "uid"); } static void virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { virDomainZPCIAddressReleaseId(set, &addr->fid, "fid"); }
+int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the next address regardless of what's currently stored in the device info, no?
I think it's hard to do it as what you said. We process assigned zpci addresses firstly. And then reserve next address for empty zpci addresses. If we don't check this, we might reserve an address for a reserved one.
Shouldn't the EnsureAddr() function take care of avoiding that? It will call ReserveAddr() or ReserveNextAddr() based on whether or not an address has already been provided by the user. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/17 下午8:05, Andrea Bolognani 写道:
On Mon, 2018-09-17 at 13:43 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午9:59, Andrea Bolognani 写道:
+static void +virDomainZPCIAddressReleaseUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (virHashRemoveEntry(set, &addr->uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release uid %u failed"), addr->uid); + } You should have a generic virDomainZPCIAddressReleaseId() function that you call from ReleaseUid() and ReleaseFid(), just like you have for reserving them. Actually there's the function ***ReleaseId() like you said. Don't you see it? No such function exists; there is a virDomainZPCIAddressReleaseIds() but that doesn't do what I had in mind, which is along the lines of
static void virDomainZPCIAddressReleaseId(virHashTablePtr set, unsigned int *id, const char *name) { if (virHashRemoveEntry(set, id) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Release %s %u failed"), name, *id); }
*id = 0; }
static void virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { virDomainZPCIAddressReleaseId(set, &addr->uid, "uid"); }
static void virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { virDomainZPCIAddressReleaseId(set, &addr->fid, "fid"); } Got it.
+int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) { You shouldn't need the second check: just go ahead and reserve the next address regardless of what's currently stored in the device info, no? I think it's hard to do it as what you said. We process assigned zpci addresses firstly. And then reserve next address for empty zpci addresses. If we don't check this, we might reserve an address for a reserved one. Shouldn't the EnsureAddr() function take care of avoiding that? It will call ReserveAddr() or ReserveNextAddr() based on whether or not an address has already been provided by the user.
IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also called in startup stage. After reserve defined addresses, RerserveNextAddr() is called to allocate addresses. Here, we should check if it's reserved. You could see virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) in qemuDomainAssignPCIAddresses(). -- Yi Min

On Tue, 2018-09-18 at 13:51 +0800, Yi Min Zhao wrote:
+int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the next address regardless of what's currently stored in the device info, no?
I think it's hard to do it as what you said. We process assigned zpci addresses firstly. And then reserve next address for empty zpci addresses. If we don't check this, we might reserve an address for a reserved one.
Shouldn't the EnsureAddr() function take care of avoiding that? It will call ReserveAddr() or ReserveNextAddr() based on whether or not an address has already been provided by the user.
IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also called in startup stage. After reserve defined addresses, RerserveNextAddr() is called to allocate addresses. Here, we should check if it's reserved. You could see virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) in qemuDomainAssignPCIAddresses().
Okay, qemuDomainAssignPCIAddresses() doesn't actually call virDomainPCIAddressEnsureAddr() but it's basically doing the same thing: after the first dry-run used to figure out how many PCI buses are necessary, it will call qemuDomainPCIAddressSetCreate() which internally calls qemuDomainCollectPCIAddress() on all devices, thus picking up all PCI addresses that were already provided by the user; then it calls qemuDomainAssignDevicePCISlots(), which calls qemuDomainPCIAddressReserveNextAddr() on all devices, but *only* after making sure with virDeviceInfoPCIAddressIsWanted() that they hadn't already been assigned an address. The end result is that qemuDomainPCIAddressReserveNextAddr() will only ever be called on devices that were not already assigned a PCI address, and thus virDomainPCIAddressReserveNextAddr() can afford to simply pick an address and reserve it without checking first whether the device in question had one already. To keep things easy to understand, your functions should follow the same semantics. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/18 下午4:59, Andrea Bolognani 写道:
+int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&dev->zpci)) { You shouldn't need the second check: just go ahead and reserve the next address regardless of what's currently stored in the device info, no? I think it's hard to do it as what you said. We process assigned zpci addresses firstly. And then reserve next address for empty zpci addresses. If we don't check this, we might reserve an address for a reserved one. Shouldn't the EnsureAddr() function take care of avoiding that? It will call ReserveAddr() or ReserveNextAddr() based on whether or not an address has already been provided by the user. IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also called in startup stage. After reserve defined addresses, RerserveNextAddr() is called to allocate addresses. Here, we should check if it's reserved. You could see virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) in qemuDomainAssignPCIAddresses(). Okay, qemuDomainAssignPCIAddresses() doesn't actually call virDomainPCIAddressEnsureAddr() but it's basically doing the same
On Tue, 2018-09-18 at 13:51 +0800, Yi Min Zhao wrote: thing: after the first dry-run used to figure out how many PCI buses are necessary, it will call qemuDomainPCIAddressSetCreate() which internally calls qemuDomainCollectPCIAddress() on all devices, thus picking up all PCI addresses that were already provided by the user; then it calls qemuDomainAssignDevicePCISlots(), which calls qemuDomainPCIAddressReserveNextAddr() on all devices, but *only* after making sure with virDeviceInfoPCIAddressIsWanted() that they hadn't already been assigned an address.
The end result is that qemuDomainPCIAddressReserveNextAddr() will only ever be called on devices that were not already assigned a PCI address, and thus virDomainPCIAddressReserveNextAddr() can afford to simply pick an address and reserve it without checking first whether the device in question had one already.
To keep things easy to understand, your functions should follow the same semantics.
Yeah, in my new version I have introduced a similar function like ***IsWanted(). I think it's same with what your said. -- Yi Min

Add new functions to generate zPCI command string and append it to QEMU command line. And the related tests are added. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 95 ++++++++++++++++++++++ src/qemu/qemu_command.h | 2 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 1 + .../hostdev-vfio-zpci-autogenerate.args | 25 ++++++ .../hostdev-vfio-zpci-autogenerate.xml | 18 ++++ .../hostdev-vfio-zpci-boundaries.args | 29 +++++++ .../hostdev-vfio-zpci-boundaries.xml | 26 ++++++ .../hostdev-vfio-zpci-multidomain-many.args | 39 +++++++++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 +++++++++++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 2 + tests/qemuxml2argvtest.c | 13 +++ .../hostdev-vfio-zpci-autogenerate.xml | 30 +++++++ .../hostdev-vfio-zpci-boundaries.xml | 42 ++++++++++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 ++++++++++++++++++ tests/qemuxml2xmltest.c | 11 +++ 15 files changed, 479 insertions(+) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8aa20496bc..afb25b1976 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2155,6 +2155,50 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, return NULL; } +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); + + if (virBufferCheckError(&buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +static int +qemuAppendZPCIDevStr(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) +{ + char *devstr = NULL; + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildZPCIDevStr(dev))) + return -1; + + virCommandAddArg(cmd, devstr); + + VIR_FREE(devstr); + return 0; +} + +static int +qemuBuildExtensionCommandLine(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) +{ + if (!virZPCIDeviceAddressIsEmpty(&dev->addr.pci.zpci)) + return qemuAppendZPCIDevStr(cmd, dev); + + return 0; +} static int qemuBuildFloppyCommandLineControllerOptions(virCommandPtr cmd, @@ -2391,6 +2435,9 @@ qemuBuildDiskCommandLine(virCommandPtr cmd, if (!qemuDiskBusNeedsDriveArg(disk->bus)) { if (disk->bus != VIR_DOMAIN_DISK_BUS_FDC || virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) { + if (qemuBuildExtensionCommandLine(cmd, &disk->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildDiskDeviceStr(def, disk, bootindex, @@ -2591,6 +2638,9 @@ qemuBuildFSDevCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, optstr); VIR_FREE(optstr); + if (qemuBuildExtensionCommandLine(cmd, &fs->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) return -1; @@ -3076,6 +3126,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, goto cleanup; if (devstr) { + if (qemuBuildExtensionCommandLine(cmd, &cont->info) < 0) { + VIR_FREE(devstr); + goto cleanup; + } virCommandAddArg(cmd, "-device"); virCommandAddArg(cmd, devstr); VIR_FREE(devstr); @@ -3880,6 +3934,9 @@ qemuBuildWatchdogCommandLine(virCommandPtr cmd, if (!def->watchdog) return 0; + if (qemuBuildExtensionCommandLine(cmd, &def->watchdog->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps); @@ -3964,6 +4021,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, if (qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio, qemuCaps) < 0) goto error; + if (qemuBuildExtensionCommandLine(cmd, &def->memballoon->info) < 0) + goto error; + virCommandAddArg(cmd, "-device"); virCommandAddArgBuffer(cmd, &buf); return 0; @@ -4186,6 +4246,9 @@ qemuBuildInputCommandLine(virCommandPtr cmd, virDomainInputDefPtr input = def->inputs[i]; char *devstr = NULL; + if (qemuBuildExtensionCommandLine(cmd, &input->info) < 0) + return -1; + if (qemuBuildInputDevStr(&devstr, def, input, qemuCaps) < 0) return -1; @@ -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; + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps))) return -1; @@ -4566,6 +4632,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, if (video->primary) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) { + if (qemuBuildExtensionCommandLine(cmd, + &def->videos[i]->info) < 0) + return -1; virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps))) @@ -4578,6 +4647,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, return -1; } } else { + if (qemuBuildExtensionCommandLine(cmd, &def->videos[i]->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps))) @@ -5449,6 +5521,10 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); } } + + if (qemuBuildExtensionCommandLine(cmd, hostdev->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); devstr = qemuBuildPCIHostdevDevStr(def, hostdev, bootIndex, configfd_name, qemuCaps); @@ -5915,6 +5991,9 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager, virCommandAddArgBuffer(cmd, &buf); /* add the device */ + if (qemuBuildExtensionCommandLine(cmd, &rng->info) < 0) + return -1; + if (!(tmp = qemuBuildRNGDevStr(def, rng, qemuCaps))) return -1; virCommandAddArgList(cmd, "-device", tmp, NULL); @@ -8385,6 +8464,9 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virCommandAddArg(cmd, "-netdev"); virCommandAddArg(cmd, netdev); + if (qemuBuildExtensionCommandLine(cmd, &net->info) < 0) + goto cleanup; + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, queues, qemuCaps))) { goto cleanup; @@ -8666,6 +8748,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, * New way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 */ if (qemuDomainSupportsNicdev(def, net)) { + if (qemuBuildExtensionCommandLine(cmd, &net->info) < 0) + goto cleanup; + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, vhostfdSize, qemuCaps))) goto cleanup; @@ -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; @@ -10287,6 +10378,10 @@ qemuBuildVsockCommandLine(virCommandPtr cmd, virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); priv->vhostfd = -1; + + if (qemuBuildExtensionCommandLine(cmd, &vsock->info) < 0) + goto cleanup; + virCommandAddArgList(cmd, "-device", devstr, NULL); ret = 0; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 98d4ac90b5..d382cd592a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -174,6 +174,8 @@ char *qemuBuildRedirdevDevStr(const virDomainDef *def, virDomainRedirdevDefPtr dev, virQEMUCapsPtr qemuCaps); +char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev); + int qemuNetworkPrepareDevices(virDomainDefPtr def); int qemuGetDriveSourceString(virStorageSourcePtr src, diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args index 8ac435cb3e..3daa8316b6 100644 --- a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args @@ -20,6 +20,7 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ +-device zpci,uid=25,fid=31,target=virtio-disk0,id=zpci25 \ -device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk0,\ id=virtio-disk0,bootindex=1 \ -device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args new file mode 100644 index 0000000000..4309cdf2be --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-device zpci,uid=1,fid=0,target=hostdev0,id=zpci1 \ +-device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ +-device zpci,uid=2,fid=1,target=balloon0,id=zpci2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml new file mode 100644 index 0000000000..36161006ab --- /dev/null +++ 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> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args new file mode 100644 index 0000000000..01b06837c8 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args @@ -0,0 +1,29 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-device zpci,uid=3,fid=2,target=pci.1,id=zpci3 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \ +-device zpci,uid=65535,fid=4294967295,target=hostdev0,id=zpci65535 \ +-device vfio-pci,host=ffff:00:00.0,id=hostdev0,bus=pci.1,addr=0x1f \ +-device zpci,uid=1,fid=0,target=hostdev1,id=zpci1 \ +-device vfio-pci,host=00:00.0,id=hostdev1,bus=pci.0,addr=0x2 \ +-device zpci,uid=2,fid=1,target=balloon0,id=zpci2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml new file mode 100644 index 0000000000..779eb12ac2 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml @@ -0,0 +1,26 @@ +<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> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0xffff' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x01' slot='0x1f' function='0x0' fid='0xffffffff' uid='0xffff'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' fid='0x00000000' uid='0x0001'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args new file mode 100644 index 0000000000..60b6a2e0d2 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args @@ -0,0 +1,39 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-device zpci,uid=35,fid=63,target=hostdev0,id=zpci35 \ +-device vfio-pci,host=0001:00:00.0,id=hostdev0,bus=pci.0,addr=0x3 \ +-device zpci,uid=53,fid=104,target=hostdev1,id=zpci53 \ +-device vfio-pci,host=0002:00:00.0,id=hostdev1,bus=pci.0,addr=0x1 \ +-device zpci,uid=1,fid=1,target=hostdev2,id=zpci1 \ +-device vfio-pci,host=0003:00:00.0,id=hostdev2,bus=pci.0,addr=0x2 \ +-device zpci,uid=2,fid=2,target=hostdev3,id=zpci2 \ +-device vfio-pci,host=0004:00:00.0,id=hostdev3,bus=pci.0,addr=0x5 \ +-device zpci,uid=83,fid=0,target=hostdev4,id=zpci83 \ +-device vfio-pci,host=0005:00:00.0,id=hostdev4,bus=pci.0,addr=0x7 \ +-device zpci,uid=3,fid=114,target=hostdev5,id=zpci3 \ +-device vfio-pci,host=0006:00:00.0,id=hostdev5,bus=pci.0,addr=0x9 \ +-device zpci,uid=23,fid=3,target=hostdev6,id=zpci23 \ +-device vfio-pci,host=0007:00:00.0,id=hostdev6,bus=pci.0,addr=0x4 \ +-device zpci,uid=4,fid=40,target=hostdev7,id=zpci4 \ +-device vfio-pci,host=0008:00:00.0,id=hostdev7,bus=pci.0,addr=0x6 \ +-device zpci,uid=5,fid=4,target=balloon0,id=zpci5 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml new file mode 100644 index 0000000000..a6c36e1f12 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml @@ -0,0 +1,67 @@ +<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> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0023' fid='0x0000003f'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' uid='0x0035' fid='0x00000068'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0' uid='0x0053'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0006' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' uid='0x0003' fid='0x00000072'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' uid='0x0017' fid='0x00000003'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' uid='0x0004' fid='0x00000028'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args index d6b1520c47..7ca4257a34 100644 --- a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args @@ -19,5 +19,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ +-device zpci,uid=25,fid=31,target=hostdev0,id=zpci25 \ -device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x8 \ +-device zpci,uid=1,fid=0,target=balloon0,id=zpci1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e96d00cec7..e69f196acc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1658,6 +1658,19 @@ mymain(void) DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-multidomain-many", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-boundaries", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST_PARSE_ERROR("hostdev-vfio-zpci", + QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml new file mode 100644 index 0000000000..8647cab1fc --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0001' fid='0x00000000'/> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0002' fid='0x00000001'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml new file mode 100644 index 0000000000..0b48c7658a --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml @@ -0,0 +1,42 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='1' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0xffff' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x01' slot='0x1f' function='0x0' uid='0xffff' fid='0xffffffff'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0001' fid='0x00000000'/> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0002' fid='0x00000001'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml new file mode 100644 index 0000000000..2197493393 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml @@ -0,0 +1,79 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0023' fid='0x0000003f'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0035' fid='0x00000068'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0001' fid='0x00000001'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0' uid='0x0002' fid='0x00000002'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0' uid='0x0053' fid='0x00000000'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0006' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' uid='0x0003' fid='0x00000072'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' uid='0x0017' fid='0x00000003'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' uid='0x0004' fid='0x00000028'/> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0005' fid='0x00000004'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 47f3b9431b..96820a1f6b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -466,6 +466,17 @@ mymain(void) DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW); + DO_TEST("hostdev-vfio-zpci-multidomain-many", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-boundaries", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); DO_TEST("hostdev-mdev-precreated", NONE); DO_TEST("hostdev-mdev-display", QEMU_CAPS_VFIO_PCI_DISPLAY); DO_TEST("pci-rom", NONE); -- Yi Min

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. [...]
+static int +qemuAppendZPCIDevStr(virCommandPtr cmd, + virDomainDeviceInfoPtr dev)
I'd call this qemuCommandAddZPCIDevice() or something like that.
+{ + char *devstr = NULL; + + virCommandAddArg(cmd, "-device");
Empty line here.
+ if (!(devstr = qemuBuildZPCIDevStr(dev))) + return -1;
[...]
+static int +qemuBuildExtensionCommandLine(virCommandPtr cmd, + virDomainDeviceInfoPtr dev)
Same comment about the naming as above.
+{ + 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. [...]
@@ -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 :) [...]
@@ -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. -- Andrea Bolognani / Red Hat / Virtualization

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
在 2018/9/11 下午10:31, Andrea Bolognani 写道: 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

On Mon, 2018-09-17 at 13:51 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午10:31, Andrea Bolognani 写道:
+{ + 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.
You can't really sensibly use a switch() for flags. Unless I have mistaken what you had in mind... -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/17 下午8:33, Andrea Bolognani 写道:
On Mon, 2018-09-17 at 13:51 +0800, Yi Min Zhao wrote:
+{ + 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
在 2018/9/11 下午10:31, Andrea Bolognani 写道: then build command for each case although there's only zpci case. You can't really sensibly use a switch() for flags. Unless I have mistaken what you had in mind...
Like below simple code, if (dev->type != PCI_TYPE || dev->addr.pci.extFlags == NONE) return 0; if (dev->addr.pci.extFlags & ZPCI_FLAG) return qemuAppendZPCIDevStr(cmd, dev); /* There might be new extensions in future. */ return 0; -- Yi Min

On Tue, 2018-09-18 at 15:40 +0800, Yi Min Zhao wrote:
在 2018/9/17 下午8:33, Andrea Bolognani 写道:
On Mon, 2018-09-17 at 13:51 +0800, Yi Min Zhao wrote:
How about using switch? I think extension flag should be check and then build command for each case although there's only zpci case.
You can't really sensibly use a switch() for flags. Unless I have mistaken what you had in mind...
Like below simple code,
if (dev->type != PCI_TYPE || dev->addr.pci.extFlags == NONE) return 0;
if (dev->addr.pci.extFlags & ZPCI_FLAG) return qemuAppendZPCIDevStr(cmd, dev);
/* There might be new extensions in future. */
Sure, that works. -- Andrea Bolognani / Red Hat / Virtualization

This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way. Currently, the following PCI devices are supported: virtio-blk-pci virtio-net-pci virtio-rng-pci virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci vfio-pci SCSIVhost device Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_hotplug.c | 151 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 141 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4f290b5648..65d25d776b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -154,6 +154,70 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, } +static int +qemuDomainAttachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + int ret = -1; + char *devstr_zpci = NULL; + + if (!(devstr_zpci = qemuBuildZPCIDevStr(info))) + goto cleanup; + + if (qemuMonitorAddDevice(mon, devstr_zpci) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(devstr_zpci); + return ret; +} + + +static int +qemuDomainDetachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + char *zpciAlias = NULL; + int ret = -1; + + if (virAsprintf(&zpciAlias, "zpci%d", info->addr.pci.zpci.uid) < 0) + goto cleanup; + + if (qemuMonitorDelDevice(mon, zpciAlias) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(zpciAlias); + return ret; +} + + +static int +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) + return qemuDomainAttachZPCIDevice(mon, info); + + return 0; +} + + +static int +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) + return qemuDomainDetachZPCIDevice(mon, info); + + return 0; +} + + static int qemuHotplugWaitForTrayEject(virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; @@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &controller->info)); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; ret = -1; @@ -1377,7 +1454,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - if (qemuDomainIsS390CCW(vm->def) && + if (net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + qemuDomainIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; if (!(ccwaddrs = virDomainCCWAddressSetCreateFromDomain(vm->def))) @@ -1447,7 +1525,15 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto try_remove; qemuDomainObjEnterMonitor(driver, vm); + + if (qemuDomainAttachExtensionDevice(priv->mon, &net->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto try_remove; + } + if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &net->info)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); goto try_remove; @@ -1665,8 +1751,15 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, goto error; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, - configfd, configfd_name); + + if ((ret = qemuDomainAttachExtensionDevice(priv->mon, hostdev->info)) < 0) + goto exit_monitor; + + if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info)); + + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) goto error; @@ -2322,8 +2415,13 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (qemuMonitorAddObject(priv->mon, &props, &objAlias) < 0) goto exit_monitor; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &rng->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &rng->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; @@ -2816,8 +2914,16 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, vhostfdName); + if ((ret = qemuDomainAttachExtensionDevice(priv->mon, hostdev->info)) < 0) + goto exit_monitor; + + if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, + vhostfdName)) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info)); + goto exit_monitor; + } + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) goto audit; @@ -3062,9 +3168,14 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, release_backing = true; - if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &shmem->info) < 0) goto exit_monitor; + if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &shmem->info)); + goto exit_monitor; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { release_address = false; goto cleanup; @@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) { + if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0) + goto exit_monitor; + } + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; @@ -3315,9 +3435,15 @@ qemuDomainAttachVsockDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname) < 0) + + if (qemuDomainAttachExtensionDevice(priv->mon, &vsock->info) < 0) goto exit_monitor; + if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &vsock->info)); + goto exit_monitor; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; goto cleanup; @@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); + if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; -- Yi Min

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
+static int +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) + return qemuDomainAttachZPCIDevice(mon, info); + + return 0;
Same comment as with qemuBuildExtensionCommandLine(). [...]
+static int +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) + return qemuDomainDetachZPCIDevice(mon, info); + + return 0;
Here, too. [...]
@@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; +
Since the zpci device refers to the underlying device through the target= option, does it make sense to attach the zpci device first and the target device second? I would expect it to fail unless you attach them the other way around... [...]
@@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + }
I'm not sure this is entirely safe. Perhaps you should introduce an exit_monitor label that ensures failure to exit the monitor is also taken into account, and jump to that one instead? [...]
+ if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
Parentheses around here, please. [...]
@@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) { + if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0) + goto exit_monitor; + } + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info)); goto exit_monitor; + }
Shouldn't you rather check for the address type here? IIUC an input device with BUS_VIRTIO could be virtio-ccw or virtio-mmio, for example, and you don't want to try adding a zpci device in those cases. [...]
@@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm); + if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup;
This one too looks like it would benefit from an exit_monitor lable. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/11 下午11:21, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote: [...]
+static int +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) + return qemuDomainAttachZPCIDevice(mon, info); + + return 0; Same comment as with qemuBuildExtensionCommandLine(). yes.
[...]
+static int +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) + return qemuDomainDetachZPCIDevice(mon, info); + + return 0; Here, too. I posted my idea in previous mail. Let's see that.
[...]
@@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; + Since the zpci device refers to the underlying device through the target= option, does it make sense to attach the zpci device first and the target device second? I would expect it to fail unless you attach them the other way around... This attaching order is right. Qemu requires that attach zpci first and target device second. In Qemu, zpci is created firstly and ready to build connection with target device, and after target device is plugged, there's a mapping built for zpci and target device. If the order is reversed, there would be error.
[...]
@@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } I'm not sure this is entirely safe. Perhaps you should introduce an exit_monitor label that ensures failure to exit the monitor is also taken into account, and jump to that one instead? I see there're a lot of code ignoring monitor exit failure. Of course, taking it into account is proper. Do you strongly suggest to add that?
[...]
+ if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info)); Parentheses around here, please. ok.
[...]
@@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) { + if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0) + goto exit_monitor; + } + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info)); goto exit_monitor; + } Shouldn't you rather check for the address type here? IIUC an input device with BUS_VIRTIO could be virtio-ccw or virtio-mmio, for example, and you don't want to try adding a zpci device in those cases. Yes, you're right. As my idea regarding using switch in attach***() and detach***(), we could move the check there.
[...]
@@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm); + if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; This one too looks like it would benefit from an exit_monitor lable.
Exactly. I will reorganise the code. -- Yi Min

On Mon, 2018-09-17 at 14:10 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午11:21, Andrea Bolognani 写道:
@@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; +
Since the zpci device refers to the underlying device through the target= option, does it make sense to attach the zpci device first and the target device second? I would expect it to fail unless you attach them the other way around...
This attaching order is right. Qemu requires that attach zpci first and target device second. In Qemu, zpci is created firstly and ready to build connection with target device, and after target device is plugged, there's a mapping built for zpci and target device. If the order is reversed, there would be error.
Cool, let's leave it as it is then.
[...]
@@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + }
I'm not sure this is entirely safe. Perhaps you should introduce an exit_monitor label that ensures failure to exit the monitor is also taken into account, and jump to that one instead?
I see there're a lot of code ignoring monitor exit failure. Of course, taking it into account is proper. Do you strongly suggest to add that?
As mentioned a while ago, I'm not particularly comfortable around this part of libvirt, so we should definitely have someone with more experience look it over before merging. That said, while there are indeed a few existing places where the return value of qemuDomainObjExitMonitor() is ignored, it seems like it's far more common to take it into account, so I would say do that unless you have a very good reason not to.
[...]
+ if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
Parentheses around here, please.
ok.
(Of course I meant curly braces, not parentheses :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/9/17 下午8:40, Andrea Bolognani 写道:
qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + }
I'm not sure this is entirely safe. Perhaps you should introduce an exit_monitor label that ensures failure to exit the monitor is also taken into account, and jump to that one instead? I see there're a lot of code ignoring monitor exit failure. Of course, taking it into account is proper. Do you strongly suggest to add that? As mentioned a while ago, I'm not particularly comfortable around this part of libvirt, so we should definitely have someone with more experience look it over before merging.
That said, while there are indeed a few existing places where the return value of qemuDomainObjExitMonitor() is ignored, it seems like it's far more common to take it into account, so I would say do that unless you have a very good reason not to.
I will take it into account in next version. -- Yi Min

Update 'Device address' section to describe the 'uid' and 'fid' attributes. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb619a1656..52dc3f688f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3921,7 +3921,14 @@ (<span class="since">since 0.9.7, requires QEMU 0.13</span>). <code>multifunction</code> defaults to 'off', but should be set to 'on' for function 0 of a slot that will - have multiple functions used.<br/> + have multiple functions used. (<span class="since">Since 4.8.0 + </span>), PCI address extensions depending on the architecture + are supported. E.g. for S390 guests, PCI addresses have + additional attributes: <code>uid</code> (a hex value between + 0x0001 and 0xffff, inclusive), and <code>fid</code> (a hex + value between 0x00000000 and 0xffffffff, inclusive), which are + used by PCI devices on S390 for User-defined Identifiers and + Function Identifiers.<br/> <span class="since">Since 1.3.5</span>, some hypervisor drivers may accept an <code><address type='pci'/></code> element with no other attributes as an explicit request to -- Yi Min

Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 9a17b2f612..fc54a3b6ff 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,16 @@ <libvirt> <release version="v4.8.0" date="unreleased"> <section title="New features"> + <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + PCI addresses can now include the new uid (user-defined identifier) + and fid (PCI function identifier) attributes, which make the + corresponding devices usable by S390 guests. + </description> + </change> </section> <section title="Improvements"> </section> -- Yi Min
participants (2)
-
Andrea Bolognani
-
Yi Min Zhao