Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/8/23 下午7:01, Andrea Bolognani 写道:
On Thu, 2018-08-23 at 18:01 +0800, Yi Min Zhao wrote:
在 2018/8/22 下午5:56, Andrea Bolognani 写道:
The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :)
I saw them. Could I give my ACKed?
Feel free to do that, but before pushing I'd like to have Laine's
ACK since he's the one who introduced the functions in the first
place.
I have a question about your previous comment about error report.
You thought we should report more specific information.
+virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
+
+if (zpci && !dev->pciAddressExtFlags) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not
supported."));
+return -1;
+}
It's called by all device types which possibly use PCI address.
I'm not sure how to report device's name in error string.
I think reporting the address is enough. The idea is to give the
user some information they can use to figure out which device is
misconfigured rather than just telling them there is an issue and
leaving it at that.
I also think we might be at a point where enough big changes have
been made that the best way forward is probably to post a v3 that
includes those and then use that as the basis for more fine-tuning
for minor stuff such as error messages and the like. Does that
sound reasonable?
Yes, it makes sense.
I'm preparing V4 and could post it today.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
On Thu, 2018-08-23 at 13:01 +0200, Andrea Bolognani wrote: > I also think we might be at a point where enough big changes have > been made that the best way forward is probably to post a v3 that > includes those and then use that as the basis for more fine-tuning > for minor stuff such as error messages and the like. Does that > sound reasonable? Of course I meant to say v4, not v3, above :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
On Thu, 2018-08-23 at 18:01 +0800, Yi Min Zhao wrote:
> 在 2018/8/22 下午5:56, Andrea Bolognani 写道:
> > The patches I said I'd write are now on the list:
> >
> >https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
> >
> > No reviews yet, we'll see whether they get ACKed or NACKed :)
>
> I saw them. Could I give my ACKed?
Feel free to do that, but before pushing I'd like to have Laine's
ACK since he's the one who introduced the functions in the first
place.
> I have a question about your previous comment about error report.
> You thought we should report more specific information.
>
> > +virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
> > +
> > +if (zpci && !dev->pciAddressExtFlags) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not
> > supported."));
> > +return -1;
> > +}
>
> It's called by all device types which possibly use PCI address.
> I'm not sure how to report device's name in error string.
I think reporting the address is enough. The idea is to give the
user some information they can use to figure out which device is
misconfigured rather than just telling them there is an issue and
leaving it at that.
I also think we might be at a point where enough big changes have
been made that the best way forward is probably to post a v3 that
includes those and then use that as the basis for more fine-tuning
for minor stuff such as error messages and the like. Does that
sound reasonable?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/8/22 下午5:56, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
在 2018/8/17 上午12:06, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+static inline bool
+virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
+{
+return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+info->addr.pci.zpci;
+}
This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.
Got it. Thank!
The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :)
I saw them. Could I give my ACKed?
[...]
+static int
+virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
+virZPCIDeviceAddressPtr addr)
+{
+if (!addr->uid_assigned &&
+virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
+return -1;
+}
+
+if (!addr->fid_assigned &&
+virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
+virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+return -1;
+}
Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.
If uid/fid is assigned, we call ***Reserve***().
If uid/fid is unassigned, we call ***ReserveNext***().
But I'm not very clear about your concern.
That in ReserveNext() you're checking whether addr->*id_assigned
when you know that ids have not been assigned or you wouldn't have
called ReserveNext() in the first place... It seems unnecessary
and confusing to me, so unless I've missed something that makes it
necessary please just drop those checks.
As our discussion on the 1st patch, boolean values are removed.
So we don't need checks here.
[...]
+static int
+virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
+ virDomainDeviceInfoPtr dev)
+{
It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.
We have to pass DeviceInfo which already has extFlags. If we pass
extFlags again,
isn't it redundant? This function is only called in one place for
hotplug case.
I wanted the API to be more consistent but I realize now you have
to pass either virPCIDeviceAddress or virDomainDeviceInfo depending
on the context, so it doesn't really matter, you can leave it as it
is. The signatures for the corresponding PCI functions are not
entirely consistent either :)
I have a question about your previous comment about error report.
You thought we should report more specific information.
+virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
+
+if (zpci && !dev->pciAddressExtFlags) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not
supported."));
+return -1;
+}
It's called by all device types which possibly use PCI address.
I'm not sure how to report device's name in error string.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
> 在 2018/8/17 上午12:06, Andrea Bolognani 写道:
> > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> > [...]
> > > +static inline bool
> > > +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
> > > +{
> > > +return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> > > +info->addr.pci.zpci;
> > > +}
> >
> > This should be called virDeviceInfoPCIAddressExtensionIsPresent()
> > since it's a predicate. I know the corresponding PCI function gets
> > the naming wrong, but that doesn't mean you should too :)
> >
> > In the same vein, I don't think this (or the PCI version, for that
> > matter) need to be inline... I'd rather have them both as regular
> > functions in the .c file. I'll probably cook up a patch cleaning
> > up the PCI part later, see what the feedback is.
>
> Got it. Thank!
The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :)
> > [...]
> > > +static int
> > > +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> > > +virZPCIDeviceAddressPtr addr)
> > > +{
> > > +if (!addr->uid_assigned &&
> > > +virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
> > > +return -1;
> > > +}
> > > +
> > > +if (!addr->fid_assigned &&
> > > +virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
> > > +virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> > > +return -1;
> > > +}
> >
> > Not sure I get the logic here... ReserveNextAddress() is supposed
> > to pick the next available address and reserve it, but IIUC this
> > will skip picking either id based on whether they were assigned.
>
> If uid/fid is assigned, we call ***Reserve***().
> If uid/fid is unassigned, we call ***ReserveNext***().
>
> But I'm not very clear about your concern.
That in ReserveNext() you're checking whether addr->*id_assigned
when you know that ids have not been assigned or you wouldn't have
called ReserveNext() in the first place... It seems unnecessary
and confusing to me, so unless I've missed something that makes it
necessary please just drop those checks.
> > [...]
> > > +static int
> > > +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> > > + virDomainDeviceInfoPtr dev)
> > > +{
> >
> > It's weird that this function doesn't get extFlags as an argument,
> > unlike the other ones you've introduced. Better make it consistent.
>
> We have to pass DeviceInfo which already has extFlags. If we pass
> extFlags again,
> isn't it redundant? This function is only called in one place for
> hotplug case.
I wanted the API to be more consistent but I realize now you have
to pass either virPCIDeviceAddress or virDomainDeviceInfo depending
on the context, so it doesn't really matter, you can leave it as it
is. The signatures for the corresponding PCI functions are not
entirely consistent either :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/8/17 上午12:06, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+static inline bool
+virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
+{
+return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+info->addr.pci.zpci;
+}
This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.
Got it. Thank!
[...]
+static int
+virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
+virZPCIDeviceAddressPtr addr)
+{
+if (!addr->uid_assigned &&
+virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
+return -1;
+}
+
+if (!addr->fid_assigned &&
+virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
+virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+return -1;
+}
Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.
If uid/fid is assigned, we call ***Reserve***().
If uid/fid is unassigned, we call ***ReserveNext***().
But I'm not very clear about your concern.
I don't think that's what we want.
Also all functions that return an int should be called like
if (virDomainZPCIAddressReserveNextUid() < 0) {
...
}
OK.
[...]
+static int
+virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
+ virDomainDeviceInfoPtr dev)
+{
It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.
We have to pass DeviceInfo which already has extFlags. If we pass
extFlags again,
isn't it redundant? This function is only called in one place for
hotplug case.
+virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
+
+if (zpci && !dev->pciAddressExtFlags) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not
supported."));
+return -1;
+}
The error message is very generic here, it should at the very least
make it clear that zPCI is not supported *for the specific device*.
I'm not sure this is the best place to perform that kind of check,
either.
This is only called by virDomainPCIAddressEnsureAddr() for hotplug case.
I thought this again. If we remove those two boolean members from zPCI
address structure as what we discuss on the 1st patch, we could move this
check to virDomainPCIAddressEnsureAddr() like what PCI addr check does.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +static inline bool
> +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
> +{
> +return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +info->addr.pci.zpci;
> +}
This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.
[...]
> +static int
> +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> +virZPCIDeviceAddressPtr addr)
> +{
> +if (!addr->uid_assigned &&
> +virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
> +return -1;
> +}
> +
> +if (!addr->fid_assigned &&
> +virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
> +virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> +return -1;
> +}
Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.
I don't think that's what we want.
Also all functions that return an int should be called like
if (virDomainZPCIAddressReserveNextUid() < 0) {
...
}
[...]
> +static int
> +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> + virDomainDeviceInfoPtr dev)
> +{
It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.
> +virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
> +
> +if (zpci && !dev->pciAddressExtFlags) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not
> supported."));
> +return -1;
> +}
The error message is very generic here, it should at the very least
make it clear that zPCI is not supported *for the specific device*.
I'm not sure this is the best place to perform that kind of check,
either.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
This patch adds new functions for reservation, assignment and release
to handle the uid/fid. If the uid/fid is defined in the domain XML,
they will be reserved directly in collecting phase. If any of them is
not defined, we will find out an available value for it from zPCI
address hashtable, and reserve it. For hotplug case, there might be or
not zPCI definition. So allocate and reserve uid/fid for undefined
case. Assign if needed and reserve uid/fid for defined case. If the user
define zPCI extension address but zPCI capability doesn't exist, an
error will be reported.
Signed-off-by: Yi Min Zhao
Reviewed-by: Boris Fiuczynski
Reviewed-by: Ján Tomko
---
src/conf/device_conf.h | 7 +
src/conf/domain_addr.c | 291 +
src/conf/domain_addr.h | 15 +++
src/libvirt_private.syms | 3 +
src/qemu/qemu_domain_address.c | 58 +++-
5 files changed, 373 insertions(+), 1 deletion(-)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 6f926dff1d..40838a9401 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -211,6 +211,13 @@ virDeviceInfoPCIAddressPresent(const virDomainDeviceInfo
*info)
!virPCIDeviceAddressIsEmpty(>addr.pci);
}
+static inline bool
+virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
+{
+return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+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 90d8abc1f4..cf1fc467f2 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -33,6 +33,170 @@
VIR_LOG_INIT("conf.domain_addr");
+static int
+virDomainZPCIAddressReserveId(virHashTablePtr set,
+ unsigned int id,
+ const char *name)
+{
+if (virHashLookup(set, )) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("zPCI %s %u is already reserved"),
+ name, id);
+return -1;
+}
+
+if (virHashAddEntry(set, , (void*)1) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to reserve %s %u"),
+ name, id);
+return -1;
+}
+
+return 0;
+}
+
+
+static int
+virDomainZPCIAddressReserveUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressReserveId(set, addr->zpci_uid, "uid");
+}
+
+
+static int
+virDomainZPCIAddressReserveFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressReserveId(set, addr->zpci_fid, "fid");
+}
+
+
+static bool
+virDomainZPCIAddressAssignId(virHashTablePtr set,
+ unsigned int *id,
+ unsigned int min,
+ unsigned int max,
+ const char *name)
+{
+while (virHashLookup(set, )) {
+if (min == max) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("There is no more free %s."),
+ name);
+return false;
+}
+++min;
+}
+*id = min;
+
+return true;
+}
+
+
+static int
+virDomainZPCIAddressAssignUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (addr->uid_assigned)
+return 0;
+
+addr->uid_assigned =
+virDomainZPCIAddressAssignId(set, >zpci_uid, 1,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid");
+return addr->uid_assigned ? 0 : -1;
+}
+
+
+static int
+virDomainZPCIAddressAssignFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (addr->fid_assigned)
+return 0;
+
+addr->fid_assigned =
+virDomainZPCIAddressAssignId(set, >zpci_fid, 0,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid");
+return addr->fid_assigned ? 0 : -1;
+}
+
+
+static void
+virDomainZPCIAddressReleaseUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (!addr->uid_assigned)
+return;
+
+if (virHashRemoveEntry(set, >zpci_uid)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Release uid %u failed"), addr->zpci_uid);
+}
+
+addr->uid_assigned = false;
+}
+
+
+static void
+virDomainZPCIAddressReleaseFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (!addr->fid_assigned)
+return;
+
+if (virHashRemoveEntry(set, >zpci_fid)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Release fid %u failed"), addr->zpci_fid);
+}
+
+addr->fid_assigned = false;
+}
+
+
+static void
