[libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
This patch introduces new XML parser/formatter functions. Uid is
16-bit and non-zero. Fid is 32-bit. They are added as two new
attributes of PCI address, and parsed/formatted along with PCI
address parser/formatter. The related test is also added.
Signed-off-by: Yi Min Zhao
Reviewed-by: Boris Fiuczynski
Reviewed-by: Stefan Zimmermann
Reviewed-by: Bjoern Walk
Reviewed-by: Ján Tomko
---
docs/schemas/basictypes.rng| 23
docs/schemas/domaincommon.rng | 1 +
src/conf/device_conf.c | 69 ++
src/conf/device_conf.h | 2 +
src/conf/domain_addr.c | 3 +
src/conf/domain_conf.c | 6 ++
src/libvirt_private.syms | 1 +
src/util/virpci.h | 3 +
tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 25
tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 ++
tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 23
tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++
tests/qemuxml2argvtest.c | 7 +++
tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 +
tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++
tests/qemuxml2xmltest.c| 6 ++
16 files changed, 264 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args
create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args
create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml
create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml
create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 14a3670e5c..3defb56316 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -65,6 +65,17 @@
+
+
+
+(0x)?[0-9a-fA-F]{1,8}
+
+
+0
+4294967295
+
+
+
@@ -111,6 +122,18 @@
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3796eb4b5e..2ccf777d20 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5220,6 +5220,7 @@
pci
+
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 7a8f84e036..5ae990afdb 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -32,6 +32,65 @@
#define VIR_FROM_THIS VIR_FROM_DEVICE
+static int
+virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
+{
+/* We don't need to check fid because fid covers
+ * all range of uint32 type.
+ */
+if (zpci->uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+zpci->uid == 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%x', "
+ "must be > 0x0 and <= 0x%x"),
+ zpci->uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
+return 0;
+}
+
+return 1;
+}
+
+static int
+virZPCIDeviceAddressParseXML(xmlNodePtr node,
+ virPCIDeviceAddressPtr addr)
+{
+char *uid, *fid;
+int ret = -1;
+virZPCIDeviceAddress def = { 0 };
+
+uid = virXMLPropString(node, "uid");
+fid = virXMLPropString(node, "fid");
+
+if (uid) {
+if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'uid' attribute"));
+goto cleanup;
+}
+}
+
+if (fid) {
+if (virStrToLong_uip(fid, NULL, 0, &def.fid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'fid' attribute"));
+goto cleanup;
+}
+}
+
+if (!virZPCIDeviceAddressIsEmpty(&def) &&
+!virZPCIDeviceAddressIsValid(&def))
+goto cleanup;
+
+addr->zpci = def;
+ret = 0;
+
+ cleanup:
+VIR_FREE(uid);
+VIR_FREE(fid);
+return ret;
+}
+
int
virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
virDomainDeviceInfoPtr src)
@@ -213,6 +272,13 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo
*info)
}
+bool
+virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+{
+return !(addr->uid || addr->fid);
+}
+
+
int
virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr,
true))
goto
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> +static int
> +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
This is a predicate, so it should return a bool.
[...]
> +virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid PCI address uid='0x%x', "
> + "must be > 0x0 and <= 0x%x"),
> + zpci->uid,
> + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
You should always use "0x%.4x" when printing uids.
[...]
> +static int
> +virZPCIDeviceAddressParseXML(xmlNodePtr node,
> + virPCIDeviceAddressPtr addr)
> +{
> +char *uid, *fid;
> +int ret = -1;
> +virZPCIDeviceAddress def = { 0 };
One variable per line, please.
Also structs are usually declared first and 'ret' is usually last,
but that's admittedly not very important :)
[...]
> +if (uid) {
> +if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Cannot parse 'uid' attribute"));
> +goto cleanup;
> +}
> +}
You can convert the above to
if (uid &&
virStrToLong_uip(...) < 0) {
...
}
and do the same with fid.
[...]
> +bool
> +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
> +{
> +return !(addr->uid || addr->fid);
> +}
This function belongs in virpci, besides the struct definition and
the very much related virPCIDeviceAddressIsEmpty().
[...]
> @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> if (!virPCIDeviceAddressIsEmpty(addr) &&
> !virPCIDeviceAddressIsValid(addr, true))
> goto cleanup;
>
> +if (virZPCIDeviceAddressParseXML(node, addr) < 0)
> +goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse
the zPCI part, and that's literally embedded in the PCI part now;
on the other hand, we have functions to verify separately whether
the PCI and zPCI parts are empty, which is pretty weird.
I guess we can leave as it is for now, but the semantics are muddy
enough that I can pretty much guarantee someone will have to clean
them up at some point down the line.
[...]
> @@ -1044,6 +1044,9 @@
> virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> dev->isolationGroup, function) < 0)
> return -1;
>
> +if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
> +addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set
of zPCI ids, the same way it's allocating a new PCI address right
above, or do nothing at all because zPCI address allocation is
handled by later calling virDomainZPCIAddressReserveNextAddr()?
This is basically another manifestation of the issue I mentioned
above: we don't seem to have a well-defined and strictly adhered
to idea of how the PCI part and zPCI part should relate to each
other, so they're either considered separate entities or part of
the same entity depending on which APIs you're looking at.
[...]
> +if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
> +virBufferAsprintf(buf, " uid='0x%.4x'",
> + info->addr.pci.zpci.uid);
> +virBufferAsprintf(buf, " fid='0x%.8x'",
> + info->addr.pci.zpci.fid);
You only need a single call to virBufferAsprintf() here.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
在 2018/9/11 下午8:05, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
+static int
+virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
This is a predicate, so it should return a bool.
OK.
[...]
+virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%x', "
+ "must be > 0x0 and <= 0x%x"),
+ zpci->uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
You should always use "0x%.4x" when printing uids.
ditto
[...]
+static int
+virZPCIDeviceAddressParseXML(xmlNodePtr node,
+ virPCIDeviceAddressPtr addr)
+{
+char *uid, *fid;
+int ret = -1;
+virZPCIDeviceAddress def = { 0 };
One variable per line, please.
Also structs are usually declared first and 'ret' is usually last,
but that's admittedly not very important :)
ditto.
I'm very glad that you could give me so many valuable
comments even if it's just about code style.
[...]
+if (uid) {
+if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'uid' attribute"));
+goto cleanup;
+}
+}
You can convert the above to
if (uid &&
virStrToLong_uip(...) < 0) {
...
}
and do the same with fid.
OK.
[...]
+bool
+virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+{
+return !(addr->uid || addr->fid);
+}
This function belongs in virpci, besides the struct definition and
the very much related virPCIDeviceAddressIsEmpty().
I'm not very clear with your comment. Do you mean I
should move it to other place?
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
if (!virPCIDeviceAddressIsEmpty(addr) &&
!virPCIDeviceAddressIsValid(addr, true))
goto cleanup;
+if (virZPCIDeviceAddressParseXML(node, addr) < 0)
+goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse
the zPCI part, and that's literally embedded in the PCI part now;
on the other hand, we have functions to verify separately whether
the PCI and zPCI parts are empty, which is pretty weird.
It's weird indeed. But if we merge zPCI part check into PCI code. We might
have to merge other code. But PCI address has extension only on S390
at least now.
I guess we can leave as it is for now, but the semantics are muddy
enough that I can pretty much guarantee someone will have to clean
them up at some point down the line.
OK.
[...]
@@ -1044,6 +1044,9 @@
virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
dev->isolationGroup, function) < 0)
return -1;
+if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
+addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set
of zPCI ids, the same way it's allocating a new PCI address right
above, or do nothing at all because zPCI address allocation is
handled by later calling virDomainZPCIAddressReserveNextAddr()?
Here, we want to store the defined zPCI address which has been reserved.
If we don't do this, we might lose zPCI address and do reservation again
but the reserved one are still existing in zPCI set.
For this problem, I once thought about adding extension address into
DeviceInfo next to PCI address embraced in a struct. Then here is
not a problem.
This is basically another manifestation of the issue I mentioned
above: we don't seem to have a well-defined and strictly adhered
to idea of how the PCI part and zPCI part should relate to each
other, so they're either considered separate entities or part of
the same entity depending on which APIs you're looking at.
I think the above idea I thought about before is like what you said?
[...]
+if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+virBufferAsprintf(buf, " uid='0x%.4x'",
+ info->addr.pci.zpci.uid);
+virBufferAsprintf(buf, " fid='0x%.8x'",
+ info->addr.pci.zpci.fid);
You only need a single call to virBufferAsprintf() here.
OK.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午8:05, Andrea Bolognani 写道:
> > On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> > [...]
> > > +bool
> > > +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
> > > +{
> > > +return !(addr->uid || addr->fid);
> > > +}
> >
> > This function belongs in virpci, besides the struct definition and
> > the very much related virPCIDeviceAddressIsEmpty().
>
> I'm not very clear with your comment. Do you mean I
> should move it to other place?
Yeah, the function and its definition should be in src/util/virpci.c
and src/util/virpci.h respectively.
I realize now that virPCIDeviceAddressIsValid() and
virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I
swear that I posted patches moving them over... My bad, I'll do
that right away.
Sorry for the confusion.
> > [...]
> > > @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > if (!virPCIDeviceAddressIsEmpty(addr) &&
> > > !virPCIDeviceAddressIsValid(addr, true))
> > > goto cleanup;
> > >
> > > +if (virZPCIDeviceAddressParseXML(node, addr) < 0)
> > > +goto cleanup;
> >
> > I'm not super convinced about this.
> >
> > On one hand, it makes it so callers can't possibly forget to parse
> > the zPCI part, and that's literally embedded in the PCI part now;
> > on the other hand, we have functions to verify separately whether
> > the PCI and zPCI parts are empty, which is pretty weird.
>
> It's weird indeed. But if we merge zPCI part check into PCI code. We might
> have to merge other code. But PCI address has extension only on S390
> at least now.
That's not necessarily a problem, at least in principle.
See all the code dealing with "isolation groups", for example:
while the concept is only ever really used for pSeries guests, it's
implemented in a way that's designed to stay out of the way in all
other cases, and the result is that you won't have to worry about
it except when needed.
The zPCI code should ideally behave similarly.
> > I guess we can leave as it is for now, but the semantics are muddy
> > enough that I can pretty much guarantee someone will have to clean
> > them up at some point down the line.
>
> OK.
> >
> > [...]
> > > @@ -1044,6 +1044,9 @@
> > > virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> > > dev->isolationGroup, function) <
> > > 0)
> > > return -1;
> > >
> > > +if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
> > > +addr.zpci = dev->addr.pci.zpci;
> >
> > I'm confused by this part. Shouldn't this either request a new set
> > of zPCI ids, the same way it's allocating a new PCI address right
> > above, or do nothing at all because zPCI address allocation is
> > handled by later calling virDomainZPCIAddressReserveNextAddr()?
>
> Here, we want to store the defined zPCI address which has been reserved.
> If we don't do this, we might lose zPCI address and do reservation again
> but the reserved one are still existing in zPCI set.
I can't picture the exact scenario right now but I assume something
like that can happen if you have
in which case the zPCI part has already been provided by the user
but we still need to allocate the PCI part ourselves.
Speaking of which, I wonder if something like
would be a more appropriate syntax: not only it clearly shows that
the uid and fid attributes are connected to zPCI, but if we ever
introduce additional PCI address extensions they could be similarly
be represented as childs of instead of adding even more
attributes to the existing element.
Either way, this kind of ad-hoc messing with the zPCI part seems
to me like clear evidence of the fact that we need to step back and
rethink the way the various pieces of the puzzle fit together.
>From the high level point of view, code outside of conf/ should,
for the most part, not need to concern itself with zPCI at all: it
would eg. ask for a PCI address to be allocated, and if the device
in question can be a zPCI device then a zPCI extension address will
be allocated for it as part of the same function call; the only
part of qemu/ that should care about the zPCI address is the one
generating the relevant command line arguments.
Can you try and see whether this kind of API would work?
> For this problem, I once thought about adding extension address into
> DeviceInfo next to PCI address embraced in a struct. Then here is
> not a problem.
That could almost work, seeing how virDomainDeviceInfoFormat()
doesn't use virPCIDeviceAddressFormat() to format the PCI address[1],
but at least in theory you should be able to take a
virPCIDeviceAddress and turn it into a string without having to peek
into other objects such as the parent virDomainDeviceInfo, so that
approach wouldn't be very clean at all.
> > This is basically another manifestation of the issue I mentioned
> > above: we don't seem to
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
在 2018/9/13 下午9:58, Andrea Bolognani 写道:
On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午8:05, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
+bool
+virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+{
+return !(addr->uid || addr->fid);
+}
This function belongs in virpci, besides the struct definition and
the very much related virPCIDeviceAddressIsEmpty().
I'm not very clear with your comment. Do you mean I
should move it to other place?
Yeah, the function and its definition should be in src/util/virpci.c
and src/util/virpci.h respectively.
I realize now that virPCIDeviceAddressIsValid() and
virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I
swear that I posted patches moving them over... My bad, I'll do
that right away.
Sorry for the confusion.
Got it.
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
if (!virPCIDeviceAddressIsEmpty(addr) &&
!virPCIDeviceAddressIsValid(addr, true))
goto cleanup;
+if (virZPCIDeviceAddressParseXML(node, addr) < 0)
+goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse
the zPCI part, and that's literally embedded in the PCI part now;
on the other hand, we have functions to verify separately whether
the PCI and zPCI parts are empty, which is pretty weird.
It's weird indeed. But if we merge zPCI part check into PCI code. We might
have to merge other code. But PCI address has extension only on S390
at least now.
That's not necessarily a problem, at least in principle.
See all the code dealing with "isolation groups", for example:
while the concept is only ever really used for pSeries guests, it's
implemented in a way that's designed to stay out of the way in all
other cases, and the result is that you won't have to worry about
it except when needed.
The zPCI code should ideally behave similarly.
I guess we can leave as it is for now, but the semantics are muddy
enough that I can pretty much guarantee someone will have to clean
them up at some point down the line.
OK.
[...]
@@ -1044,6 +1044,9 @@
virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
dev->isolationGroup, function) < 0)
return -1;
+if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
+addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set
of zPCI ids, the same way it's allocating a new PCI address right
above, or do nothing at all because zPCI address allocation is
handled by later calling virDomainZPCIAddressReserveNextAddr()?
Here, we want to store the defined zPCI address which has been reserved.
If we don't do this, we might lose zPCI address and do reservation again
but the reserved one are still existing in zPCI set.
I can't picture the exact scenario right now but I assume something
like that can happen if you have
in which case the zPCI part has already been provided by the user
but we still need to allocate the PCI part ourselves.
Speaking of which, I wonder if something like
would be a more appropriate syntax: not only it clearly shows that
the uid and fid attributes are connected to zPCI, but if we ever
introduce additional PCI address extensions they could be similarly
be represented as childs of instead of adding even more
attributes to the existing element.
Either way, this kind of ad-hoc messing with the zPCI part seems
to me like clear evidence of the fact that we need to step back and
rethink the way the various pieces of the puzzle fit together.
>From the high level point of view, code outside of conf/ should,
for the most part, not need to concern itself with zPCI at all: it
would eg. ask for a PCI address to be allocated, and if the device
in question can be a zPCI device then a zPCI extension address will
be allocated for it as part of the same function call; the only
part of qemu/ that should care about the zPCI address is the one
generating the relevant command line arguments.
Can you try and see whether this kind of API would work?
I did a simple test. It worked. Do you prefer this way?
For this problem, I once thought about adding extension address into
DeviceInfo next to PCI address embraced in a struct. Then here is
not a problem.
That could almost work, seeing how virDomainDeviceInfoFormat()
doesn't use virPCIDeviceAddressFormat() to format the PCI address[1],
but at least in theory you should be able to take a
virPCIDeviceAddress and turn it into a string without having to peek
into other objects such as the parent virDomainDeviceInfo, so that
approach wouldn't be very clean at all.
Right. That's why I finally gave up it.
This is basically another manifestation of the issue I mentioned
above: we don't seem to have a well-defined and strictly adhered
to idea of how the PCI part and zPCI
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
On Wed, 2018-09-19 at 16:59 +0800, Yi Min Zhao wrote: > 在 2018/9/13 下午9:58, Andrea Bolognani 写道: > > I realize now that virPCIDeviceAddressIsValid() and > > virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I > > swear that I posted patches moving them over... My bad, I'll do > > that right away. > > > > Sorry for the confusion. > > Got it. The patches mentioned above have been posted and merged in the meantime :) > > From the high level point of view, code outside of conf/ should, > > for the most part, not need to concern itself with zPCI at all: it > > would eg. ask for a PCI address to be allocated, and if the device > > in question can be a zPCI device then a zPCI extension address will > > be allocated for it as part of the same function call; the only > > part of qemu/ that should care about the zPCI address is the one > > generating the relevant command line arguments. > > > > Can you try and see whether this kind of API would work? > > I did a simple test. It worked. Do you prefer this way? Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
在 2018/9/20 下午4:28, Andrea Bolognani 写道: On Wed, 2018-09-19 at 16:59 +0800, Yi Min Zhao wrote: 在 2018/9/13 下午9:58, Andrea Bolognani 写道: I realize now that virPCIDeviceAddressIsValid() and virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I swear that I posted patches moving them over... My bad, I'll do that right away. Sorry for the confusion. Got it. The patches mentioned above have been posted and merged in the meantime :) From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments. Can you try and see whether this kind of API would work? I did a simple test. It worked. Do you prefer this way? Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now. So do you mean we use this kind of API in next version for review? -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote: > > > > From the high level point of view, code outside of conf/ should, > > > > for the most part, not need to concern itself with zPCI at all: it > > > > would eg. ask for a PCI address to be allocated, and if the device > > > > in question can be a zPCI device then a zPCI extension address will > > > > be allocated for it as part of the same function call; the only > > > > part of qemu/ that should care about the zPCI address is the one > > > > generating the relevant command line arguments. > > > > > > > > Can you try and see whether this kind of API would work? > > > > > > I did a simple test. It worked. Do you prefer this way? > > > > Yes please, I'd very much like to see what that looks like and > > whether it addresses the problems caused by the ambiguity of the > > API we've used until now. > > So do you mean we use this kind of API in next version for review? Yes, please :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
在 2018/9/25 下午4:02, Andrea Bolognani 写道: On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote: From the high level point of view, code outside of conf/ should, for the most part, not need to concern itself with zPCI at all: it would eg. ask for a PCI address to be allocated, and if the device in question can be a zPCI device then a zPCI extension address will be allocated for it as part of the same function call; the only part of qemu/ that should care about the zPCI address is the one generating the relevant command line arguments. Can you try and see whether this kind of API would work? I did a simple test. It worked. Do you prefer this way? Yes please, I'd very much like to see what that looks like and whether it addresses the problems caused by the ambiguity of the API we've used until now. So do you mean we use this kind of API in next version for review? Yes, please :) I'm not sure how big the change is. So I might take more time to prepare the new version. So that we can't catch up with the new release. Thanks for your kindness and comments! -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
On Tue, 2018-09-25 at 17:16 +0800, Yi Min Zhao wrote: > > > So do you mean we use this kind of API in next version for review? > > > > Yes, please :) > > I'm not sure how big the change is. So I might take more time > to prepare the new version. So that we can't catch up with > the new release. Thanks for your kindness and comments! I think it's going to be a fairly sizeable change, but as we're getting awfully close to freeze anyway I would have advised against merging zPCI support this late in the cycle anyway, so timing-wise I'd say it probably won't change much. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
