Add new functions to generate zPCI command string and append it to
QEMU command line. And the related tests are added.

Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_command.c                            | 114 +++++++++++++++++++++
 src/qemu/qemu_command.h                            |   4 +
 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args  |   1 +
 .../hostdev-vfio-zpci-autogenerate.args            |  26 +++++
 .../hostdev-vfio-zpci-autogenerate.xml             |  18 ++++
 .../hostdev-vfio-zpci-boundaries.args              |  30 ++++++
 .../hostdev-vfio-zpci-boundaries.xml               |  26 +++++
 .../hostdev-vfio-zpci-multidomain-many.args        |  40 ++++++++
 .../hostdev-vfio-zpci-multidomain-many.xml         |  67 ++++++++++++
 tests/qemuxml2argvdata/hostdev-vfio-zpci.args      |   2 +
 tests/qemuxml2argvtest.c                           |  13 +++
 .../hostdev-vfio-zpci-autogenerate.xml             |  30 ++++++
 .../hostdev-vfio-zpci-boundaries.xml               |  42 ++++++++
 .../hostdev-vfio-zpci-multidomain-many.xml         |  79 ++++++++++++++
 tests/qemuxml2xmltest.c                            |  11 ++
 15 files changed, 503 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml
 create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml
 create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml
 create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d148db90fa..e46c362646 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2151,6 +2151,69 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
     return NULL;
 }
 
+char *
+qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+    virBufferAddLit(&buf, "zpci");
+    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
+    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
+    virBufferAsprintf(&buf, ",target=%s", dev->alias);
+    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
+
+    if (virBufferCheckError(&buf) < 0) {
+        virBufferFreeAndReset(&buf);
+        return NULL;
+    }
+
+    return virBufferContentAndReset(&buf);
+}
+
+int
+qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
+{
+    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+        info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+        return 1;
+    }
+
+    if (info->addr.pci.zpci) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("This QEMU doesn't support zpci devices"));
+        return -1;
+    }
+
+    return 0;
+}
+
+static int
+qemuAppendZPCIDevStr(virCommandPtr cmd,
+                     virDomainDeviceInfoPtr dev)
+{
+    char *devstr = NULL;
+
+    virCommandAddArg(cmd, "-device");
+    if (!(devstr = qemuBuildZPCIDevStr(dev)))
+        return -1;
+
+    virCommandAddArg(cmd, devstr);
+
+    VIR_FREE(devstr);
+    return 0;
+}
+
+static int
+qemuBuildExtensionCommandLine(virCommandPtr cmd,
+                              virDomainDeviceInfoPtr dev)
+{
+    int ret;
+
+    if ((ret = qemuCheckDeviceIsZPCI(dev)) == 1)
+        return qemuAppendZPCIDevStr(cmd, dev);
+
+    return ret;
+}
 
 static int
 qemuBuildFloppyCommandLineOptions(virCommandPtr cmd,
@@ -2294,6 +2357,9 @@ qemuBuildDiskCommandLine(virCommandPtr cmd,
                                                   bootindex) < 0)
                 return -1;
         } else {
+            if (qemuBuildExtensionCommandLine(cmd, &disk->info) < 0)
+                    return -1;
+
             virCommandAddArg(cmd, "-device");
 
             if (!(optstr = qemuBuildDiskDeviceStr(def, disk, bootindex,
@@ -2493,6 +2559,9 @@ qemuBuildFSDevCommandLine(virCommandPtr cmd,
         virCommandAddArg(cmd, optstr);
         VIR_FREE(optstr);
 
+        if (qemuBuildExtensionCommandLine(cmd, &fs->info) < 0)
+            return -1;
+
         virCommandAddArg(cmd, "-device");
         if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps)))
             return -1;
@@ -2977,6 +3046,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
                 goto cleanup;
 
             if (devstr) {
+                if (qemuBuildExtensionCommandLine(cmd, &cont->info) < 0) {
+                    VIR_FREE(devstr);
+                    goto cleanup;
+                }
                 virCommandAddArg(cmd, "-device");
                 virCommandAddArg(cmd, devstr);
                 VIR_FREE(devstr);
@@ -3780,6 +3853,9 @@ qemuBuildWatchdogCommandLine(virCommandPtr cmd,
     if (!def->watchdog)
         return 0;
 
+    if (qemuBuildExtensionCommandLine(cmd, &def->watchdog->info) < 0)
+        return -1;
+
     virCommandAddArg(cmd, "-device");
 
     optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps);
@@ -3864,6 +3940,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,
     if (qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio, qemuCaps) < 0)
         goto error;
 
+    if (qemuBuildExtensionCommandLine(cmd, &def->memballoon->info) < 0)
+        goto error;
+
     virCommandAddArg(cmd, "-device");
     virCommandAddArgBuffer(cmd, &buf);
     return 0;
@@ -4086,6 +4165,9 @@ qemuBuildInputCommandLine(virCommandPtr cmd,
         virDomainInputDefPtr input = def->inputs[i];
         char *devstr = NULL;
 
+        if (qemuBuildExtensionCommandLine(cmd, &input->info) < 0)
+            return -1;
+
         if (qemuBuildInputDevStr(&devstr, def, input, qemuCaps) < 0)
             return -1;
 
@@ -4227,6 +4309,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd,
         if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) {
             virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL);
         } else {
+            if (qemuBuildExtensionCommandLine(cmd, &sound->info) < 0)
+                return -1;
+
             virCommandAddArg(cmd, "-device");
             if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps)))
                 return -1;
@@ -4466,6 +4551,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
         if (video->primary) {
             if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
 
+                if (qemuBuildExtensionCommandLine(cmd,
+                                                  &def->videos[i]->info) < 0)
+                    return -1;
                 virCommandAddArg(cmd, "-device");
 
                 if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps)))
@@ -4478,6 +4566,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
                     return -1;
             }
         } else {
+            if (qemuBuildExtensionCommandLine(cmd, &def->videos[i]->info) < 0)
+                return -1;
+
             virCommandAddArg(cmd, "-device");
 
             if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps)))
@@ -5349,6 +5440,10 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
                                      VIR_COMMAND_PASS_FD_CLOSE_PARENT);
                 }
             }
+
+            if (qemuBuildExtensionCommandLine(cmd, hostdev->info) < 0)
+                return -1;
+
             virCommandAddArg(cmd, "-device");
             devstr = qemuBuildPCIHostdevDevStr(def, hostdev, bootIndex,
                                                configfd_name, qemuCaps);
@@ -5815,6 +5910,9 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
         virCommandAddArgBuffer(cmd, &buf);
 
         /* add the device */
+        if (qemuBuildExtensionCommandLine(cmd, &rng->info) < 0)
+            return -1;
+
         if (!(tmp = qemuBuildRNGDevStr(def, rng, qemuCaps)))
             return -1;
         virCommandAddArgList(cmd, "-device", tmp, NULL);
@@ -8371,6 +8469,9 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
     virCommandAddArg(cmd, "-netdev");
     virCommandAddArg(cmd, netdev);
 
+    if (qemuBuildExtensionCommandLine(cmd, &net->info) < 0)
+        goto cleanup;
+
     if (!(nic = qemuBuildNicDevStr(def, net, bootindex,
                                    queues, qemuCaps))) {
         goto cleanup;
@@ -8652,6 +8753,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
      *   New way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1
      */
     if (qemuDomainSupportsNicdev(def, net)) {
+        if (qemuBuildExtensionCommandLine(cmd, &net->info) < 0)
+            goto cleanup;
+
         if (!(nic = qemuBuildNicDevStr(def, net, bootindex,
                                        vhostfdSize, qemuCaps)))
             goto cleanup;
@@ -9076,6 +9180,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
 
     switch ((virDomainShmemModel)shmem->model) {
     case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
+        if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
+            return -1;
+
         devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps);
         break;
 
@@ -9094,6 +9201,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
 
         ATTRIBUTE_FALLTHROUGH;
     case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
+        if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
+            return -1;
+
         devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps);
         break;
 
@@ -10253,6 +10363,10 @@ qemuBuildVsockCommandLine(virCommandPtr cmd,
 
     virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
     priv->vhostfd = -1;
+
+    if (qemuBuildExtensionCommandLine(cmd, &vsock->info) < 0)
+        goto cleanup;
+
     virCommandAddArgList(cmd, "-device", devstr, NULL);
 
     ret = 0;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index cf17dc1ede..bee4029b75 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -175,6 +175,10 @@ char *qemuBuildRedirdevDevStr(const virDomainDef *def,
                               virDomainRedirdevDefPtr dev,
                               virQEMUCapsPtr qemuCaps);
 
+char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev);
+
+int qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info);
+
 int qemuNetworkPrepareDevices(virDomainDefPtr def);
 
 int qemuGetDriveSourceString(virStorageSourcePtr src,
diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
index 20e63a15b5..ffb5d1597e 100644
--- a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
+++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
@@ -21,6 +21,7 @@ server,nowait \
 -no-shutdown \
 -boot c \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
+-device zpci,uid=25,fid=31,target=virtio-disk0,id=zpci25 \
 -device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk0,\
 id=virtio-disk0 \
 -device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args
new file mode 100644
index 0000000000..e5987e1053
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args
@@ -0,0 +1,26 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+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 \
+-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 \
+-boot c \
+-device zpci,uid=1,fid=0,target=hostdev0,id=zpci1 \
+-device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \
+-device zpci,uid=2,fid=1,target=balloon0,id=zpci2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
new file mode 100644
index 0000000000..36161006ab
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
@@ -0,0 +1,18 @@
+<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'/>
+    </hostdev>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args
new file mode 100644
index 0000000000..dcbb179a7d
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args
@@ -0,0 +1,30 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+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 \
+-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 \
+-boot c \
+-device zpci,uid=3,fid=2,target=pci.1,id=zpci3 \
+-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \
+-device zpci,uid=65535,fid=4294967295,target=hostdev0,id=zpci65535 \
+-device vfio-pci,host=ffff:00:00.0,id=hostdev0,bus=pci.1,addr=0x1f \
+-device zpci,uid=1,fid=0,target=hostdev1,id=zpci1 \
+-device vfio-pci,host=00:00.0,id=hostdev1,bus=pci.0,addr=0x2 \
+-device zpci,uid=2,fid=1,target=balloon0,id=zpci2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml
new file mode 100644
index 0000000000..779eb12ac2
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml
@@ -0,0 +1,26 @@
+<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'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0xffff' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x1f' function='0x0' fid='0xffffffff' uid='0xffff'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' fid='0x00000000' uid='0x0001'/>
+    </hostdev>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
new file mode 100644
index 0000000000..476237fc45
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
@@ -0,0 +1,40 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+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 \
+-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 \
+-boot c \
+-device zpci,uid=35,fid=63,target=hostdev0,id=zpci35 \
+-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=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=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=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 \
+-device zpci,uid=23,fid=3,target=hostdev6,id=zpci23 \
+-device vfio-pci,host=0007:00:00.0,id=hostdev6,bus=pci.0,addr=0x4 \
+-device zpci,uid=4,fid=40,target=hostdev7,id=zpci4 \
+-device vfio-pci,host=0008:00:00.0,id=hostdev7,bus=pci.0,addr=0x6 \
+-device zpci,uid=5,fid=4,target=balloon0,id=zpci5 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml
new file mode 100644
index 0000000000..e69714fc9c
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml
@@ -0,0 +1,67 @@
+<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'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0023' fid='0x0000003f'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' uid='0x0035' fid='0x00000068'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0' uid='0x0053'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0006' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' fid='0x00000072'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' uid='0x0017'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' fid='0x00000028'/>
+    </hostdev>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args
index 622c504da0..80de60acaa 100644
--- a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args
@@ -20,5 +20,7 @@ server,nowait \
 -rtc base=utc \
 -no-shutdown \
 -boot c \
+-device zpci,uid=25,fid=31,target=hostdev0,id=zpci25 \
 -device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x8 \
+-device zpci,uid=1,fid=0,target=balloon0,id=zpci1 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x1
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 2bfc3ed215..21890a7a4e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1595,6 +1595,19 @@ mymain(void)
             QEMU_CAPS_VFIO_PCI_DISPLAY);
     DO_TEST("hostdev-vfio-zpci",
             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,
+            QEMU_CAPS_DEVICE_ZPCI);
+    DO_TEST("hostdev-vfio-zpci-autogenerate",
+            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,
+            QEMU_CAPS_DEVICE_ZPCI);
+    DO_TEST_FAILURE("hostdev-vfio-zpci",
+                    QEMU_CAPS_DEVICE_VFIO_PCI);
     DO_TEST("pci-rom", NONE);
     DO_TEST("pci-rom-disabled", NONE);
     DO_TEST("pci-rom-disabled-invalid", NONE);
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml
new file mode 100644
index 0000000000..8647cab1fc
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml
@@ -0,0 +1,30 @@
+<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' uid='0x0001' fid='0x00000000'/>
+    </hostdev>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0002' fid='0x00000001'/>
+    </memballoon>
+    <panic model='s390'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml
new file mode 100644
index 0000000000..0b48c7658a
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml
@@ -0,0 +1,42 @@
+<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'/>
+    <controller type='pci' index='1' model='pci-bridge'>
+      <model name='pci-bridge'/>
+      <target chassisNr='1'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
+    </controller>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0xffff' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x1f' function='0x0' uid='0xffff' fid='0xffffffff'/>
+    </hostdev>
+    <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='0x02' function='0x0' uid='0x0001' fid='0x00000000'/>
+    </hostdev>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0002' fid='0x00000001'/>
+    </memballoon>
+    <panic model='s390'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
new file mode 100644
index 0000000000..9a31057d0b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
@@ -0,0 +1,79 @@
+<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='0x0001' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0023' fid='0x0000003f'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0035' fid='0x00000068'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0001' fid='0x00000000'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0' uid='0x0002' fid='0x00000001'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0' uid='0x0053' fid='0x00000002'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0006' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' uid='0x0003' fid='0x00000072'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' uid='0x0017' fid='0x00000003'/>
+    </hostdev>
+    <hostdev mode='subsystem' type='pci' managed='no'>
+      <driver name='vfio'/>
+      <source>
+        <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/>
+      </source>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' uid='0x0004' fid='0x00000028'/>
+    </hostdev>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0005' fid='0x00000004'/>
+    </memballoon>
+    <panic model='s390'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 6e41fd9bff..ed78233754 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -463,6 +463,17 @@ mymain(void)
     DO_TEST("hostdev-pci-address", NONE);
     DO_TEST("hostdev-vfio", NONE);
     DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW);
+    DO_TEST("hostdev-vfio-zpci-multidomain-many",
+            QEMU_CAPS_DEVICE_VFIO_PCI,
+            QEMU_CAPS_DEVICE_PCI_BRIDGE,
+            QEMU_CAPS_DEVICE_ZPCI);
+    DO_TEST("hostdev-vfio-zpci-autogenerate",
+            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,
+            QEMU_CAPS_DEVICE_ZPCI);
     DO_TEST("hostdev-mdev-precreated", NONE);
     DO_TEST("hostdev-mdev-display", QEMU_CAPS_VFIO_PCI_DISPLAY);
     DO_TEST("pci-rom", NONE);
-- 
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +char *
> +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferAddLit(&buf, "zpci");
> +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
> +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
> +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
> +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);

Just making sure: is the uid a good choice for naming the zpci
device? I would perhaps expect the fid to be in there as well,
but since both have to be unique (right?) I guess it doesn't
really matter...

> +
> +    if (virBufferCheckError(&buf) < 0) {
> +        virBufferFreeAndReset(&buf);
> +        return NULL;
> +    }
> +
> +    return virBufferContentAndReset(&buf);
> +}
> +
> +int
> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)

Based on the name, this is a predicate: it should return a
boolean value rather than an int.

> +{
> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +        info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +        return 1;
> +    }
> +
> +    if (info->addr.pci.zpci) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("This QEMU doesn't support zpci devices"));
> +        return -1;
> +    }

Not sure about this second check. I guess the logic is supposed to
be that, when passed a "proper" zPCI device, the function would
have returned with the first check, and if we find a zPCI address
after that it's an error. Makes sense, but it's kinda obfuscated
and I'm not sure the error message is accurate: can it really only
be a problem of the QEMU binary not supporting the zPCI feature,
or could we have gotten here because the user is trying to assing
zPCI addresses to devices that don't support them?

Either way, calling a predicate should never result in an error
being reported, so you need to move this check somewhere else.

[...]
> +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
> @@ -0,0 +1,18 @@
> +<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'/>
> +    </hostdev>
> +  </devices>
> +</domain>

This test case would have been a great addition to the previous
commit, where you actually introduced the ability to automatically
allocate zPCI addresses...


Another thing that I forgot to ask earlier and this is as good a
time as any: once zPCI support has been merged, will users have
to opt-in to it using

  <address type='pci'/>

or will they get zPCI devices by default? And if so, will it still
be possible for them to get CCW devices instead if wanted?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 08/16/2018 06:31 PM, Andrea Bolognani wrote:
> Another thing that I forgot to ask earlier and this is as good a
> time as any: once zPCI support has been merged, will users have
> to opt-in to it using
> 
>    <address type='pci'/>
> 
> or will they get zPCI devices by default? And if so, will it still
> be possible for them to get CCW devices instead if wanted?

On S390 the default behavior is to assign CCW addresses. This has not 
and is not going to be changed. Therefore a user has to explicitly 
specify a PCI address or in your words: zPCI is an opt-in.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, 2018-08-17 at 08:35 +0200, Boris Fiuczynski wrote:
> On 08/16/2018 06:31 PM, Andrea Bolognani wrote:
> > Another thing that I forgot to ask earlier and this is as good a
> > time as any: once zPCI support has been merged, will users have
> > to opt-in to it using
> > 
> >    <address type='pci'/>
> > 
> > or will they get zPCI devices by default? And if so, will it still
> > be possible for them to get CCW devices instead if wanted?
> 
> On S390 the default behavior is to assign CCW addresses. This has not
> and is not going to be changed. Therefore a user has to explicitly
> specify a PCI address or in your words: zPCI is an opt-in.

Sounds good to me! Just making sure :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/17 上午12:31, Andrea Bolognani 写道:
> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> [...]
>> +char *
>> +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
>> +{
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    virBufferAddLit(&buf, "zpci");
>> +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
>> +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
>> +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
>> +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
> Just making sure: is the uid a good choice for naming the zpci
> device? I would perhaps expect the fid to be in there as well,
> but since both have to be unique (right?) I guess it doesn't
> really matter...
Either uid or fid, the value must be unique. But uid is defined by user.
Of course, we also give the permission to define fid. But uid is more
appropriate.
>> +
>> +    if (virBufferCheckError(&buf) < 0) {
>> +        virBufferFreeAndReset(&buf);
>> +        return NULL;
>> +    }
>> +
>> +    return virBufferContentAndReset(&buf);
>> +}
>> +
>> +int
>> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
> Based on the name, this is a predicate: it should return a
> boolean value rather than an int.
0 means it's not zpci. 1 means it's zpci. -1 means error.
>
>> +{
>> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
>> +        info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
>> +        return 1;
>> +    }
>> +
>> +    if (info->addr.pci.zpci) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                       _("This QEMU doesn't support zpci devices"));
>> +        return -1;
>> +    }
> Not sure about this second check. I guess the logic is supposed to
> be that, when passed a "proper" zPCI device, the function would
> have returned with the first check, and if we find a zPCI address
> after that it's an error. Makes sense, but it's kinda obfuscated
> and I'm not sure the error message is accurate: can it really only
> be a problem of the QEMU binary not supporting the zPCI feature,
> or could we have gotten here because the user is trying to assing
> zPCI addresses to devices that don't support them?
Yes, it's because user could define zpci address in xml but
there's no zpci support.
>
> Either way, calling a predicate should never result in an error
> being reported, so you need to move this check somewhere else.
Acutally I have found out a proper place to add this check for a long time.
So this is the best place to add, at least I think for now.
>
> [...]
>> +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
>> @@ -0,0 +1,18 @@
>> +<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'/>
>> +    </hostdev>
>> +  </devices>
>> +</domain>
> This test case would have been a great addition to the previous
> commit, where you actually introduced the ability to automatically
> allocate zPCI addresses...
>
>
> Another thing that I forgot to ask earlier and this is as good a
> time as any: once zPCI support has been merged, will users have
> to opt-in to it using
>
>    <address type='pci'/>
>
> or will they get zPCI devices by default? And if so, will it still
> be possible for them to get CCW devices instead if wanted?
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote:
> 在 2018/8/17 上午12:31, Andrea Bolognani 写道:
> > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> > > +    virBufferAddLit(&buf, "zpci");
> > > +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
> > > +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
> > > +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
> > > +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
> > 
> > Just making sure: is the uid a good choice for naming the zpci
> > device? I would perhaps expect the fid to be in there as well,
> > but since both have to be unique (right?) I guess it doesn't
> > really matter...
> 
> Either uid or fid, the value must be unique. But uid is defined by user.
> Of course, we also give the permission to define fid. But uid is more
> appropriate.

So which is unique: uid, fid, or the combination of the two?
Could I have

  -device zpci,uid=1,fid=1
  -device zpci,uid=1,fid=2

or would that not work? What about

  -device zpci,uid=1,fid=1
  -device zpci,uid=2,fid=1

would that be okay?

> > > +int
> > > +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
> > 
> > Based on the name, this is a predicate: it should return a
> > boolean value rather than an int.
> 
> 0 means it's not zpci. 1 means it's zpci. -1 means error.

My point is that a funtion called fooIsBaz() should only ever
check whether the foo in question is indeed a baz, without taking
any other action such as reporting errors. Leave that to the
caller, or give the function a different name.

> > Either way, calling a predicate should never result in an error
> > being reported, so you need to move this check somewhere else.
> 
> Acutally I have found out a proper place to add this check for a long time.
> So this is the best place to add, at least I think for now.

qemuDomainDeviceDefValidate() looks like a reasonable entry point
for checks such as "you specified a zPCI address but this is not
an s390 guest" or "your configuration requires zPCI but the QEMU
binary doesn't support that feature". Both of the cases above
should have proper test suite coverage, by the way.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/20 下午7:14, Andrea Bolognani 写道:
> On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote:
>> 在 2018/8/17 上午12:31, Andrea Bolognani 写道:
>>> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
>>>> +    virBufferAddLit(&buf, "zpci");
>>>> +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
>>>> +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
>>>> +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
>>>> +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
>>> Just making sure: is the uid a good choice for naming the zpci
>>> device? I would perhaps expect the fid to be in there as well,
>>> but since both have to be unique (right?) I guess it doesn't
>>> really matter...
>> Either uid or fid, the value must be unique. But uid is defined by user.
>> Of course, we also give the permission to define fid. But uid is more
>> appropriate.
> So which is unique: uid, fid, or the combination of the two?
> Could I have
>
>    -device zpci,uid=1,fid=1
>    -device zpci,uid=1,fid=2
>
> or would that not work? What about
>
>    -device zpci,uid=1,fid=1
>    -device zpci,uid=2,fid=1
>
> would that be okay?
Both can't work. FID must be unique and UID must be also unique.
They are independent.
>
>>>> +int
>>>> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
>>> Based on the name, this is a predicate: it should return a
>>> boolean value rather than an int.
>> 0 means it's not zpci. 1 means it's zpci. -1 means error.
> My point is that a funtion called fooIsBaz() should only ever
> check whether the foo in question is indeed a baz, without taking
> any other action such as reporting errors. Leave that to the
> caller, or give the function a different name.
I think moving to the caller is proper.
>
>>> Either way, calling a predicate should never result in an error
>>> being reported, so you need to move this check somewhere else.
>> Acutally I have found out a proper place to add this check for a long time.
>> So this is the best place to add, at least I think for now.
> qemuDomainDeviceDefValidate() looks like a reasonable entry point
> for checks such as "you specified a zPCI address but this is not
> an s390 guest" or "your configuration requires zPCI but the QEMU
> binary doesn't support that feature". Both of the cases above
> should have proper test suite coverage, by the way.
>
How about my idea mentioned above?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote:
> 在 2018/8/20 下午7:14, Andrea Bolognani 写道:
> > So which is unique: uid, fid, or the combination of the two?
> > Could I have
> > 
> >    -device zpci,uid=1,fid=1
> >    -device zpci,uid=1,fid=2
> > 
> > or would that not work? What about
> > 
> >    -device zpci,uid=1,fid=1
> >    -device zpci,uid=2,fid=1
> > 
> > would that be okay?
> 
> Both can't work. FID must be unique and UID must be also unique.
> They are independent.

Got it, thanks.

> > My point is that a funtion called fooIsBaz() should only ever
> > check whether the foo in question is indeed a baz, without taking
> > any other action such as reporting errors. Leave that to the
> > caller, or give the function a different name.
> 
> I think moving to the caller is proper.
> 
> > qemuDomainDeviceDefValidate() looks like a reasonable entry point
> > for checks such as "you specified a zPCI address but this is not
> > an s390 guest" or "your configuration requires zPCI but the QEMU
> > binary doesn't support that feature". Both of the cases above
> > should have proper test suite coverage, by the way.
> 
> How about my idea mentioned above?

You mean moving the check to the caller?

I think qemuDomainDeviceDefValidate() is a better choice because
it should allow you to implement the checks once, potentially with
more context such as information about the domain and QEMU
capabilities, and have them performed regardless of whether the
device is being processed eg. during regular parsing or hotplug.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/22 下午4:42, Andrea Bolognani 写道:
> On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote:
>> 在 2018/8/20 下午7:14, Andrea Bolognani 写道:
>>> So which is unique: uid, fid, or the combination of the two?
>>> Could I have
>>>
>>>     -device zpci,uid=1,fid=1
>>>     -device zpci,uid=1,fid=2
>>>
>>> or would that not work? What about
>>>
>>>     -device zpci,uid=1,fid=1
>>>     -device zpci,uid=2,fid=1
>>>
>>> would that be okay?
>> Both can't work. FID must be unique and UID must be also unique.
>> They are independent.
> Got it, thanks.
>
>>> My point is that a funtion called fooIsBaz() should only ever
>>> check whether the foo in question is indeed a baz, without taking
>>> any other action such as reporting errors. Leave that to the
>>> caller, or give the function a different name.
>> I think moving to the caller is proper.
>>
>>> qemuDomainDeviceDefValidate() looks like a reasonable entry point
>>> for checks such as "you specified a zPCI address but this is not
>>> an s390 guest" or "your configuration requires zPCI but the QEMU
>>> binary doesn't support that feature". Both of the cases above
>>> should have proper test suite coverage, by the way.
>> How about my idea mentioned above?
> You mean moving the check to the caller?
Yes.
>
> I think qemuDomainDeviceDefValidate() is a better choice because
> it should allow you to implement the checks once, potentially with
> more context such as information about the domain and QEMU
> capabilities, and have them performed regardless of whether the
> device is being processed eg. during regular parsing or hotplug.
>
IIUC, in qemuDomainDeviceDefValidate(), we have to directly access
device_info from DeviceDef to check zpci. I think direct accessing is
insecure and we have to process those device types that has device_info.
In addition, these is a special case that address is not defined and pci
address type will be chosen by default.

I found there are a lot of caps check code while building command line.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list