This patch introduces new XML parser/formatter functions. Uid is
16-bit and non-zero. Fid is 32-bit. They are added as two new
attributes of PCI address, and parsed/formatted along with PCI
address parser/formatter. The related test is also 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>
---
docs/schemas/basictypes.rng | 23 ++++++++
docs/schemas/domaincommon.rng | 1 +
src/conf/device_conf.c | 69 ++++++++++++++++++++++
src/conf/device_conf.h | 2 +
src/conf/domain_addr.c | 3 +
src/conf/domain_conf.c | 6 ++
src/libvirt_private.syms | 1 +
src/util/virpci.h | 3 +
tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 25 ++++++++
tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 ++++++
tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 23 ++++++++
tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++++++
tests/qemuxml2argvtest.c | 7 +++
tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 +++++++++
tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++++++++++
tests/qemuxml2xmltest.c | 6 ++
16 files changed, 264 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml
create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml
create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 14a3670e5c..3defb56316 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -65,6 +65,17 @@
</data>
</choice>
</define>
+ <define name="uint32">
+ <choice>
+ <data type="string">
+ <param name="pattern">(0x)?[0-9a-fA-F]{1,8}</param>
+ </data>
+ <data type="unsignedInt">
+ <param name="minInclusive">0</param>
+ <param name="maxInclusive">4294967295</param>
+ </data>
+ </choice>
+ </define>
<define name="UUID">
<choice>
@@ -111,6 +122,18 @@
</attribute>
</optional>
</define>
+ <define name="zpciaddress">
+ <optional>
+ <attribute name="uid">
+ <ref name="uint16"/>
+ </attribute>
+ </optional>
+ <optional>
+ <attribute name="fid">
+ <ref name="uint32"/>
+ </attribute>
+ </optional>
+ </define>
<!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" -->
<!-- The lowest bit of the 1st byte is the "multicast" bit. a -->
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3796eb4b5e..2ccf777d20 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5220,6 +5220,7 @@
<value>pci</value>
</attribute>
<ref name="pciaddress"/>
+ <ref name="zpciaddress"/>
</group>
<group>
<attribute name="type">
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 7a8f84e036..5ae990afdb 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -32,6 +32,65 @@
#define VIR_FROM_THIS VIR_FROM_DEVICE
+static int
+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) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%x', "
+ "must be > 0x0 and <= 0x%x"),
+ zpci->uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
+ return 0;
+ }
+
+ return 1;
+}
+
+static int
+virZPCIDeviceAddressParseXML(xmlNodePtr node,
+ virPCIDeviceAddressPtr addr)
+{
+ char *uid, *fid;
+ int ret = -1;
+ virZPCIDeviceAddress def = { 0 };
+
+ uid = virXMLPropString(node, "uid");
+ fid = virXMLPropString(node, "fid");
+
+ if (uid) {
+ if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'uid' attribute"));
+ goto cleanup;
+ }
+ }
+
+ if (fid) {
+ if (virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'fid' attribute"));
+ goto cleanup;
+ }
+ }
+
+ if (!virZPCIDeviceAddressIsEmpty(&def) &&
+ !virZPCIDeviceAddressIsValid(&def))
+ goto cleanup;
+
+ addr->zpci = def;
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(uid);
+ VIR_FREE(fid);
+ return ret;
+}
+
int
virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
virDomainDeviceInfoPtr src)
@@ -213,6 +272,13 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info)
}
+bool
+virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+{
+ return !(addr->uid || addr->fid);
+}
+
+
int
virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
goto cleanup;
+ if (virZPCIDeviceAddressParseXML(node, addr) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 11baf0c1e3..7e98b4ace0 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -194,6 +194,8 @@ bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr);
bool virDeviceInfoPCIAddressIsWanted(const virDomainDeviceInfo *info);
bool virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info);
+bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr);
+
int virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr);
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 363e5b21dc..62932e4e62 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
dev->isolationGroup, function) < 0)
return -1;
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
+ addr.zpci = dev->addr.pci.zpci;
+
if (virDomainPCIAddressReserveAddrInternal(addrs, &addr, flags,
dev->isolationGroup, false) < 0)
return -1;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 38cac07913..85234bc8d2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6514,6 +6514,12 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
info->addr.pci.slot,
info->addr.pci.function);
}
+ if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+ virBufferAsprintf(buf, " uid='0x%.4x'",
+ info->addr.pci.zpci.uid);
+ virBufferAsprintf(buf, " fid='0x%.8x'",
+ info->addr.pci.zpci.fid);
+ }
if (info->addr.pci.multi) {
virBufferAsprintf(buf, " multifunction='%s'",
virTristateSwitchTypeToString(info->addr.pci.multi));
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7ee524ead0..12f051468e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -104,6 +104,7 @@ virPCIDeviceAddressFormat;
virPCIDeviceAddressIsEmpty;
virPCIDeviceAddressIsValid;
virPCIDeviceAddressParseXML;
+virZPCIDeviceAddressIsEmpty;
# conf/domain_addr.h
diff --git a/src/util/virpci.h b/src/util/virpci.h
index b7bcfa6d9f..e2ffe11625 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr;
typedef struct _virPCIDeviceList virPCIDeviceList;
typedef virPCIDeviceList *virPCIDeviceListPtr;
+# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX
+# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX
+
typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
struct _virZPCIDeviceAddress {
diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
new file mode 100644
index 0000000000..8ac435cb3e
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
@@ -0,0 +1,25 @@
+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 \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
+-device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk0,\
+id=virtio-disk0,bootindex=1 \
+-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000
diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml
new file mode 100644
index 0000000000..7887b97b2c
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='virtio'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/>
+ </disk>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args
new file mode 100644
index 0000000000..d6b1520c47
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args
@@ -0,0 +1,23 @@
+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 \
+-device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x8 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x1
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml
new file mode 100644
index 0000000000..cde333e220
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml
@@ -0,0 +1,19 @@
+<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' managed='no'>
+ <driver name='vfio'/>
+ <source>
+ <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
+ </source>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 9bfe864597..e96d00cec7 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1058,6 +1058,10 @@ mymain(void)
QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI,
QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
+ DO_TEST("disk-virtio-s390-zpci",
+ QEMU_CAPS_DEVICE_ZPCI,
+ QEMU_CAPS_CCW,
+ QEMU_CAPS_VIRTIO_S390);
DO_TEST("disk-order", QEMU_CAPS_VIRTIO_BLK_SCSI);
DO_TEST("disk-virtio-queues",
QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES);
@@ -1651,6 +1655,9 @@ mymain(void)
DO_TEST_PARSE_ERROR("hostdev-mdev-display-missing-graphics",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_VFIO_PCI_DISPLAY);
+ DO_TEST("hostdev-vfio-zpci",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
DO_TEST("pci-rom", NONE);
DO_TEST("pci-rom-disabled", NONE);
DO_TEST("pci-rom-disabled-invalid", NONE);
diff --git a/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml
new file mode 100644
index 0000000000..39b5acdf3b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml
@@ -0,0 +1,29 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</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>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='virtio'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/>
+ </disk>
+ <controller type='pci' index='0' model='pci-root'/>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
+ </memballoon>
+ <panic model='s390'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml
new file mode 100644
index 0000000000..1f48c44e30
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.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='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/>
+ </hostdev>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
+ </memballoon>
+ <panic model='s390'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 43fd4e5f0f..47f3b9431b 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -379,6 +379,9 @@ mymain(void)
QEMU_CAPS_VIRTIO_SCSI);
DO_TEST("disk-virtio-scsi-ioeventfd",
QEMU_CAPS_VIRTIO_SCSI);
+ DO_TEST("disk-virtio-s390-zpci",
+ QEMU_CAPS_DEVICE_ZPCI,
+ QEMU_CAPS_CCW);
DO_TEST("disk-scsi-megasas",
QEMU_CAPS_SCSI_MEGASAS);
DO_TEST("disk-scsi-mptsas1068",
@@ -460,6 +463,9 @@ mymain(void)
DO_TEST("hostdev-usb-address", NONE);
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-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-listOn Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> +static int
> +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
This is a predicate, so it should return a bool.
[...]
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid PCI address uid='0x%x', "
> + "must be > 0x0 and <= 0x%x"),
> + zpci->uid,
> + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
You should always use "0x%.4x" when printing uids.
[...]
> +static int
> +virZPCIDeviceAddressParseXML(xmlNodePtr node,
> + virPCIDeviceAddressPtr addr)
> +{
> + char *uid, *fid;
> + int ret = -1;
> + virZPCIDeviceAddress def = { 0 };
One variable per line, please.
Also structs are usually declared first and 'ret' is usually last,
but that's admittedly not very important :)
[...]
> + if (uid) {
> + if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Cannot parse <address> 'uid' attribute"));
> + goto cleanup;
> + }
> + }
You can convert the above to
if (uid &&
virStrToLong_uip(...) < 0) {
...
}
and do the same with fid.
[...]
> +bool
> +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
> +{
> + return !(addr->uid || addr->fid);
> +}
This function belongs in virpci, besides the struct definition and
the very much related virPCIDeviceAddressIsEmpty().
[...]
> @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
> goto cleanup;
>
> + if (virZPCIDeviceAddressParseXML(node, addr) < 0)
> + goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse
the zPCI part, and that's literally embedded in the PCI part now;
on the other hand, we have functions to verify separately whether
the PCI and zPCI parts are empty, which is pretty weird.
I guess we can leave as it is for now, but the semantics are muddy
enough that I can pretty much guarantee someone will have to clean
them up at some point down the line.
[...]
> @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> dev->isolationGroup, function) < 0)
> return -1;
>
> + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
> + addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set
of zPCI ids, the same way it's allocating a new PCI address right
above, or do nothing at all because zPCI address allocation is
handled by later calling virDomainZPCIAddressReserveNextAddr()?
This is basically another manifestation of the issue I mentioned
above: we don't seem to have a well-defined and strictly adhered
to idea of how the PCI part and zPCI part should relate to each
other, so they're either considered separate entities or part of
the same entity depending on which APIs you're looking at.
[...]
> + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
> + virBufferAsprintf(buf, " uid='0x%.4x'",
> + info->addr.pci.zpci.uid);
> + virBufferAsprintf(buf, " fid='0x%.8x'",
> + info->addr.pci.zpci.fid);
You only need a single call to virBufferAsprintf() here.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
在 2018/9/11 下午8:05, Andrea Bolognani 写道:
> On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> [...]
>> +static int
>> +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
> This is a predicate, so it should return a bool.
OK.
>
> [...]
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("Invalid PCI address uid='0x%x', "
>> + "must be > 0x0 and <= 0x%x"),
>> + zpci->uid,
>> + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
> You should always use "0x%.4x" when printing uids.
ditto
>
> [...]
>> +static int
>> +virZPCIDeviceAddressParseXML(xmlNodePtr node,
>> + virPCIDeviceAddressPtr addr)
>> +{
>> + char *uid, *fid;
>> + int ret = -1;
>> + virZPCIDeviceAddress def = { 0 };
> One variable per line, please.
>
> Also structs are usually declared first and 'ret' is usually last,
> but that's admittedly not very important :)
ditto.
I'm very glad that you could give me so many valuable
comments even if it's just about code style.
>
> [...]
>> + if (uid) {
>> + if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot parse <address> 'uid' attribute"));
>> + goto cleanup;
>> + }
>> + }
> You can convert the above to
>
> if (uid &&
> virStrToLong_uip(...) < 0) {
> ...
> }
>
> and do the same with fid.
OK.
>
> [...]
>> +bool
>> +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
>> +{
>> + return !(addr->uid || addr->fid);
>> +}
> This function belongs in virpci, besides the struct definition and
> the very much related virPCIDeviceAddressIsEmpty().
I'm not very clear with your comment. Do you mean I
should move it to other place?
>
> [...]
>> @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
>> if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
>> goto cleanup;
>>
>> + if (virZPCIDeviceAddressParseXML(node, addr) < 0)
>> + goto cleanup;
> I'm not super convinced about this.
>
> On one hand, it makes it so callers can't possibly forget to parse
> the zPCI part, and that's literally embedded in the PCI part now;
> on the other hand, we have functions to verify separately whether
> the PCI and zPCI parts are empty, which is pretty weird.
It's weird indeed. But if we merge zPCI part check into PCI code. We might
have to merge other code. But PCI address has extension only on S390
at least now.
>
> I guess we can leave as it is for now, but the semantics are muddy
> enough that I can pretty much guarantee someone will have to clean
> them up at some point down the line.
OK.
>
> [...]
>> @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>> dev->isolationGroup, function) < 0)
>> return -1;
>>
>> + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
>> + addr.zpci = dev->addr.pci.zpci;
> I'm confused by this part. Shouldn't this either request a new set
> of zPCI ids, the same way it's allocating a new PCI address right
> above, or do nothing at all because zPCI address allocation is
> handled by later calling virDomainZPCIAddressReserveNextAddr()?
Here, we want to store the defined zPCI address which has been reserved.
If we don't do this, we might lose zPCI address and do reservation again
but the reserved one are still existing in zPCI set.
For this problem, I once thought about adding extension address into
DeviceInfo next to PCI address embraced in a struct. Then here is
not a problem.
>
> This is basically another manifestation of the issue I mentioned
> above: we don't seem to have a well-defined and strictly adhered
> to idea of how the PCI part and zPCI part should relate to each
> other, so they're either considered separate entities or part of
> the same entity depending on which APIs you're looking at.
I think the above idea I thought about before is like what you said?
>
> [...]
>> + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
>> + virBufferAsprintf(buf, " uid='0x%.4x'",
>> + info->addr.pci.zpci.uid);
>> + virBufferAsprintf(buf, " fid='0x%.8x'",
>> + info->addr.pci.zpci.fid);
> You only need a single call to virBufferAsprintf() here.
>
OK.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午8:05, Andrea Bolognani 写道:
> > On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> > [...]
> > > +bool
> > > +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
> > > +{
> > > + return !(addr->uid || addr->fid);
> > > +}
> >
> > This function belongs in virpci, besides the struct definition and
> > the very much related virPCIDeviceAddressIsEmpty().
>
> I'm not very clear with your comment. Do you mean I
> should move it to other place?
Yeah, the function and its definition should be in src/util/virpci.c
and src/util/virpci.h respectively.
I realize now that virPCIDeviceAddressIsValid() and
virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I
swear that I posted patches moving them over... My bad, I'll do
that right away.
Sorry for the confusion.
> > [...]
> > > @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
> > > goto cleanup;
> > >
> > > + if (virZPCIDeviceAddressParseXML(node, addr) < 0)
> > > + goto cleanup;
> >
> > I'm not super convinced about this.
> >
> > On one hand, it makes it so callers can't possibly forget to parse
> > the zPCI part, and that's literally embedded in the PCI part now;
> > on the other hand, we have functions to verify separately whether
> > the PCI and zPCI parts are empty, which is pretty weird.
>
> It's weird indeed. But if we merge zPCI part check into PCI code. We might
> have to merge other code. But PCI address has extension only on S390
> at least now.
That's not necessarily a problem, at least in principle.
See all the code dealing with "isolation groups", for example:
while the concept is only ever really used for pSeries guests, it's
implemented in a way that's designed to stay out of the way in all
other cases, and the result is that you won't have to worry about
it except when needed.
The zPCI code should ideally behave similarly.
> > I guess we can leave as it is for now, but the semantics are muddy
> > enough that I can pretty much guarantee someone will have to clean
> > them up at some point down the line.
>
> OK.
> >
> > [...]
> > > @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> > > dev->isolationGroup, function) < 0)
> > > return -1;
> > >
> > > + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
> > > + addr.zpci = dev->addr.pci.zpci;
> >
> > I'm confused by this part. Shouldn't this either request a new set
> > of zPCI ids, the same way it's allocating a new PCI address right
> > above, or do nothing at all because zPCI address allocation is
> > handled by later calling virDomainZPCIAddressReserveNextAddr()?
>
> Here, we want to store the defined zPCI address which has been reserved.
> If we don't do this, we might lose zPCI address and do reservation again
> but the reserved one are still existing in zPCI set.
I can't picture the exact scenario right now but I assume something
like that can happen if you have
<address type='pci' uid='0x0001' fid='0x00000001'/>
in which case the zPCI part has already been provided by the user
but we still need to allocate the PCI part ourselves.
Speaking of which, I wonder if something like
<address type='pci'>
<zpci uid='0x0001' fid='0x00000001'/>
</address>
would be a more appropriate syntax: not only it clearly shows that
the uid and fid attributes are connected to zPCI, but if we ever
introduce additional PCI address extensions they could be similarly
be represented as childs of <address> instead of adding even more
attributes to the existing element.
Either way, this kind of ad-hoc messing with the zPCI part seems
to me like clear evidence of the fact that we need to step back and
rethink the way the various pieces of the puzzle fit together.
>From the high level point of view, code outside of conf/ should,
for the most part, not need to concern itself with zPCI at all: it
would eg. ask for a PCI address to be allocated, and if the device
in question can be a zPCI device then a zPCI extension address will
be allocated for it as part of the same function call; the only
part of qemu/ that should care about the zPCI address is the one
generating the relevant command line arguments.
Can you try and see whether this kind of API would work?
> For this problem, I once thought about adding extension address into
> DeviceInfo next to PCI address embraced in a struct. Then here is
> not a problem.
That could almost work, seeing how virDomainDeviceInfoFormat()
doesn't use virPCIDeviceAddressFormat() to format the PCI address[1],
but at least in theory you should be able to take a
virPCIDeviceAddress and turn it into a string without having to peek
into other objects such as the parent virDomainDeviceInfo, so that
approach wouldn't be very clean at all.
> > This is basically another manifestation of the issue I mentioned
> > above: we don't seem to have a well-defined and strictly adhered
> > to idea of how the PCI part and zPCI part should relate to each
> > other, so they're either considered separate entities or part of
> > the same entity depending on which APIs you're looking at.
>
> I think the above idea I thought about before is like what you said?
Do you mean the idea of moving the zPCI part out of the PCI address?
If so, I've already replied above; if not, please be more specific :)
[1] Which is arguably an issue that should be resolved as well...
There are a few places where virPCIDeviceAddressFormat() *is*
used, and those won't ever format the zPCI stuff.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
在 2018/9/13 下午9:58, Andrea Bolognani 写道:
> On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
>> 在 2018/9/11 下午8:05, Andrea Bolognani 写道:
>>> On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
>>> [...]
>>>> +bool
>>>> +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
>>>> +{
>>>> + return !(addr->uid || addr->fid);
>>>> +}
>>> This function belongs in virpci, besides the struct definition and
>>> the very much related virPCIDeviceAddressIsEmpty().
>> I'm not very clear with your comment. Do you mean I
>> should move it to other place?
> Yeah, the function and its definition should be in src/util/virpci.c
> and src/util/virpci.h respectively.
>
> I realize now that virPCIDeviceAddressIsValid() and
> virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I
> swear that I posted patches moving them over... My bad, I'll do
> that right away.
>
> Sorry for the confusion.
Got it.
>
>>> [...]
>>>> @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
>>>> if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
>>>> goto cleanup;
>>>>
>>>> + if (virZPCIDeviceAddressParseXML(node, addr) < 0)
>>>> + goto cleanup;
>>> I'm not super convinced about this.
>>>
>>> On one hand, it makes it so callers can't possibly forget to parse
>>> the zPCI part, and that's literally embedded in the PCI part now;
>>> on the other hand, we have functions to verify separately whether
>>> the PCI and zPCI parts are empty, which is pretty weird.
>> It's weird indeed. But if we merge zPCI part check into PCI code. We might
>> have to merge other code. But PCI address has extension only on S390
>> at least now.
> That's not necessarily a problem, at least in principle.
>
> See all the code dealing with "isolation groups", for example:
> while the concept is only ever really used for pSeries guests, it's
> implemented in a way that's designed to stay out of the way in all
> other cases, and the result is that you won't have to worry about
> it except when needed.
>
> The zPCI code should ideally behave similarly.
>
>>> I guess we can leave as it is for now, but the semantics are muddy
>>> enough that I can pretty much guarantee someone will have to clean
>>> them up at some point down the line.
>> OK.
>>> [...]
>>>> @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>>>> dev->isolationGroup, function) < 0)
>>>> return -1;
>>>>
>>>> + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
>>>> + addr.zpci = dev->addr.pci.zpci;
>>> I'm confused by this part. Shouldn't this either request a new set
>>> of zPCI ids, the same way it's allocating a new PCI address right
>>> above, or do nothing at all because zPCI address allocation is
>>> handled by later calling virDomainZPCIAddressReserveNextAddr()?
>> Here, we want to store the defined zPCI address which has been reserved.
>> If we don't do this, we might lose zPCI address and do reservation again
>> but the reserved one are still existing in zPCI set.
> I can't picture the exact scenario right now but I assume something
> like that can happen if you have
>
> <address type='pci' uid='0x0001' fid='0x00000001'/>
>
> in which case the zPCI part has already been provided by the user
> but we still need to allocate the PCI part ourselves.
>
> Speaking of which, I wonder if something like
>
> <address type='pci'>
> <zpci uid='0x0001' fid='0x00000001'/>
> </address>
>
> would be a more appropriate syntax: not only it clearly shows that
> the uid and fid attributes are connected to zPCI, but if we ever
> introduce additional PCI address extensions they could be similarly
> be represented as childs of <address> instead of adding even more
> attributes to the existing element.
>
> Either way, this kind of ad-hoc messing with the zPCI part seems
> to me like clear evidence of the fact that we need to step back and
> rethink the way the various pieces of the puzzle fit together.
>
> >From the high level point of view, code outside of conf/ should,
> for the most part, not need to concern itself with zPCI at all: it
> would eg. ask for a PCI address to be allocated, and if the device
> in question can be a zPCI device then a zPCI extension address will
> be allocated for it as part of the same function call; the only
> part of qemu/ that should care about the zPCI address is the one
> generating the relevant command line arguments.
>
> Can you try and see whether this kind of API would work?
I did a simple test. It worked. Do you prefer this way?
>
>> For this problem, I once thought about adding extension address into
>> DeviceInfo next to PCI address embraced in a struct. Then here is
>> not a problem.
> That could almost work, seeing how virDomainDeviceInfoFormat()
> doesn't use virPCIDeviceAddressFormat() to format the PCI address[1],
> but at least in theory you should be able to take a
> virPCIDeviceAddress and turn it into a string without having to peek
> into other objects such as the parent virDomainDeviceInfo, so that
> approach wouldn't be very clean at all.
Right. That's why I finally gave up it.
>
>>> This is basically another manifestation of the issue I mentioned
>>> above: we don't seem to have a well-defined and strictly adhered
>>> to idea of how the PCI part and zPCI part should relate to each
>>> other, so they're either considered separate entities or part of
>>> the same entity depending on which APIs you're looking at.
>> I think the above idea I thought about before is like what you said?
> Do you mean the idea of moving the zPCI part out of the PCI address?
> If so, I've already replied above; if not, please be more specific :)
Yes.
>
>
> [1] Which is arguably an issue that should be resolved as well...
> There are a few places where virPCIDeviceAddressFormat() *is*
> used, and those won't ever format the zPCI stuff.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2018-09-19 at 16:59 +0800, Yi Min Zhao wrote: > 在 2018/9/13 下午9:58, Andrea Bolognani 写道: > > I realize now that virPCIDeviceAddressIsValid() and > > virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I > > swear that I posted patches moving them over... My bad, I'll do > > that right away. > > > > Sorry for the confusion. > > Got it. The patches mentioned above have been posted and merged in the meantime :) > > From the high level point of view, code outside of conf/ should, > > for the most part, not need to concern itself with zPCI at all: it > > would eg. ask for a PCI address to be allocated, and if the device > > in question can be a zPCI device then a zPCI extension address will > > be allocated for it as part of the same function call; the only > > part of qemu/ that should care about the zPCI address is the one > > generating the relevant command line arguments. > > > > Can you try and see whether this kind of API would work? > > I did a simple test. It worked. Do you prefer this way? Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
在 2018/9/20 下午4:28, Andrea Bolognani 写道: > On Wed, 2018-09-19 at 16:59 +0800, Yi Min Zhao wrote: >> 在 2018/9/13 下午9:58, Andrea Bolognani 写道: >>> I realize now that virPCIDeviceAddressIsValid() and >>> virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I >>> swear that I posted patches moving them over... My bad, I'll do >>> that right away. >>> >>> Sorry for the confusion. >> Got it. > The patches mentioned above have been posted and merged in the > meantime :) > >>> From the high level point of view, code outside of conf/ should, >>> for the most part, not need to concern itself with zPCI at all: it >>> would eg. ask for a PCI address to be allocated, and if the device >>> in question can be a zPCI device then a zPCI extension address will >>> be allocated for it as part of the same function call; the only >>> part of qemu/ that should care about the zPCI address is the one >>> generating the relevant command line arguments. >>> >>> Can you try and see whether this kind of API would work? >> I did a simple test. It worked. Do you prefer this way? > Yes please, I'd very much like to see what that looks like and > whether it addresses the problems caused by the ambiguity of the > API we've used until now. > So do you mean we use this kind of API in next version for review? -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote: > > > > From the high level point of view, code outside of conf/ should, > > > > for the most part, not need to concern itself with zPCI at all: it > > > > would eg. ask for a PCI address to be allocated, and if the device > > > > in question can be a zPCI device then a zPCI extension address will > > > > be allocated for it as part of the same function call; the only > > > > part of qemu/ that should care about the zPCI address is the one > > > > generating the relevant command line arguments. > > > > > > > > Can you try and see whether this kind of API would work? > > > > > > I did a simple test. It worked. Do you prefer this way? > > > > Yes please, I'd very much like to see what that looks like and > > whether it addresses the problems caused by the ambiguity of the > > API we've used until now. > > So do you mean we use this kind of API in next version for review? Yes, please :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
在 2018/9/25 下午4:02, Andrea Bolognani 写道: > On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote: >>>>> From the high level point of view, code outside of conf/ should, >>>>> for the most part, not need to concern itself with zPCI at all: it >>>>> would eg. ask for a PCI address to be allocated, and if the device >>>>> in question can be a zPCI device then a zPCI extension address will >>>>> be allocated for it as part of the same function call; the only >>>>> part of qemu/ that should care about the zPCI address is the one >>>>> generating the relevant command line arguments. >>>>> >>>>> Can you try and see whether this kind of API would work? >>>> I did a simple test. It worked. Do you prefer this way? >>> Yes please, I'd very much like to see what that looks like and >>> whether it addresses the problems caused by the ambiguity of the >>> API we've used until now. >> So do you mean we use this kind of API in next version for review? > Yes, please :) > I'm not sure how big the change is. So I might take more time to prepare the new version. So that we can't catch up with the new release. Thanks for your kindness and comments! -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2018-09-25 at 17:16 +0800, Yi Min Zhao wrote: > > > So do you mean we use this kind of API in next version for review? > > > > Yes, please :) > > I'm not sure how big the change is. So I might take more time > to prepare the new version. So that we can't catch up with > the new release. Thanks for your kindness and comments! I think it's going to be a fairly sizeable change, but as we're getting awfully close to freeze anyway I would have advised against merging zPCI support this late in the cycle anyway, so timing-wise I'd say it probably won't change much. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.