[PATCH libvirt v2 0/5] Fix zPCI address auto-generation on s390
The zPCI address validation or autogeneration does not work as expected in the following scenarios 1. uid = 0 and fid = 0 2. uid = 0 and fid > 0 3. uid = 0 and fid not specified 4. uid not specified and fid > 0 5. 2 zPCI devices with uid > 0 and fid not specified. This is because of the following reasons 1. If uid = 0 or fid = 0 the code assumes that user has not specified the corresponding address 2. If either uid or fid is provided, the code assumes that both uid and fid addresses are specified by the user. This patch fixes these issues. --- v2: - Call same function to reserve zPCI address when zPCI address is fully/partially/not specified by the user. - Redefine structure of zPCI address. - Add tests to verify the auto-generated xml(qemuxml2xmltest.c). - Separate/merge patches. - Minor changes based on review feedback. v1: https://www.redhat.com/archives/libvir-list/2020-April/msg00479.html Shalini Chellathurai Saroja (5): conf: use g_autofree to ensure automatic cleanup conf: fix zPCI address auto-generation on s390 qemu: move ZPCI uid validation into device validation tests: qemu: add more tests for ZPCI on S390 tests: add test with PCI and CCW device src/conf/device_conf.c | 45 +++++------ src/conf/domain_addr.c | 77 ++++++------------- src/conf/domain_conf.c | 10 ++- src/libvirt_private.syms | 4 +- src/qemu/qemu_command.c | 6 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_validate.c | 18 ++++- src/util/virpci.c | 23 ++---- src/util/virpci.h | 15 +++- .../hostdev-vfio-zpci-autogenerate-fids.args | 31 ++++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++++++ .../hostdev-vfio-zpci-autogenerate-uids.args | 31 ++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++++++ .../hostdev-vfio-zpci-ccw-memballoon.args | 28 +++++++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 17 ++++ .../hostdev-vfio-zpci-duplicate.xml | 30 ++++++++ ...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +++++ .../hostdev-vfio-zpci-set-zero.xml | 21 +++++ .../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++ tests/qemuxml2argvtest.c | 25 ++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 43 +++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 43 +++++++++++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 32 ++++++++ tests/qemuxml2xmltest.c | 10 +++ 24 files changed, 498 insertions(+), 112 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml -- 2.25.4
Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/device_conf.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 4dbd5c1a..1d06981a 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -53,9 +53,8 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr) { virZPCIDeviceAddress def = { 0 }; - char *uid; - char *fid; - int ret = -1; + g_autofree char *uid = NULL; + g_autofree char *fid = NULL; uid = virXMLPropString(node, "uid"); fid = virXMLPropString(node, "fid"); @@ -64,27 +63,23 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node, virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot parse <address> 'uid' attribute")); - goto cleanup; + return -1; } if (fid && virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot parse <address> 'fid' attribute")); - goto cleanup; + return -1; } if (!virZPCIDeviceAddressIsEmpty(&def) && !virZPCIDeviceAddressIsValid(&def)) - goto cleanup; + return -1; addr->zpci = def; - ret = 0; - cleanup: - VIR_FREE(uid); - VIR_FREE(fid); - return ret; + return 0; } void -- 2.25.4
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/device_conf.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Let us fix the issues with zPCI address validation and auto-generation on s390. Currently, there are two issues with handling the ZPCI address extension. Firstly, when the uid is to be auto-generated with a specified fid, .i.e.: ... <address type='pci'> <zpci fid='0x0000001f'/> </address> ... we expect uid='0x0001' (or the next available uid for the domain). However, we get a parsing error: $ virsh define zpci.xml error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000 and <= 0xffff Secondly, when the uid is specified explicitly with the invalid numerical value '0x0000', we actually expect the parsing error above. However, the domain is being defined and the uid value is silently changed to a valid value. The first issue is a bug and the second one is undesired behaviour, and both issues are related to how we (in-band) signal invalid values for uid and fid. So let's fix the XML parsing to do validation based on what is actually specified in the XML. The first issue is also related to the current code behaviour, which is, if either uid or fid is specified by the user, it is incorrectly assumed that both uid and fid are specified. This bug is fixed by identifying when the user specified ZPCI address is incomplete and auto-generating the missing ZPCI address. Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> --- src/conf/device_conf.c | 33 ++++---- src/conf/domain_addr.c | 77 ++++++------------- src/conf/domain_conf.c | 10 ++- src/libvirt_private.syms | 3 +- src/qemu/qemu_command.c | 6 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_validate.c | 2 +- src/util/virpci.c | 19 +++-- src/util/virpci.h | 14 +++- .../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++ tests/qemuxml2argvtest.c | 3 + 11 files changed, 101 insertions(+), 88 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 1d06981a..21398e90 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -59,22 +59,25 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node, 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")); - return -1; + if (uid) { + if (virStrToLong_uip(uid, NULL, 0, &def.uid.value) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'uid' attribute")); + return -1; + } + def.uid.isSet = true; } - if (fid && - virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'fid' attribute")); - return -1; + if (fid) { + if (virStrToLong_uip(fid, NULL, 0, &def.fid.value) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'fid' attribute")); + return -1; + } + def.fid.isSet = true; } - if (!virZPCIDeviceAddressIsEmpty(&def) && - !virZPCIDeviceAddressIsValid(&def)) + if (!virZPCIDeviceAddressIsValid(&def)) return -1; addr->zpci = def; @@ -190,22 +193,20 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info) !virPCIDeviceAddressIsEmpty(&info->addr.pci); } - bool virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info) { return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && - virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci); + virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci); } bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) { return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && - !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci); + virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci); } - int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8623e79d..90ed31ef 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -58,7 +58,7 @@ static int virDomainZPCIAddressReserveUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - return virDomainZPCIAddressReserveId(set, addr->uid, "uid"); + return virDomainZPCIAddressReserveId(set, addr->uid.value, "uid"); } @@ -66,7 +66,7 @@ static int virDomainZPCIAddressReserveFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - return virDomainZPCIAddressReserveId(set, addr->fid, "fid"); + return virDomainZPCIAddressReserveId(set, addr->fid.value, "fid"); } @@ -96,7 +96,7 @@ static int virDomainZPCIAddressAssignUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - return virDomainZPCIAddressAssignId(set, &addr->uid, 1, + return virDomainZPCIAddressAssignId(set, &addr->uid.value, 1, VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid"); } @@ -105,7 +105,7 @@ static int virDomainZPCIAddressAssignFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - return virDomainZPCIAddressAssignId(set, &addr->fid, 0, + return virDomainZPCIAddressAssignId(set, &addr->fid.value, 0, VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid"); } @@ -129,7 +129,8 @@ static void virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - virDomainZPCIAddressReleaseId(set, &addr->uid, "uid"); + if (addr->uid.isSet) + virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid"); } @@ -137,7 +138,8 @@ static void virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - virDomainZPCIAddressReleaseId(set, &addr->fid, "fid"); + if (addr->fid.isSet) + virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid"); } @@ -145,47 +147,26 @@ static void virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds, virZPCIDeviceAddressPtr addr) { - if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr)) + if (!zpciIds) 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) +virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) { - if (virDomainZPCIAddressAssignFid(fids, zpci) < 0) + if (!addr->uid.isSet && + virDomainZPCIAddressAssignUid(zpciIds->uids, addr) < 0) return -1; - if (virDomainZPCIAddressReserveFid(fids, zpci) < 0) + if (!addr->fid.isSet && + virDomainZPCIAddressAssignFid(zpciIds->fids, addr) < 0) return -1; - return 0; -} - - -static int -virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds, - virZPCIDeviceAddressPtr addr) -{ if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0) return -1; @@ -194,21 +175,8 @@ virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds, 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; - } + addr->uid.isSet = true; + addr->fid.isSet = true; return 0; } @@ -234,9 +202,9 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) { if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { - virZPCIDeviceAddress zpci = { 0 }; + virZPCIDeviceAddress zpci = addr->zpci; - if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0) + if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, &zpci) < 0) return -1; if (!addrs->dryRun) @@ -246,6 +214,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, return 0; } + static int virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) @@ -253,10 +222,8 @@ virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, 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); + if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0) + return -1; } return 0; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e9336fd7..d5594681 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7519,11 +7519,15 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virTristateSwitchTypeToString(info->addr.pci.multi)); } - if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) { + if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing uid or fid attribute of zPCI address")); + } + if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) { virBufferAsprintf(&childBuf, "<zpci uid='0x%.4x' fid='0x%.8x'/>\n", - info->addr.pci.zpci.uid, - info->addr.pci.zpci.fid); + info->addr.pci.zpci.uid.value, + info->addr.pci.zpci.fid.value); } break; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc7406f2..a4a09cf9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2837,7 +2837,8 @@ virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; -virZPCIDeviceAddressIsEmpty; +virZPCIDeviceAddressIsIncomplete; +virZPCIDeviceAddressIsPresent; virZPCIDeviceAddressIsValid; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f27246b4..36349870 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1902,10 +1902,10 @@ qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev) virBufferAsprintf(&buf, "zpci,uid=%u,fid=%u,target=%s,id=zpci%u", - dev->addr.pci.zpci.uid, - dev->addr.pci.zpci.fid, + dev->addr.pci.zpci.uid.value, + dev->addr.pci.zpci.fid.value, dev->alias, - dev->addr.pci.zpci.uid); + dev->addr.pci.zpci.uid.value); return virBufferContentAndReset(&buf); } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index dc3bd824..3954ad11 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -157,7 +157,7 @@ qemuDomainDetachZPCIDevice(qemuMonitorPtr mon, { g_autofree char *zpciAlias = NULL; - zpciAlias = g_strdup_printf("zpci%d", info->addr.pci.zpci.uid); + zpciAlias = g_strdup_printf("zpci%d", info->addr.pci.zpci.uid.value); if (qemuMonitorDelDevice(mon, zpciAlias) < 0) return -1; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b1a81ab1..0372ae7d 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1018,7 +1018,7 @@ static int qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) { - if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci) && + if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/util/virpci.c b/src/util/virpci.c index 786b1393..40ae5aec 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2173,12 +2173,13 @@ 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) { + if (zpci->uid.isSet && + (zpci->uid.value > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || + zpci->uid.value == 0)) { virReportError(VIR_ERR_XML_ERROR, _("Invalid PCI address uid='0x%.4x', " "must be > 0x0000 and <= 0x%.4x"), - zpci->uid, + zpci->uid.value, VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); return false; } @@ -2187,11 +2188,19 @@ virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) } bool -virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr) +virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr) { - return !(addr->uid || addr->fid); + return !addr->uid.isSet || !addr->fid.isSet; } + +bool +virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr) +{ + return addr->uid.isSet || addr->fid.isSet; +} + + #ifdef __linux__ virPCIDeviceAddressPtr diff --git a/src/util/virpci.h b/src/util/virpci.h index f16d2361..f198df5d 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -38,11 +38,18 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIDeviceList, virObjectUnref); #define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX #define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX +typedef struct _virZPCIDeviceAddressID virZPCIDeviceAddressID; typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; + +struct _virZPCIDeviceAddressID { + unsigned int value; + bool isSet; +}; + struct _virZPCIDeviceAddress { - unsigned int uid; /* exempt from syntax-check */ - unsigned int fid; + virZPCIDeviceAddressID uid; /* exempt from syntax-check */ + virZPCIDeviceAddressID fid; /* Don't forget to update virPCIDeviceAddressCopy if needed. */ }; @@ -245,8 +252,9 @@ char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); +bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr); +bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr); bool virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci); -bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, int pfNetDevIdx, diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml new file mode 100644 index 00000000..6bfbfe61 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml @@ -0,0 +1,20 @@ +<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'> + <zpci uid='0x0'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d5b2a21b..765e6fdf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1750,6 +1750,9 @@ mymain(void) DO_TEST("hostdev-vfio-zpci-autogenerate", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-uid-set-zero", + 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, -- 2.25.4
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
@@ -129,7 +129,8 @@ static void virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - virDomainZPCIAddressReleaseId(set, &addr->uid, "uid"); + if (addr->uid.isSet) + virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid"); }
@@ -137,7 +138,8 @@ static void virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - virDomainZPCIAddressReleaseId(set, &addr->fid, "fid"); + if (addr->fid.isSet) + virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid"); }
-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; - } + addr->uid.isSet = true; + addr->fid.isSet = true;
First of all, this is much closer to what I had in mind. Good job! We're not quite there yet, though: as you can see from the hunks above, there are still many scenarios in which we're either manipulating id->value and id->isSet separately, or we needlessly duplicate checks and don't take advantage of the fact that we now have the virZPCIDeviceAddressID struct. I have attached a patch that fixes these issues: as you can see, it's pretty simple, because you've laid all the groundwork :) so it just needs to polish things up a bit. It also solves the situation where calling virDomainZPCIAddressRelease{U,F}id() would not set id->isSet back to false. Speaking of polish, while functionally this doesn't make any difference it would be IMHO better to rename the current virDomainZPCIAddressReserveAddr() to virDomainZPCIAddressEnsureAddr(), since in terms of semantics it more closely matches those of the PCI address function of the same name. This will hopefully help reduce confusion. I've attached a patch that performs this change as well. Everything else looks good. Please let me know what you think of the changes in the attached patches! -- Andrea Bolognani / Red Hat / Virtualization
On 6/25/20 7:43 PM, Andrea Bolognani wrote:
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
@@ -129,7 +129,8 @@ static void virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - virDomainZPCIAddressReleaseId(set, &addr->uid, "uid"); + if (addr->uid.isSet) + virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid"); }
@@ -137,7 +138,8 @@ static void virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { - virDomainZPCIAddressReleaseId(set, &addr->fid, "fid"); + if (addr->fid.isSet) + virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid"); }
-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; - } + addr->uid.isSet = true; + addr->fid.isSet = true;
First of all, this is much closer to what I had in mind. Good job!
We're not quite there yet, though: as you can see from the hunks above, there are still many scenarios in which we're either manipulating id->value and id->isSet separately, or we needlessly duplicate checks and don't take advantage of the fact that we now have the virZPCIDeviceAddressID struct.
I have attached a patch that fixes these issues: as you can see, it's pretty simple, because you've laid all the groundwork :) so it just needs to polish things up a bit. It also solves the situation where calling virDomainZPCIAddressRelease{U,F}id() would not set id->isSet back to false.
Speaking of polish, while functionally this doesn't make any difference it would be IMHO better to rename the current virDomainZPCIAddressReserveAddr() to virDomainZPCIAddressEnsureAddr(), since in terms of semantics it more closely matches those of the PCI address function of the same name. This will hopefully help reduce confusion. I've attached a patch that performs this change as well.
Everything else looks good. Please let me know what you think of the changes in the attached patches!
Hi Andrea, I agree that it looks nice and sparkling. :D Thanks. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 6/25/20 7:43 PM, Andrea Bolognani wrote:
First of all, this is much closer to what I had in mind. Good job!
Hi Andrea, Thank you:-)
We're not quite there yet, though: as you can see from the hunks above, there are still many scenarios in which we're either manipulating id->value and id->isSet separately, or we needlessly duplicate checks and don't take advantage of the fact that we now have the virZPCIDeviceAddressID struct.
I have attached a patch that fixes these issues: as you can see, it's pretty simple, because you've laid all the groundwork:) so it just needs to polish things up a bit. It also solves the situation where calling virDomainZPCIAddressRelease{U,F}id() would not set id->isSet back to false.
Speaking of polish, while functionally this doesn't make any difference it would be IMHO better to rename the current virDomainZPCIAddressReserveAddr() to virDomainZPCIAddressEnsureAddr(), since in terms of semantics it more closely matches those of the PCI address function of the same name. This will hopefully help reduce confusion. I've attached a patch that performs this change as well.
Everything else looks good. Please let me know what you think of the changes in the attached patches!
As Boris has already mentioned, these patches provide a much better code, thank you:-)
Andrea Bolognani <abologna@redhat.com> [2020-06-25, 07:43PM +0200]:
From 82197ea6d794144e51b72f97df2315a65134b385 Mon Sep 17 00:00:00 2001 From: Andrea Bolognani <abologna@redhat.com> Date: Thu, 25 Jun 2020 18:37:27 +0200 Subject: [libvirt PATCH 1/2] conf: Move id->{value,isSet} handling further down the stack
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 61 ++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 90ed31ef4e..493b155129 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -33,20 +33,27 @@ VIR_LOG_INIT("conf.domain_addr");
static int virDomainZPCIAddressReserveId(virHashTablePtr set, - unsigned int id, + virZPCIDeviceAddressID *id, const char *name) { - if (virHashLookup(set, &id)) { + if (!id->isSet) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No zPCI %s to reserve"), + name);
Maybe it's better to fail silently here (or not fail at all and just make it no-op)? This is more of an assert that concerns the developer and not something the user can understand.
+ return -1; + } + + if (virHashLookup(set, &id->value)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("zPCI %s %o is already reserved"), - name, id); + name, id->value); return -1; }
- if (virHashAddEntry(set, &id, (void*)1) < 0) { + if (virHashAddEntry(set, &id->value, (void*)1) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to reserve %s %o"), - name, id); + name, id->value); return -1; }
[...]
static void virDomainZPCIAddressReleaseId(virHashTablePtr set, - unsigned int *id, + virZPCIDeviceAddressID *id, const char *name) { - if (virHashRemoveEntry(set, id) < 0) { + if (!id->isSet) + return; + + if (virHashRemoveEntry(set, &id->value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Release %s %o failed"), - name, *id); + name, id->value); }
- *id = 0; + id->value = 0; + id->isSet = false;
Not sure if I don't want to leave this to the caller. 99% of time it shouldn't matter because the released device is deleted from the domain anyways, but this tight coupling would prevent hypothetical re-use of the id. Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> -- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, 2020-06-26 at 16:08 +0200, Bjoern Walk wrote:
Andrea Bolognani <abologna@redhat.com> [2020-06-25, 07:43PM +0200]:
static int virDomainZPCIAddressReserveId(virHashTablePtr set, - unsigned int id, + virZPCIDeviceAddressID *id, const char *name) { - if (virHashLookup(set, &id)) { + if (!id->isSet) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No zPCI %s to reserve"), + name);
Maybe it's better to fail silently here (or not fail at all and just make it no-op)? This is more of an assert that concerns the developer and not something the user can understand.
VIR_ERR_INTERNAL_ERROR is commonly used when encountering situations that Will Never Happen™.
static void virDomainZPCIAddressReleaseId(virHashTablePtr set, - unsigned int *id, + virZPCIDeviceAddressID *id, const char *name) { - if (virHashRemoveEntry(set, id) < 0) { + if (!id->isSet) + return; + + if (virHashRemoveEntry(set, &id->value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Release %s %o failed"), - name, *id); + name, id->value); }
- *id = 0; + id->value = 0; + id->isSet = false;
Not sure if I don't want to leave this to the caller. 99% of time it shouldn't matter because the released device is deleted from the domain anyways, but this tight coupling would prevent hypothetical re-use of the id.
id->value is zeroed anyway, so there's not much re-using that can happen. If you're not deleting the device, then you're going to call virDomainZPCIAddressAssign{U,F}id() next, and those expect id->isSet to be false. -- Andrea Bolognani / Red Hat / Virtualization
Hello, This patch is generating errors when booting Libvirt in a Power 8 host: 2020-06-26 21:35:49.703+0000: 139213: error : virDomainDeviceInfoFormat:7527 : internal error: Missing uid or fid attribute of zPCI address 2020-06-26 21:35:49.703+0000: 139213: error : virDomainDeviceInfoFormat:7527 : internal error: Missing uid or fid attribute of zPCI address This is not related to Power though. The check for ZPCI address is incomplete is being done prior to the check of ZPCI address is present, in virDomainDeviceInfoFormat(). I already posted a patch that moves the verification to the right code block, avoiding the errors when there is no ZPCI device in the domain: https://www.redhat.com/archives/libvir-list/2020-June/msg01261.html Thanks, DHB
The ZPCI device validation is specific to qemu. So, let us move the ZPCI uid validation out of domain xml parsing into qemu domain device validation. Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/device_conf.c | 3 --- src/libvirt_private.syms | 1 - src/qemu/qemu_validate.c | 18 +++++++++++++++++- src/util/virpci.c | 20 -------------------- src/util/virpci.h | 1 - 5 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 21398e90..a641f3ce 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -77,9 +77,6 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node, def.fid.isSet = true; } - if (!virZPCIDeviceAddressIsValid(&def)) - return -1; - addr->zpci = def; return 0; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a4a09cf9..f0543bcc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2839,7 +2839,6 @@ virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; virZPCIDeviceAddressIsIncomplete; virZPCIDeviceAddressIsPresent; -virZPCIDeviceAddressIsValid; # util/virperf.h diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 0372ae7d..37c59416 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1018,7 +1018,9 @@ static int qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) { - if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci) && + virZPCIDeviceAddressPtr zpci = &info->addr.pci.zpci; + + if (virZPCIDeviceAddressIsPresent(zpci) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1026,6 +1028,20 @@ qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info, return -1; } + /* We don't need to check fid because fid covers + * all range of uint32 type. + */ + if (zpci->uid.isSet && + (zpci->uid.value > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || + zpci->uid.value == 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%.4x', " + "must be > 0x0000 and <= 0x%.4x"), + zpci->uid.value, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); + return -1; + } + return 0; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 40ae5aec..3685f901 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2167,26 +2167,6 @@ virPCIDeviceAddressParse(char *address, } -bool -virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) -{ - /* We don't need to check fid because fid covers - * all range of uint32 type. - */ - if (zpci->uid.isSet && - (zpci->uid.value > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || - zpci->uid.value == 0)) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid PCI address uid='0x%.4x', " - "must be > 0x0000 and <= 0x%.4x"), - zpci->uid.value, - VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); - return false; - } - - return true; -} - bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr) { diff --git a/src/util/virpci.h b/src/util/virpci.h index f198df5d..b3322ba6 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -254,7 +254,6 @@ int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr); bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr); -bool virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, int pfNetDevIdx, -- 2.25.4
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
qemu: move ZPCI uid validation into device validation
The ZPCI device validation is specific to qemu. So, let us move the ZPCI uid validation out of domain xml parsing into qemu domain device validation.
We can just talk about "zPCI validation" instead of singling out the uid attribute specifically. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
1. Test for auto-generating uids while specifying valid fids 2. Test for auto-generating fids while specifying valid uids 3. Test for parse error while specifying a valid fid and an invalid uid 4. Test for parse error while specifying two ZPCI devices with same uid and fid addresses 5. Test for parse error when both uid and fid are set to zero 6. Test for error while specifying uid and not providing ZPCI capability. Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- .../hostdev-vfio-zpci-autogenerate-fids.args | 31 +++++++++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.args | 31 +++++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++++++++++++ .../hostdev-vfio-zpci-duplicate.xml | 30 +++++++++++++ ...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +++++++++ .../hostdev-vfio-zpci-set-zero.xml | 21 +++++++++ tests/qemuxml2argvtest.c | 18 ++++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 43 +++++++++++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 43 +++++++++++++++++++ tests/qemuxml2xmltest.c | 6 +++ 11 files changed, 302 insertions(+) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args new file mode 100644 index 00000000..6485433a --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +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 \ +-realtime mlock=off \ +-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=0000:00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ +-device zpci,uid=5,fid=1,target=hostdev1,id=zpci5 \ +-device vfio-pci,host=0001:00:00.0,id=hostdev1,bus=pci.0,addr=0x2 \ +-device zpci,uid=2,fid=2,target=balloon0,id=zpci2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml new file mode 100644 index 00000000..49c26e24 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml @@ -0,0 +1,29 @@ +<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'> + <zpci uid='0x0001'/> + </address> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'> + <zpci uid='0x0005'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args new file mode 100644 index 00000000..181652cc --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +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 \ +-realtime mlock=off \ +-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=0000:00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ +-device zpci,uid=2,fid=31,target=hostdev1,id=zpci2 \ +-device vfio-pci,host=0000:00:01.0,id=hostdev1,bus=pci.0,addr=0x2 \ +-device zpci,uid=3,fid=1,target=balloon0,id=zpci3 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml new file mode 100644 index 00000000..e74e0116 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml @@ -0,0 +1,29 @@ +<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'> + <zpci fid='0x00000000'/> + </address> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </source> + <address type='pci'> + <zpci fid='0x0000001f'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml new file mode 100644 index 00000000..6062ae49 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.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' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'> + <zpci uid='0x1234' fid='0x00001234'/> + </address> + </hostdev> + <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='0x00' function='0x0'> + <zpci uid='0x1234' fid='0x00001234'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml new file mode 100644 index 00000000..6e710148 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.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='0x000' fid='0x0000001f'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml new file mode 100644 index 00000000..fd3d1193 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.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='0x000' fid='0x00000000'/> + </address> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 765e6fdf..1aa12693 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1743,6 +1743,11 @@ mymain(void) DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-autogenerate-fids", + QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-invalid-uid-valid-fid", + 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, @@ -1750,6 +1755,12 @@ mymain(void) DO_TEST("hostdev-vfio-zpci-autogenerate", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate-uids", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate-fids", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-uid-set-zero", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); @@ -1759,6 +1770,13 @@ mymain(void) QEMU_CAPS_DEVICE_ZPCI); DO_TEST_PARSE_ERROR("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-duplicate", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-set-zero", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("pci-rom", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml new file mode 100644 index 00000000..846b809e --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml @@ -0,0 +1,43 @@ +<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> + <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='0x02' function='0x0'> + <zpci uid='0x0005' fid='0x00000001'/> + </address> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'> + <zpci uid='0x0002' fid='0x00000002'/> + </address> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml new file mode 100644 index 00000000..01647550 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml @@ -0,0 +1,43 @@ +<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> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'> + <zpci uid='0x0002' fid='0x0000001f'/> + </address> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'> + <zpci uid='0x0003' fid='0x00000001'/> + </address> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7fc8a7d6..20243a97 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -519,6 +519,12 @@ mymain(void) DO_TEST("hostdev-vfio-zpci-autogenerate", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate-uids", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate-fids", + 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, -- 2.25.4
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
1. Test for auto-generating uids while specifying valid fids 2. Test for auto-generating fids while specifying valid uids 3. Test for parse error while specifying a valid fid and an invalid uid 4. Test for parse error while specifying two ZPCI devices with same uid and fid addresses 5. Test for parse error when both uid and fid are set to zero 6. Test for error while specifying uid and not providing ZPCI capability.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- .../hostdev-vfio-zpci-autogenerate-fids.args | 31 +++++++++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.args | 31 +++++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++++++++++++ .../hostdev-vfio-zpci-duplicate.xml | 30 +++++++++++++ ...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +++++++++ .../hostdev-vfio-zpci-set-zero.xml | 21 +++++++++ tests/qemuxml2argvtest.c | 18 ++++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 43 +++++++++++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 43 +++++++++++++++++++ tests/qemuxml2xmltest.c | 6 +++ 11 files changed, 302 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Add test with a ZPCI host device and a CCW memballoon device to ensure that CCW address remains the default address assigned. Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- .../hostdev-vfio-zpci-ccw-memballoon.args | 28 ++++++++++++++++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 17 ++++++++++ tests/qemuxml2argvtest.c | 4 +++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 32 +++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +++ 5 files changed, 85 insertions(+) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args new file mode 100644 index 00000000..103c4f72 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args @@ -0,0 +1,28 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-KVMGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-KVMGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-KVMGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-KVMGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name KVMGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=kvm,usb=off,dump-guest-core=off \ +-m 214 \ +-realtime mlock=off \ +-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-KVMGuest1/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=0000:00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml new file mode 100644 index 00000000..ef5e97fc --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml @@ -0,0 +1,17 @@ +<domain type='kvm'> + <name>KVMGuest1</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> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1aa12693..6f5c52b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1776,6 +1776,10 @@ mymain(void) DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-set-zero", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-ccw-memballoon", + QEMU_CAPS_CCW, + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); DO_TEST("pci-rom", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom-disabled", NONE); diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml new file mode 100644 index 00000000..0d18e2d6 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml @@ -0,0 +1,32 @@ +<domain type='kvm'> + <name>KVMGuest1</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='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 20243a97..c2644a18 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -529,6 +529,10 @@ mymain(void) QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-ccw-memballoon", + QEMU_CAPS_CCW, + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); DO_TEST("hostdev-mdev-precreated", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("hostdev-mdev-display", QEMU_CAPS_DEVICE_QXL, -- 2.25.4
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
Add test with a ZPCI host device and a CCW memballoon device to ensure that CCW address remains the default address assigned.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- .../hostdev-vfio-zpci-ccw-memballoon.args | 28 ++++++++++++++++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 17 ++++++++++ tests/qemuxml2argvtest.c | 4 +++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 32 +++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +++ 5 files changed, 85 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Ping, in case you missed it. On 6/18/20 10:25 AM, Shalini Chellathurai Saroja wrote:
The zPCI address validation or autogeneration does not work as expected in the following scenarios 1. uid = 0 and fid = 0 2. uid = 0 and fid > 0 3. uid = 0 and fid not specified 4. uid not specified and fid > 0 5. 2 zPCI devices with uid > 0 and fid not specified.
This is because of the following reasons 1. If uid = 0 or fid = 0 the code assumes that user has not specified the corresponding address 2. If either uid or fid is provided, the code assumes that both uid and fid addresses are specified by the user.
This patch fixes these issues. --- v2: - Call same function to reserve zPCI address when zPCI address is fully/partially/not specified by the user. - Redefine structure of zPCI address. - Add tests to verify the auto-generated xml(qemuxml2xmltest.c). - Separate/merge patches. - Minor changes based on review feedback.
v1: https://www.redhat.com/archives/libvir-list/2020-April/msg00479.html
Shalini Chellathurai Saroja (5): conf: use g_autofree to ensure automatic cleanup conf: fix zPCI address auto-generation on s390 qemu: move ZPCI uid validation into device validation tests: qemu: add more tests for ZPCI on S390 tests: add test with PCI and CCW device
src/conf/device_conf.c | 45 +++++------ src/conf/domain_addr.c | 77 ++++++------------- src/conf/domain_conf.c | 10 ++- src/libvirt_private.syms | 4 +- src/qemu/qemu_command.c | 6 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_validate.c | 18 ++++- src/util/virpci.c | 23 ++---- src/util/virpci.h | 15 +++- .../hostdev-vfio-zpci-autogenerate-fids.args | 31 ++++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++++++ .../hostdev-vfio-zpci-autogenerate-uids.args | 31 ++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++++++ .../hostdev-vfio-zpci-ccw-memballoon.args | 28 +++++++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 17 ++++ .../hostdev-vfio-zpci-duplicate.xml | 30 ++++++++ ...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +++++ .../hostdev-vfio-zpci-set-zero.xml | 21 +++++ .../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++ tests/qemuxml2argvtest.c | 25 ++++++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 43 +++++++++++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 43 +++++++++++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 32 ++++++++ tests/qemuxml2xmltest.c | 10 +++ 24 files changed, 498 insertions(+), 112 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml
On Wed, 2020-06-24 at 18:59 +0200, Shalini Chellathurai Saroja wrote:
Ping, in case you missed it.
Please wait at least a week or so before pinging a series, and refrain from CCing individual developers when you do - we're all subscribed to libvir-list. Anyway I've seen the series and I'm planning to review it, I just haven't gotten around to it yet :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
Shalini Chellathurai Saroja (5): conf: use g_autofree to ensure automatic cleanup conf: fix zPCI address auto-generation on s390 qemu: move ZPCI uid validation into device validation tests: qemu: add more tests for ZPCI on S390 tests: add test with PCI and CCW device
Aside from the comments in patch 2/5, everything looks good to me. The series does, however, not update the release notes (NEWS.rst): can you please post a 6/5 patch that takes care of that, assuming we decide not to go with a respin? Thanks! -- Andrea Bolognani / Red Hat / Virtualization
On 6/25/20 8:01 PM, Andrea Bolognani wrote:
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
Shalini Chellathurai Saroja (5): conf: use g_autofree to ensure automatic cleanup conf: fix zPCI address auto-generation on s390 qemu: move ZPCI uid validation into device validation tests: qemu: add more tests for ZPCI on S390 tests: add test with PCI and CCW device Aside from the comments in patch 2/5, everything looks good to me.
The series does, however, not update the release notes (NEWS.rst): can you please post a 6/5 patch that takes care of that, assuming we decide not to go with a respin?
Hi Andrea, Thank you :-). I have attached the patch to update the release notes with this email.
Thanks!
On Fri, 2020-06-26 at 14:13 +0200, Shalini Chellathurai Saroja wrote:
On 6/25/20 8:01 PM, Andrea Bolognani wrote:
Aside from the comments in patch 2/5, everything looks good to me.
The series does, however, not update the release notes (NEWS.rst): can you please post a 6/5 patch that takes care of that, assuming we decide not to go with a respin?
Hi Andrea,
Thank you :-). I have attached the patch to update the release notes with this email.
Thanks. I tweaked the commit message a bit, and I also had to squash the diff below into patch 2 to make CentOS 7 and macOS happy. The series is now pushed. Have a nice weekend :) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 21398e90f2..64a713a5f9 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -52,7 +52,7 @@ static int virZPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr) { - virZPCIDeviceAddress def = { 0 }; + virZPCIDeviceAddress def = { .uid = { 0 }, .fid = { 0 } }; g_autofree char *uid = NULL; g_autofree char *fid = NULL; -- Andrea Bolognani / Red Hat / Virtualization
participants (5)
-
Andrea Bolognani -
Bjoern Walk -
Boris Fiuczynski -
Daniel Henrique Barboza -
Shalini Chellathurai Saroja