Currently, if uid/fid is specified as zero by the user, it is treated
as if the value is not specified. This bug is fixed by introducing two
boolean values to identify if uid and fid are specified by the user.

Also, 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: 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>
---
 src/conf/device_conf.c                        | 12 ++--
 src/conf/domain_addr.c                        | 59 +++++++++++++------
 src/conf/domain_conf.c                        |  2 +-
 src/libvirt_private.syms                      |  3 +-
 src/qemu/qemu_validate.c                      |  2 +-
 src/util/virpci.c                             | 17 ++++--
 src/util/virpci.h                             |  5 +-
 .../hostdev-vfio-zpci-multidomain-many.args   |  6 +-
 .../hostdev-vfio-zpci-multidomain-many.xml    |  6 +-
 9 files changed, 75 insertions(+), 37 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 001ed929..c03356e7 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -65,8 +65,7 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
                            _("Cannot parse <address> 'uid' attribute"));
             return -1;
         }
-        if (!virZPCIDeviceAddressIsValid(&def))
-            return -1;
+        def.uid_set = true;
     }
 
     if (fid) {
@@ -75,8 +74,11 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
                            _("Cannot parse <address> 'fid' attribute"));
             return -1;
         }
+        def.fid_set = true;
     }
 
+    if (!virZPCIDeviceAddressIsValid(&def))
+        return -1;
 
     addr->zpci = def;
 
@@ -191,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 e8629b3e..d1cd0804 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -145,12 +145,18 @@ static void
 virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
                                virZPCIDeviceAddressPtr addr)
 {
-    if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
+    if (!zpciIds)
         return;
 
-    virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+    if (addr->uid_set) {
+        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+        addr->uid_set = false;
+    }
 
-    virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
+    if (addr->fid_set) {
+        virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
+        addr->fid_set = false;
+    }
 }
 
 
@@ -186,12 +192,16 @@ static int
 virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
                                 virZPCIDeviceAddressPtr addr)
 {
-    if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
+    if (addr->uid_set &&
+        virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
         return -1;
 
-    if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
-        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
-        return -1;
+    if (addr->fid_set) {
+        if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
+            if (addr->uid_set)
+                virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+            return -1;
+        }
     }
 
     return 0;
@@ -202,14 +212,28 @@ static int
 virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
                                     virZPCIDeviceAddressPtr addr)
 {
-    if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
-        return -1;
+    bool uid_set, fid_set = false;
 
-    if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
-        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
-        return -1;
+    if (!addr->uid_set) {
+        if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
+            return -1;
+        uid_set = true;
+    }
+
+    if (!addr->fid_set) {
+        if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
+            if (uid_set)
+                virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+            return -1;
+        }
+        fid_set = true;
     }
 
+    if (uid_set)
+        addr->uid_set = true;
+    if (fid_set)
+        addr->fid_set = true;
+
     return 0;
 }
 
@@ -234,7 +258,7 @@ 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)
             return -1;
@@ -246,6 +270,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
     return 0;
 }
 
+
 static int
 virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
                                        virPCIDeviceAddressPtr addr)
@@ -253,10 +278,10 @@ 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 virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
     }
 
     return 0;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c478d795..5df80cae 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
                               virTristateSwitchTypeToString(info->addr.pci.multi));
         }
 
-        if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+        if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
             virBufferAsprintf(&childBuf,
                               "<zpci uid='0x%.4x' fid='0x%.8x'/>\n",
                               info->addr.pci.zpci.uid,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ec367653..18687563 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2821,7 +2821,8 @@ virPCIHeaderTypeToString;
 virPCIIsVirtualFunction;
 virPCIStubDriverTypeFromString;
 virPCIStubDriverTypeToString;
-virZPCIDeviceAddressIsEmpty;
+virZPCIDeviceAddressIsIncomplete;
+virZPCIDeviceAddressIsPresent;
 virZPCIDeviceAddressIsValid;
 
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 149b9e73..22fdfd11 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -952,7 +952,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 6c7e6bbc..b38f717c 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2172,8 +2172,9 @@ 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_set &&
+        (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+         zpci->uid == 0)) {
         virReportError(VIR_ERR_XML_ERROR,
                        _("Invalid PCI address uid='0x%.4x', "
                          "must be > 0x0000 and <= 0x%.4x"),
@@ -2186,11 +2187,19 @@ virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
 }
 
 bool
-virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr)
 {
-    return !(addr->uid || addr->fid);
+    return !addr->uid_set || !addr->fid_set;
 }
 
+
+bool
+virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr)
+{
+    return addr->uid_set || addr->fid_set;
+}
+
+
 #ifdef __linux__
 
 virPCIDeviceAddressPtr
diff --git a/src/util/virpci.h b/src/util/virpci.h
index f16d2361..e937348f 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
 struct _virZPCIDeviceAddress {
     unsigned int uid; /* exempt from syntax-check */
     unsigned int fid;
+    bool uid_set;
+    bool fid_set;
     /* Don't forget to update virPCIDeviceAddressCopy if needed. */
 };
 
@@ -245,8 +247,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-multidomain-many.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
index 3dd9a25f..0fae78db 100644
--- a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
@@ -27,11 +27,11 @@ server,nowait \
 -device vfio-pci,host=0001:00:00.0,id=hostdev0,bus=pci.0,addr=0x3 \
 -device zpci,uid=53,fid=104,target=hostdev1,id=zpci53 \
 -device vfio-pci,host=0002:00:00.0,id=hostdev1,bus=pci.0,addr=0x1 \
--device zpci,uid=1,fid=1,target=hostdev2,id=zpci1 \
+-device zpci,uid=1,fid=0,target=hostdev2,id=zpci1 \
 -device vfio-pci,host=0003:00:00.0,id=hostdev2,bus=pci.0,addr=0x2 \
--device zpci,uid=2,fid=2,target=hostdev3,id=zpci2 \
+-device zpci,uid=2,fid=1,target=hostdev3,id=zpci2 \
 -device vfio-pci,host=0004:00:00.0,id=hostdev3,bus=pci.0,addr=0x5 \
--device zpci,uid=83,fid=0,target=hostdev4,id=zpci83 \
+-device zpci,uid=83,fid=2,target=hostdev4,id=zpci83 \
 -device vfio-pci,host=0005:00:00.0,id=hostdev4,bus=pci.0,addr=0x7 \
 -device zpci,uid=3,fid=114,target=hostdev5,id=zpci3 \
 -device vfio-pci,host=0006:00:00.0,id=hostdev5,bus=pci.0,addr=0x9 \
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
index e56106d1..ecfc15c4 100644
--- a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
@@ -39,7 +39,7 @@
         <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
       </source>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'>
-        <zpci uid='0x0001' fid='0x00000001'/>
+        <zpci uid='0x0001' fid='0x00000000'/>
       </address>
     </hostdev>
     <hostdev mode='subsystem' type='pci' managed='no'>
@@ -48,7 +48,7 @@
         <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
       </source>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'>
-        <zpci uid='0x0002' fid='0x00000002'/>
+        <zpci uid='0x0002' fid='0x00000001'/>
       </address>
     </hostdev>
     <hostdev mode='subsystem' type='pci' managed='no'>
@@ -57,7 +57,7 @@
         <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
       </source>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'>
-        <zpci uid='0x0053' fid='0x00000000'/>
+        <zpci uid='0x0053' fid='0x00000002'/>
       </address>
     </hostdev>
     <hostdev mode='subsystem' type='pci' managed='no'>
-- 
2.21.1

On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
> +++ b/src/conf/domain_addr.c
> @@ -145,12 +145,18 @@ static void
>  virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
>                                 virZPCIDeviceAddressPtr addr)
>  {
> -    if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
> +    if (!zpciIds)
>          return;
>  
> -    virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> +    if (addr->uid_set) {
> +        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> +        addr->uid_set = false;
> +    }

I think it would be cleaner to handle the boolean flags in the same
spot the values are taken care of, that is, in the Release{U,F}id()
functions.

> @@ -186,12 +192,16 @@ static int
>  virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
>                                  virZPCIDeviceAddressPtr addr)
>  {
> -    if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
> +    if (addr->uid_set &&
> +        virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
>          return -1;

Same here...

>  
> -    if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> -        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> -        return -1;
> +    if (addr->fid_set) {
> +        if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> +            if (addr->uid_set)
> +                virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> +            return -1;
> +        }
>      }
>  
>      return 0;
> @@ -202,14 +212,28 @@ static int
>  virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
>                                      virZPCIDeviceAddressPtr addr)
>  {
> -    if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> -        return -1;
> +    bool uid_set, fid_set = false;
>  
> -    if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
> -        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> -        return -1;
> +    if (!addr->uid_set) {
> +        if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> +            return -1;
> +        uid_set = true;
> +    }

... and here. Basicall, push all handling of boolean flags one layer
down, where the actual values are taken care of.

> @@ -234,7 +258,7 @@ 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)
>              return -1;
> @@ -246,6 +270,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>      return 0;
>  }
>  
> +
>  static int
>  virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
>                                         virPCIDeviceAddressPtr addr)
> @@ -253,10 +278,10 @@ 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 virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
>      }

I think the semantics for these functions need to be reconsidered.

The way they currently work, as evidenced by EnsureAddr(), is that
you either have a fully-specified address provided by the user, in
which case you want to reserve that address, or you have an empty
address because the user didn't provide ZPCI information, in which
case you allocate a new full address based on what uids and fids are
still available and use that one.

With your changes, we can now find ourselves in a hybrid situation,
where half of the ZPCI address has been provided by the user but the
remaining half is up to us... Ultimately, it might make sense to have
ReserveAddr(), ReserveNextAddr() and EnsureAddr() all call to the
same function which does something along the lines of

  if (!zpci->uid_set)
      AssignUid(zpci);
  if (!zpci->fid_set)
      AssignFid(zpci);

  ReserveUid(zpci);
  ReserveFid(zpci);

because that's ultimately what we're achieving anyway, only with
more obfuscation.

> +++ b/src/conf/domain_conf.c
> @@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>                                virTristateSwitchTypeToString(info->addr.pci.multi));
>          }
>  
> -        if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
> +        if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
>              virBufferAsprintf(&childBuf,
>                                "<zpci uid='0x%.4x' fid='0x%.8x'/>\n",
>                                info->addr.pci.zpci.uid,

By the time we get here, we should either have a complete ZPCI
address or no ZPCI address at all. So I would rewrite this as

  if (IsIncomplete(zpci))
      virReportError(VIR_ERR_INTERNAL, ...);

  if (!IsPresent(zpci))
      virBufferAsprintf(...);

with the first check being there just for extra safety (This Should
Never Happen™).

> +++ b/src/util/virpci.h
> @@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
>  struct _virZPCIDeviceAddress {
>      unsigned int uid; /* exempt from syntax-check */
>      unsigned int fid;
> +    bool uid_set;
> +    bool fid_set;

I believe we mostly use camelCase for struct members, although I
don't think we're too consistent with that :)

I wonder if it would make sense to tie the two bits of information
together more explicitly, eg.

  typedef struct _virZPCIDeviceAddressID {
      unsigned int value;
      bool isSet;
  } virZPCIDeviceAddressID;

  typedef struct _virZPCIDeviceAddress {
      virZPCIDeviceAddressID uid;
      virZPCIDeviceAddressID fid;
  } virZPCIDeviceAddress;

> +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
> @@ -39,7 +39,7 @@
>          <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
>        </source>
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'>
> -        <zpci uid='0x0001' fid='0x00000001'/>
> +        <zpci uid='0x0001' fid='0x00000000'/>
>        </address>
>      </hostdev>
>      <hostdev mode='subsystem' type='pci' managed='no'>
> @@ -48,7 +48,7 @@
>          <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
>        </source>
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'>
> -        <zpci uid='0x0002' fid='0x00000002'/>
> +        <zpci uid='0x0002' fid='0x00000001'/>
>        </address>
>      </hostdev>
>      <hostdev mode='subsystem' type='pci' managed='no'>
> @@ -57,7 +57,7 @@
>          <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
>        </source>
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'>
> -        <zpci uid='0x0053' fid='0x00000000'/>
> +        <zpci uid='0x0053' fid='0x00000002'/>

I'm not entirely clear on why these generated ZPCI addresses would
change. Can you explain that to me?

-- 
Andrea Bolognani / Red Hat / Virtualization

Hi Andrea,

Thank you for the review.

On 6/3/20 12:58 PM, Andrea Bolognani wrote:
> On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
>> +++ b/src/conf/domain_addr.c
>> @@ -145,12 +145,18 @@ static void
>>   virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
>>                                  virZPCIDeviceAddressPtr addr)
>>   {
>> -    if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
>> +    if (!zpciIds)
>>           return;
>>   
>> -    virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
>> +    if (addr->uid_set) {
>> +        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
>> +        addr->uid_set = false;
>> +    }
> I think it would be cleaner to handle the boolean flags in the same
> spot the values are taken care of, that is, in the Release{U,F}id()
> functions.
ok, I will do so.
>
>> @@ -186,12 +192,16 @@ static int
>>   virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
>>                                   virZPCIDeviceAddressPtr addr)
>>   {
>> -    if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
>> +    if (addr->uid_set &&
>> +        virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
>>           return -1;
> Same here...
>
>>   
>> -    if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
>> -        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
>> -        return -1;
>> +    if (addr->fid_set) {
>> +        if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
>> +            if (addr->uid_set)
>> +                virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
>> +            return -1;
>> +        }
>>       }
>>   
>>       return 0;
>> @@ -202,14 +212,28 @@ static int
>>   virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
>>                                       virZPCIDeviceAddressPtr addr)
>>   {
>> -    if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
>> -        return -1;
>> +    bool uid_set, fid_set = false;
>>   
>> -    if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
>> -        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
>> -        return -1;
>> +    if (!addr->uid_set) {
>> +        if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
>> +            return -1;
>> +        uid_set = true;
>> +    }
> ... and here. Basicall, push all handling of boolean flags one layer
> down, where the actual values are taken care of.
>
>> @@ -234,7 +258,7 @@ 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)
>>               return -1;
>> @@ -246,6 +270,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>>       return 0;
>>   }
>>   
>> +
>>   static int
>>   virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
>>                                          virPCIDeviceAddressPtr addr)
>> @@ -253,10 +278,10 @@ 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 virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
>>       }
> I think the semantics for these functions need to be reconsidered.
>
> The way they currently work, as evidenced by EnsureAddr(), is that
> you either have a fully-specified address provided by the user, in
> which case you want to reserve that address, or you have an empty
> address because the user didn't provide ZPCI information, in which
> case you allocate a new full address based on what uids and fids are
> still available and use that one.
>
> With your changes, we can now find ourselves in a hybrid situation,
> where half of the ZPCI address has been provided by the user but the
> remaining half is up to us... Ultimately, it might make sense to have
> ReserveAddr(), ReserveNextAddr() and EnsureAddr() all call to the
> same function which does something along the lines of
>
>    if (!zpci->uid_set)
>        AssignUid(zpci);
>    if (!zpci->fid_set)
>        AssignFid(zpci);
>
>    ReserveUid(zpci);
>    ReserveFid(zpci);
>
> because that's ultimately what we're achieving anyway, only with
> more obfuscation.
ok, I will do so.
>> +++ b/src/conf/domain_conf.c
>> @@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>>                                 virTristateSwitchTypeToString(info->addr.pci.multi));
>>           }
>>   
>> -        if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
>> +        if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
>>               virBufferAsprintf(&childBuf,
>>                                 "<zpci uid='0x%.4x' fid='0x%.8x'/>\n",
>>                                 info->addr.pci.zpci.uid,
> By the time we get here, we should either have a complete ZPCI
> address or no ZPCI address at all. So I would rewrite this as
>
>    if (IsIncomplete(zpci))
>        virReportError(VIR_ERR_INTERNAL, ...);
>
>    if (!IsPresent(zpci))
>        virBufferAsprintf(...);
>
> with the first check being there just for extra safety (This Should
> Never Happen™).
Sure.
>
>> +++ b/src/util/virpci.h
>> @@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
>>   struct _virZPCIDeviceAddress {
>>       unsigned int uid; /* exempt from syntax-check */
>>       unsigned int fid;
>> +    bool uid_set;
>> +    bool fid_set;
> I believe we mostly use camelCase for struct members, although I
> don't think we're too consistent with that :)
ok, I will camelCase the struct members.
>
> I wonder if it would make sense to tie the two bits of information
> together more explicitly, eg.
>
>    typedef struct _virZPCIDeviceAddressID {
>        unsigned int value;
>        bool isSet;
>    } virZPCIDeviceAddressID;
>
>    typedef struct _virZPCIDeviceAddress {
>        virZPCIDeviceAddressID uid;
>        virZPCIDeviceAddressID fid;
>    } virZPCIDeviceAddress;
I will redefine the struct as above.
>
>> +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
>> @@ -39,7 +39,7 @@
>>           <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
>>         </source>
>>         <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'>
>> -        <zpci uid='0x0001' fid='0x00000001'/>
>> +        <zpci uid='0x0001' fid='0x00000000'/>
>>         </address>
>>       </hostdev>
>>       <hostdev mode='subsystem' type='pci' managed='no'>
>> @@ -48,7 +48,7 @@
>>           <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
>>         </source>
>>         <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'>
>> -        <zpci uid='0x0002' fid='0x00000002'/>
>> +        <zpci uid='0x0002' fid='0x00000001'/>
>>         </address>
>>       </hostdev>
>>       <hostdev mode='subsystem' type='pci' managed='no'>
>> @@ -57,7 +57,7 @@
>>           <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
>>         </source>
>>         <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'>
>> -        <zpci uid='0x0053' fid='0x00000000'/>
>> +        <zpci uid='0x0053' fid='0x00000002'/>
> I'm not entirely clear on why these generated ZPCI addresses would
> change. Can you explain that to me?

Sure:-). It changes in this version because at first the user specified 
addresses are reserved and then the addresses which are not specified by 
the user are assigned and reserved.

In the current master code, as uid and fid are correlated, both uid and 
fid are reserved, when either one of them is specified by the user. So 
for the pci device with uid =  '0x0053', the code assumes that user has 
specified fid as 0 (which is not true) and reserves fid as 0.

Warm Regards

Shalini C S



>

On Tue, 2020-06-16 at 12:43 +0200, Shalini Chellathurai Saroja wrote:
> On 6/3/20 12:58 PM, Andrea Bolognani wrote:
> > On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
> > > +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
> > > @@ -39,7 +39,7 @@
> > >           <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
> > >         </source>
> > >         <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'>
> > > -        <zpci uid='0x0001' fid='0x00000001'/>
> > > +        <zpci uid='0x0001' fid='0x00000000'/>
> > >         </address>
> > >       </hostdev>
> > >       <hostdev mode='subsystem' type='pci' managed='no'>
> > > @@ -48,7 +48,7 @@
> > >           <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
> > >         </source>
> > >         <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'>
> > > -        <zpci uid='0x0002' fid='0x00000002'/>
> > > +        <zpci uid='0x0002' fid='0x00000001'/>
> > >         </address>
> > >       </hostdev>
> > >       <hostdev mode='subsystem' type='pci' managed='no'>
> > > @@ -57,7 +57,7 @@
> > >           <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
> > >         </source>
> > >         <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'>
> > > -        <zpci uid='0x0053' fid='0x00000000'/>
> > > +        <zpci uid='0x0053' fid='0x00000002'/>
> > 
> > I'm not entirely clear on why these generated ZPCI addresses would
> > change. Can you explain that to me?
> 
> Sure:-). It changes in this version because at first the user specified 
> addresses are reserved and then the addresses which are not specified by 
> the user are assigned and reserved.
> 
> In the current master code, as uid and fid are correlated, both uid and 
> fid are reserved, when either one of them is specified by the user. So 
> for the pci device with uid =  '0x0053', the code assumes that user has 
> specified fid as 0 (which is not true) and reserves fid as 0.

Makes sense! Thanks for the explanation :)

-- 
Andrea Bolognani / Red Hat / Virtualization