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: 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>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 1d06981a617b84f2625ae0ec8ac7c25c3a1c9ff9..64a713a5f909f17c624d3d4c8df81cc6e7cc4655 100644 (file)
--- a/src/conf/device_conf.c
+++ b/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)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 8623e79daf8e5c9734b7b98481055582acdab448..2f9ff899d7f293caed16c4d63189bea79358e0f2 100644 (file)
--- 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);
+        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;

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fc7fcfb0c6b3e232abebed998fd49fa56446cdcc..31ba78b950059567819d778608ff326f4ab2c066 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/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;
 

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index df85b751ca35824ac049ccf46a43fca251b8465d..83e38dfc9ad35035cf492e4aed15eb9d89174d34 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2839,7 +2839,8 @@ virPCIHeaderTypeToString;
 virPCIIsVirtualFunction;
 virPCIStubDriverTypeFromString;
 virPCIStubDriverTypeToString;
-virZPCIDeviceAddressIsEmpty;
+virZPCIDeviceAddressIsIncomplete;
+virZPCIDeviceAddressIsPresent;
 virZPCIDeviceAddressIsValid;
 
 

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f355ddbfd50447a1d116a68c3932b8b764331c78..6e7fd59561ea134a1fe366d94d15c9cf0a766a0d 100644 (file)
--- 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 dc3bd8245fd5217929e5ba2efebe026f5601f6da..3954ad1109ad43a7aacc6cbcbe80a3d82b6e4106 100644 (file)
--- 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 b13c03759e28ea3808235cb016c8a1748e0c5b31..07826fb7ab92ad7b7c2f5ef81cc3f3bd0615846a 100644 (file)
--- 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 eb7ca501087cb9f7b1310843e98b62ba253781da..4843195a6701adfcf75934c00c411eec0b633e2a 100644 (file)
--- 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 f16d23614a6303a3f84c6030ac7307e95765b9a9..f198df5d7ce4de64bd70beaba83fd45744bbefd0 100644 (file)
--- 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 (file)
index 0000000..6bfbfe6
--- /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 248ce0781155605b773969458df1fa85a438b634..60943876466303e6d59a65ab588489c8f0bd80d6 100644 (file)
--- a/tests/qemuxml2argvtest.c
+++ b/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,