Re: [libvirt] [PATCH v5 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/9/18 下午4:59, Andrea Bolognani 写道:
On Tue, 2018-09-18 at 13:51 +0800, Yi Min Zhao wrote:
+int
+virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
+virPCIDeviceAddressPtr dev,
+virDomainPCIAddressExtensionFlags
extFlags)
+{
+if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the
next address regardless of what's currently stored in the device
info, no?
I think it's hard to do it as what you said. We process assigned zpci
addresses
firstly. And then reserve next address for empty zpci addresses. If we don't
check this, we might reserve an address for a reserved one.
Shouldn't the EnsureAddr() function take care of avoiding that?
It will call ReserveAddr() or ReserveNextAddr() based on whether
or not an address has already been provided by the user.
IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also
called in startup stage. After reserve defined addresses, RerserveNextAddr()
is called to allocate addresses. Here, we should check if it's reserved.
You could see virDomainDeviceInfoIterate(def,
qemuDomainAssignPCIAddressExtension, addrs)
in qemuDomainAssignPCIAddresses().
Okay, qemuDomainAssignPCIAddresses() doesn't actually call
virDomainPCIAddressEnsureAddr() but it's basically doing the same
thing: after the first dry-run used to figure out how many PCI buses
are necessary, it will call qemuDomainPCIAddressSetCreate() which
internally calls qemuDomainCollectPCIAddress() on all devices, thus
picking up all PCI addresses that were already provided by the user;
then it calls qemuDomainAssignDevicePCISlots(), which calls
qemuDomainPCIAddressReserveNextAddr() on all devices, but *only*
after making sure with virDeviceInfoPCIAddressIsWanted() that they
hadn't already been assigned an address.
The end result is that qemuDomainPCIAddressReserveNextAddr() will
only ever be called on devices that were not already assigned a PCI
address, and thus virDomainPCIAddressReserveNextAddr() can afford
to simply pick an address and reserve it without checking first
whether the device in question had one already.
To keep things easy to understand, your functions should follow the
same semantics.
Yeah, in my new version I have introduced a similar function like
***IsWanted().
I think it's same with what your said.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Tue, 2018-09-18 at 13:51 +0800, Yi Min Zhao wrote:
> > > > > +int
> > > > > +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr
> > > > > addrs,
> > > > > +virPCIDeviceAddressPtr
> > > > > dev,
> > > > > +
> > > > > virDomainPCIAddressExtensionFlags extFlags)
> > > > > +{
> > > > > +if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> > > > > +virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
> > > >
> > > > You shouldn't need the second check: just go ahead and reserve the
> > > > next address regardless of what's currently stored in the device
> > > > info, no?
> > >
> > > I think it's hard to do it as what you said. We process assigned zpci
> > > addresses
> > > firstly. And then reserve next address for empty zpci addresses. If we
> > > don't
> > > check this, we might reserve an address for a reserved one.
> >
> > Shouldn't the EnsureAddr() function take care of avoiding that?
> > It will call ReserveAddr() or ReserveNextAddr() based on whether
> > or not an address has already been provided by the user.
>
> IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also
> called in startup stage. After reserve defined addresses, RerserveNextAddr()
> is called to allocate addresses. Here, we should check if it's reserved.
> You could see virDomainDeviceInfoIterate(def,
> qemuDomainAssignPCIAddressExtension, addrs)
> in qemuDomainAssignPCIAddresses().
Okay, qemuDomainAssignPCIAddresses() doesn't actually call
virDomainPCIAddressEnsureAddr() but it's basically doing the same
thing: after the first dry-run used to figure out how many PCI buses
are necessary, it will call qemuDomainPCIAddressSetCreate() which
internally calls qemuDomainCollectPCIAddress() on all devices, thus
picking up all PCI addresses that were already provided by the user;
then it calls qemuDomainAssignDevicePCISlots(), which calls
qemuDomainPCIAddressReserveNextAddr() on all devices, but *only*
after making sure with virDeviceInfoPCIAddressIsWanted() that they
hadn't already been assigned an address.
The end result is that qemuDomainPCIAddressReserveNextAddr() will
only ever be called on devices that were not already assigned a PCI
address, and thus virDomainPCIAddressReserveNextAddr() can afford
to simply pick an address and reserve it without checking first
whether the device in question had one already.
To keep things easy to understand, your functions should follow the
same semantics.
--
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 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/9/17 下午8:05, Andrea Bolognani 写道:
On Mon, 2018-09-17 at 13:43 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午9:59, Andrea Bolognani 写道:
+static void
+virDomainZPCIAddressReleaseUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (virHashRemoveEntry(set, &addr->uid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Release uid %u failed"), addr->uid);
+}
You should have a generic virDomainZPCIAddressReleaseId() function
that you call from ReleaseUid() and ReleaseFid(), just like you
have for reserving them.
Actually there's the function ***ReleaseId() like you said. Don't you
see it?
No such function exists; there is a virDomainZPCIAddressReleaseIds()
but that doesn't do what I had in mind, which is along the lines of
static void
virDomainZPCIAddressReleaseId(virHashTablePtr set,
unsigned int *id,
const char *name)
{
if (virHashRemoveEntry(set, id) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Release %s %u failed"), name, *id);
}
*id = 0;
}
static void
virDomainZPCIAddressReleaseUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
}
static void
virDomainZPCIAddressReleaseFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
}
Got it.
+int
+virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
+virPCIDeviceAddressPtr dev,
+virDomainPCIAddressExtensionFlags
extFlags)
+{
+if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the
next address regardless of what's currently stored in the device
info, no?
I think it's hard to do it as what you said. We process assigned zpci
addresses
firstly. And then reserve next address for empty zpci addresses. If we don't
check this, we might reserve an address for a reserved one.
Shouldn't the EnsureAddr() function take care of avoiding that?
It will call ReserveAddr() or ReserveNextAddr() based on whether
or not an address has already been provided by the user.
IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also
called in startup stage. After reserve defined addresses, RerserveNextAddr()
is called to allocate addresses. Here, we should check if it's reserved.
You could see virDomainDeviceInfoIterate(def,
qemuDomainAssignPCIAddressExtension, addrs)
in qemuDomainAssignPCIAddresses().
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Mon, 2018-09-17 at 13:43 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午9:59, Andrea Bolognani 写道:
> > > +static void
> > > +virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> > > + virZPCIDeviceAddressPtr addr)
> > > +{
> > > +if (virHashRemoveEntry(set, &addr->uid) < 0) {
> > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > + _("Release uid %u failed"), addr->uid);
> > > +}
> >
> > You should have a generic virDomainZPCIAddressReleaseId() function
> > that you call from ReleaseUid() and ReleaseFid(), just like you
> > have for reserving them.
>
> Actually there's the function ***ReleaseId() like you said. Don't you
> see it?
No such function exists; there is a virDomainZPCIAddressReleaseIds()
but that doesn't do what I had in mind, which is along the lines of
static void
virDomainZPCIAddressReleaseId(virHashTablePtr set,
unsigned int *id,
const char *name)
{
if (virHashRemoveEntry(set, id) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Release %s %u failed"), name, *id);
}
*id = 0;
}
static void
virDomainZPCIAddressReleaseUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
}
static void
virDomainZPCIAddressReleaseFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
}
> > > +int
> > > +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr
> > > addrs,
> > > +virPCIDeviceAddressPtr dev,
> > > +
> > > virDomainPCIAddressExtensionFlags extFlags)
> > > +{
> > > +if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> > > +virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
> >
> > You shouldn't need the second check: just go ahead and reserve the
> > next address regardless of what's currently stored in the device
> > info, no?
>
> I think it's hard to do it as what you said. We process assigned zpci
> addresses
> firstly. And then reserve next address for empty zpci addresses. If we don't
> check this, we might reserve an address for a reserved one.
Shouldn't the EnsureAddr() function take care of avoiding that?
It will call ReserveAddr() or ReserveNextAddr() based on whether
or not an address has already been provided by the user.
--
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 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/9/11 下午9:59, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
If the user
define zPCI extension address but zPCI capability doesn't exist, an
error will be reported.
You're (no longer) checking for the capability here, so the commit
message should be updated accordingly.
Good catch!
[...]
+bool
+virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
+{
+return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
The second line is not aligned properly.
Actually, you'll probably want to restructure this a bit so that it
only looks into info->addr.pci.zpci if the zPCI extension flag is
present, after which the comment above will likely no longer apply.
[...]
OK.
+static int
+virDomainZPCIAddressReserveId(virHashTablePtr set,
+ unsigned int id,
+ const char *name)
+{
+if (virHashLookup(set, &id)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("zPCI %s %u is already reserved"),
+ name, id);
Please format the id as octal for easier debugging when something
goes wrong. Same below.
OK.
[...]
+static void
+virDomainZPCIAddressReleaseUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (virHashRemoveEntry(set, &addr->uid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Release uid %u failed"), addr->uid);
+}
You should have a generic virDomainZPCIAddressReleaseId() function
that you call from ReleaseUid() and ReleaseFid(), just like you
have for reserving them.
Actually there's the function ***ReleaseId() like you said. Don't you
see it?
Additionally, it looks like failure to release an id is not a
fatal error since processing continue, so you should use VIR_WARN()
or something like that instead of virReportError() when that
happens.
Sure.
[...]
+int
+virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
+virPCIDeviceAddressPtr dev,
+virDomainPCIAddressExtensionFlags
extFlags)
+{
+if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the
next address regardless of what's currently stored in the device
info, no?
I think it's hard to do it as what you said. We process assigned zpci
addresses
firstly. And then reserve next address for empty zpci addresses. If we don't
check this, we might reserve an address for a reserved one.
[...]
+void
+virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
+virPCIDeviceAddressPtr addr,
+int extFlags)
+{
+if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))
You don't need two sets of parentheses here :)
yes. typo.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> If the user
> define zPCI extension address but zPCI capability doesn't exist, an
> error will be reported.
You're (no longer) checking for the capability here, so the commit
message should be updated accordingly.
[...]
> +bool
> +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
> +{
> +return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
The second line is not aligned properly.
Actually, you'll probably want to restructure this a bit so that it
only looks into info->addr.pci.zpci if the zPCI extension flag is
present, after which the comment above will likely no longer apply.
[...]
> +static int
> +virDomainZPCIAddressReserveId(virHashTablePtr set,
> + unsigned int id,
> + const char *name)
> +{
> +if (virHashLookup(set, &id)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("zPCI %s %u is already reserved"),
> + name, id);
Please format the id as octal for easier debugging when something
goes wrong. Same below.
[...]
> +static void
> +virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> + virZPCIDeviceAddressPtr addr)
> +{
> +if (virHashRemoveEntry(set, &addr->uid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Release uid %u failed"), addr->uid);
> +}
You should have a generic virDomainZPCIAddressReleaseId() function
that you call from ReleaseUid() and ReleaseFid(), just like you
have for reserving them.
Additionally, it looks like failure to release an id is not a
fatal error since processing continue, so you should use VIR_WARN()
or something like that instead of virReportError() when that
happens.
[...]
> +int
> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr dev,
> +
> virDomainPCIAddressExtensionFlags extFlags)
> +{
> +if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> +virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the
next address regardless of what's currently stored in the device
info, no?
[...]
> +void
> +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr addr,
> +int extFlags)
> +{
> +if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))
You don't need two sets of parentheses here :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 09/13] 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.c | 7 ++
src/conf/device_conf.h | 1 +
src/conf/domain_addr.c | 242 +
src/conf/domain_addr.h | 15 +++
src/libvirt_private.syms | 4 +
src/qemu/qemu_domain_address.c | 56 +-
6 files changed, 324 insertions(+), 1 deletion(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 5ae990afdb..692d4c31ea 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -278,6 +278,13 @@ virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress
*addr)
return !(addr->uid || addr->fid);
}
+bool
+virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
+{
+return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
+}
+
int
virPCIDeviceAddressParseXML(xmlNodePtr node,
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 7e98b4ace0..c5629dc26b 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -195,6 +195,7 @@ bool virDeviceInfoPCIAddressIsWanted(const
virDomainDeviceInfo *info);
bool virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info);
bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr);
+bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo
*info);
int virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr);
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 62932e4e62..1c3248f04b 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -33,6 +33,152 @@
VIR_LOG_INIT("conf.domain_addr");
+static int
+virDomainZPCIAddressReserveId(virHashTablePtr set,
+ unsigned int id,
+ const char *name)
+{
+if (virHashLookup(set, &id)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("zPCI %s %u is already reserved"),
+ name, id);
+return -1;
+}
+
+if (virHashAddEntry(set, &id, (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->uid, "uid");
+}
+
+
+static int
+virDomainZPCIAddressReserveFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressReserveId(set, addr->fid, "fid");
+}
+
+
+static int
+virDomainZPCIAddressAssignId(virHashTablePtr set,
+ unsigned int *id,
+ unsigned int min,
+ unsigned int max,
+ const char *name)
+{
+while (virHashLookup(set, &min)) {
+if (min == max) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("There is no more free %s."),
+ name);
+return -1;
+}
+++min;
+}
+*id = min;
+
+return 0;
+}
+
+
+static int
+virDomainZPCIAddressAssignUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressAssignId(set, &addr->uid, 1,
+VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid");
+}
+
+
+static int
+virDomainZPCIAddressAssignFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressAssignId(set, &addr->fid, 0,
+VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid");
+}
+
+
+static void
+virDomainZPCIAddressReleaseUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (virHashRemoveEntry(set, &addr->uid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Release uid %u failed"), addr->uid);
+}
+
+addr->uid = 0;
+}
+
+
+static void
+virDomainZPCIAddressReleaseFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+if (virHashRemoveEntry(set
