[libvirt] [PATCH v6 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, a new element of PCI address is introduced. It has two optional attributes, @uid and @fid. For example: <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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev> 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. zPCI address as an extension of the PCI address 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 zPCI 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: b1a0f691ce4652fc71aca6776f085355040322da spec: Build ceph and gluster support everywhere Change Log ========== v5->v6: 1. Modify zPCI XML definition. 2. Optimize the logic of zPCI address assignment and reservation. 3. Add extension flag into PCI address structure. 4. Update commit messages. 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 | 11 + docs/schemas/basictypes.rng | 27 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 90 +++++ src/conf/device_conf.h | 7 + src/conf/domain_addr.c | 332 ++++++++++++++++++ src/conf/domain_addr.h | 30 ++ src/conf/domain_conf.c | 14 +- src/libvirt_private.syms | 7 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 102 ++++++ src/qemu/qemu_command.h | 2 + src/qemu/qemu_domain.c | 39 ++ src/qemu/qemu_domain_address.c | 204 ++++++++++- src/qemu/qemu_hotplug.c | 162 ++++++++- src/util/virpci.c | 7 + src/util/virpci.h | 14 + .../caps_2.10.0.s390x.xml | 1 + .../caps_2.11.0.s390x.xml | 1 + .../caps_2.12.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + .../disk-virtio-s390-zpci.args | 26 ++ .../disk-virtio-s390-zpci.xml | 19 + .../hostdev-vfio-zpci-autogenerate.args | 25 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 + .../hostdev-vfio-zpci-boundaries.args | 29 ++ .../hostdev-vfio-zpci-boundaries.xml | 30 ++ .../hostdev-vfio-zpci-multidomain-many.args | 39 ++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 25 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 21 ++ tests/qemuxml2argvtest.c | 20 ++ .../disk-virtio-s390-zpci.xml | 31 ++ .../hostdev-vfio-zpci-autogenerate.xml | 34 ++ .../hostdev-vfio-zpci-boundaries.xml | 48 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 97 +++++ .../qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 32 ++ tests/qemuxml2xmltest.c | 17 + 44 files changed, 1648 insertions(+), 15 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> --- cfg.mk | 1 + src/util/virpci.h | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/cfg.mk b/cfg.mk index 4790d0b7e7..1fc6a2dabb 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 16c2eded5e..4cc9298d85 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 Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ + virZPCIDeviceAddress zpci; };
It's kinda weird that we add the zPCI part right away but the flags which should be used to figure out whether the zPCI part should be taken into account only later, but I can live with that :) Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/11 下午3:17, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ + virZPCIDeviceAddress zpci; }; It's kinda weird that we add the zPCI part right away but the flags which should be used to figure out whether the zPCI part should be taken into account only later, but I can live with that :)
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks! -- Yi Min

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 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + 9 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e228f52ec0..ee6a7462f6 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", ); @@ -1092,6 +1093,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 934620ed31..cd417c9062 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 e000aac384..3c311042f3 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -113,6 +113,7 @@ <flag name='blockdev-del'/> <flag name='vhost-vsock'/> <flag name='egl-headless'/> + <flag name='zpci'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>306247</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index 4eb8a39d94..48db1dbf2d 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -120,6 +120,7 @@ <flag name='vhost-vsock'/> <flag name='tpm-emulator'/> <flag name='egl-headless'/> + <flag name='zpci'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>345099</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 79320d5229..4c561f6214 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -128,6 +128,7 @@ <flag name='tpm-emulator'/> <flag name='egl-headless'/> <flag name='vfio-pci.display'/> + <flag name='zpci'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>374287</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml index b30c31cafc..de87692857 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml @@ -100,6 +100,7 @@ <flag name='nbd-tls'/> <flag name='virtual-css-bridge'/> <flag name='sdl-gl'/> + <flag name='zpci'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>219140</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml index b010f731a5..f3a32ad376 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -103,6 +103,7 @@ <flag name='virtual-css-bridge'/> <flag name='sdl-gl'/> <flag name='vhost-vsock'/> + <flag name='zpci'/> <version>2007093</version> <kvmVersion>0</kvmVersion> <microcodeVersion>244554</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index 5a4371ab83..f1e05ab1c4 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -107,6 +107,7 @@ <flag name='sdl-gl'/> <flag name='blockdev-del'/> <flag name='vhost-vsock'/> + <flag name='zpci'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>267973</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml index 3b5f9818a5..c841030b2b 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml @@ -130,6 +130,7 @@ <flag name='tpm-emulator'/> <flag name='egl-headless'/> <flag name='vfio-pci.display'/> + <flag name='zpci'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>387601</microcodeVersion> -- Yi Min

This patch introduces PCI address extension flag for virDomainDeviceInfo and virPCIDeviceAddress. The extension flag in virDomainDeviceInfo is used internally during calculating PCI extension flag. The one in virPCIDeviceAddress is the duplicate to indicate extension address is being used. Currently only zPCI extension address is introduced to deal with '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 | 4 + src/conf/domain_addr.h | 5 ++ src/qemu/qemu_domain_address.c | 138 ++++++++++++++++++++++++++++++++- src/util/virpci.h | 1 + 4 files changed, 146 insertions(+), 2 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 407956bd02..1ee6905024 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -166,6 +166,10 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + /* pciAddrExtFlags is only used interanlly to calculate PCI + * address extension flag before address assignment. + */ + int pciAddrExtFlags; /* 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 2a9af9c00b..b8525e1403 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 */ +} virPCIDeviceAddressExtensionFlags; + 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 8a8764cff5..f8241bed3a 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 virPCIDeviceAddressExtensionFlags +qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCapsPtr qemuCaps, + virDomainDeviceDefPtr dev) +{ + virPCIDeviceAddressExtensionFlags extFlags = 0; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + qemuDomainDeviceSupportZPCI(dev)) { + extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI; + } + + return extFlags; +} + + /** * qemuDomainDeviceCalculatePCIConnectFlags: * @@ -990,6 +1048,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->pciAddrExtFlags = + 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 @@ -1264,6 +1372,25 @@ 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) +{ + info->pciAddrExtFlags = + qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); +} + + static int qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) @@ -2400,6 +2527,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 +2565,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, */ virDomainDeviceInfo info = { .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCI_DEVICE) + VIR_PCI_CONNECT_TYPE_PCI_DEVICE), + .pciAddrExtFlags = VIR_PCI_ADDRESS_EXTENSION_NONE }; bool buses_reserved = true; @@ -2472,7 +2603,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), + .pciAddrExtFlags = VIR_PCI_ADDRESS_EXTENSION_NONE }; /* if there isn't an empty pcie-root-port, this will @@ -2989,6 +3121,8 @@ qemuDomainEnsurePCIAddress(virDomainObjPtr obj, qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps, driver); + qemuDomainFillDevicePCIExtensionFlags(dev, info, priv->qemuCaps); + return virDomainPCIAddressEnsureAddr(priv->pciaddrs, info, info->pciConnectFlags); } diff --git a/src/util/virpci.h b/src/util/virpci.h index 4cc9298d85..0e40d86b97 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -50,6 +50,7 @@ struct _virPCIDeviceAddress { unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ + int extFlags; /* enum virPCIDeviceAddressExtensionFlags */ virZPCIDeviceAddress zpci; }; -- Yi Min

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -166,6 +166,10 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + /* pciAddrExtFlags is only used interanlly to calculate PCI + * address extension flag before address assignment. + */
s/interanlly/internally/ s/flag before/flags during/ [...]
+typedef enum { + VIR_PCI_ADDRESS_EXTENSION_NONE = 0, /* no extension */ + VIR_PCI_ADDRESS_EXTENSION_ZPCI = 1 << 0, /* zpci support */
s/zpci/zPCI/ [...]
+static bool +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) {
No space after the cast, please. This would ideally have been caught by 'make syntax-check' but currently that's not the case (see [1]). [...]
+static void +qemuDomainFillDevicePCIExtensionFlags(virDomainDeviceDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + info->pciAddrExtFlags = + qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev);
This will not build because 'info' is not defined: you need to either pass it to the function or obtain it from 'dev' using virDomainDeviceGetInfo(). qemuDomainFillDevicePCIConnectFlags() is doing the latter, but you seem to be going for the former since... [...]
@@ -2989,6 +3121,8 @@ qemuDomainEnsurePCIAddress(virDomainObjPtr obj,
qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps, driver);
+ qemuDomainFillDevicePCIExtensionFlags(dev, info, priv->qemuCaps);
... you're passing it to the function here, which again the compiler very understandably complains about. With the above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> [1] https://www.redhat.com/archives/libvir-list/2018-October/msg00641.html -- Andrea Bolognani / Red Hat / Virtualization

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -166,6 +166,10 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + /* pciAddrExtFlags is only used interanlly to calculate PCI + * address extension flag before address assignment. + */ s/interanlly/internally/ s/flag before/flags during/
[...]
+typedef enum { + VIR_PCI_ADDRESS_EXTENSION_NONE = 0, /* no extension */ + VIR_PCI_ADDRESS_EXTENSION_ZPCI = 1 << 0, /* zpci support */ s/zpci/zPCI/
[...]
在 2018/10/11 下午4:49, Andrea Bolognani 写道: thanks.
+static bool +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { No space after the cast, please.
This would ideally have been caught by 'make syntax-check' but currently that's not the case (see [1]). Got it.
[...]
+static void +qemuDomainFillDevicePCIExtensionFlags(virDomainDeviceDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + info->pciAddrExtFlags = + qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); This will not build because 'info' is not defined: you need to either pass it to the function or obtain it from 'dev' using virDomainDeviceGetInfo().
qemuDomainFillDevicePCIConnectFlags() is doing the latter, but you seem to be going for the former since...
[...]
@@ -2989,6 +3121,8 @@ qemuDomainEnsurePCIAddress(virDomainObjPtr obj,
qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps, driver);
+ qemuDomainFillDevicePCIExtensionFlags(dev, info, priv->qemuCaps); ... you're passing it to the function here, which again the compiler very understandably complains about. There might be something I do wrong. Have been updated. Thanks for your comments.
With the above addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Thanks!
[1] https://www.redhat.com/archives/libvir-list/2018-October/msg00641.html
-- Yi Min

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 ee6a7462f6..bf1b90875b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1700,6 +1700,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 939b2a3da2..fa9113e542 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3288,6 +3288,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 | 13 ++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 +++ 4 files changed, 106 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 442e6aab94..deb58fa7c9 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) { @@ -727,6 +739,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, + virPCIDeviceAddressExtensionFlags 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) { @@ -753,6 +837,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 b8525e1403..c87e30786a 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -116,6 +116,12 @@ typedef struct { } virDomainPCIAddressBus; typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr; +typedef struct { + virHashTablePtr uids; + virHashTablePtr fids; +} virDomainZPCIAddressIds; +typedef virDomainZPCIAddressIds *virDomainZPCIAddressIdsPtr; + struct _virDomainPCIAddressSet { virDomainPCIAddressBus *buses; size_t nbuses; @@ -125,10 +131,17 @@ 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; +char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) + ATTRIBUTE_NONNULL(1); + +int virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressExtensionFlags extFlags); + virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses); void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 92363913e3..b662d2f01e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -124,6 +124,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 f8241bed3a..90f2afadcd 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1509,6 +1509,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 Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
+static void +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) +{ + if (!addrs || !addrs->zpciIds) + return; + + virHashFree(addrs->zpciIds->uids); + virHashFree(addrs->zpciIds->fids); + VIR_FREE(addrs->zpciIds); +}
Please keep the Free() function closer to the Alloc() function. [...]
+int +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressExtensionFlags 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)))
The function names are inconsistent here: they all deal with hash keys, but only the free function contains the word "key" in its name. Please change them like so: virZPCIAddrCode() => virZPCIAddrKeyCode() virZPCIAddrEqual() => virZPCIAddrKeyEqual() virZPCIAddrCopy() => virZPCIAddrKeyCopy() [...]
typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
+char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) + ATTRIBUTE_NONNULL(1);
This hunk is a leftover from a previous respin, please drop it. [...]
@@ -124,6 +124,7 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; +virDomainPCIAddressSetExtensionAlloc;
Hm. Instead of exporting this function just so you can call it from qemuDomainPCIAddressSetCreate(), wouldn't it make more sense to call it directly from inside virDomainPCIAddressSetAlloc()? You'd have to pass virPCIDeviceAddressExtensionFlags to the latter, of course, but that sounds fairly sensible; the only other user, the bhyve driver, can just pass _NONE. [...]
@@ -1509,6 +1509,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)) {
This should be if (... && virDomainPCIAddressSetExtensionAlloc(...) < 0) { ... With all of the above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/11 下午6:31, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
+static void +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) +{ + if (!addrs || !addrs->zpciIds) + return; + + virHashFree(addrs->zpciIds->uids); + virHashFree(addrs->zpciIds->fids); + VIR_FREE(addrs->zpciIds); +} Please keep the Free() function closer to the Alloc() function. OK.
[...]
+int +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressExtensionFlags 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))) The function names are inconsistent here: they all deal with hash keys, but only the free function contains the word "key" in its name.
Please change them like so:
virZPCIAddrCode() => virZPCIAddrKeyCode() virZPCIAddrEqual() => virZPCIAddrKeyEqual() virZPCIAddrCopy() => virZPCIAddrKeyCopy() Sure. [...]
typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
+char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) + ATTRIBUTE_NONNULL(1); This hunk is a leftover from a previous respin, please drop it. ok.
[...]
@@ -124,6 +124,7 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; +virDomainPCIAddressSetExtensionAlloc; Hm. Instead of exporting this function just so you can call it from qemuDomainPCIAddressSetCreate(), wouldn't it make more sense to call it directly from inside virDomainPCIAddressSetAlloc()?
You'd have to pass virPCIDeviceAddressExtensionFlags to the latter, of course, but that sounds fairly sensible; the only other user, the bhyve driver, can just pass _NONE. Yeah. [...]
@@ -1509,6 +1509,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)) { This should be
if (... && virDomainPCIAddressSetExtensionAlloc(...) < 0) { ... OK
With all of the above addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks! -- Yi Min

This patch introduces new XML parser/formatter functions. Uid is 16-bit and non-zero. Fid is 32-bit. They are the two attributes of zpci which is introduced as PCI address element. Zpci element is parsed and formatted along with PCI address. And add the related test cases. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> --- docs/schemas/basictypes.rng | 27 +++++++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 74 +++++++++++++++++++ src/conf/domain_addr.c | 3 + src/conf/domain_conf.c | 14 +++- src/libvirt_private.syms | 1 + src/util/virpci.c | 7 ++ src/util/virpci.h | 5 ++ .../disk-virtio-s390-zpci.args | 25 +++++++ .../disk-virtio-s390-zpci.xml | 19 +++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 23 ++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 21 ++++++ tests/qemuxml2argvtest.c | 7 ++ .../disk-virtio-s390-zpci.xml | 31 ++++++++ .../qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 32 ++++++++ tests/qemuxml2xmltest.c | 6 ++ 16 files changed, 295 insertions(+), 1 deletion(-) 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..71a6db3bb4 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,22 @@ </attribute> </optional> </define> + <define name="zpciaddress"> + <optional> + <element name="zpci"> + <optional> + <attribute name="uid"> + <ref name="uint16"/> + </attribute> + </optional> + <optional> + <attribute name="fid"> + <ref name="uint32"/> + </attribute> + </optional> + </element> + </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 099a949cf8..9eeae80452 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 98a419f40f..d9b2c8f477 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -47,6 +47,65 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "dimm", ); +static bool +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%.4x', " + "must be > 0x0000 and <= 0x%.4x"), + zpci->uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); + return false; + } + + return true; +} + +static int +virZPCIDeviceAddressParseXML(xmlNodePtr node, + virPCIDeviceAddressPtr addr) +{ + virZPCIDeviceAddress def = { 0 }; + char *uid; + char *fid; + int ret = -1; + + uid = virXMLPropString(node, "uid"); + fid = virXMLPropString(node, "fid"); + + if (uid && + virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'uid' attribute")); + goto cleanup; + } + + if (fid && + 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) @@ -181,6 +240,8 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr) { char *domain, *slot, *bus, *function, *multi; + xmlNodePtr cur; + xmlNodePtr zpci = NULL; int ret = -1; memset(addr, 0, sizeof(*addr)); @@ -227,9 +288,22 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, goto cleanup; } + if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup; + cur = node->children; + while (cur) { + if (cur->type == XML_ELEMENT_NODE && + virXMLNodeNameEqual(cur, "zpci")) { + zpci = cur; + } + cur = cur->next; + } + + if (zpci && virZPCIDeviceAddressParseXML(zpci, addr) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index deb58fa7c9..7e5ce48b3f 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1034,6 +1034,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, false) < 0) return -1; + addr.extFlags = dev->addr.pci.extFlags; + addr.zpci = dev->addr.pci.zpci; + if (!addrs->dryRun) { dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; dev->addr.pci = addr; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9911d56130..6dfa0aef27 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6528,7 +6528,19 @@ virDomainDeviceInfoFormat(virBufferPtr buf, break; } - virBufferAddLit(buf, "/>\n"); + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, + "<zpci uid='0x%.4x' fid='0x%.8x'/>\n", + info->addr.pci.zpci.uid, + info->addr.pci.zpci.fid); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</address>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } } static int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b662d2f01e..64f894ef74 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2566,6 +2566,7 @@ virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; +virZPCIDeviceAddressIsEmpty; # util/virperf.h diff --git a/src/util/virpci.c b/src/util/virpci.c index 0f680efbe6..8fdbad3b98 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -3236,3 +3236,10 @@ virPCIDeviceAddressFree(virPCIDeviceAddressPtr address) { VIR_FREE(address); } + + +bool +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +{ + return !(addr->uid || addr->fid); +} diff --git a/src/util/virpci.h b/src/util/virpci.h index 0e40d86b97..ea35234d7c 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 { @@ -272,4 +275,6 @@ VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree) VIR_DEFINE_AUTOPTR_FUNC(virPCIDeviceAddress, virPCIDeviceAddressFree) VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree) +bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr); + #endif /* __VIR_PCI_H__ */ 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..8bf4a23670 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml @@ -0,0 +1,19 @@ +<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'> + <zpci uid='0x0019' fid='0x0000001f'/> + </address> + </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..002b99c52d --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml @@ -0,0 +1,21 @@ +<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'> + <zpci uid='0x0019' fid='0x0000001f'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a7cde3ed7e..7ecd9342bc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1076,6 +1076,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); @@ -1667,6 +1671,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..37684c82b1 --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml @@ -0,0 +1,31 @@ +<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'> + <zpci uid='0x0019' fid='0x0000001f'/> + </address> + </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..fc8c38ab66 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml @@ -0,0 +1,32 @@ +<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'> + <zpci uid='0x0019' fid='0x0000001f'/> + </address> + </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 89640f641a..53de9726f4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -376,6 +376,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", @@ -458,6 +461,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 Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
This patch introduces new XML parser/formatter functions. Uid is 16-bit and non-zero. Fid is 32-bit. They are the two attributes of zpci which is introduced as PCI address element. Zpci element is parsed and formatted along with PCI address. And add the related test cases.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> ---
No internal reviews this time around? :) [...]
@@ -227,9 +288,22 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, goto cleanup;
} + if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup;
Spurious whitespace change. [...]
@@ -6528,7 +6528,19 @@ virDomainDeviceInfoFormat(virBufferPtr buf, break; }
- virBufferAddLit(buf, "/>\n"); + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, + "<zpci uid='0x%.4x' fid='0x%.8x'/>\n", + info->addr.pci.zpci.uid, + info->addr.pci.zpci.fid); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</address>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + }
This looks like a perfect candidate for virXMLFormatElement(). You should probably convert the original code to use that function in a separate commit, then start actually using a child buffer here. [...]
@@ -2566,6 +2566,7 @@ virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; +virZPCIDeviceAddressIsEmpty;
You can move virZPCIDeviceAddressIsValid() to util/virpci and export it too, to be consistent with virPCIDeviceAddress*(). [...]
@@ -272,4 +275,6 @@ VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree) VIR_DEFINE_AUTOPTR_FUNC(virPCIDeviceAddress, virPCIDeviceAddressFree) VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree)
+bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr); +
Please place this further up in the file, eg. after virPCIDeviceAddressParse(). Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/11 下午7:45, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
This patch introduces new XML parser/formatter functions. Uid is 16-bit and non-zero. Fid is 32-bit. They are the two attributes of zpci which is introduced as PCI address element. Zpci element is parsed and formatted along with PCI address. And add the related test cases.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> --- No internal reviews this time around? :) Yes, I just want to quickly know if this change is exact.
[...]
@@ -227,9 +288,22 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, goto cleanup;
} + if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup; Spurious whitespace change. Oh...good catch.
[...]
@@ -6528,7 +6528,19 @@ virDomainDeviceInfoFormat(virBufferPtr buf, break; }
- virBufferAddLit(buf, "/>\n"); + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, + "<zpci uid='0x%.4x' fid='0x%.8x'/>\n", + info->addr.pci.zpci.uid, + info->addr.pci.zpci.fid); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</address>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } This looks like a perfect candidate for virXMLFormatElement(). You should probably convert the original code to use that function in a separate commit, then start actually using a child buffer here. Sure.
[...]
@@ -2566,6 +2566,7 @@ virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; +virZPCIDeviceAddressIsEmpty; You can move virZPCIDeviceAddressIsValid() to util/virpci and export it too, to be consistent with virPCIDeviceAddress*(). It has been moved to util/virpci. Isn't it?
[...]
@@ -272,4 +275,6 @@ VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree) VIR_DEFINE_AUTOPTR_FUNC(virPCIDeviceAddress, virPCIDeviceAddressFree) VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree)
+bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr); + Please place this further up in the file, eg. after virPCIDeviceAddressParse(). Sure.
Everything else looks good.
-- Yi Min

On Tue, 2018-10-16 at 11:12 +0800, Yi Min Zhao wrote:
在 2018/10/11 下午7:45, Andrea Bolognani 写道:
You can move virZPCIDeviceAddressIsValid() to util/virpci and export it too, to be consistent with virPCIDeviceAddress*().
It has been moved to util/virpci. Isn't it?
No, it's in conf/device_conf and private to that file. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/16 下午11:38, Andrea Bolognani 写道:
On Tue, 2018-10-16 at 11:12 +0800, Yi Min Zhao wrote:
在 2018/10/11 下午7:45, Andrea Bolognani 写道:
You can move virZPCIDeviceAddressIsValid() to util/virpci and export it too, to be consistent with virPCIDeviceAddress*(). It has been moved to util/virpci. Isn't it? No, it's in conf/device_conf and private to that file.
Oh, yes, this patch series is different. -- Yi Min

We should ensure that the QEMU should support zPCI when zPCI address is defined in XML. Otherwise the error should be reported. This patch introduces a generic validation function qemuDomainDeviceDefValidateAddress() which calls qemuDomainDeviceDefValidateZPCIAddress() if address type is PCI address. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> --- src/qemu/qemu_domain.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fa9113e542..94a14c582b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5798,6 +5798,39 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input, } +static int +qemuDomainDeviceDefValidateZPCIAddress(virDomainDeviceInfoPtr info, + virQEMUCapsPtr qemuCaps) +{ + 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 +qemuDomainDeviceDefValidateAddress(const virDomainDeviceDef *dev, + virQEMUCapsPtr qemuCaps) +{ + virDomainDeviceInfoPtr info = + virDomainDeviceGetInfo((virDomainDeviceDef *)dev); + + if (!info) + return 0; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return qemuDomainDeviceDefValidateZPCIAddress(info, qemuCaps); + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -5811,6 +5844,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, def->emulator))) return -1; + ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps); + if (ret < 0) + goto out; + switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_NET: ret = qemuDomainDeviceDefValidateNetwork(dev->data.net); @@ -5886,6 +5923,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; } + out: virObjectUnref(qemuCaps); return ret; } -- Yi Min

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
+static int +qemuDomainDeviceDefValidateAddress(const virDomainDeviceDef *dev, + virQEMUCapsPtr qemuCaps) +{ + virDomainDeviceInfoPtr info = + virDomainDeviceGetInfo((virDomainDeviceDef *)dev); + + if (!info) + return 0;
Using virDomainDeviceInfoPtr info; if (!(info = virDomainDeviceGetInfo((virDomainDeviceDef *)dev))) return 0; here would look much better. [...]
@@ -5811,6 +5844,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, def->emulator))) return -1;
+ ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps); + if (ret < 0) + goto out;
This could be if ((ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps)) < 0) ... [...]
@@ -5886,6 +5923,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; }
+ out: virObjectUnref(qemuCaps); return ret;
'cleanup' would be a more appropriate name for the label since you're releasing resources when you reach it. With the above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

On Thu, 2018-10-11 at 14:44 +0200, Andrea Bolognani wrote: [...]
With the above addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Forgot to mention: it would be really nice if you added a negative test case showing that using zPCI addresses on eg. x86 will result in a validation error. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/11 下午9:08, Andrea Bolognani 写道:
On Thu, 2018-10-11 at 14:44 +0200, Andrea Bolognani wrote: [...]
With the above addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Forgot to mention: it would be really nice if you added a negative test case showing that using zPCI addresses on eg. x86 will result in a validation error.
OK. Let me have a try. Haven't added this kind of test before. -- Yi Min

On Mon, 2018-10-15 at 09:31 +0800, Yi Min Zhao wrote:
在 2018/10/11 下午9:08, Andrea Bolognani 写道:
Forgot to mention: it would be really nice if you added a negative test case showing that using zPCI addresses on eg. x86 will result in a validation error.
OK. Let me have a try. Haven't added this kind of test before.
It's very easy: just use DO_TEST_PARSE_ERROR() or DO_TEST_FAILURE() instead of DO_TEST() :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/15 下午2:59, Andrea Bolognani 写道:
On Mon, 2018-10-15 at 09:31 +0800, Yi Min Zhao wrote:
在 2018/10/11 下午9:08, Andrea Bolognani 写道:
Forgot to mention: it would be really nice if you added a negative test case showing that using zPCI addresses on eg. x86 will result in a validation error. OK. Let me have a try. Haven't added this kind of test before. It's very easy: just use DO_TEST_PARSE_ERROR() or DO_TEST_FAILURE() instead of DO_TEST() :)
Yes, I have found sample code. Thanks! -- Yi Min

在 2018/10/11 下午8:44, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
+static int +qemuDomainDeviceDefValidateAddress(const virDomainDeviceDef *dev, + virQEMUCapsPtr qemuCaps) +{ + virDomainDeviceInfoPtr info = + virDomainDeviceGetInfo((virDomainDeviceDef *)dev); + + if (!info) + return 0; Using
virDomainDeviceInfoPtr info;
if (!(info = virDomainDeviceGetInfo((virDomainDeviceDef *)dev))) return 0;
here would look much better.
[...]
@@ -5811,6 +5844,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, def->emulator))) return -1;
+ ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps); + if (ret < 0) + goto out; This could be
if ((ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps)) < 0) ...
[...]
@@ -5886,6 +5923,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; }
+ out: virObjectUnref(qemuCaps); return ret; 'cleanup' would be a more appropriate name for the label since you're releasing resources when you reach it.
With the above addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks! Will update the code as your comments. -- 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 zPCI address is empty, 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. Whether using zPCI address refers to extension flags in PCIDeviceAddress. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> --- src/conf/device_conf.c | 16 +++ src/conf/device_conf.h | 3 + src/conf/domain_addr.c | 244 +++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 12 ++ src/libvirt_private.syms | 5 + src/qemu/qemu_domain_address.c | 59 +++++++- 6 files changed, 338 insertions(+), 1 deletion(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index d9b2c8f477..042c6f097b 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -28,6 +28,7 @@ #include "viruuid.h" #include "virbuffer.h" #include "device_conf.h" +#include "domain_addr.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_DEVICE @@ -235,6 +236,21 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info) } +bool +virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info) +{ + return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci); +} + +bool +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) +{ + return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci); +} + + int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 1ee6905024..c93615567a 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -198,6 +198,9 @@ bool virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, bool virDeviceInfoPCIAddressIsWanted(const virDomainDeviceInfo *info); bool virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info); +bool virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info); +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 7e5ce48b3f..8e033b23d0 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -33,6 +33,157 @@ 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 %o is already reserved"), + name, id); + return -1; + } + + if (virHashAddEntry(set, &id, (void*)1) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to reserve %s %o"), + 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 +virDomainZPCIAddressReleaseId(virHashTablePtr set, + unsigned int *id, + const char *name) +{ + if (virHashRemoveEntry(set, id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release %s %o 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"); +} + + +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 +195,87 @@ 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) +{ + if (addr->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 addr) +{ + if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + virZPCIDeviceAddress zpci = { 0 }; + + if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0) + return -1; + + if (!addrs->dryRun) + addr->zpci = zpci; + } + + return 0; +} + +static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr) +{ + if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + virZPCIDeviceAddressPtr zpci = &addr->zpci; + + if (virZPCIDeviceAddressIsEmpty(zpci)) + return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci); + else + return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci); + } + + return 0; +} + virDomainPCIConnectFlags virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) { @@ -726,12 +958,24 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, ret = virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1); } + dev->addr.pci.extFlags = dev->pciAddrExtFlags; + ret = virDomainPCIAddressExtensionEnsureAddr(addrs, &dev->addr.pci); + cleanup: VIR_FREE(addrStr); return ret; } +void +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr) +{ + if (addr->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 c87e30786a..b60d1dd7ec 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -165,6 +165,14 @@ bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int virDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr, virDomainPCIConnectFlags flags, @@ -186,6 +194,10 @@ void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr) + 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 64f894ef74..27bf7303f3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -93,6 +93,8 @@ virCPUModeTypeToString; # conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; 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 90f2afadcd..eaa528147f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1400,6 +1400,24 @@ 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; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + addr->extFlags = info->pciAddrExtFlags; + + if (virDeviceInfoPCIAddressExtensionIsWanted(info)) + return virDomainPCIAddressExtensionReserveNextAddr(addrs, addr); + + return 0; +} + static int qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceDefPtr device, @@ -1493,6 +1511,31 @@ 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 (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + addr->extFlags = info->pciAddrExtFlags; + + 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); +} + static virDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -1587,6 +1630,12 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, addrs) < 0) goto error; + if (virDomainDeviceInfoIterate(def, + qemuDomainCollectPCIAddressExtension, + addrs) < 0) { + goto error; + } + return addrs; error: @@ -2591,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, @@ -2685,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 */ @@ -3144,8 +3199,10 @@ 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); + } if (virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) VIR_WARN("Unable to release USB address on %s", NULLSTR(devstr)); -- Yi Min

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
# conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr;
I'm not quite quire we need to export these functions. With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress... So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something? Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/11 下午10:50, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
# conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr; I'm not quite quire we need to export these functions.
With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress...
So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something? I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better.
Everything else looks good.
-- Yi Min

On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote:
在 2018/10/11 下午10:50, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
# conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr;
I'm not quite quire we need to export these functions.
With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress...
So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something?
I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better.
Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/17 下午10:30, Andrea Bolognani 写道:
On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote:
在 2018/10/11 下午10:50, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
# conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr; I'm not quite quire we need to export these functions.
With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress...
So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something? I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better. Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself.
For example, during collection stage, checking both PCI address and extension address is requried, and still need to separately do some additional checks for PCI address if it is present, at last in reserving addr function we still check if PCI normal address or extension address exists to separately reserve present one. So that we have to do the check on the same condition repetively. If you don't have strong opposition, I want to send the new version ASAP. -- Yi Min

On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote:
在 2018/10/17 下午10:30, Andrea Bolognani 写道:
On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote:
I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better.
Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself.
For example, during collection stage, checking both PCI address and extension address is requried, and still need to separately do some additional checks for PCI address if it is present, at last in reserving addr function we still check if PCI normal address or extension address exists to separately reserve present one. So that we have to do the check on the same condition repetively. If you don't have strong opposition, I want to send the new version ASAP.
So I gave an half-hearted stab at implementing my own suggestion today, and quite unsurprisingly I have gained more sympathy for your argument in the process :) The main problem I see is that, as you noticed, we have a lot of calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() where really we should be using EnsureAddr() pretty much all of the time and hide most of the details from the drivers, which in turn would make it easier to change them in a transparent manner. Another big problem, which I highlighted in the past, is that the current API was not designed with the idea that PCI addresses could have "parts" in mind, and so it's not nuanced enough: if I call IsEmpty() on and address where the PCI part itself has been filled in but the zPCI part hasn't, or vice versa, what should I get back? The answer is probably that, after we've made sure those functions are used as little as possible thanks to the changes outlined above, we should replace them with better named alternatives. Of course it would be entirely unfair to ask you to perform such a massive refactoring before your series can be considered for inclusion, so at this point I'm okay with merging it and cleaning up the pre-existing mess afterwards. There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed. -- Andrea Bolognani / Red Hat / Virtualization

On 10/18/2018 05:44 PM, Andrea Bolognani wrote:
On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote:
在 2018/10/17 下午10:30, Andrea Bolognani 写道:
On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote:
I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better.
Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself.
For example, during collection stage, checking both PCI address and extension address is requried, and still need to separately do some additional checks for PCI address if it is present, at last in reserving addr function we still check if PCI normal address or extension address exists to separately reserve present one. So that we have to do the check on the same condition repetively. If you don't have strong opposition, I want to send the new version ASAP.
So I gave an half-hearted stab at implementing my own suggestion today, and quite unsurprisingly I have gained more sympathy for your argument in the process :)
The main problem I see is that, as you noticed, we have a lot of calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() where really we should be using EnsureAddr() pretty much all of the time and hide most of the details from the drivers, which in turn would make it easier to change them in a transparent manner.
Another big problem, which I highlighted in the past, is that the current API was not designed with the idea that PCI addresses could have "parts" in mind, and so it's not nuanced enough: if I call IsEmpty() on and address where the PCI part itself has been filled in but the zPCI part hasn't, or vice versa, what should I get back? The answer is probably that, after we've made sure those functions are used as little as possible thanks to the changes outlined above, we should replace them with better named alternatives.
Of course it would be entirely unfair to ask you to perform such a massive refactoring before your series can be considered for inclusion, so at this point I'm okay with merging it and cleaning up the pre-existing mess afterwards.
There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed.
As I said before, I think the current XML is the right variant. This is exactly how QEMU implements it (have a real classic pci bus naming scheme augmented with some additional data named uid/fid). So having an zcpi name space (instead of a pci one) would be wrong. Daniel, having said this, are you ok with the current variant?

在 2018/10/18 下午11:44, Andrea Bolognani 写道:
On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote:
在 2018/10/17 下午10:30, Andrea Bolognani 写道:
On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote:
I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better. Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself. For example, during collection stage, checking both PCI address and extension address is requried, and still need to separately do some additional checks for PCI address if it is present, at last in reserving addr function we still check if PCI normal address or extension address exists to separately reserve present one. So that we have to do the check on the same condition repetively. If you don't have strong opposition, I want to send the new version ASAP. So I gave an half-hearted stab at implementing my own suggestion today, and quite unsurprisingly I have gained more sympathy for your argument in the process :)
The main problem I see is that, as you noticed, we have a lot of calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() where really we should be using EnsureAddr() pretty much all of the time and hide most of the details from the drivers, which in turn would make it easier to change them in a transparent manner.
Another big problem, which I highlighted in the past, is that the current API was not designed with the idea that PCI addresses could have "parts" in mind, and so it's not nuanced enough: if I call IsEmpty() on and address where the PCI part itself has been filled in but the zPCI part hasn't, or vice versa, what should I get back? The answer is probably that, after we've made sure those functions are used as little as possible thanks to the changes outlined above, we should replace them with better named alternatives.
Of course it would be entirely unfair to ask you to perform such a massive refactoring before your series can be considered for inclusion, so at this point I'm okay with merging it and cleaning up the pre-existing mess afterwards.
There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed.
Do you mean expect Laine to take a quick look at this series, or the next one? -- Yi Min

On Fri, 2018-10-19 at 11:29 +0800, Yi Min Zhao wrote:
在 2018/10/18 下午11:44, Andrea Bolognani 写道:
There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed.
Do you mean expect Laine to take a quick look at this series, or the next one?
The next one :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/19 下午3:54, Andrea Bolognani 写道:
On Fri, 2018-10-19 at 11:29 +0800, Yi Min Zhao wrote:
在 2018/10/18 下午11:44, Andrea Bolognani 写道:
There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed. Do you mean expect Laine to take a quick look at this series, or the next one? The next one :)
I have posted v7 patches. Thanks! -- 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> --- src/qemu/qemu_command.c | 102 ++++++++++++++++++ src/qemu/qemu_command.h | 2 + .../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 | 30 ++++++ .../hostdev-vfio-zpci-multidomain-many.args | 39 +++++++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 ++++++++++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 2 + tests/qemuxml2argvtest.c | 13 +++ .../hostdev-vfio-zpci-autogenerate.xml | 34 ++++++ .../hostdev-vfio-zpci-boundaries.xml | 48 +++++++++ .../hostdev-vfio-zpci-multidomain-many.xml | 97 +++++++++++++++++ tests/qemuxml2xmltest.c | 11 ++ 15 files changed, 530 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 887947dc11..3847d07bb1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2195,6 +2195,57 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, return NULL; } +char * +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, + "zpci,uid=%u,fid=%u,target=%s,id=zpci%u", + dev->addr.pci.zpci.uid, + dev->addr.pci.zpci.fid, + dev->alias, + dev->addr.pci.zpci.uid); + + if (virBufferCheckError(&buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +static int +qemuCommandAddZPCIDevice(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 +qemuCommandAddExtDevice(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) +{ + if (dev->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || + dev->addr.pci.extFlags == VIR_PCI_ADDRESS_EXTENSION_NONE) { + return 0; + } + + if (dev->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + return qemuCommandAddZPCIDevice(cmd, dev); + + return 0; +} static int qemuBuildFloppyCommandLineControllerOptions(virCommandPtr cmd, @@ -2431,6 +2482,9 @@ qemuBuildDiskCommandLine(virCommandPtr cmd, if (!qemuDiskBusNeedsDriveArg(disk->bus)) { if (disk->bus != VIR_DOMAIN_DISK_BUS_FDC || virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) { + if (qemuCommandAddExtDevice(cmd, &disk->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildDiskDeviceStr(def, disk, bootindex, @@ -2629,6 +2683,9 @@ qemuBuildFSDevCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, optstr); VIR_FREE(optstr); + if (qemuCommandAddExtDevice(cmd, &fs->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) return -1; @@ -3089,6 +3146,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, goto cleanup; if (devstr) { + if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) { + VIR_FREE(devstr); + goto cleanup; + } virCommandAddArg(cmd, "-device"); virCommandAddArg(cmd, devstr); VIR_FREE(devstr); @@ -3887,6 +3948,9 @@ qemuBuildWatchdogCommandLine(virCommandPtr cmd, if (!def->watchdog) return 0; + if (qemuCommandAddExtDevice(cmd, &def->watchdog->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps); @@ -3959,6 +4023,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, if (qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio, qemuCaps) < 0) goto error; + if (qemuCommandAddExtDevice(cmd, &def->memballoon->info) < 0) + goto error; + virCommandAddArg(cmd, "-device"); virCommandAddArgBuffer(cmd, &buf); return 0; @@ -4153,6 +4220,9 @@ qemuBuildInputCommandLine(virCommandPtr cmd, virDomainInputDefPtr input = def->inputs[i]; char *devstr = NULL; + if (qemuCommandAddExtDevice(cmd, &input->info) < 0) + return -1; + if (qemuBuildInputDevStr(&devstr, def, input, qemuCaps) < 0) return -1; @@ -4294,6 +4364,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd, if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) { virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL); } else { + if (qemuCommandAddExtDevice(cmd, &sound->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps))) return -1; @@ -4531,6 +4604,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, if (video->primary) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) { + if (qemuCommandAddExtDevice(cmd, + &def->videos[i]->info) < 0) + return -1; virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps))) @@ -4543,6 +4619,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, return -1; } } else { + if (qemuCommandAddExtDevice(cmd, &def->videos[i]->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps))) @@ -5372,6 +5451,10 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); } } + + if (qemuCommandAddExtDevice(cmd, hostdev->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); devstr = qemuBuildPCIHostdevDevStr(def, hostdev, bootIndex, configfd_name, qemuCaps); @@ -5818,6 +5901,9 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager, virCommandAddArgBuffer(cmd, &buf); /* add the device */ + if (qemuCommandAddExtDevice(cmd, &rng->info) < 0) + return -1; + if (!(tmp = qemuBuildRNGDevStr(def, rng, qemuCaps))) return -1; virCommandAddArgList(cmd, "-device", tmp, NULL); @@ -8282,6 +8368,9 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virCommandAddArg(cmd, "-netdev"); virCommandAddArg(cmd, netdev); + if (qemuCommandAddExtDevice(cmd, &net->info) < 0) + goto cleanup; + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, queues, qemuCaps))) { goto cleanup; @@ -8563,6 +8652,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, * New way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 */ if (qemuDomainSupportsNicdev(def, net)) { + if (qemuCommandAddExtDevice(cmd, &net->info) < 0) + goto cleanup; + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, vhostfdSize, qemuCaps))) goto cleanup; @@ -9010,6 +9102,12 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, if (!devstr) return -1; + + if (qemuCommandAddExtDevice(cmd, &shmem->info) < 0) { + VIR_FREE(devstr); + return -1; + } + virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); @@ -10177,6 +10275,10 @@ qemuBuildVsockCommandLine(virCommandPtr cmd, virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); priv->vhostfd = -1; + + if (qemuCommandAddExtDevice(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..1e6060345b --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml @@ -0,0 +1,30 @@ +<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'> + <zpci uid='0xffff' fid='0xffffffff'/> + </address> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'> + <zpci uid='0x0001' fid='0x00000000'/> + </address> + </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..da8305dd6d --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml @@ -0,0 +1,79 @@ +<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'> + <zpci uid='0x0023' fid='0x0000003f'/> + </address> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'> + <zpci uid='0x0035' fid='0x00000068'/> + </address> + </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'> + <zpci uid='0x0053'/> + </address> + </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'> + <zpci uid='0x0003' fid='0x00000072'/> + </address> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'> + <zpci uid='0x0017' fid='0x00000003'/> + </address> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'> + <zpci uid='0x0004' fid='0x00000028'/> + </address> + </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 7ecd9342bc..b8637a40d4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1674,6 +1674,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..e94e63bd0a --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml @@ -0,0 +1,34 @@ +<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'> + <zpci uid='0x0001' fid='0x00000000'/> + </address> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'> + <zpci uid='0x0002' fid='0x00000001'/> + </address> + </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..81d2146188 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml @@ -0,0 +1,48 @@ +<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'> + <zpci uid='0xffff' fid='0xffffffff'/> + </address> + </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'> + <zpci uid='0x0001' fid='0x00000000'/> + </address> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'> + <zpci uid='0x0002' fid='0x00000001'/> + </address> + </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..e56106d103 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml @@ -0,0 +1,97 @@ +<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'> + <zpci uid='0x0023' fid='0x0000003f'/> + </address> + </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'> + <zpci uid='0x0035' fid='0x00000068'/> + </address> + </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'> + <zpci uid='0x0001' fid='0x00000001'/> + </address> + </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'> + <zpci uid='0x0002' fid='0x00000002'/> + </address> + </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'> + <zpci uid='0x0053' fid='0x00000000'/> + </address> + </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'> + <zpci uid='0x0003' fid='0x00000072'/> + </address> + </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'> + <zpci uid='0x0017' fid='0x00000003'/> + </address> + </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'> + <zpci uid='0x0004' fid='0x00000028'/> + </address> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'> + <zpci uid='0x0005' fid='0x00000004'/> + </address> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 53de9726f4..4d1a3610ef 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -464,6 +464,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 Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -3089,6 +3146,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, goto cleanup;
if (devstr) { + if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) { + VIR_FREE(devstr); + goto cleanup; + }
Please add an empty line here... [...]
@@ -4531,6 +4604,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, if (video->primary) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
+ if (qemuCommandAddExtDevice(cmd, + &def->videos[i]->info) < 0) + return -1;
... and one here. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/12 下午10:44, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -3089,6 +3146,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, goto cleanup;
if (devstr) { + if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) { + VIR_FREE(devstr); + goto cleanup; + } Please add an empty line here...
[...]
@@ -4531,6 +4604,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, if (video->primary) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
+ if (qemuCommandAddExtDevice(cmd, + &def->videos[i]->info) < 0) + return -1; ... and one here.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
OK. Thanks! -- Yi Min

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> --- src/qemu/qemu_hotplug.c | 162 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 152 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4558a3c02d..e1395d1674 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -154,6 +154,80 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, } +static int +qemuDomainAttachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + char *devstr_zpci = NULL; + int ret = -1; + + 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 (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || + info->addr.pci.extFlags == VIR_PCI_ADDRESS_EXTENSION_NONE) { + return 0; + } + + if (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + return qemuDomainAttachZPCIDevice(mon, info); + + return 0; +} + + +static int +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || + info->addr.pci.extFlags == VIR_PCI_ADDRESS_EXTENSION_NONE) { + return 0; + } + + if (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + return qemuDomainDetachZPCIDevice(mon, info); + + return 0; +} + + static int qemuHotplugWaitForTrayEject(virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -805,8 +879,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 +992,16 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if ((ret = qemuDomainAttachExtensionDevice(priv->mon, + &controller->info)) < 0) { + goto exit_monitor; + } + + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &controller->info)); + + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; ret = -1; @@ -1381,7 +1469,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))) @@ -1451,7 +1540,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; @@ -1668,8 +1765,16 @@ 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; @@ -2325,9 +2430,14 @@ 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; goto cleanup; @@ -2807,8 +2917,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; @@ -3053,8 +3171,13 @@ 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; @@ -3227,9 +3350,15 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + + if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0) goto exit_monitor; + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info)); + goto exit_monitor; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; goto cleanup; @@ -3306,8 +3435,14 @@ 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; @@ -5292,10 +5427,17 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); + if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) { + goto exit_monitor; + } + if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } + + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; -- Yi Min

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -1381,7 +1469,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)) {
You can put the new condition in the second line instead of the first one to reduce the size of the diff without affecting the result. With the usual caveat that someone familiar with hotplug should really give this at least a look before it gets merged, it looks sane enough to me so it gets my Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/12 下午10:57, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -1381,7 +1469,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)) { You can put the new condition in the second line instead of the first one to reduce the size of the diff without affecting the result.
With the usual caveat that someone familiar with hotplug should really give this at least a look before it gets merged, it looks sane enough to me so it gets my
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Got it. Thanks! -- Yi Min

Update 'Device address' section to describe 'zpci' element and its two attributes 'uid' and 'fid'. 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> --- 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 8189959773..a6e07e4a8a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3925,7 +3925,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 child element: <code>zpci</code>, which has + two attributes <code>uid</code> (a hex value between 0x0001 and + 0xffff, inclusive), and <code>fid</code> (a hex value between + 0x00000000 and 0xffffffff, inclusive) 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

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -3925,7 +3925,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
No space after 4.8.0; also it should read 4.9.0 now ;)
+ depending on the architecture are supported. E.g. for S390 guests, + PCI addresses have child element: <code>zpci</code>, which has + two attributes <code>uid</code> (a hex value between 0x0001 and
"[...] are supported. For example, PCI addresses for S390 guests will have a <code>zpci</code> child element, with two attributes: <code>uid</code> [...]" With the above, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/12 下午11:03, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
@@ -3925,7 +3925,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 No space after 4.8.0; also it should read 4.9.0 now ;) This patch set was posted before 4.9.0. Now it should be 4.9.0. Thanks for your reminder.
+ depending on the architecture are supported. E.g. for S390 guests, + PCI addresses have child element: <code>zpci</code>, which has + two attributes <code>uid</code> (a hex value between 0x0001 and "[...] are supported. For example, PCI addresses for S390 guests will have a <code>zpci</code> child element, with two attributes: <code>uid</code> [...]"
With the above,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks! -- 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> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 3ed6ff8aeb..9600c97102 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,17 @@ and virDomainPMWakeup APIs. </description> </change> + <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + PCI addresses can now include the new zpci element which contains + uid (user-defined identifier) and fid (PCI function identifier) + attributes and makes the corresponding devices usable by S390 + guests. + </description> + </change> </section> <section title="Removed features"> <change> -- Yi Min

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
+ <change> + <summary> + qemu: Added support for PCI device passthrough on S390
Talking about "device passthrough" is not accurate, because the new feature can be used for emulated devices too... Just use "devices". With that fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/12 下午11:05, Andrea Bolognani 写道:
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: [...]
+ <change> + <summary> + qemu: Added support for PCI device passthrough on S390 Talking about "device passthrough" is not accurate, because the new feature can be used for emulated devices too... Just use "devices".
With that fixed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Yes, you're right. Thanks! -- Yi Min

Polite ping... On 9/28/18 10:46 AM, Yi Min Zhao wrote:
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, a new element of PCI address is introduced. It has two optional attributes, @uid and @fid. For example: <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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev>
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.
zPCI address as an extension of the PCI address 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 zPCI 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: b1a0f691ce4652fc71aca6776f085355040322da spec: Build ceph and gluster support everywhere
Change Log ========== v5->v6: 1. Modify zPCI XML definition. 2. Optimize the logic of zPCI address assignment and reservation. 3. Add extension flag into PCI address structure. 4. Update commit messages.
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 | 11 + docs/schemas/basictypes.rng | 27 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 90 +++++ src/conf/device_conf.h | 7 + src/conf/domain_addr.c | 332 ++++++++++++++++++ src/conf/domain_addr.h | 30 ++ src/conf/domain_conf.c | 14 +- src/libvirt_private.syms | 7 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 102 ++++++ src/qemu/qemu_command.h | 2 + src/qemu/qemu_domain.c | 39 ++ src/qemu/qemu_domain_address.c | 204 ++++++++++- src/qemu/qemu_hotplug.c | 162 ++++++++- src/util/virpci.c | 7 + src/util/virpci.h | 14 + .../caps_2.10.0.s390x.xml | 1 + .../caps_2.11.0.s390x.xml | 1 + .../caps_2.12.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + .../disk-virtio-s390-zpci.args | 26 ++ .../disk-virtio-s390-zpci.xml | 19 + .../hostdev-vfio-zpci-autogenerate.args | 25 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 + .../hostdev-vfio-zpci-boundaries.args | 29 ++ .../hostdev-vfio-zpci-boundaries.xml | 30 ++ .../hostdev-vfio-zpci-multidomain-many.args | 39 ++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 25 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 21 ++ tests/qemuxml2argvtest.c | 20 ++ .../disk-virtio-s390-zpci.xml | 31 ++ .../hostdev-vfio-zpci-autogenerate.xml | 34 ++ .../hostdev-vfio-zpci-boundaries.xml | 48 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 97 +++++ .../qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 32 ++ tests/qemuxml2xmltest.c | 17 + 44 files changed, 1648 insertions(+), 15 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
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Fri, Sep 28, 2018 at 04:46:13PM +0800, Yi Min Zhao wrote:
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, a new element of PCI address is introduced. It has two optional attributes, @uid and @fid. For example: <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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev>
I'm not sure if this was discussed in earlier versions, but to me this use of a child element looks wrong. What we're effectively saying is that s390 has a different addressing scheme. It happens to share some fields with the current PCI addressing scheme, but it is none the less a distinct scheme. IOW, I think it should be <address type='zpci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> Of course internally we can still share much logic for assigning the addreses between "pci" and "zpci". Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Fri, 2018-10-12 at 16:04 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 28, 2018 at 04:46:13PM +0800, Yi Min Zhao wrote: [...]
<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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev>
I'm not sure if this was discussed in earlier versions, but to me this use of a child element looks wrong.
What we're effectively saying is that s390 has a different addressing scheme. It happens to share some fields with the current PCI addressing scheme, but it is none the less a distinct scheme.
IOW, I think it should be
<address type='zpci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/>
Of course internally we can still share much logic for assigning the addreses between "pci" and "zpci".
So what happens with PCI devices on s390 is that *two* devices will be added to the guest: one is the usual virtio-net-pci or what have you, which has its own PCI address allocated using the same algorithm as other architectures; the other one is a '-device zpci', which IIUC works basically like an adapter between the PCI device itself and the guest OS, and which is identified using uid and fid. Calling it a completely different address type seems like a bit of a stretch: there is definitely a PCI address involved, which is why the zPCI part was implemented through a potentially reusable "PCI address extension" mechanism. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/10/14 下午8:47, Andrea Bolognani 写道:
On Fri, 2018-10-12 at 16:04 +0100, Daniel P. Berrangé wrote:
<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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev> I'm not sure if this was discussed in earlier versions, but to me
On Fri, Sep 28, 2018 at 04:46:13PM +0800, Yi Min Zhao wrote: [...] this use of a child element looks wrong.
What we're effectively saying is that s390 has a different addressing scheme. It happens to share some fields with the current PCI addressing scheme, but it is none the less a distinct scheme.
IOW, I think it should be
<address type='zpci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/>
Of course internally we can still share much logic for assigning the addreses between "pci" and "zpci". So what happens with PCI devices on s390 is that *two* devices will be added to the guest: one is the usual virtio-net-pci or what have you, which has its own PCI address allocated using the same algorithm as other architectures; the other one is a '-device zpci', which IIUC works basically like an adapter between the PCI device itself and the guest OS, and which is identified using uid and fid.
Calling it a completely different address type seems like a bit of a stretch: there is definitely a PCI address involved, which is why the zPCI part was implemented through a potentially reusable "PCI address extension" mechanism.
Sorry, this mail went into trash box.. -- Yi Min

On 10/14/18 2:47 PM, Andrea Bolognani wrote:
On Fri, 2018-10-12 at 16:04 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 28, 2018 at 04:46:13PM +0800, Yi Min Zhao wrote: [...]
<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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev>
I'm not sure if this was discussed in earlier versions, but to me this use of a child element looks wrong.
What we're effectively saying is that s390 has a different addressing scheme. It happens to share some fields with the current PCI addressing scheme, but it is none the less a distinct scheme.
IOW, I think it should be
<address type='zpci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/>
Of course internally we can still share much logic for assigning the addreses between "pci" and "zpci".
So what happens with PCI devices on s390 is that *two* devices will be added to the guest: one is the usual virtio-net-pci or what have you, which has its own PCI address allocated using the same algorithm as other architectures; the other one is a '-device zpci', which IIUC works basically like an adapter between the PCI device itself and the guest OS, and which is identified using uid and fid.
Calling it a completely different address type seems like a bit of a stretch: there is definitely a PCI address involved, which is why the zPCI part was implemented through a potentially reusable "PCI address extension" mechanism.
I thought that we discussed this in v1 or v2 already when uid anf fid were still embedded in the address element itself. In v5 Andrea suggested to model the two zpci extension parameters outside as a child element of address which corresponds kind of to what is happening in qemu (see Andreas paragraph above). The original idea was for users on s390 to make pci no different than on other platforms. Creating a zpci address type would introduce the opposite. Currently uid and fid are optional attributes for the user on s390. He can simply enter any kind of pci address as for other platforms. If he does so on s390 the uid and fid would be automatically generated for him. Only if he chooses to specifically set these attributes himself he has to specify uid and/or fid. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 10/14/18 2:47 PM, Andrea Bolognani wrote:
On Fri, 2018-10-12 at 16:04 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 28, 2018 at 04:46:13PM +0800, Yi Min Zhao wrote: [...]
<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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev>
I'm not sure if this was discussed in earlier versions, but to me this use of a child element looks wrong.
What we're effectively saying is that s390 has a different addressing scheme. It happens to share some fields with the current PCI addressing scheme, but it is none the less a distinct scheme.
IOW, I think it should be
<address type='zpci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/>
Of course internally we can still share much logic for assigning the addreses between "pci" and "zpci".
So what happens with PCI devices on s390 is that *two* devices will be added to the guest: one is the usual virtio-net-pci or what have you, which has its own PCI address allocated using the same algorithm as other architectures; the other one is a '-device zpci', which IIUC works basically like an adapter between the PCI device itself and the guest OS, and which is identified using uid and fid.
Calling it a completely different address type seems like a bit of a stretch: there is definitely a PCI address involved, which is why the zPCI part was implemented through a potentially reusable "PCI address extension" mechanism.
I thought that we discussed this in v1 or v2 already when uid anf fid were still embedded in the address element itself. In v5 Andrea suggested to model the two zpci extension parameters outside as a child element of address which corresponds kind of to what is happening in qemu (see Andreas paragraph above). The original idea was for users on s390 to make pci no different than on other platforms. Creating a zpci address type would introduce the opposite. Currently uid and fid are optional attributes for the user on s390. He can simply enter any kind of pci address as for other platforms. If he does so on s390 the uid and fid would be automatically generated for him. Only if he chooses to specifically set these attributes himself he has to specify uid and/or fid. Agreed. In QEMU this is really modelled as PCI (with the classic bus addresses). The fact that the guest uses the fid/uid instead does not make the PCI bus addresses in QEMU go away and it should not change the modelling of
On 10/15/2018 09:30 AM, Boris Fiuczynski wrote: the devices. So I think that the current proposal from Andrea implemented by Yi Min and endorsed by Boris: <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'> <zpci uid='0x0003' fid='0x00000027'/> is really the best solution.

在 2018/10/12 下午11:04, Daniel P. Berrangé 写道:
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, a new element of PCI address is introduced. It has two optional attributes, @uid and @fid. For example: <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'> <zpci uid='0x0003' fid='0x00000027'/> </address> </hostdev> I'm not sure if this was discussed in earlier versions, but to me
On Fri, Sep 28, 2018 at 04:46:13PM +0800, Yi Min Zhao wrote: this use of a child element looks wrong.
What we're effectively saying is that s390 has a different addressing scheme. It happens to share some fields with the current PCI addressing scheme, but it is none the less a distinct scheme.
IOW, I think it should be
<address type='zpci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/>
Of course internally we can still share much logic for assigning the addreses between "pci" and "zpci".
Regards, Daniel Hi Daniel,
Thanks for your comment! We have discussed on this in previous review rounds. Treating zpci as an element is our latest agreement. If do it as your suggestion, we have to re-start reviewing the code. But I hope we could do it in the most appropriate way eventually. How about Andrea's idea? @Andrea Regards, Yi Min -- Yi Min
participants (5)
-
Andrea Bolognani
-
Boris Fiuczynski
-
Christian Borntraeger
-
Daniel P. Berrangé
-
Yi Min Zhao