5:30 a.m.
The ZPCI address validation or autogeneration does not work as
expected in the following scenarios
1. uid = 0 and fid = 0
2. uid = 0 and fid > 0
3. uid = 0 and fid not specified
4. uid not specified and fid > 0
5. 2 ZPCI devices with uid > 0 and fid not specified.
This is because of the following reasons
1. If uid = 0 or fid = 0 the code assumes that user has not specified
the corresponding address
2. If either uid or fid is provided, the code assumes that both uid
and fid addresses are specified by the user.
This patch fixes these issues.
Shalini Chellathurai Saroja (6):
conf: fix ZPCI address validation on s390
tests: qemu: add tests for ZPCI on s390
conf: fix ZPCI address auto-generation on s390
qemu: move ZPCI uid validation into device validation
tests: qemu: add more tests for ZPCI on S390
tests: add test with PCI and CCW device
src/conf/device_conf.c | 45 ++++++--------
src/conf/domain_addr.c | 59 +++++++++++++------
src/conf/domain_conf.c | 2 +-
src/libvirt_private.syms | 4 +-
src/qemu/qemu_validate.c | 26 +++++++-
src/util/virpci.c | 23 ++------
src/util/virpci.h | 6 +-
.../hostdev-vfio-zpci-autogenerate-fids.args | 31 ++++++++++
.../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++++++++
.../hostdev-vfio-zpci-autogenerate-uids.args | 31 ++++++++++
.../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++++++++
.../hostdev-vfio-zpci-ccw-memballoon.args | 28 +++++++++
.../hostdev-vfio-zpci-ccw-memballoon.xml | 17 ++++++
.../hostdev-vfio-zpci-duplicate.xml | 30 ++++++++++
...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +++++++
.../hostdev-vfio-zpci-multidomain-many.args | 6 +-
.../hostdev-vfio-zpci-set-zero.xml | 21 +++++++
.../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++++
tests/qemuxml2argvtest.c | 25 ++++++++
.../hostdev-vfio-zpci-multidomain-many.xml | 6 +-
20 files changed, 387 insertions(+), 72 deletions(-)
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
--
2.21.1
5:31 a.m.
New subject: [PATCH libvirt v1 1/6] conf: fix ZPCI address validation 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 and in later code assume that invalid
numerical values mean that they have been unspecified.
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/conf/device_conf.c | 38 +++++++++++++++++---------------------
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 4dbd5c1a..001ed929 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -53,38 +53,34 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
{
virZPCIDeviceAddress def = { 0 };
- char *uid;
- char *fid;
- int ret = -1;
+ g_autofree char *uid = NULL;
+ g_autofree char *fid = NULL;
uid = virXMLPropString(node, "uid");
fid = virXMLPropString(node, "fid");
- if (uid &&
- virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot parse <address> 'uid'
attribute"));
- goto cleanup;
+ if (uid) {
+ if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'uid'
attribute"));
+ return -1;
+ }
+ if (!virZPCIDeviceAddressIsValid(&def))
+ return -1;
}
- if (fid &&
- virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot parse <address> 'fid'
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"));
+ return -1;
+ }
}
- if (!virZPCIDeviceAddressIsEmpty(&def) &&
- !virZPCIDeviceAddressIsValid(&def))
- goto cleanup;
addr->zpci = def;
- ret = 0;
- cleanup:
- VIR_FREE(uid);
- VIR_FREE(fid);
- return ret;
+ return 0;
}
void
--
2.21.1
5:09 a.m.
New subject: [PATCH libvirt v1 1/6] conf: fix ZPCI address validation on s390
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
- if (uid &&
- virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot parse <address> 'uid'
attribute"));
- goto cleanup;
+ if (uid) {
+ if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'uid'
attribute"));
+ return -1;
+ }
+ if (!virZPCIDeviceAddressIsValid(&def))
+ return -1;
}
This doesn't seem right.
I understand that you're moving the call to
virZPCIDeviceAddressIsValid() inside the if(uid) branch so that you
won't get an error for something like
<zpci fid='0x00000005'/>
but this is not a good way to do it: in fact, you change it again in
patch 3/6 and adopt the much better approach of storing directly in
the struct whether uid and fid have been set.
You should go for that approach right away instead of implementing
this intermediate solution first.
- cleanup:
- VIR_FREE(uid);
- VIR_FREE(fid);
- return ret;
+ return 0;
Switching to g_autofree is a nice improvement, but it's a completely
independent one so please make that a separate patch that doesn't
touch the logic.
--
Andrea Bolognani / Red Hat / Virtualization
3:13 a.m.
New subject: [PATCH libvirt v1 1/6] conf: fix ZPCI address validation on s390
Hi Andrea,
Thank you for the review.
On 6/3/20 12:09 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja
wrote:
> - if (uid &&
> - virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Cannot parse <address> 'uid'
attribute"));
> - goto cleanup;
> + if (uid) {
> + if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Cannot parse <address> 'uid'
attribute"));
> + return -1;
> + }
> + if (!virZPCIDeviceAddressIsValid(&def))
> + return -1;
> }
This doesn't seem right.
I understand that you're moving the call to
virZPCIDeviceAddressIsValid() inside the if(uid) branch so that you
won't get an error for something like
<zpci fid='0x00000005'/
but this is not a good way to do it: in fact, you change it again in
patch 3/6 and adopt the much better approach of storing directly in
the struct whether uid and fid have been set.
You should go for that approach right away instead of implementing
this intermediate solution first.
ok, I will do it.
> - cleanup:
> - VIR_FREE(uid);
> - VIR_FREE(fid);
> - return ret;
> + return 0;
Switching to g_autofree is a nice improvement, but it's a completely
independent one so please make that a separate patch that doesn't
touch the logic.
ok, I will make this as a separate patch.
5:31 a.m.
New subject: [PATCH libvirt v1 2/6] tests: qemu: add tests for ZPCI on s390
Add test to verify ZPCI address validation with uid set to 0x0
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
.../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++++++++++++++++
tests/qemuxml2argvtest.c | 3 +++
2 files changed, 23 insertions(+)
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
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 405227fd..dd467cb3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1650,6 +1650,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.21.1
5:11 a.m.
New subject: [PATCH libvirt v1 2/6] tests: qemu: add tests for ZPCI on s390
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
Add test to verify ZPCI address validation with uid set to 0x0
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
.../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++++++++++++++++
tests/qemuxml2argvtest.c | 3 +++
2 files changed, 23 insertions(+)
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
The test cases for the new behavior should go into the patch that
implements it.
+++ b/tests/qemuxml2argvtest.c
@@ -1650,6 +1650,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);
Indentation is off.
--
Andrea Bolognani / Red Hat / Virtualization
3:15 a.m.
New subject: [PATCH libvirt v1 2/6] tests: qemu: add tests for ZPCI on s390
Hi Andrea,
Thank you for the review.
On 6/3/20 12:11 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja
wrote:
> Add test to verify ZPCI address validation with uid set to 0x0
> Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com > Signed-off-by: Shalini Chellathurai Saroja
<shalini(a)linux.ibm.com > Reviewed-by: Bjoern Walk
<bwalk(a)linux.ibm.com > Reviewed-by: Boris Fiuczynski
<fiuczy(a)linux.ibm.com > ---
> .../hostdev-vfio-zpci-uid-set-zero.xml | 20 +++++++++++++++++++
> tests/qemuxml2argvtest.c | 3 +++
> 2 files changed, 23 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml
The test cases for the new behavior should go into the patch that
implements it.
ok.
> +++ b/tests/qemuxml2argvtest.c
> @@ -1650,6 +1650,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);
Indentation is off.
ok, I will correct it.
5:31 a.m.
New subject: [PATCH libvirt v1 3/6] conf: fix ZPCI address auto-generation on s390
Currently, if uid/fid is specified as zero by the user, it is treated
as if the value is not specified. This bug is fixed by introducing two
boolean values to identify if uid and fid are specified by the user.
Also, if either uid or fid is specified by the user, it is incorrectly
assumed that both uid and fid are specified. This bug is fixed by
identifying when the user specified ZPCI address is incomplete and
auto-generating the missing ZPCI address.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/conf/device_conf.c | 12 ++--
src/conf/domain_addr.c | 59 +++++++++++++------
src/conf/domain_conf.c | 2 +-
src/libvirt_private.syms | 3 +-
src/qemu/qemu_validate.c | 2 +-
src/util/virpci.c | 17 ++++--
src/util/virpci.h | 5 +-
.../hostdev-vfio-zpci-multidomain-many.args | 6 +-
.../hostdev-vfio-zpci-multidomain-many.xml | 6 +-
9 files changed, 75 insertions(+), 37 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 001ed929..c03356e7 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -65,8 +65,7 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
_("Cannot parse <address> 'uid'
attribute"));
return -1;
}
- if (!virZPCIDeviceAddressIsValid(&def))
- return -1;
+ def.uid_set = true;
}
if (fid) {
@@ -75,8 +74,11 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
_("Cannot parse <address> 'fid'
attribute"));
return -1;
}
+ def.fid_set = true;
}
+ if (!virZPCIDeviceAddressIsValid(&def))
+ return -1;
addr->zpci = def;
@@ -191,22 +193,20 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info)
!virPCIDeviceAddressIsEmpty(&info->addr.pci);
}
-
bool
virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info)
{
return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
- virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
+ virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci);
}
bool
virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
{
return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
- !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
+ virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci);
}
-
int
virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index e8629b3e..d1cd0804 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -145,12 +145,18 @@ static void
virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
+ if (!zpciIds)
return;
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ if (addr->uid_set) {
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ addr->uid_set = false;
+ }
- virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
+ if (addr->fid_set) {
+ virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
+ addr->fid_set = false;
+ }
}
@@ -186,12 +192,16 @@ static int
virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
+ if (addr->uid_set &&
+ virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
return -1;
- if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
- return -1;
+ if (addr->fid_set) {
+ if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
+ if (addr->uid_set)
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ return -1;
+ }
}
return 0;
@@ -202,14 +212,28 @@ static int
virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
- return -1;
+ bool uid_set, fid_set = false;
- if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
- return -1;
+ if (!addr->uid_set) {
+ if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
+ return -1;
+ uid_set = true;
+ }
+
+ if (!addr->fid_set) {
+ if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
+ if (uid_set)
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ return -1;
+ }
+ fid_set = true;
}
+ if (uid_set)
+ addr->uid_set = true;
+ if (fid_set)
+ addr->fid_set = true;
+
return 0;
}
@@ -234,7 +258,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr
addrs,
virPCIDeviceAddressPtr addr)
{
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
- virZPCIDeviceAddress zpci = { 0 };
+ virZPCIDeviceAddress zpci = addr->zpci;
if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0)
return -1;
@@ -246,6 +270,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr
addrs,
return 0;
}
+
static int
virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
@@ -253,10 +278,10 @@ virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr
addrs,
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
virZPCIDeviceAddressPtr zpci = &addr->zpci;
- if (virZPCIDeviceAddressIsEmpty(zpci))
- return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
- else
- return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci);
+ if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0)
+ return -1;
+
+ return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
}
return 0;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c478d795..5df80cae 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virTristateSwitchTypeToString(info->addr.pci.multi));
}
- if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+ if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
virBufferAsprintf(&childBuf,
"<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
info->addr.pci.zpci.uid,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ec367653..18687563 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2821,7 +2821,8 @@ virPCIHeaderTypeToString;
virPCIIsVirtualFunction;
virPCIStubDriverTypeFromString;
virPCIStubDriverTypeToString;
-virZPCIDeviceAddressIsEmpty;
+virZPCIDeviceAddressIsIncomplete;
+virZPCIDeviceAddressIsPresent;
virZPCIDeviceAddressIsValid;
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 149b9e73..22fdfd11 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -952,7 +952,7 @@ static int
qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
virQEMUCapsPtr qemuCaps)
{
- if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci) &&
+ if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci) &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 6c7e6bbc..b38f717c 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2172,8 +2172,9 @@ virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
/* We don't need to check fid because fid covers
* all range of uint32 type.
*/
- if (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
- zpci->uid == 0) {
+ if (zpci->uid_set &&
+ (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+ zpci->uid == 0)) {
virReportError(VIR_ERR_XML_ERROR,
_("Invalid PCI address uid='0x%.4x', "
"must be > 0x0000 and <= 0x%.4x"),
@@ -2186,11 +2187,19 @@ virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
}
bool
-virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr)
{
- return !(addr->uid || addr->fid);
+ return !addr->uid_set || !addr->fid_set;
}
+
+bool
+virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr)
+{
+ return addr->uid_set || addr->fid_set;
+}
+
+
#ifdef __linux__
virPCIDeviceAddressPtr
diff --git a/src/util/virpci.h b/src/util/virpci.h
index f16d2361..e937348f 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
struct _virZPCIDeviceAddress {
unsigned int uid; /* exempt from syntax-check */
unsigned int fid;
+ bool uid_set;
+ bool fid_set;
/* Don't forget to update virPCIDeviceAddressCopy if needed. */
};
@@ -245,8 +247,9 @@ char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr)
int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf);
+bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr);
+bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr);
bool virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci);
-bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr);
int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
int pfNetDevIdx,
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
index 3dd9a25f..0fae78db 100644
--- a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args
@@ -27,11 +27,11 @@ server,nowait \
-device vfio-pci,host=0001:00:00.0,id=hostdev0,bus=pci.0,addr=0x3 \
-device zpci,uid=53,fid=104,target=hostdev1,id=zpci53 \
-device vfio-pci,host=0002:00:00.0,id=hostdev1,bus=pci.0,addr=0x1 \
--device zpci,uid=1,fid=1,target=hostdev2,id=zpci1 \
+-device zpci,uid=1,fid=0,target=hostdev2,id=zpci1 \
-device vfio-pci,host=0003:00:00.0,id=hostdev2,bus=pci.0,addr=0x2 \
--device zpci,uid=2,fid=2,target=hostdev3,id=zpci2 \
+-device zpci,uid=2,fid=1,target=hostdev3,id=zpci2 \
-device vfio-pci,host=0004:00:00.0,id=hostdev3,bus=pci.0,addr=0x5 \
--device zpci,uid=83,fid=0,target=hostdev4,id=zpci83 \
+-device zpci,uid=83,fid=2,target=hostdev4,id=zpci83 \
-device vfio-pci,host=0005:00:00.0,id=hostdev4,bus=pci.0,addr=0x7 \
-device zpci,uid=3,fid=114,target=hostdev5,id=zpci3 \
-device vfio-pci,host=0006:00:00.0,id=hostdev5,bus=pci.0,addr=0x9 \
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
index e56106d1..ecfc15c4 100644
--- a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
@@ -39,7 +39,7 @@
<address domain='0x0003' bus='0x00' slot='0x00'
function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x02' function='0x0'>
- <zpci uid='0x0001' fid='0x00000001'/>
+ <zpci uid='0x0001' fid='0x00000000'/>
</address>
</hostdev>
<hostdev mode='subsystem' type='pci' managed='no'>
@@ -48,7 +48,7 @@
<address domain='0x0004' bus='0x00' slot='0x00'
function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x05' function='0x0'>
- <zpci uid='0x0002' fid='0x00000002'/>
+ <zpci uid='0x0002' fid='0x00000001'/>
</address>
</hostdev>
<hostdev mode='subsystem' type='pci' managed='no'>
@@ -57,7 +57,7 @@
<address domain='0x0005' bus='0x00' slot='0x00'
function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x07' function='0x0'>
- <zpci uid='0x0053' fid='0x00000000'/>
+ <zpci uid='0x0053' fid='0x00000002'/>
</address>
</hostdev>
<hostdev mode='subsystem' type='pci' managed='no'>
--
2.21.1
5:58 a.m.
New subject: [PATCH libvirt v1 3/6] conf: fix ZPCI address auto-generation on s390
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
+++ b/src/conf/domain_addr.c
@@ -145,12 +145,18 @@ static void
virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
+ if (!zpciIds)
return;
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ if (addr->uid_set) {
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ addr->uid_set = false;
+ }
I think it would be cleaner to handle the boolean flags in the same
spot the values are taken care of, that is, in the Release{U,F}id()
functions.
@@ -186,12 +192,16 @@ static int
virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
+ if (addr->uid_set &&
+ virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
return -1;
Same here...
- if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
- return -1;
+ if (addr->fid_set) {
+ if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
+ if (addr->uid_set)
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ return -1;
+ }
}
return 0;
@@ -202,14 +212,28 @@ static int
virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
- return -1;
+ bool uid_set, fid_set = false;
- if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
- virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
- return -1;
+ if (!addr->uid_set) {
+ if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
+ return -1;
+ uid_set = true;
+ }
... and here. Basicall, push all handling of boolean flags one layer
down, where the actual values are taken care of.
@@ -234,7 +258,7 @@
virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
{
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
- virZPCIDeviceAddress zpci = { 0 };
+ virZPCIDeviceAddress zpci = addr->zpci;
if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0)
return -1;
@@ -246,6 +270,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr
addrs,
return 0;
}
+
static int
virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
@@ -253,10 +278,10 @@ virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr
addrs,
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
virZPCIDeviceAddressPtr zpci = &addr->zpci;
- if (virZPCIDeviceAddressIsEmpty(zpci))
- return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
- else
- return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci);
+ if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0)
+ return -1;
+
+ return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
}
I think the semantics for these functions need to be reconsidered.
The way they currently work, as evidenced by EnsureAddr(), is that
you either have a fully-specified address provided by the user, in
which case you want to reserve that address, or you have an empty
address because the user didn't provide ZPCI information, in which
case you allocate a new full address based on what uids and fids are
still available and use that one.
With your changes, we can now find ourselves in a hybrid situation,
where half of the ZPCI address has been provided by the user but the
remaining half is up to us... Ultimately, it might make sense to have
ReserveAddr(), ReserveNextAddr() and EnsureAddr() all call to the
same function which does something along the lines of
if (!zpci->uid_set)
AssignUid(zpci);
if (!zpci->fid_set)
AssignFid(zpci);
ReserveUid(zpci);
ReserveFid(zpci);
because that's ultimately what we're achieving anyway, only with
more obfuscation.
+++ b/src/conf/domain_conf.c
@@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virTristateSwitchTypeToString(info->addr.pci.multi));
}
- if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+ if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
virBufferAsprintf(&childBuf,
"<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
info->addr.pci.zpci.uid,
By the time we get here, we should either have a complete ZPCI
address or no ZPCI address at all. So I would rewrite this as
if (IsIncomplete(zpci))
virReportError(VIR_ERR_INTERNAL, ...);
if (!IsPresent(zpci))
virBufferAsprintf(...);
with the first check being there just for extra safety (This Should
Never Happen™).
+++ b/src/util/virpci.h
@@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
struct _virZPCIDeviceAddress {
unsigned int uid; /* exempt from syntax-check */
unsigned int fid;
+ bool uid_set;
+ bool fid_set;
I believe we mostly use camelCase for struct members, although I
don't think we're too consistent with that :)
I wonder if it would make sense to tie the two bits of information
together more explicitly, eg.
typedef struct _virZPCIDeviceAddressID {
unsigned int value;
bool isSet;
} virZPCIDeviceAddressID;
typedef struct _virZPCIDeviceAddress {
virZPCIDeviceAddressID uid;
virZPCIDeviceAddressID fid;
} virZPCIDeviceAddress;
+++
b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
@@ -39,7 +39,7 @@
<address domain='0x0003' bus='0x00' slot='0x00'
function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x02' function='0x0'>
- <zpci uid='0x0001' fid='0x00000001'/>
+ <zpci uid='0x0001' fid='0x00000000'/>
</address>
</hostdev>
<hostdev mode='subsystem' type='pci' managed='no'>
@@ -48,7 +48,7 @@
<address domain='0x0004' bus='0x00' slot='0x00'
function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x05' function='0x0'>
- <zpci uid='0x0002' fid='0x00000002'/>
+ <zpci uid='0x0002' fid='0x00000001'/>
</address>
</hostdev>
<hostdev mode='subsystem' type='pci' managed='no'>
@@ -57,7 +57,7 @@
<address domain='0x0005' bus='0x00' slot='0x00'
function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x07' function='0x0'>
- <zpci uid='0x0053' fid='0x00000000'/>
+ <zpci uid='0x0053' fid='0x00000002'/>
I'm not entirely clear on why these generated ZPCI addresses would
change. Can you explain that to me?
--
Andrea Bolognani / Red Hat / Virtualization
5:43 a.m.
New subject: [PATCH libvirt v1 3/6] conf: fix ZPCI address auto-generation on s390
Hi Andrea,
Thank you for the review.
On 6/3/20 12:58 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja
wrote:
> +++ b/src/conf/domain_addr.c
> @@ -145,12 +145,18 @@ static void
> virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> {
> - if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
> + if (!zpciIds)
> return;
>
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + if (addr->uid_set) {
> + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + addr->uid_set = false;
> + }
I think it would be cleaner to handle the boolean flags in the same
spot the values are taken care of, that is, in the Release{U,F}id()
functions.
ok, I will do so.
> @@ -186,12 +192,16 @@ static int
> virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> {
> - if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
> + if (addr->uid_set &&
> + virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
> return -1;
Same here...
>
> - if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> - return -1;
> + if (addr->fid_set) {
> + if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> + if (addr->uid_set)
> + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + return -1;
> + }
> }
>
> return 0;
> @@ -202,14 +212,28 @@ static int
> virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> {
> - if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> - return -1;
> + bool uid_set, fid_set = false;
>
> - if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> - return -1;
> + if (!addr->uid_set) {
> + if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> + return -1;
> + uid_set = true;
> + }
... and here. Basicall, push all handling of boolean flags one layer
down, where the actual values are taken care of.
> @@ -234,7 +258,7 @@
virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr)
> {
> if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> - virZPCIDeviceAddress zpci = { 0 };
> + virZPCIDeviceAddress zpci = addr->zpci;
>
> if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) <
0)
> return -1;
> @@ -246,6 +270,7 @@
virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> return 0;
> }
>
> +
> static int
> virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr)
> @@ -253,10 +278,10 @@
virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> virZPCIDeviceAddressPtr zpci = &addr->zpci;
>
> - if (virZPCIDeviceAddressIsEmpty(zpci))
> - return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
> - else
> - return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci);
> + if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0)
> + return -1;
> +
> + return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
> }
I think the semantics for these functions need to be reconsidered.
The way they currently work, as evidenced by EnsureAddr(), is that
you either have a fully-specified address provided by the user, in
which case you want to reserve that address, or you have an empty
address because the user didn't provide ZPCI information, in which
case you allocate a new full address based on what uids and fids are
still available and use that one.
With your changes, we can now find ourselves in a hybrid situation,
where half of the ZPCI address has been provided by the user but the
remaining half is up to us... Ultimately, it might make sense to have
ReserveAddr(), ReserveNextAddr() and EnsureAddr() all call to the
same function which does something along the lines of
if (!zpci->uid_set)
AssignUid(zpci);
if (!zpci->fid_set)
AssignFid(zpci);
ReserveUid(zpci);
ReserveFid(zpci);
because that's ultimately what we're achieving anyway, only with
more obfuscation.
ok, I will do so.
> +++ b/src/conf/domain_conf.c
> @@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>
virTristateSwitchTypeToString(info->addr.pci.multi));
> }
>
> - if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
> + if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
> virBufferAsprintf(&childBuf,
> "<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
> info->addr.pci.zpci.uid,
By the time we get here, we should either have a complete ZPCI
address or no ZPCI address at all. So I would rewrite this as
if (IsIncomplete(zpci))
virReportError(VIR_ERR_INTERNAL, ...);
if (!IsPresent(zpci))
virBufferAsprintf(...);
with the first check being there just for extra safety (This Should
Never Happen™).
Sure.
> +++ b/src/util/virpci.h
> @@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
> struct _virZPCIDeviceAddress {
> unsigned int uid; /* exempt from syntax-check */
> unsigned int fid;
> + bool uid_set;
> + bool fid_set;
I believe we mostly use camelCase for struct members, although I
don't think we're too consistent with that :)
ok, I will camelCase the
struct members.
I wonder if it would make sense to tie the two bits of information
together more explicitly, eg.
typedef struct _virZPCIDeviceAddressID {
unsigned int value;
bool isSet;
} virZPCIDeviceAddressID;
typedef struct _virZPCIDeviceAddress {
virZPCIDeviceAddressID uid;
virZPCIDeviceAddressID fid;
} virZPCIDeviceAddress;
I will redefine the struct as above.
> +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
> @@ -39,7 +39,7 @@
> <address domain='0x0003' bus='0x00' slot='0x00'
function='0x0'/ > </source > <address type='pci'
domain='0x0000' bus='0x00' slot='0x02' function='0x0' > - <zpci uid='0x0001'
fid='0x00000001'/ > + <zpci
uid='0x0001' fid='0x00000000'/ >
</address > </hostdev > <hostdev mode='subsystem'
type='pci' managed='no' > @@
-48,7 +48,7 @@
> <address domain='0x0004' bus='0x00' slot='0x00'
function='0x0'/ > </source > <address type='pci'
domain='0x0000' bus='0x00' slot='0x05' function='0x0' > - <zpci uid='0x0002'
fid='0x00000002'/ > + <zpci
uid='0x0002' fid='0x00000001'/ >
</address > </hostdev > <hostdev mode='subsystem'
type='pci' managed='no' > @@
-57,7 +57,7 @@
> <address domain='0x0005' bus='0x00' slot='0x00'
function='0x0'/ > </source > <address type='pci'
domain='0x0000' bus='0x00' slot='0x07' function='0x0' > - <zpci uid='0x0053'
fid='0x00000000'/ > + <zpci
uid='0x0053' fid='0x00000002'/ I'm
not entirely clear on why these generated ZPCI addresses would
change. Can you explain that to me?
Sure:-). It changes in this version because at first the user specified
addresses are reserved and then the addresses which are not specified by
the user are assigned and reserved.
In the current master code, as uid and fid are correlated, both uid and
fid are reserved, when either one of them is specified by the user. So
for the pci device with uid = '0x0053', the code assumes that user has
specified fid as 0 (which is not true) and reserves fid as 0.
Warm Regards
Shalini C S
6:41 a.m.
New subject: [PATCH libvirt v1 3/6] conf: fix ZPCI address auto-generation on s390
On Tue, 2020-06-16 at 12:43 +0200, Shalini Chellathurai Saroja wrote:
On 6/3/20 12:58 PM, Andrea Bolognani wrote:
> On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
> > +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
> > @@ -39,7 +39,7 @@
> > <address domain='0x0003' bus='0x00'
slot='0x00' function='0x0'/>
> > </source>
> > <address type='pci' domain='0x0000'
bus='0x00' slot='0x02' function='0x0'>
> > - <zpci uid='0x0001' fid='0x00000001'/>
> > + <zpci uid='0x0001' fid='0x00000000'/>
> > </address>
> > </hostdev>
> > <hostdev mode='subsystem' type='pci'
managed='no'>
> > @@ -48,7 +48,7 @@
> > <address domain='0x0004' bus='0x00'
slot='0x00' function='0x0'/>
> > </source>
> > <address type='pci' domain='0x0000'
bus='0x00' slot='0x05' function='0x0'>
> > - <zpci uid='0x0002' fid='0x00000002'/>
> > + <zpci uid='0x0002' fid='0x00000001'/>
> > </address>
> > </hostdev>
> > <hostdev mode='subsystem' type='pci'
managed='no'>
> > @@ -57,7 +57,7 @@
> > <address domain='0x0005' bus='0x00'
slot='0x00' function='0x0'/>
> > </source>
> > <address type='pci' domain='0x0000'
bus='0x00' slot='0x07' function='0x0'>
> > - <zpci uid='0x0053' fid='0x00000000'/>
> > + <zpci uid='0x0053' fid='0x00000002'/>
>
> I'm not entirely clear on why these generated ZPCI addresses would
> change. Can you explain that to me?
Sure:-). It changes in this version because at first the user specified
addresses are reserved and then the addresses which are not specified by
the user are assigned and reserved.
In the current master code, as uid and fid are correlated, both uid and
fid are reserved, when either one of them is specified by the user. So
for the pci device with uid = '0x0053', the code assumes that user has
specified fid as 0 (which is not true) and reserves fid as 0.
Makes sense! Thanks for the explanation :)
--
Andrea Bolognani / Red Hat / Virtualization
5:31 a.m.
New subject: [PATCH libvirt v1 4/6] qemu: move ZPCI uid validation into device validation
The ZPCI device validation is specific to qemu. So, let us move the
ZPCI uid validation out of domain xml parsing into qemu domain device
validation.
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/conf/device_conf.c | 3 ---
src/libvirt_private.syms | 1 -
src/qemu/qemu_validate.c | 24 ++++++++++++++++++++++++
src/util/virpci.c | 20 --------------------
src/util/virpci.h | 1 -
5 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index c03356e7..048d2b16 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -77,9 +77,6 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
def.fid_set = true;
}
- if (!virZPCIDeviceAddressIsValid(&def))
- return -1;
-
addr->zpci = def;
return 0;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 18687563..a4c888bb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2823,7 +2823,6 @@ virPCIStubDriverTypeFromString;
virPCIStubDriverTypeToString;
virZPCIDeviceAddressIsIncomplete;
virZPCIDeviceAddressIsPresent;
-virZPCIDeviceAddressIsValid;
# util/virperf.h
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 22fdfd11..d85d8321 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -948,6 +948,24 @@ qemuValidateDomainDef(const virDomainDef *def,
}
+static bool
+qemuDomainDeviceDefValidateZPCIUid(virZPCIDeviceAddressPtr zpci)
+{
+ if (zpci->uid_set &&
+ (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+ zpci->uid == 0)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%.4x', "
+ "must be > 0x0000 and <= 0x%.4x"),
+ zpci->uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
+ return false;
+ }
+
+ return true;
+}
+
+
static int
qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
virQEMUCapsPtr qemuCaps)
@@ -960,6 +978,12 @@ qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
return -1;
}
+ /* We don't need to check fid because fid covers
+ * all range of uint32 type.
+ */
+ if (!qemuDomainDeviceDefValidateZPCIUid(&info->addr.pci.zpci))
+ return -1;
+
return 0;
}
diff --git a/src/util/virpci.c b/src/util/virpci.c
index b38f717c..349f313c 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2166,26 +2166,6 @@ virPCIDeviceAddressParse(char *address,
}
-bool
-virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
-{
- /* We don't need to check fid because fid covers
- * all range of uint32 type.
- */
- if (zpci->uid_set &&
- (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
- zpci->uid == 0)) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Invalid PCI address uid='0x%.4x', "
- "must be > 0x0000 and <= 0x%.4x"),
- zpci->uid,
- VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
- return false;
- }
-
- return true;
-}
-
bool
virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr)
{
diff --git a/src/util/virpci.h b/src/util/virpci.h
index e937348f..e9c49716 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -249,7 +249,6 @@ int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr
bdf);
bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr);
bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr);
-bool virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci);
int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
int pfNetDevIdx,
--
2.21.1
7:15 a.m.
New subject: [PATCH libvirt v1 4/6] qemu: move ZPCI uid validation into device validation
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
+static bool
+qemuDomainDeviceDefValidateZPCIUid(virZPCIDeviceAddressPtr zpci)
+{
+ if (zpci->uid_set &&
+ (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+ zpci->uid == 0)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%.4x', "
+ "must be > 0x0000 and <= 0x%.4x"),
+ zpci->uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
+ return false;
+ }
+
+ return true;
+}
+
+
static int
qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
virQEMUCapsPtr qemuCaps)
@@ -960,6 +978,12 @@ qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
return -1;
}
+ /* We don't need to check fid because fid covers
+ * all range of uint32 type.
+ */
+ if (!qemuDomainDeviceDefValidateZPCIUid(&info->addr.pci.zpci))
+ return -1;
No need to create a separate function, just perform the check inline
here.
--
Andrea Bolognani / Red Hat / Virtualization
3:17 a.m.
New subject: [PATCH libvirt v1 4/6] qemu: move ZPCI uid validation into device validation
Hi Andrea,
Thank you for the review.
On 6/3/20 2:15 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja
wrote:
> +static bool
> +qemuDomainDeviceDefValidateZPCIUid(virZPCIDeviceAddressPtr zpci)
> +{
> + if (zpci->uid_set &&
> + (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
> + zpci->uid == 0)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid PCI address uid='0x%.4x', "
> + "must be > 0x0000 and <= 0x%.4x"),
> + zpci->uid,
> + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +
> static int
> qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info,
> virQEMUCapsPtr qemuCaps)
> @@ -960,6 +978,12 @@ qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr
info,
> return -1;
> }
>
> + /* We don't need to check fid because fid covers
> + * all range of uint32 type.
> + */
> + if (!qemuDomainDeviceDefValidateZPCIUid(&info->addr.pci.zpci))
> + return -1;
No need to create a separate function, just perform the check inline
here.
ok, I will do it.
5:31 a.m.
New subject: [PATCH libvirt v1 5/6] tests: qemu: add more tests for ZPCI on S390
1. Test for auto-generating uids while specifying valid fids
2. Test for auto-generating fids while specifying valid uids
3. Test for parse error while specifying a valid fid and an invalid
uid
4. Test for parse error while specifying two ZPCI devices with same
uid and fid addresses
5. Test for parse error when both uid and fid are set to zero
6. Test for error while specifying uid and not providing ZPCI
capability.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
.../hostdev-vfio-zpci-autogenerate-fids.args | 31 +++++++++++++++++++
.../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++++++++++++++++
.../hostdev-vfio-zpci-autogenerate-uids.args | 31 +++++++++++++++++++
.../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++++++++++++++++
.../hostdev-vfio-zpci-duplicate.xml | 30 ++++++++++++++++++
...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +++++++++++++
.../hostdev-vfio-zpci-set-zero.xml | 21 +++++++++++++
tests/qemuxml2argvtest.c | 18 +++++++++++
8 files changed, 210 insertions(+)
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args
new file mode 100644
index 00000000..6485433a
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+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 \
+-realtime mlock=off \
+-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 zpci,uid=1,fid=0,target=hostdev0,id=zpci1 \
+-device vfio-pci,host=0000:00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \
+-device zpci,uid=5,fid=1,target=hostdev1,id=zpci5 \
+-device vfio-pci,host=0001:00:00.0,id=hostdev1,bus=pci.0,addr=0x2 \
+-device zpci,uid=2,fid=2,target=balloon0,id=zpci2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml
new file mode 100644
index 00000000..49c26e24
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml
@@ -0,0 +1,29 @@
+<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='0x0001'/>
+ </address>
+ </hostdev>
+ <hostdev mode='subsystem' type='pci'>
+ <driver name='vfio'/>
+ <source>
+ <address domain='0x0001' bus='0x00' slot='0x00'
function='0x0'/>
+ </source>
+ <address type='pci'>
+ <zpci uid='0x0005'/>
+ </address>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args
new file mode 100644
index 00000000..181652cc
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+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 \
+-realtime mlock=off \
+-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 zpci,uid=1,fid=0,target=hostdev0,id=zpci1 \
+-device vfio-pci,host=0000:00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \
+-device zpci,uid=2,fid=31,target=hostdev1,id=zpci2 \
+-device vfio-pci,host=0000:00:01.0,id=hostdev1,bus=pci.0,addr=0x2 \
+-device zpci,uid=3,fid=1,target=balloon0,id=zpci3 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml
new file mode 100644
index 00000000..e74e0116
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml
@@ -0,0 +1,29 @@
+<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 fid='0x00000000'/>
+ </address>
+ </hostdev>
+ <hostdev mode='subsystem' type='pci'>
+ <driver name='vfio'/>
+ <source>
+ <address domain='0x0000' bus='0x00' slot='0x01'
function='0x0'/>
+ </source>
+ <address type='pci'>
+ <zpci fid='0x0000001f'/>
+ </address>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml
new file mode 100644
index 00000000..6062ae49
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml
@@ -0,0 +1,30 @@
+<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='0x00' function='0x0'>
+ <zpci uid='0x1234' fid='0x00001234'/>
+ </address>
+ </hostdev>
+ <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='0x00' function='0x0'>
+ <zpci uid='0x1234' fid='0x00001234'/>
+ </address>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml
new file mode 100644
index 00000000..6e710148
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml
@@ -0,0 +1,21 @@
+<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'>
+ <zpci uid='0x000' fid='0x0000001f'/>
+ </address>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml
new file mode 100644
index 00000000..fd3d1193
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml
@@ -0,0 +1,21 @@
+<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'>
+ <zpci uid='0x000' fid='0x00000000'/>
+ </address>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index dd467cb3..6d723675 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1643,6 +1643,11 @@ mymain(void)
DO_TEST("hostdev-vfio-zpci",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-autogenerate-fids",
+ QEMU_CAPS_DEVICE_VFIO_PCI);
+ DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-invalid-uid-valid-fid",
+ 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,
@@ -1650,6 +1655,12 @@ mymain(void)
DO_TEST("hostdev-vfio-zpci-autogenerate",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST("hostdev-vfio-zpci-autogenerate-uids",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST("hostdev-vfio-zpci-autogenerate-fids",
+ 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);
@@ -1659,6 +1670,13 @@ mymain(void)
QEMU_CAPS_DEVICE_ZPCI);
DO_TEST_PARSE_ERROR("hostdev-vfio-zpci",
QEMU_CAPS_DEVICE_VFIO_PCI);
+ DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-duplicate",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-set-zero",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
+
DO_TEST("pci-rom", QEMU_CAPS_DEVICE_VFIO_PCI);
DO_TEST("pci-rom-disabled", NONE);
DO_TEST("pci-rom-disabled-invalid", NONE);
--
2.21.1
7:45 a.m.
New subject: [PATCH libvirt v1 5/6] tests: qemu: add more tests for ZPCI on S390
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
+++ b/tests/qemuxml2argvtest.c
@@ -1650,6 +1655,12 @@ mymain(void)
DO_TEST("hostdev-vfio-zpci-autogenerate",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST("hostdev-vfio-zpci-autogenerate-uids",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST("hostdev-vfio-zpci-autogenerate-fids",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
You want to add the successfult tests to qemuxml2xmltest.c as well.
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
3:18 a.m.
New subject: [PATCH libvirt v1 5/6] tests: qemu: add more tests for ZPCI on S390
Hi Andrea,
Thank you for the review.
On 6/3/20 2:45 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja
wrote:
> +++ b/tests/qemuxml2argvtest.c
> @@ -1650,6 +1655,12 @@ mymain(void)
> DO_TEST("hostdev-vfio-zpci-autogenerate",
> QEMU_CAPS_DEVICE_VFIO_PCI,
> QEMU_CAPS_DEVICE_ZPCI);
> + DO_TEST("hostdev-vfio-zpci-autogenerate-uids",
> + QEMU_CAPS_DEVICE_VFIO_PCI,
> + QEMU_CAPS_DEVICE_ZPCI);
> + DO_TEST("hostdev-vfio-zpci-autogenerate-fids",
> + QEMU_CAPS_DEVICE_VFIO_PCI,
> + QEMU_CAPS_DEVICE_ZPCI);
You want to add the successfult tests to qemuxml2xmltest.c as well.
Everything else looks good.
Thank you, ok I will add them.
5:31 a.m.
New subject: [PATCH libvirt v1 6/6] tests: add test with PCI and CCW device
Add test with a ZPCI host device and a CCW memballoon device to ensure
that CCW address remains the default address assigned.
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
.../hostdev-vfio-zpci-ccw-memballoon.args | 28 +++++++++++++++++++
.../hostdev-vfio-zpci-ccw-memballoon.xml | 17 +++++++++++
tests/qemuxml2argvtest.c | 4 +++
3 files changed, 49 insertions(+)
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args
new file mode 100644
index 00000000..103c4f72
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-KVMGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-KVMGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-KVMGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-KVMGuest1/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-s390x \
+-name KVMGuest1 \
+-S \
+-machine s390-ccw-virtio,accel=kvm,usb=off,dump-guest-core=off \
+-m 214 \
+-realtime mlock=off \
+-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-KVMGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-device zpci,uid=1,fid=0,target=hostdev0,id=zpci1 \
+-device vfio-pci,host=0000:00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \
+-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml
b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml
new file mode 100644
index 00000000..ef5e97fc
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml
@@ -0,0 +1,17 @@
+<domain type='kvm'>
+ <name>KVMGuest1</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>
+ </hostdev>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6d723675..1f087e62 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1676,6 +1676,10 @@ mymain(void)
DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-set-zero",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST("hostdev-vfio-zpci-ccw-memballoon",
+ QEMU_CAPS_CCW,
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
DO_TEST("pci-rom", QEMU_CAPS_DEVICE_VFIO_PCI);
DO_TEST("pci-rom-disabled", NONE);
--
2.21.1
7:45 a.m.
New subject: [PATCH libvirt v1 6/6] tests: add test with PCI and CCW device
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
+++ b/tests/qemuxml2argvtest.c
@@ -1676,6 +1676,10 @@ mymain(void)
DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-set-zero",
QEMU_CAPS_DEVICE_VFIO_PCI,
QEMU_CAPS_DEVICE_ZPCI);
+ DO_TEST("hostdev-vfio-zpci-ccw-memballoon",
+ QEMU_CAPS_CCW,
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_DEVICE_ZPCI);
Same comment as the previous patch - you want to add this to
qemuxml2xmltest.c as well.
--
Andrea Bolognani / Red Hat / Virtualization
3:19 a.m.
New subject: [PATCH libvirt v1 6/6] tests: add test with PCI and CCW device
Hi Andrea,
Thank you for the review.
On 6/3/20 2:45 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja
wrote:
> +++ b/tests/qemuxml2argvtest.c
> @@ -1676,6 +1676,10 @@ mymain(void)
> DO_TEST_PARSE_ERROR("hostdev-vfio-zpci-set-zero",
> QEMU_CAPS_DEVICE_VFIO_PCI,
> QEMU_CAPS_DEVICE_ZPCI);
> + DO_TEST("hostdev-vfio-zpci-ccw-memballoon",
> + QEMU_CAPS_CCW,
> + QEMU_CAPS_DEVICE_VFIO_PCI,
> + QEMU_CAPS_DEVICE_ZPCI);
Same comment as the previous patch - you want to add this to
qemuxml2xmltest.c as well.
ok, I will add it.
7:06 a.m.
On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:
The ZPCI address validation or autogeneration does not work as
expected in the following scenarios
1. uid = 0 and fid = 0
2. uid = 0 and fid > 0
3. uid = 0 and fid not specified
4. uid not specified and fid > 0
5. 2 ZPCI devices with uid > 0 and fid not specified.
This is because of the following reasons
1. If uid = 0 or fid = 0 the code assumes that user has not specified
the corresponding address
2. If either uid or fid is provided, the code assumes that both uid
and fid addresses are specified by the user.
I'd have to dig up the old threads, but based on what I remember the
behaviors you describe are entirely intentional.
For PCI addresses, setting all parts of the address to zero or not
setting it at all is equivalent, and we wanted to be consistent with
that behavior for ZPCI; additionally, zero is not a valid value for
uid so of course neither is the address uid=0 fid=0, which means that
we're not preventing the user from specifying a valid address by
conflating the all-zero address with the unspecified address.
For partially-specified addresses, the behavior is also the same as
PCI: any part you don't specify is considered to be zero, which
results in
uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=0 -> uid=0 fid=0 -> address gets autogenerated
fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=x -> uid=x fid=0 -> address is accepted
So, just like for PCI addresses, you have basically two reasonable
options: either don't specify any zPCI address and leave allocation
entirely up to libvirt, or specify all of the addresses completely:
anything in between will likely not work as you'd expect or want.
Again, this is based purely on my recollection of design discussions
that happened one and a half years ago, so I might have gotten some
of the details wrong - in which case by all means call me out on
that O:-)
--
Andrea Bolognani / Red Hat / Virtualization
2:55 p.m.
On 4/10/20 2:06 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja
wrote:
> The ZPCI address validation or autogeneration does not work as
> expected in the following scenarios
> 1. uid = 0 and fid = 0
> 2. uid = 0 and fid > 0
> 3. uid = 0 and fid not specified
> 4. uid not specified and fid > 0
> 5. 2 ZPCI devices with uid > 0 and fid not specified.
>
> This is because of the following reasons
> 1. If uid = 0 or fid = 0 the code assumes that user has not specified
> the corresponding address
> 2. If either uid or fid is provided, the code assumes that both uid
> and fid addresses are specified by the user.
I'd have to dig up the old threads, but based on what I remember the
behaviors you describe are entirely intentional.
For PCI addresses, setting all parts of the address to zero or not
setting it at all is equivalent, and we wanted to be consistent with
that behavior for ZPCI; additionally, zero is not a valid value for
uid so of course neither is the address uid=0 fid=0, which means that
we're not preventing the user from specifying a valid address by
conflating the all-zero address with the unspecified address.
For partially-specified addresses, the behavior is also the same as
PCI: any part you don't specify is considered to be zero, which
results in
uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=0 -> uid=0 fid=0 -> address gets autogenerated
fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=x -> uid=x fid=0 -> address is accepted
So, just like for PCI addresses, you have basically two reasonable
options: either don't specify any zPCI address and leave allocation
entirely up to libvirt, or specify all of the addresses completely:
anything in between will likely not work as you'd expect or want.
Again, this is based purely on my recollection of design discussions
that happened one and a half years ago, so I might have gotten some
of the details wrong - in which case by all means call me out on
that O:-)
Hi Andrea,
sorry for the delayed answer. I (and some others as well) lost some
emails on my IMAP account and I just found your answer today.
I can remember that you had a discussion with the original author of the
zpci code. There are a few issues with the currently implemented "rules"
which partially are not even working as you outlined above in more
complex scenarios.
First: Setting uid=0 or uid='0x0000'
The architecture allows to do that BUT if you do than you are NOT using
the uid mode which results for the guest OS to use the legacy mode for
assigning PCI addresses starting with 0 increasing by one following an
unpredictable order by which the pci device are presented to guest OS.
Since we never ever wanted to support the legacy mode in KVM guests we
decided to never allow uid=0. Allowing the uid to be set to 0 is a
contradiction.
Actually the user can also set uid='0x0000' which I consider very
specific and one would end up with something like uid='0x0001' and even
more confusing is that suddenly setting uid='0x0000' on more than one
PCI device is allowed.
Besides that the current zpci code still contains at least one flaw that
is simply caused by the fact that there is no knowledge about which
value was specified by the user.
In Shalini's and your list it is case 5: This scenario runs into errors
when another PCI device already has a fid set to 0 OR another PCI device
exists specified with a uid > 0 and without a fid. The user gets the
error message for something he did not specify:
error: Failed to define domain from pci-addr-test.xml
error: internal error: zPCI fid 0 is already reserved
Regarding setting fid=0 or fid='0x00000000'
Since it is a legal value for fid specifying it should not be considered
as a wildcard or set equivalent to not specifying it at all.
Doing so things like this happen and for the user it certainly seems
like a bug:
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
Another example:
Specify this in the domain:
pcidev1: fid='0x00000000'
pcidev2: fid='0x00000000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
BUT
Specify this in the domain:
pcidev1: uid='0x0002' fid='0x00000000'
pcidev2: uid='0x0001' fid='0x00000000'
Results in error:
error: Failed to define domain from pci-addr-test.xml
error: internal error: zPCI fid 0 is already reserved
(Btw remove one of the fids results in the flaw described above.)
I think that Shalini's patch series improves the zpci address generation
to better meet the users expected behavior. It also removes a
correlation between uid and fid that does not really exist.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:25 a.m.
Hi Andrea,
Ping, in case you missed it.
On 4/20/20 9:55 PM, Boris Fiuczynski wrote:
On 4/10/20 2:06 PM, Andrea Bolognani wrote:
> On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:
>> The ZPCI address validation or autogeneration does not work as
>> expected in the following scenarios
>> 1. uid = 0 and fid = 0
>> 2. uid = 0 and fid > 0
>> 3. uid = 0 and fid not specified
>> 4. uid not specified and fid > 0
>> 5. 2 ZPCI devices with uid > 0 and fid not specified.
>>
>> This is because of the following reasons
>> 1. If uid = 0 or fid = 0 the code assumes that user has not specified
>> the corresponding address
>> 2. If either uid or fid is provided, the code assumes that both uid
>> and fid addresses are specified by the user.
>
> I'd have to dig up the old threads, but based on what I remember the
> behaviors you describe are entirely intentional.
>
> For PCI addresses, setting all parts of the address to zero or not
> setting it at all is equivalent, and we wanted to be consistent with
> that behavior for ZPCI; additionally, zero is not a valid value for
> uid so of course neither is the address uid=0 fid=0, which means that
> we're not preventing the user from specifying a valid address by
> conflating the all-zero address with the unspecified address.
>
> For partially-specified addresses, the behavior is also the same as
> PCI: any part you don't specify is considered to be zero, which
> results in
>
> uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> uid=0 -> uid=0 fid=0 -> address gets autogenerated
> fid=x -> uid=0 fid=x -> address is rejected as invalid
> uid=x -> uid=x fid=0 -> address is accepted
>
> So, just like for PCI addresses, you have basically two reasonable
> options: either don't specify any zPCI address and leave allocation
> entirely up to libvirt, or specify all of the addresses completely:
> anything in between will likely not work as you'd expect or want.
>
> Again, this is based purely on my recollection of design discussions
> that happened one and a half years ago, so I might have gotten some
> of the details wrong - in which case by all means call me out on
> that O:-)
>
Hi Andrea,
sorry for the delayed answer. I (and some others as well) lost some
emails on my IMAP account and I just found your answer today.
I can remember that you had a discussion with the original author of
the zpci code. There are a few issues with the currently implemented
"rules" which partially are not even working as you outlined above in
more complex scenarios.
First: Setting uid=0 or uid='0x0000'
The architecture allows to do that BUT if you do than you are NOT
using the uid mode which results for the guest OS to use the legacy
mode for assigning PCI addresses starting with 0 increasing by one
following an unpredictable order by which the pci device are presented
to guest OS. Since we never ever wanted to support the legacy mode in
KVM guests we decided to never allow uid=0. Allowing the uid to be set
to 0 is a contradiction.
Actually the user can also set uid='0x0000' which I consider very
specific and one would end up with something like uid='0x0001' and
even more confusing is that suddenly setting uid='0x0000' on more than
one PCI device is allowed.
Besides that the current zpci code still contains at least one flaw
that is simply caused by the fact that there is no knowledge about
which value was specified by the user.
In Shalini's and your list it is case 5: This scenario runs into
errors when another PCI device already has a fid set to 0 OR another
PCI device exists specified with a uid > 0 and without a fid. The user
gets the error message for something he did not specify:
error: Failed to define domain from pci-addr-test.xml
error: internal error: zPCI fid 0 is already reserved
Regarding setting fid=0 or fid='0x00000000'
Since it is a legal value for fid specifying it should not be
considered as a wildcard or set equivalent to not specifying it at all.
Doing so things like this happen and for the user it certainly seems
like a bug:
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
Another example:
Specify this in the domain:
pcidev1: fid='0x00000000'
pcidev2: fid='0x00000000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
BUT
Specify this in the domain:
pcidev1: uid='0x0002' fid='0x00000000'
pcidev2: uid='0x0001' fid='0x00000000'
Results in error:
error: Failed to define domain from pci-addr-test.xml
error: internal error: zPCI fid 0 is already reserved
(Btw remove one of the fids results in the flaw described above.)
I think that Shalini's patch series improves the zpci address
generation to better meet the users expected behavior. It also removes
a correlation between uid and fid that does not really exist.
12:48 p.m.
On Mon, 2020-04-20 at 21:55 +0200, Boris Fiuczynski wrote:
On 4/10/20 2:06 PM, Andrea Bolognani wrote:
> On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:
> > The ZPCI address validation or autogeneration does not work as
> > expected in the following scenarios
> > 1. uid = 0 and fid = 0
> > 2. uid = 0 and fid > 0
> > 3. uid = 0 and fid not specified
> > 4. uid not specified and fid > 0
> > 5. 2 ZPCI devices with uid > 0 and fid not specified.
> >
> > This is because of the following reasons
> > 1. If uid = 0 or fid = 0 the code assumes that user has not specified
> > the corresponding address
> > 2. If either uid or fid is provided, the code assumes that both uid
> > and fid addresses are specified by the user.
>
> I'd have to dig up the old threads, but based on what I remember the
> behaviors you describe are entirely intentional.
>
> For PCI addresses, setting all parts of the address to zero or not
> setting it at all is equivalent, and we wanted to be consistent with
> that behavior for ZPCI; additionally, zero is not a valid value for
> uid so of course neither is the address uid=0 fid=0, which means that
> we're not preventing the user from specifying a valid address by
> conflating the all-zero address with the unspecified address.
>
> For partially-specified addresses, the behavior is also the same as
> PCI: any part you don't specify is considered to be zero, which
> results in
>
> uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> uid=0 -> uid=0 fid=0 -> address gets autogenerated
> fid=x -> uid=0 fid=x -> address is rejected as invalid
> uid=x -> uid=x fid=0 -> address is accepted
>
> So, just like for PCI addresses, you have basically two reasonable
> options: either don't specify any zPCI address and leave allocation
> entirely up to libvirt, or specify all of the addresses completely:
> anything in between will likely not work as you'd expect or want.
>
> Again, this is based purely on my recollection of design discussions
> that happened one and a half years ago, so I might have gotten some
> of the details wrong - in which case by all means call me out on
> that O:-)
>
Hi Andrea,
sorry for the delayed answer. I (and some others as well) lost some
emails on my IMAP account and I just found your answer today.
No apologies needed: I also took a long time to reply to your
message, and in my case there's no mail server malfunction that I
can assign the blame to O:-)
I can remember that you had a discussion with the original author of
the
zpci code. There are a few issues with the currently implemented "rules"
which partially are not even working as you outlined above in more
complex scenarios.
I disagree with this assessment - they work exactly as designed and
as described above. Whether we *want* them to behave that way... Now
that's a different topic :)
I think the disconnect lies in what the user's expectations are and
what libvirt actually implements. Basically the user expects that
* if either one of uid and fid is explictly assigned a value by
the user, then the guest will use that value - unless such value
is invalid, in which case libvirt will report an error;
* if either one of uid and fid is absent from the user-provided
configuration, then libvirt will automatically pick a valid value
for the attribute.
This is not how the current zPCI implementation works, or how PCI
address assignment works in libvirt; on the other hand, I think these
expectations are in fact completely reasonable, as the examples you
provide illustrate quite well.
I think you successfully convinced me that the current approach is
not good for users and we should fix it; my only doubt at this point
is whether can we safely do that without breaking libvirt's backwards
compatibility guarantees.
Dan, Laine, what's your take?
--
Andrea Bolognani / Red Hat / Virtualization
4:19 a.m.
On 5/6/20 7:48 PM, Andrea Bolognani wrote:
> I can remember that you had a discussion with the original author
of the
> zpci code. There are a few issues with the currently implemented "rules"
> which partially are not even working as you outlined above in more
> complex scenarios.
I disagree with this assessment - they work exactly as designed and
as described above. Whether we*want* them to behave that way... Now
that's a different topic:)
I agree that my wording was imprecise.
It works exactly like you described!
The problem is that even so it was designed that way it is currently
working incorrect. Assigning fid the value 0 is a legal assignment. It
must not be considered as wildcard or "not specified by user".
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
10:51 a.m.
On 5/6/20 1:48 PM, Andrea Bolognani wrote:
On Mon, 2020-04-20 at 21:55 +0200, Boris Fiuczynski wrote:
> On 4/10/20 2:06 PM, Andrea Bolognani wrote:
>> On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:
>>> The ZPCI address validation or autogeneration does not work as
>>> expected in the following scenarios
>>> 1. uid = 0 and fid = 0
>>> 2. uid = 0 and fid > 0
>>> 3. uid = 0 and fid not specified
>>> 4. uid not specified and fid > 0
>>> 5. 2 ZPCI devices with uid > 0 and fid not specified.
>>>
>>> This is because of the following reasons
>>> 1. If uid = 0 or fid = 0 the code assumes that user has not specified
>>> the corresponding address
>>> 2. If either uid or fid is provided, the code assumes that both uid
>>> and fid addresses are specified by the user.
>>
>> I'd have to dig up the old threads, but based on what I remember the
>> behaviors you describe are entirely intentional.
>>
>> For PCI addresses, setting all parts of the address to zero or not
>> setting it at all is equivalent, and we wanted to be consistent with
>> that behavior for ZPCI; additionally, zero is not a valid value for
>> uid so of course neither is the address uid=0 fid=0, which means that
>> we're not preventing the user from specifying a valid address by
>> conflating the all-zero address with the unspecified address.
>>
>> For partially-specified addresses, the behavior is also the same as
>> PCI: any part you don't specify is considered to be zero, which
>> results in
>>
>> uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
>> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
>> uid=0 -> uid=0 fid=0 -> address gets autogenerated
>> fid=x -> uid=0 fid=x -> address is rejected as invalid
>> uid=x -> uid=x fid=0 -> address is accepted
>>
>> So, just like for PCI addresses, you have basically two reasonable
>> options: either don't specify any zPCI address and leave allocation
>> entirely up to libvirt, or specify all of the addresses completely:
>> anything in between will likely not work as you'd expect or want.
>>
>> Again, this is based purely on my recollection of design discussions
>> that happened one and a half years ago, so I might have gotten some
>> of the details wrong - in which case by all means call me out on
>> that O:-)
>>
> Hi Andrea,
> sorry for the delayed answer. I (and some others as well) lost some
> emails on my IMAP account and I just found your answer today.
No apologies needed: I also took a long time to reply to your
message, and in my case there's no mail server malfunction that I
can assign the blame to O:-)
> I can remember that you had a discussion with the original author of the
> zpci code. There are a few issues with the currently implemented "rules"
> which partially are not even working as you outlined above in more
> complex scenarios.
I disagree with this assessment - they work exactly as designed and
as described above. Whether we *want* them to behave that way... Now
that's a different topic :)
I think the disconnect lies in what the user's expectations are and
what libvirt actually implements. Basically the user expects that
* if either one of uid and fid is explictly assigned a value by
the user, then the guest will use that value - unless such value
is invalid, in which case libvirt will report an error;
* if either one of uid and fid is absent from the user-provided
configuration, then libvirt will automatically pick a valid value
for the attribute.
This is not how the current zPCI implementation works, or how PCI
address assignment works in libvirt; on the other hand, I think these
expectations are in fact completely reasonable, as the examples you
provide illustrate quite well.
I think you successfully convinced me that the current approach is
not good for users and we should fix it; my only doubt at this point
is whether can we safely do that without breaking libvirt's backwards
compatibility guarantees.
Dan, Laine, what's your take?
It sounds like the same semantics were applied to the zPCI address
element as are applied to <address type='pci'> (if I understand
correctly, while an PCI address is considered "valid and complete" if
any of its attributes is non-0, for the zPCI extensions, each attribute
is taken on its own. Is that correct?
In any case, it sounds like current behavior for zPCI is broken for some
use cases, and imo this is new enough and usage is narrow enough that if
the maintainers (who I would guess represent the actual users) think
fixing the bug is more important than 100% backward compatibility in a
corner case, then they should be able to change it.
Now if you wanted to change the way regular PCI address attributes are
handled, *then* I would have a totally different opinion :-)
5:13 a.m.
On 5/7/20 5:51 PM, Laine Stump wrote:
On 5/6/20 1:48 PM, Andrea Bolognani wrote:
> On Mon, 2020-04-20 at 21:55 +0200, Boris Fiuczynski wrote:
>> On 4/10/20 2:06 PM, Andrea Bolognani wrote:
>>> On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:
>>>> The ZPCI address validation or autogeneration does not work as
>>>> expected in the following scenarios
>>>> 1. uid = 0 and fid = 0
>>>> 2. uid = 0 and fid > 0
>>>> 3. uid = 0 and fid not specified
>>>> 4. uid not specified and fid > 0
>>>> 5. 2 ZPCI devices with uid > 0 and fid not specified.
>>>>
>>>> This is because of the following reasons
>>>> 1. If uid = 0 or fid = 0 the code assumes that user has not specified
>>>> the corresponding address
>>>> 2. If either uid or fid is provided, the code assumes that both uid
>>>> and fid addresses are specified by the user.
>>>
>>> I'd have to dig up the old threads, but based on what I remember the
>>> behaviors you describe are entirely intentional.
>>>
>>> For PCI addresses, setting all parts of the address to zero or not
>>> setting it at all is equivalent, and we wanted to be consistent with
>>> that behavior for ZPCI; additionally, zero is not a valid value for
>>> uid so of course neither is the address uid=0 fid=0, which means that
>>> we're not preventing the user from specifying a valid address by
>>> conflating the all-zero address with the unspecified address.
>>>
>>> For partially-specified addresses, the behavior is also the same as
>>> PCI: any part you don't specify is considered to be zero, which
>>> results in
>>>
>>> uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
>>> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
>>> uid=0 -> uid=0 fid=0 -> address gets autogenerated
>>> fid=x -> uid=0 fid=x -> address is rejected as invalid
>>> uid=x -> uid=x fid=0 -> address is accepted
>>>
>>> So, just like for PCI addresses, you have basically two reasonable
>>> options: either don't specify any zPCI address and leave allocation
>>> entirely up to libvirt, or specify all of the addresses completely:
>>> anything in between will likely not work as you'd expect or want.
>>>
>>> Again, this is based purely on my recollection of design discussions
>>> that happened one and a half years ago, so I might have gotten some
>>> of the details wrong - in which case by all means call me out on
>>> that O:-)
>>>
>> Hi Andrea,
>> sorry for the delayed answer. I (and some others as well) lost some
>> emails on my IMAP account and I just found your answer today.
>
> No apologies needed: I also took a long time to reply to your
> message, and in my case there's no mail server malfunction that I
> can assign the blame to O:-)
>
>> I can remember that you had a discussion with the original author of the
>> zpci code. There are a few issues with the currently implemented
"rules"
>> which partially are not even working as you outlined above in more
>> complex scenarios.
>
> I disagree with this assessment - they work exactly as designed and
> as described above. Whether we *want* them to behave that way... Now
> that's a different topic :)
>
> I think the disconnect lies in what the user's expectations are and
> what libvirt actually implements. Basically the user expects that
>
> * if either one of uid and fid is explictly assigned a value by
> the user, then the guest will use that value - unless such value
> is invalid, in which case libvirt will report an error;
>
> * if either one of uid and fid is absent from the user-provided
> configuration, then libvirt will automatically pick a valid value
> for the attribute.
>
> This is not how the current zPCI implementation works, or how PCI
> address assignment works in libvirt; on the other hand, I think these
> expectations are in fact completely reasonable, as the examples you
> provide illustrate quite well.
>
> I think you successfully convinced me that the current approach is
> not good for users and we should fix it; my only doubt at this point
> is whether can we safely do that without breaking libvirt's backwards
> compatibility guarantees.
>
> Dan, Laine, what's your take?
It sounds like the same semantics were applied to the zPCI address
element as are applied to <address type='pci'> (if I understand
correctly, while an PCI address is considered "valid and complete" if
any of its attributes is non-0, for the zPCI extensions, each attribute
is taken on its own. Is that correct?
Yes, that is correct.
uid and fid can be set independently from each other within their ranges.
uid (a hex value between 0x0001 and 0xffff, inclusive)
fid (a hex value between 0x00000000 and 0xffffffff, inclusive)
Both must be unique within a domain.
In any case, it sounds like current behavior for zPCI is broken for some
use cases, and imo this is new enough and usage is narrow enough that if
the maintainers (who I would guess represent the actual users) think
fixing the bug is more important than 100% backward compatibility in a
corner case, then they should be able to change it.
I would like to get this fixed such that the behavior is architecture
compliant.
The behavior change would be
Current code:
uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=0 -> uid=0 fid=0 -> address gets autogenerated
With the series applied
uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid
uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=0 -> uid=0 fid=0 -> address is rejected as invalid
The documentation already specifies the uid value range correctly.
The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is
simple: Remove the zpci definition completely.
My expectation is that most users not interested in defining the values
never specified the two scenarios anyway. So I agree with you that it is
an incompatible change in a corner case.
Now if you wanted to change the way regular PCI address attributes are
handled, *then* I would have a totally different opinion :-)
I am not asking for such change even so I have in the past asked about
strange behavior when specifying a slot... :-)
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:32 a.m.
On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote:
On 5/7/20 5:51 PM, Laine Stump wrote:
> In any case, it sounds like current behavior for zPCI is broken for some
> use cases, and imo this is new enough and usage is narrow enough that if
> the maintainers (who I would guess represent the actual users) think
> fixing the bug is more important than 100% backward compatibility in a
> corner case, then they should be able to change it.
I would like to get this fixed such that the behavior is architecture
compliant.
The behavior change would be
Current code:
uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=0 -> uid=0 fid=0 -> address gets autogenerated
IIUC, in the two cases here where the address gets auto-generated,
the resulting guest VM successfully boots & runs....
With the series applied
uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid
uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
uid=0 -> uid=0 fid=0 -> address is rejected as invalid
...so this proposed change is a functional regression for the
user.
The documentation already specifies the uid value range correctly.
The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is
simple: Remove the zpci definition completely.
This would be taking a users' currently working VM, intentionally
breaking it, and then making the user pick up the pieces. This is
an example of a behaviour regression that libvirt promises to not
do to users.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
12:41 p.m.
On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote:
On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote:
> The behavior change would be
> Current code:
> uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> uid=0 -> uid=0 fid=0 -> address gets autogenerated
IIUC, in the two cases here where the address gets auto-generated,
the resulting guest VM successfully boots & runs....
> With the series applied
> uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid
> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> uid=0 -> uid=0 fid=0 -> address is rejected as invalid
...so this proposed change is a functional regression for the
user.
> The documentation already specifies the uid value range correctly.
> The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is
> simple: Remove the zpci definition completely.
This would be taking a users' currently working VM, intentionally
breaking it, and then making the user pick up the pieces. This is
an example of a behaviour regression that libvirt promises to not
do to users.
The bit of nuance that might be missing here is that existing guests
already have a full zPCI address stored in the domain XML, which
means the wouldn't be affected in any way; additionally, the case
where no zPCI address is provided when defining a new guest, which I
assume is the most common one, will keep working.
The only scenarios that would no longer work are:
* the user manually specifies uid=0 fid=0;
* the user manually specifies uid=0 and doesn't specify fid.
In both cases the user would have gone out of their way to specify
a value for the uid attribute that is documented as being invalid:
PCI addresses for S390 guests will have a zpci child element, with
two attributes: uid (a hex value between 0x0001 and 0xffff [...]
https://libvirt.org/formatdomain.html#elementsAddress
As a result, they'd now get a pretty clear error message at define
time instead of confusing behavior across the board. I'm not really
sure anyone would complain about such a change.
--
Andrea Bolognani / Red Hat / Virtualization
3:37 a.m.
On Wed, May 13, 2020 at 07:41:34PM +0200, Andrea Bolognani wrote:
On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote:
> On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote:
> > The behavior change would be
> > Current code:
> > uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
> > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> > uid=0 -> uid=0 fid=0 -> address gets autogenerated
>
> IIUC, in the two cases here where the address gets auto-generated,
> the resulting guest VM successfully boots & runs....
>
> > With the series applied
> > uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid
> > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> > uid=0 -> uid=0 fid=0 -> address is rejected as invalid
>
> ...so this proposed change is a functional regression for the
> user.
>
> > The documentation already specifies the uid value range correctly.
> > The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is
> > simple: Remove the zpci definition completely.
>
> This would be taking a users' currently working VM, intentionally
> breaking it, and then making the user pick up the pieces. This is
> an example of a behaviour regression that libvirt promises to not
> do to users.
The bit of nuance that might be missing here is that existing guests
already have a full zPCI address stored in the domain XML, which
means the wouldn't be affected in any way; additionally, the case
where no zPCI address is provided when defining a new guest, which I
assume is the most common one, will keep working.
The only scenarios that would no longer work are:
* the user manually specifies uid=0 fid=0;
* the user manually specifies uid=0 and doesn't specify fid.
In both cases the user would have gone out of their way to specify
a value for the uid attribute that is documented as being invalid:
PCI addresses for S390 guests will have a zpci child element, with
two attributes: uid (a hex value between 0x0001 and 0xffff [...]
https://libvirt.org/formatdomain.html#elementsAddress
The effect of specifying zero though is that we perform allocation
to assign a non-zero address, which is then valid. The same happens
with regular PCI devices if you give slot="0".
As a result, they'd now get a pretty clear error message at
define
time instead of confusing behavior across the board. I'm not really
sure anyone would complain about such a change.
I don't see this existing behaviour as confusing. It looks like mostly
being a docs ommission about auto-allocation taking place.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:34 a.m.
On 5/14/20 10:37 AM, Daniel P. Berrangé wrote:
On Wed, May 13, 2020 at 07:41:34PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote:
>> On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote:
>>> The behavior change would be
>>> Current code:
>>> uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
>>> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
>>> uid=0 -> uid=0 fid=0 -> address gets autogenerated
>>
>> IIUC, in the two cases here where the address gets auto-generated,
>> the resulting guest VM successfully boots & runs....
>>
>>> With the series applied
>>> uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid
>>> uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
>>> uid=0 -> uid=0 fid=0 -> address is rejected as invalid
>>
>> ...so this proposed change is a functional regression for the
>> user.
>>
>>> The documentation already specifies the uid value range correctly.
>>> The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is
>>> simple: Remove the zpci definition completely.
>>
>> This would be taking a users' currently working VM, intentionally
>> breaking it, and then making the user pick up the pieces. This is
>> an example of a behaviour regression that libvirt promises to not
>> do to users.
>
> The bit of nuance that might be missing here is that existing guests
> already have a full zPCI address stored in the domain XML, which
> means the wouldn't be affected in any way; additionally, the case
> where no zPCI address is provided when defining a new guest, which I
> assume is the most common one, will keep working.
>
> The only scenarios that would no longer work are:
>
> * the user manually specifies uid=0 fid=0;
> * the user manually specifies uid=0 and doesn't specify fid.
>
> In both cases the user would have gone out of their way to specify
> a value for the uid attribute that is documented as being invalid:
>
> PCI addresses for S390 guests will have a zpci child element, with
> two attributes: uid (a hex value between 0x0001 and 0xffff [...]
>
> https://libvirt.org/formatdomain.html#elementsAddress
The effect of specifying zero though is that we perform allocation
to assign a non-zero address, which is then valid. The same happens
with regular PCI devices if you give slot="0".
> As a result, they'd now get a pretty clear error message at define
> time instead of confusing behavior across the board. I'm not really
> sure anyone would complain about such a change.
I don't see this existing behaviour as confusing. It looks like mostly
being a docs ommission about auto-allocation taking place.
Maybe I am repeating myself but I find e.g the below example confusing
if I take into consideration that uid=0 is NOT a valid value and fid is
a valid value. Please note that the valid fid is dislocated from its
original device!
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
If the user would be tying to fix the dislocating fid... one would very
likely try this:
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000' fid='0x00000001'
Result:
error: Failed to define domain from mini-pcis.xml
error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000 and
<= 0xffff
Btw setting uid=0 is defined by architecture for a mode that we do not
support in qemu AND setting fid=0 is an architectural valid assignment
which in the example above is not granted to the device it was defined for.
Regards,
Daniel
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:49 a.m.
On Thu, May 14, 2020 at 03:34:30PM +0200, Boris Fiuczynski wrote:
On 5/14/20 10:37 AM, Daniel P. Berrangé wrote:
> On Wed, May 13, 2020 at 07:41:34PM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote:
> > > On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote:
> > > > The behavior change would be
> > > > Current code:
> > > > uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
> > > > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> > > > uid=0 -> uid=0 fid=0 -> address gets autogenerated
> > >
> > > IIUC, in the two cases here where the address gets auto-generated,
> > > the resulting guest VM successfully boots & runs....
> > >
> > > > With the series applied
> > > > uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid
> > > > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
> > > > uid=0 -> uid=0 fid=0 -> address is rejected as invalid
> > >
> > > ...so this proposed change is a functional regression for the
> > > user.
> > >
> > > > The documentation already specifies the uid value range correctly.
> > > > The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0)
is
> > > > simple: Remove the zpci definition completely.
> > >
> > > This would be taking a users' currently working VM, intentionally
> > > breaking it, and then making the user pick up the pieces. This is
> > > an example of a behaviour regression that libvirt promises to not
> > > do to users.
> >
> > The bit of nuance that might be missing here is that existing guests
> > already have a full zPCI address stored in the domain XML, which
> > means the wouldn't be affected in any way; additionally, the case
> > where no zPCI address is provided when defining a new guest, which I
> > assume is the most common one, will keep working.
> >
> > The only scenarios that would no longer work are:
> >
> > * the user manually specifies uid=0 fid=0;
> > * the user manually specifies uid=0 and doesn't specify fid.
> >
> > In both cases the user would have gone out of their way to specify
> > a value for the uid attribute that is documented as being invalid:
> >
> > PCI addresses for S390 guests will have a zpci child element, with
> > two attributes: uid (a hex value between 0x0001 and 0xffff [...]
> >
> > https://libvirt.org/formatdomain.html#elementsAddress
>
> The effect of specifying zero though is that we perform allocation
> to assign a non-zero address, which is then valid. The same happens
> with regular PCI devices if you give slot="0".
>
> > As a result, they'd now get a pretty clear error message at define
> > time instead of confusing behavior across the board. I'm not really
> > sure anyone would complain about such a change.
>
> I don't see this existing behaviour as confusing. It looks like mostly
> being a docs ommission about auto-allocation taking place.
Maybe I am repeating myself but I find e.g the below example confusing if I
take into consideration that uid=0 is NOT a valid value and fid is a valid
value. Please note that the valid fid is dislocated from its original
device!
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
If the user would be tying to fix the dislocating fid... one would very
likely try this:
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000' fid='0x00000001'
Result:
error: Failed to define domain from mini-pcis.xml
error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000 and
<=
0xffff
Btw setting uid=0 is defined by architecture for a mode that we do not
support in qemu AND setting fid=0 is an architectural valid assignment which
in the example above is not granted to the device it was defined for.
Ok, that's the bit I didn't understand. I was assuming 0 was not a valid
number, but it is valid for a feature we don't currently use. Thus we
should be reporting usage as an error, such that we can implement correct
semantics at a later date if desired.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:59 a.m.
On Thu, 2020-05-14 at 15:34 +0200, Boris Fiuczynski wrote:
On 5/14/20 10:37 AM, Daniel P. Berrangé wrote:
> I don't see this existing behaviour as confusing. It looks like mostly
> being a docs ommission about auto-allocation taking place.
Maybe I am repeating myself but I find e.g the below example confusing
if I take into consideration that uid=0 is NOT a valid value and fid is
a valid value. Please note that the valid fid is dislocated from its
original device!
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000'
Results in a defined domain:
pcidev1: uid='0x0002' fid='0x00000001'
pcidev2: uid='0x0001' fid='0x00000000'
If the user would be tying to fix the dislocating fid... one would very
likely try this:
Specify this in the domain:
pcidev1: uid='0x0000' fid='0x00000000'
pcidev2: uid='0x0000' fid='0x00000001'
Result:
error: Failed to define domain from mini-pcis.xml
error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000 and
<= 0xffff
And partial assignments, which one might reasonably expect to work,
actually don't:
fid=0 -> interpreted the same as no information provided
-> valid address but probably not fid=0
fid=x -> interpreted as uid=0 fid=x
-> invalid address because uid=0 is invalid
Plus, if you have two devices:
uid=1, uid=2 -> interpreted as uid=1 fid=0, uid=2 fid=0
-> invalid addresses because of duplicated fid
Dan, please go through the entire thread and look at the other
examples that Shalini, Boris and I have provided: I think you'll
see why I feel like it's hard to argue that the current behavior can
be considered reasonable from the user's point of view.
--
Andrea Bolognani / Red Hat / Virtualization
6:32 a.m.
On 5/14/20 3:59 PM, Andrea Bolognani wrote:
On Thu, 2020-05-14 at 15:34 +0200, Boris Fiuczynski wrote:
> On 5/14/20 10:37 AM, Daniel P. Berrangé wrote:
>> I don't see this existing behaviour as confusing. It looks like mostly
>> being a docs ommission about auto-allocation taking place.
>
> Maybe I am repeating myself but I find e.g the below example confusing
> if I take into consideration that uid=0 is NOT a valid value and fid is
> a valid value. Please note that the valid fid is dislocated from its
> original device!
>
> Specify this in the domain:
> pcidev1: uid='0x0000' fid='0x00000000'
> pcidev2: uid='0x0000'
> Results in a defined domain:
> pcidev1: uid='0x0002' fid='0x00000001'
> pcidev2: uid='0x0001' fid='0x00000000'
>
> If the user would be tying to fix the dislocating fid... one would very
> likely try this:
> Specify this in the domain:
> pcidev1: uid='0x0000' fid='0x00000000'
> pcidev2: uid='0x0000' fid='0x00000001'
> Result:
> error: Failed to define domain from mini-pcis.xml
> error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000 and
> <= 0xffff
And partial assignments, which one might reasonably expect to work,
actually don't:
fid=0 -> interpreted the same as no information provided
-> valid address but probably not fid=0
fid=x -> interpreted as uid=0 fid=x
-> invalid address because uid=0 is invalid
Plus, if you have two devices:
uid=1, uid=2 -> interpreted as uid=1 fid=0, uid=2 fid=0
-> invalid addresses because of duplicated fid
Dan, please go through the entire thread and look at the other
examples that Shalini, Boris and I have provided: I think you'll
see why I feel like it's hard to argue that the current behavior can
be considered reasonable from the user's point of view.
If I do not misinterpret Dan's last mail he agrees that specifying uid=0
should be considered as error.
How do we proceed from here? Shalini series still can be applied on
master cleanly. Do you want her to resend the series anyway?
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:49 a.m.
On Wed, 2020-05-20 at 13:32 +0200, Boris Fiuczynski wrote:
If I do not misinterpret Dan's last mail he agrees that
specifying uid=0
should be considered as error.
How do we proceed from here? Shalini series still can be applied on
master cleanly. Do you want her to resend the series anyway?
I checked again and it still applies cleanly, so no need to resend.
And Dan seemed to be fine with the changes after all, so I'm going
to go ahead and review them this week.
--
Andrea Bolognani / Red Hat / Virtualization
1:16 a.m.
On 6/1/20 6:49 PM, Andrea Bolognani wrote:
On Wed, 2020-05-20 at 13:32 +0200, Boris Fiuczynski wrote:
> If I do not misinterpret Dan's last mail he agrees that specifying uid=0
> should be considered as error.
> How do we proceed from here? Shalini series still can be applied on
> master cleanly. Do you want her to resend the series anyway?
I checked again and it still applies cleanly, so no need to resend.
And Dan seemed to be fine with the changes after all, so I'm going
to go ahead and review them this week.
ok, thank you.
1619
days inactive
1687
days old
35 comments
5 participants
participants (5)
-
Andrea Bolognani -
Boris Fiuczynski -
Daniel P. Berrangé -
Laine Stump -
Shalini Chellathurai Saroja