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 <zyimin@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
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, &addr->fid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Release fid %u failed"), addr->fid);
+ }
+
+ addr->fid = 0;
+}
+
+
+static void
+virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
+ virZPCIDeviceAddressPtr addr)
+{
+ if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
+ return;
+
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+
+ virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
+}
+
+
+static int
+virDomainZPCIAddressReserveNextUid(virHashTablePtr uids,
+ virZPCIDeviceAddressPtr zpci)
+{
+ if (virDomainZPCIAddressAssignUid(uids, zpci) < 0)
+ return -1;
+
+ if (virDomainZPCIAddressReserveUid(uids, zpci) < 0)
+ return -1;
+
+ return 0;
+}
+
+
+static int
+virDomainZPCIAddressReserveNextFid(virHashTablePtr fids,
+ virZPCIDeviceAddressPtr zpci)
+{
+ if (virDomainZPCIAddressAssignFid(fids, zpci) < 0)
+ return -1;
+
+ if (virDomainZPCIAddressReserveFid(fids, zpci) < 0)
+ return -1;
+
+ return 0;
+}
+
+
static void
virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
{
@@ -44,6 +190,90 @@ virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
VIR_FREE(addrs->zpciIds);
}
+
+static int
+virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
+ virZPCIDeviceAddressPtr addr)
+{
+ if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
+ return -1;
+
+ if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
+ virZPCIDeviceAddressPtr addr)
+{
+ if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
+ return -1;
+
+ if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ return -1;
+ }
+
+ return 0;
+}
+
+
+int
+virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ virDomainPCIAddressExtensionFlags extFlags)
+{
+ if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+ /* Reserve uid/fid to ZPCI device which has defined uid/fid
+ * in the domain.
+ */
+ return virDomainZPCIAddressReserveAddr(addrs->zpciIds, &addr->zpci);
+ }
+
+ return 0;
+}
+
+
+int
+virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr dev,
+ virDomainPCIAddressExtensionFlags extFlags)
+{
+ if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+ virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
+ virZPCIDeviceAddress zpci = { 0 };
+
+ if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0)
+ return -1;
+
+ if (!addrs->dryRun)
+ dev->zpci = zpci;
+ }
+
+ return 0;
+}
+
+static int
+virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
+ virDomainDeviceInfoPtr dev)
+{
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+ virZPCIDeviceAddressPtr zpci = &dev->addr.pci.zpci;
+
+ if (virZPCIDeviceAddressIsEmpty(zpci))
+ return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
+ else
+ return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci);
+ }
+
+ return 0;
+}
+
virDomainPCIConnectFlags
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
{
@@ -740,12 +970,24 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
ret = virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1);
}
+ ret = virDomainPCIAddressExtensionEnsureAddr(addrs, dev);
+
cleanup:
VIR_FREE(addrStr);
return ret;
}
+void
+virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ int extFlags)
+{
+ if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))
+ virDomainZPCIAddressReleaseIds(addrs->zpciIds, &addr->zpci);
+}
+
+
void
virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index d1ec869da4..a1f71f15f4 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -164,6 +164,16 @@ bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ virDomainPCIAddressExtensionFlags extFlags)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+int virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ virDomainPCIAddressExtensionFlags extFlags)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
int virDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr,
virDomainPCIConnectFlags flags,
@@ -185,6 +195,11 @@ void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+void virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ int extFlags)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
void virDomainPCIAddressSetAllMulti(virDomainDefPtr def)
ATTRIBUTE_NONNULL(1);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 12f051468e..f6c512710d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -93,6 +93,7 @@ virCPUModeTypeToString;
# conf/device_conf.h
+virDeviceInfoPCIAddressExtensionIsPresent;
virDeviceInfoPCIAddressIsPresent;
virDeviceInfoPCIAddressIsWanted;
virDomainDeviceInfoAddressIsEqual;
@@ -115,6 +116,9 @@ virDomainPCIAddressAsString;
virDomainPCIAddressBusIsFullyReserved;
virDomainPCIAddressBusSetModel;
virDomainPCIAddressEnsureAddr;
+virDomainPCIAddressExtensionReleaseAddr;
+virDomainPCIAddressExtensionReserveAddr;
+virDomainPCIAddressExtensionReserveNextAddr;
virDomainPCIAddressReleaseAddr;
virDomainPCIAddressReserveAddr;
virDomainPCIAddressReserveNextAddr;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 162014e089..e9e0970567 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1405,6 +1405,22 @@ qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
}
+static int
+qemuDomainAssignPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ virDomainPCIAddressSetPtr addrs = opaque;
+ virPCIDeviceAddressPtr addr = &info->addr.pci;
+ virDomainPCIAddressExtensionFlags extFlags = info->pciAddressExtFlags;
+
+ if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+ return virDomainPCIAddressExtensionReserveNextAddr(addrs, addr, extFlags);
+
+ return 0;
+}
+
static int
qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainDeviceDefPtr device,
@@ -1498,6 +1514,29 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
return ret;
}
+static int
+qemuDomainCollectPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr device,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ virDomainPCIAddressSetPtr addrs = opaque;
+ virPCIDeviceAddressPtr addr = &info->addr.pci;
+
+ if (!virDeviceInfoPCIAddressExtensionIsPresent(info) ||
+ ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
+ (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) {
+ /* If a hostdev has a parent, its info will be a part of the
+ * parent, and will have its address collected during the scan
+ * of the parent's device type.
+ */
+ return 0;
+ }
+
+ return virDomainPCIAddressExtensionReserveAddr(addrs, addr,
+ info->pciAddressExtFlags);
+}
+
static virDomainPCIAddressSetPtr
qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
@@ -1592,6 +1631,12 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, addrs) < 0)
goto error;
+ if (virDomainDeviceInfoIterate(def,
+ qemuDomainCollectPCIAddressExtension,
+ addrs) < 0) {
+ goto error;
+ }
+
return addrs;
error:
@@ -2595,6 +2640,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
goto cleanup;
+ if (virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) < 0)
+ goto cleanup;
+
/* Only for *new* domains with pcie-root (and no other
* manually specified PCI controllers in the definition): If,
* after assigning addresses/reserving slots for all devices,
@@ -2689,6 +2737,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
goto cleanup;
+ if (virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) < 0)
+ goto cleanup;
+
/* set multi attribute for devices at function 0 of
* any slot that has multiple functions in use
*/
@@ -3148,8 +3199,11 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
if (!devstr)
devstr = info->alias;
- if (virDeviceInfoPCIAddressIsPresent(info))
+ if (virDeviceInfoPCIAddressIsPresent(info)) {
virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci);
+ virDomainPCIAddressExtensionReleaseAddr(priv->pciaddrs, &info->addr.pci,
+ info->pciAddressExtFlags);
+ }
if (virDomainUSBAddressRelease(priv->usbaddrs, info) < 0)
VIR_WARN("Unable to release USB address on %s", NULLSTR(devstr));
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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
在 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
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
在 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
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
在 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
© 2016 - 2026 Red Hat, Inc.