Let us fix the issues with zPCI address validation and auto-generation
on s390.
Currently, there are two issues with handling the ZPCI address
extension. Firstly, when the uid is to be auto-generated with a
specified fid, .i.e.:
...
<address type='pci'>
<zpci fid='0x0000001f'/>
</address>
...
we expect uid='0x0001' (or the next available uid for the domain).
However, we get a parsing error:
$ virsh define zpci.xml
error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000
and <= 0xffff
Secondly, when the uid is specified explicitly with the invalid
numerical value '0x0000', we actually expect the parsing error above.
However, the domain is being defined and the uid value is silently
changed to a valid value.
The first issue is a bug and the second one is undesired behaviour, and
both issues are related to how we (in-band) signal invalid values for
uid and fid. So let's fix the XML parsing to do validation based on what
is actually specified in the XML.
The first issue is also related to the current code behaviour, which
is, if either uid or fid is specified by the user, it is incorrectly
assumed that both uid and fid are specified. This bug is fixed by
identifying when the user specified ZPCI address is incomplete and
auto-generating the missing ZPCI address.
Signed-off-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
Signed-off-by: Shalini Chellathurai Saroja <shalini@xxxxxxxxxxxxx>
---
src/conf/device_conf.c | 33 ++++----
src/conf/domain_addr.c | 77 ++++++-------------
src/conf/domain_conf.c | 10 ++-
src/libvirt_private.syms | 3 +-
src/qemu/qemu_command.c | 6 +-
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_validate.c | 2 +-
src/util/virpci.c | 19 +++--
src/util/virpci.h | 14 +++-
.../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++
tests/qemuxml2argvtest.c | 3 +
11 files changed, 101 insertions(+), 88 deletions(-)
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 1d06981a..21398e90 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -59,22 +59,25 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
uid = virXMLPropString(node, "uid");
fid = virXMLPropString(node, "fid");
- if (uid &&
- virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot parse <address> 'uid' attribute"));
- return -1;
+ if (uid) {
+ if (virStrToLong_uip(uid, NULL, 0, &def.uid.value) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'uid' attribute"));
+ return -1;
+ }
+ def.uid.isSet = true;
}
- if (fid &&
- virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot parse <address> 'fid' attribute"));
- return -1;
+ if (fid) {
+ if (virStrToLong_uip(fid, NULL, 0, &def.fid.value) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'fid' attribute"));
+ return -1;
+ }
+ def.fid.isSet = true;
}
- if (!virZPCIDeviceAddressIsEmpty(&def) &&
- !virZPCIDeviceAddressIsValid(&def))
+ if (!virZPCIDeviceAddressIsValid(&def))
return -1;
addr->zpci = def;
@@ -190,22 +193,20 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info)
!virPCIDeviceAddressIsEmpty(&info->addr.pci);
}
-
bool
virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info)
{
return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
- virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
+ virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci);
}
bool
virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
{
return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
- !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
+ virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci);
}
-
int
virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 8623e79d..90ed31ef 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -58,7 +58,7 @@ static int
virDomainZPCIAddressReserveUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressReserveId(set, addr->uid, "uid");
+ return virDomainZPCIAddressReserveId(set, addr->uid.value, "uid");
}
@@ -66,7 +66,7 @@ static int
virDomainZPCIAddressReserveFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressReserveId(set, addr->fid, "fid");
+ return virDomainZPCIAddressReserveId(set, addr->fid.value, "fid");
}
@@ -96,7 +96,7 @@ static int
virDomainZPCIAddressAssignUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressAssignId(set, &addr->uid, 1,
+ return virDomainZPCIAddressAssignId(set, &addr->uid.value, 1,
VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid");
}
@@ -105,7 +105,7 @@ static int
virDomainZPCIAddressAssignFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressAssignId(set, &addr->fid, 0,
+ return virDomainZPCIAddressAssignId(set, &addr->fid.value, 0,
VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid");
}
@@ -129,7 +129,8 @@ static void
virDomainZPCIAddressReleaseUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
+ if (addr->uid.isSet)
+ virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid");
}
@@ -137,7 +138,8 @@ static void
virDomainZPCIAddressReleaseFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
+ if (addr->fid.isSet)
+ virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid");
}
@@ -145,47 +147,26 @@ static void
virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
+ if (!zpciIds)
return;
virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
-
virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
}
static int
-virDomainZPCIAddressReserveNextUid(virHashTablePtr uids,
- virZPCIDeviceAddressPtr zpci)
-{
- if (virDomainZPCIAddressAssignUid(uids, zpci) < 0)
- return -1;
-
- if (virDomainZPCIAddressReserveUid(uids, zpci) < 0)
- return -1;
-
- return 0;
-}
-
-
-static int
-virDomainZPCIAddressReserveNextFid(virHashTablePtr fids,
- virZPCIDeviceAddressPtr zpci)
+virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
+ virZPCIDeviceAddressPtr addr)
{
- if (virDomainZPCIAddressAssignFid(fids, zpci) < 0)
+ if (!addr->uid.isSet &&
+ virDomainZPCIAddressAssignUid(zpciIds->uids, addr) < 0)
return -1;
- if (virDomainZPCIAddressReserveFid(fids, zpci) < 0)
+ if (!addr->fid.isSet &&
+ virDomainZPCIAddressAssignFid(zpciIds->fids, addr) < 0)
return -1;
- return 0;
-}
-
-
-static int
-virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
- virZPCIDeviceAddressPtr addr)
-{
if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
return -1;
@@ -194,21 +175,8 @@ virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
return -1;
}
- return 0;
-}
-
-
-static int
-virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
- virZPCIDeviceAddressPtr addr)
-{
- if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
- return -1;
-
- if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
- return -1;
- }
+ addr->uid.isSet = true;
+ addr->fid.isSet = true;
return 0;
}
@@ -234,9 +202,9 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
{
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
- virZPCIDeviceAddress zpci = { 0 };
+ virZPCIDeviceAddress zpci = addr->zpci;
- if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0)
+ if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, &zpci) < 0)
return -1;
if (!addrs->dryRun)
@@ -246,6 +214,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
return 0;
}
+
static int
virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
@@ -253,10 +222,8 @@ virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
virZPCIDeviceAddressPtr zpci = &addr->zpci;
- if (virZPCIDeviceAddressIsEmpty(zpci))
- return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
- else
- return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci);
+ if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0)
+ return -1;
}
return 0;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e9336fd7..d5594681 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7519,11 +7519,15 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virTristateSwitchTypeToString(info->addr.pci.multi));
}
- if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+ if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing uid or fid attribute of zPCI address"));
+ }
+ if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) {
virBufferAsprintf(&childBuf,
"<zpci uid='0x%.4x' fid='0x%.8x'/>\n",
- info->addr.pci.zpci.uid,
- info->addr.pci.zpci.fid);
+ info->addr.pci.zpci.uid.value,
+ info->addr.pci.zpci.fid.value);
}
break;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fc7406f2..a4a09cf9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2837,7 +2837,8 @@ virPCIHeaderTypeToString;
virPCIIsVirtualFunction;
virPCIStubDriverTypeFromString;
virPCIStubDriverTypeToString;
-virZPCIDeviceAddressIsEmpty;
+virZPCIDeviceAddressIsIncomplete;
+virZPCIDeviceAddressIsPresent;
virZPCIDeviceAddressIsValid;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f27246b4..36349870 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1902,10 +1902,10 @@ qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
virBufferAsprintf(&buf,
"zpci,uid=%u,fid=%u,target=%s,id=zpci%u",
- dev->addr.pci.zpci.uid,
- dev->addr.pci.zpci.fid,
+ dev->addr.pci.zpci.uid.value,
+ dev->addr.pci.zpci.fid.value,
dev->alias,
- dev->addr.pci.zpci.uid);
+ dev->addr.pci.zpci.uid.value);
return virBufferContentAndReset(&buf);
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index dc3bd824..3954ad11 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -157,7 +157,7 @@ qemuDomainDetachZPCIDevice(qemuMonitorPtr mon,
{
g_autofree char *zpciAlias = NULL;
- zpciAlias = g_strdup_printf("zpci%d", info->addr.pci.zpci.uid);
+ zpciAlias = g_strdup_printf("zpci%d", info->addr.pci.zpci.uid.value);
if (qemuMonitorDelDevice(mon, zpciAlias) < 0)
return -1;
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index b1a81ab1..0372ae7d 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1018,7 +1018,7 @@ static int
qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
virQEMUCapsPtr qemuCaps)
{
- if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci) &&
+ if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci) &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 786b1393..40ae5aec 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2173,12 +2173,13 @@ virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
/* We don't need to check fid because fid covers
* all range of uint32 type.
*/
- if (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
- zpci->uid == 0) {
+ if (zpci->uid.isSet &&
+ (zpci->uid.value > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+ zpci->uid.value == 0)) {
virReportError(VIR_ERR_XML_ERROR,
_("Invalid PCI address uid='0x%.4x', "
"must be > 0x0000 and <= 0x%.4x"),
- zpci->uid,
+ zpci->uid.value,
VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
return false;
}
@@ -2187,11 +2188,19 @@ virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
}
bool
-virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr)
{
- return !(addr->uid || addr->fid);
+ return !addr->uid.isSet || !addr->fid.isSet;
}
+
+bool
+virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr)
+{
+ return addr->uid.isSet || addr->fid.isSet;
+}
+
+
#ifdef __linux__
virPCIDeviceAddressPtr
diff --git a/src/util/virpci.h b/src/util/virpci.h
index f16d2361..f198df5d 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -38,11 +38,18 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIDeviceList, virObjectUnref);
#define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX
#define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX
+typedef struct _virZPCIDeviceAddressID virZPCIDeviceAddressID;
typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
+
+struct _virZPCIDeviceAddressID {
+ unsigned int value;
+ bool isSet;
+};
+
struct _virZPCIDeviceAddress {
- unsigned int uid; /* exempt from syntax-check */
- unsigned int fid;
+ virZPCIDeviceAddressID uid; /* exempt from syntax-check */
+ virZPCIDeviceAddressID fid;
/* Don't forget to update virPCIDeviceAddressCopy if needed. */
};
@@ -245,8 +252,9 @@ char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr)
int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf);
+bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr);
+bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr);
bool virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci);
-bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr);
int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
int pfNetDevIdx,
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
new file mode 100644
index 00000000..6bfbfe61
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
@@ -0,0 +1,20 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219100</memory>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <hostdev mode='subsystem' type='pci'>
+ <driver name='vfio'/>
+ <source>
+ <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
+ </source>
+ <address type='pci'>
+ <zpci uid='0x0'/>
+ </address>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d5b2a21b..765e6fdf 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1750,6 +1750,9 @@ mymain(void)
DO_TEST("hostdev-vfio-zpci-autogenerate",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-uid-set-zero",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
DO_TEST("hostdev-vfio-zpci-boundaries",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_PCI_BRIDGE,
--
2.25.4
