conf: fix zPCI address auto-generation on s390

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: NBjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: NBoris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: NShalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: NAndrea Bolognani <abologna@redhat.com>

src/conf/device_conf.c

@@ -52,29 +52,32 @@ 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;
 
     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)

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);
+        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;
     }
 
@@ -58,7 +65,7 @@ static int
 virDomainZPCIAddressReserveUid(virHashTablePtr set,
                                virZPCIDeviceAddressPtr addr)
 {
-    return virDomainZPCIAddressReserveId(set, addr->uid, "uid");
+    return virDomainZPCIAddressReserveId(set, &addr->uid, "uid");
 }
 
 
@@ -66,17 +73,20 @@ static int
 virDomainZPCIAddressReserveFid(virHashTablePtr set,
                                virZPCIDeviceAddressPtr addr)
 {
-    return virDomainZPCIAddressReserveId(set, addr->fid, "fid");
+    return virDomainZPCIAddressReserveId(set, &addr->fid, "fid");
 }
 
 
 static int
 virDomainZPCIAddressAssignId(virHashTablePtr set,
-                             unsigned int *id,
+                             virZPCIDeviceAddressID *id,
                              unsigned int min,
                              unsigned int max,
                              const char *name)
 {
+    if (id->isSet)
+        return 0;
+
     while (virHashLookup(set, &min)) {
         if (min == max) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -86,7 +96,9 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
         }
         ++min;
     }
-    *id = min;
+
+    id->value = min;
+    id->isSet = true;
 
     return 0;
 }
@@ -112,16 +124,20 @@ virDomainZPCIAddressAssignFid(virHashTablePtr set,
 
 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;
 }
 
 
@@ -145,47 +161,24 @@ 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)
+virDomainZPCIAddressEnsureAddr(virDomainZPCIAddressIdsPtr zpciIds,
+                                virZPCIDeviceAddressPtr addr)
 {
-    if (virDomainZPCIAddressAssignFid(fids, zpci) < 0)
+    if (virDomainZPCIAddressAssignUid(zpciIds->uids, addr) < 0)
         return -1;
 
-    if (virDomainZPCIAddressReserveFid(fids, zpci) < 0)
+    if (virDomainZPCIAddressAssignFid(zpciIds->fids, addr) < 0)
         return -1;
 
-    return 0;
-}
-
-
-static int
-virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
-                                virZPCIDeviceAddressPtr addr)
-{
     if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
         return -1;
 
@@ -198,22 +191,6 @@ virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
 }
 
 
-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)
@@ -222,7 +199,7 @@ virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
         /* Reserve uid/fid to ZPCI device which has defined uid/fid
          * in the domain.
          */
-        return virDomainZPCIAddressReserveAddr(addrs->zpciIds, &addr->zpci);
+        return virDomainZPCIAddressEnsureAddr(addrs->zpciIds, &addr->zpci);
     }
 
     return 0;
@@ -234,9 +211,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 (virDomainZPCIAddressEnsureAddr(addrs->zpciIds, &zpci) < 0)
             return -1;
 
         if (!addrs->dryRun)
@@ -246,6 +223,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
     return 0;
 }
 
+
 static int
 virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
                                        virPCIDeviceAddressPtr addr)
@@ -253,10 +231,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 (virDomainZPCIAddressEnsureAddr(addrs->zpciIds, zpci) < 0)
+            return -1;
     }
 
     return 0;

src/conf/domain_conf.c

@@ -7522,11 +7522,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;
 

src/libvirt_private.syms

@@ -2839,7 +2839,8 @@ virPCIHeaderTypeToString;
 virPCIIsVirtualFunction;
 virPCIStubDriverTypeFromString;
 virPCIStubDriverTypeToString;
-virZPCIDeviceAddressIsEmpty;
+virZPCIDeviceAddressIsIncomplete;
+virZPCIDeviceAddressIsPresent;
 virZPCIDeviceAddressIsValid;
 
 

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);
 }

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;

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",

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

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,

tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml

+<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>

tests/qemuxml2argvtest.c

@@ -1752,6 +1752,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,