Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; --- 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

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(a)linux.ibm.com&gt; Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com&gt; --- 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 6/25/20 7:43 PM, Andrea Bolognani wrote: Hi Andrea, I agree that it looks nice and sparkling. :D Thanks. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; -- 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

Andrea Bolognani <abologna(a)redhat.com&gt; [2020-06-25, 07:43PM +0200]: 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. 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(a)linux.ibm.com&gt; -- 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