Re: [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 @@ > > > > > > > > > > > function='0x0'> > > > - > > > + > > > > > > > > > > > > @@ -48,7 +48,7 @@ > > > > > > > > > > > function='0x0'> > > > - > > > + > > > > > > > > > > > > @@ -57,7 +57,7 @@ > > > > > > > > > > > function='0x0'> > > > - > > > + > > > > 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
Re: [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, ) < 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 = >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(>addr.pci.zpci)) {
+if (!virZPCIDeviceAddressIsIncomplete(>addr.pci.zpci)) {
Re: [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, ) < 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 = >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(>addr.pci.zpci)) {
> +if (!virZPCIDeviceAddressIsIncomplete(>addr.pci.zpci)) {
>
[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
Signed-off-by: Shalini Chellathurai Saroja
Reviewed-by: Bjoern Walk
Reviewed-by: Boris Fiuczynski
---
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 'uid' attribute"));
return -1;
}
-if (!virZPCIDeviceAddressIsValid())
-return -1;
+def.uid_set = true;
}
if (fid) {
@@ -75,8 +74,11 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
_("Cannot parse 'fid' attribute"));
return -1;
}
+def.fid_set = true;
}
+if (!virZPCIDeviceAddressIsValid())
+return -1;
addr->zpci = def;
@@ -191,22 +193,20 @@ virDeviceInfoPCIAddressIsPresent(const
virDomainDeviceInfo *info)
!virPCIDeviceAddressIsEmpty(>addr.pci);
}
-
bool
virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info)
{
return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
- virZPCIDeviceAddressIsEmpty(>addr.pci.zpci);
+ virZPCIDeviceAddressIsIncomplete(>addr.pci.zpci);
}
bool
virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
{
return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
- !virZPCIDeviceAddressIsEmpty(>addr.pci.zpci);
+ virZPCIDeviceAddressIsPresent(>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) {
+
