10:05 p.m.
This is what remains of yesterday's VFIO groups patchset that is
considered still useful, but wasn't yet ACKed (I've pushed the 5 that
were ACKed). In addition, I found a few more bugs in the
virPCIDeviceList handling and have included patches for those. There
are a couple of these that would be really nice to get into 1.0.7, one
in particular which is new functionality, but has 4 prerequisites:
The most important patch here from a functionality point of view is
PATCH 05/12, which adds the <iommuGroup> element to the output of
"virsh nodedev-dumpxml". This is essential for users/management
applications to have adequate information to know which devices need
to be detached from host drivers. (Unfortunately, all of the patches
prior to 05/12 are required for 05/12 to cleanly apply.)
The other new patch that I've been told is important is 08/12, which
supresses reset of PCI devices that were/will be assigned using VFIO -
apparently the vfio driver already resets the devices when
appropriate, so having libvirt do it is just redundant. (In order for
08/12 to operate reliably, 07/12 is required, and in order for 07/12
to apply, 06/12 must also be applied).
If someone ACKs any of these patches prior to DV cutting the first rc
and I don't respond on IRC (not sure what my schedule is in the next
12 hours :-), please feel free to push any of them you like, as long
as you push them in order with none missing from the middle.
Laine Stump (12):
pci: eliminate unused driver arg from virPCIDeviceDetach
pci: rename virPCIDeviceGetVFIOGroupDev to
virPCIDeviceGetIOMMUGroupDev
pci: make virPCIParseDeviceAddress public
pci: new iommu_group functions
nodedev: add iommuGroup to node device object
pci: eliminate repetitive path constructions in
virPCIDeviceBindToStub
pci: update stubDriver name in virPCIDeviceBindToStub
qemu: don't reset PCI devices being assigned with VFIO
pci: virPCIDeviceListAddCopy API
pci: eliminate leak in OOM condition
pci: fix dangling pointer in qemuDomainReAttachHostdevDevices
qemu: fix infinite loop in OOM error path
docs/formatnode.html.in | 63 +++-
docs/schemas/nodedev.rng | 11 +
src/conf/node_device_conf.c | 86 +++++-
src/conf/node_device_conf.h | 5 +-
src/libvirt_private.syms | 8 +-
src/node_device/node_device_udev.c | 21 +-
src/qemu/qemu_cgroup.c | 4 +-
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_hostdev.c | 52 ++--
src/qemu/qemu_hotplug.c | 5 +-
src/security/security_apparmor.c | 2 +-
src/security/security_dac.c | 4 +-
src/security/security_selinux.c | 4 +-
src/util/virpci.c | 344 +++++++++++++++++----
src/util/virpci.h | 21 +-
src/xen/xen_driver.c | 2 +-
tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml | 16 +
tests/nodedevxml2xmltest.c | 1 +
18 files changed, 553 insertions(+), 98 deletions(-)
create mode 100644 tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
--
1.7.11.7
10:05 p.m.
New subject: [libvirt] [PATCHv2 01/12] pci: eliminate unused driver arg from virPCIDeviceDetach
The driver arg to virPCIDeviceDetach is no longer used (the name of the stub driver is now
set in the virPCIDevice object, and virPCIDeviceDetach retrieves it from there). Remove
it.
---
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_hostdev.c | 2 +-
src/util/virpci.c | 13 +++++--------
src/util/virpci.h | 3 +--
src/xen/xen_driver.c | 2 +-
5 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index efee62f..83770e5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10617,7 +10617,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
virObjectLock(driver->inactivePciHostdevs);
if (virPCIDeviceDetach(pci, driver->activePciHostdevs,
- driver->inactivePciHostdevs, NULL) < 0) {
+ driver->inactivePciHostdevs) < 0) {
goto out;
}
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 7a9e6eb..dfe39c6 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -540,7 +540,7 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
if (virPCIDeviceGetManaged(dev) &&
- virPCIDeviceDetach(dev, driver->activePciHostdevs, NULL, NULL) < 0)
+ virPCIDeviceDetach(dev, driver->activePciHostdevs, NULL) < 0)
goto reattachdevs;
}
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2f4032f..2980e22 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1238,15 +1238,12 @@ cleanup:
int
virPCIDeviceDetach(virPCIDevicePtr dev,
virPCIDeviceList *activeDevs,
- virPCIDeviceList *inactiveDevs,
- const char *driver)
+ virPCIDeviceList *inactiveDevs)
{
- if (!driver && dev->stubDriver)
- driver = dev->stubDriver;
-
- if (virPCIProbeStubDriver(driver) < 0) {
+ if (virPCIProbeStubDriver(dev->stubDriver) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to load PCI stub module %s"), driver);
+ _("Failed to load PCI stub module %s"),
+ dev->stubDriver);
return -1;
}
@@ -1256,7 +1253,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
return -1;
}
- if (virPCIDeviceBindToStub(dev, driver) < 0)
+ if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0)
return -1;
/* Add *a copy of* the dev into list inactiveDevs, if
diff --git a/src/util/virpci.h b/src/util/virpci.h
index d069adb..944aa09 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -51,8 +51,7 @@ const char *virPCIDeviceGetName(virPCIDevicePtr dev);
int virPCIDeviceDetach(virPCIDevicePtr dev,
virPCIDeviceListPtr activeDevs,
- virPCIDeviceListPtr inactiveDevs,
- const char *driver);
+ virPCIDeviceListPtr inactiveDevs);
int virPCIDeviceReattach(virPCIDevicePtr dev,
virPCIDeviceListPtr activeDevs,
virPCIDeviceListPtr inactiveDevs);
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index ffc6e41..998fc69 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -2440,7 +2440,7 @@ xenUnifiedNodeDeviceDetachFlags(virNodeDevicePtr dev,
goto out;
}
- if (virPCIDeviceDetach(pci, NULL, NULL, NULL) < 0)
+ if (virPCIDeviceDetach(pci, NULL, NULL) < 0)
goto out;
ret = 0;
--
1.7.11.7
4:50 a.m.
New subject: [libvirt] [PATCHv2 01/12] pci: eliminate unused driver arg from virPCIDeviceDetach
On Mon, Jun 24, 2013 at 11:05:27PM -0400, Laine Stump wrote:
The driver arg to virPCIDeviceDetach is no longer used (the name of
the stub driver is now set in the virPCIDevice object, and
virPCIDeviceDetach retrieves it from there). Remove it.
What happens when libvirtd is restarted ? Are we correctly preserving
the original value across restart, or are we capable of auto-detecting
the correct value again ?
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:35 a.m.
New subject: [libvirt] [PATCHv2 01/12] pci: eliminate unused driver arg from virPCIDeviceDetach
On 06/25/2013 05:50 AM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 11:05:27PM -0400, Laine Stump wrote:
> The driver arg to virPCIDeviceDetach is no longer used (the name of
> the stub driver is now set in the virPCIDevice object, and
> virPCIDeviceDetach retrieves it from there). Remove it.
What happens when libvirtd is restarted ? Are we correctly preserving
the original value across restart, or are we capable of auto-detecting
the correct value again ?
That's a good question, for two reasons: 1) I didn't know the answer for
sure when you asked it, and 2) I went back over the code, learned the
answer, and it was a good answer :-)
Originally the stubDriverName in the virPCIDevice object was only used
for a short period - during qemu's "device Preparation" it will create a
virPCIDeviceList from the virDomainHostdevDef list, then later iterate
through the virPCIDeviceList to detach all the devices from their host
drivers; during this detach loop, the original viDomainHostdevDef isn't
available, so an indicator of which stub to bind to must be stashed in
the virPCIDevice object when it is created.
Later it became useful to be able to rely on the stubDriverName always
being correct (at least in the activePciHostdevs list; it doesn't matter
for inactivePciHostdevs). Fortunately that list is recreated whenever
libvirt restarts by calling qemuUpdateActivePciHostdevs() for every
active domain. That function also updates stubDriverName for each active
hostdev based on the config (which must be the actual driver in use,
otherwise we would have encountered an error when the device was
originally being attached).
So, the answer is yes, we auto-set the correct value in the virPCIDevice
object when libvirtd restarts.
However, this entire topic brings up a question that I've had for awhile
- what happens to the "inactivePciHostdevs" list when libvirt restarts?
This is the list of devices that have been detached from the host by
libvirt's virNodeDevice API, but are not currently in use by any domain.
This list isn't stored in any status file, and is not reconstructed when
libvirtd restarts (the information to do that doesn't exist). I *think*
the answer is that it doesn't matter, because libvirt will just use the
device anyway, then put it on the inactive list when it's done with it.
But that suggests the question "Then why have the inactive list?" I
don't know the answer to that one, but it does seem like either we
should be reconstructing inactivePciHostdevs when libvirtd restarts, or
we should just give up and not use (or maintain) it.
11:46 a.m.
New subject: [libvirt] [PATCHv2 01/12] pci: eliminate unused driver arg from virPCIDeviceDetach
On Tue, Jun 25, 2013 at 12:35:03PM -0400, Laine Stump wrote:
On 06/25/2013 05:50 AM, Daniel P. Berrange wrote:
> On Mon, Jun 24, 2013 at 11:05:27PM -0400, Laine Stump wrote:
>> The driver arg to virPCIDeviceDetach is no longer used (the name of
>> the stub driver is now set in the virPCIDevice object, and
>> virPCIDeviceDetach retrieves it from there). Remove it.
> What happens when libvirtd is restarted ? Are we correctly preserving
> the original value across restart, or are we capable of auto-detecting
> the correct value again ?
That's a good question, for two reasons: 1) I didn't know the answer for
sure when you asked it, and 2) I went back over the code, learned the
answer, and it was a good answer :-)
Originally the stubDriverName in the virPCIDevice object was only used
for a short period - during qemu's "device Preparation" it will create a
virPCIDeviceList from the virDomainHostdevDef list, then later iterate
through the virPCIDeviceList to detach all the devices from their host
drivers; during this detach loop, the original viDomainHostdevDef isn't
available, so an indicator of which stub to bind to must be stashed in
the virPCIDevice object when it is created.
Later it became useful to be able to rely on the stubDriverName always
being correct (at least in the activePciHostdevs list; it doesn't matter
for inactivePciHostdevs). Fortunately that list is recreated whenever
libvirt restarts by calling qemuUpdateActivePciHostdevs() for every
active domain. That function also updates stubDriverName for each active
hostdev based on the config (which must be the actual driver in use,
otherwise we would have encountered an error when the device was
originally being attached).
So, the answer is yes, we auto-set the correct value in the virPCIDevice
object when libvirtd restarts.
ACK, to the patch then.
However, this entire topic brings up a question that I've had for
awhile
- what happens to the "inactivePciHostdevs" list when libvirt restarts?
This is the list of devices that have been detached from the host by
libvirt's virNodeDevice API, but are not currently in use by any domain.
This list isn't stored in any status file, and is not reconstructed when
libvirtd restarts (the information to do that doesn't exist). I *think*
the answer is that it doesn't matter, because libvirt will just use the
device anyway, then put it on the inactive list when it's done with it.
But that suggests the question "Then why have the inactive list?" I
don't know the answer to that one, but it does seem like either we
should be reconstructing inactivePciHostdevs when libvirtd restarts, or
we should just give up and not use (or maintain) it.
Yes, that is odd. Would have to look back to see why we introduced
this list in the first place. If we can kill it so much the better
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:05 p.m.
New subject: [libvirt] [PATCHv2 02/12] pci: rename virPCIDeviceGetVFIOGroupDev to virPCIDeviceGetIOMMUGroupDev
I realized after the fact that it's probably better in the long run to
give this function a name that matches the name of the link used in
sysfs to hold the group (iommu_group).
I'm changing it now because I'm about to add several more functions
that deal with iommu groups.
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_cgroup.c | 4 ++--
src/security/security_apparmor.c | 2 +-
src/security/security_dac.c | 4 ++--
src/security/security_selinux.c | 4 ++--
src/util/virpci.c | 6 +++---
src/util/virpci.h | 2 +-
7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7c38c58..410e932 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1692,6 +1692,7 @@ virPCIDeviceCopy;
virPCIDeviceDetach;
virPCIDeviceFileIterate;
virPCIDeviceFree;
+virPCIDeviceGetIOMMUGroupDev;
virPCIDeviceGetManaged;
virPCIDeviceGetName;
virPCIDeviceGetRemoveSlot;
@@ -1699,7 +1700,6 @@ virPCIDeviceGetReprobe;
virPCIDeviceGetStubDriver;
virPCIDeviceGetUnbindFromStub;
virPCIDeviceGetUsedBy;
-virPCIDeviceGetVFIOGroupDev;
virPCIDeviceIsAssignable;
virPCIDeviceListAdd;
virPCIDeviceListCount;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index cf46993..5f54ca6 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -277,7 +277,7 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
if (!pci)
goto cleanup;
- if (!(path = virPCIDeviceGetVFIOGroupDev(pci)))
+ if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
goto cleanup;
VIR_DEBUG("Cgroup allow %s for PCI device assignment", path);
@@ -376,7 +376,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
if (!pci)
goto cleanup;
- if (!(path = virPCIDeviceGetVFIOGroupDev(pci)))
+ if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
goto cleanup;
VIR_DEBUG("Cgroup deny %s for PCI device assignment", path);
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 87c2777..50b0e74 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -823,7 +823,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
if (dev->source.subsys.u.pci.backend
== VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- char *vfioGroupDev = virPCIDeviceGetVFIOGroupDev(pci);
+ char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
if (!vfioGroupDev) {
virPCIDeviceFree(pci);
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index b8d1a92..0d6defc 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -529,7 +529,7 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
if (dev->source.subsys.u.pci.backend
== VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- char *vfioGroupDev = virPCIDeviceGetVFIOGroupDev(pci);
+ char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
if (!vfioGroupDev) {
virPCIDeviceFree(pci);
@@ -648,7 +648,7 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
if (dev->source.subsys.u.pci.backend
== VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- char *vfioGroupDev = virPCIDeviceGetVFIOGroupDev(pci);
+ char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
if (!vfioGroupDev) {
virPCIDeviceFree(pci);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 6fe063e..7802dda 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1333,7 +1333,7 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr
def,
if (dev->source.subsys.u.pci.backend
== VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- char *vfioGroupDev = virPCIDeviceGetVFIOGroupDev(pci);
+ char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
if (!vfioGroupDev) {
virPCIDeviceFree(pci);
@@ -1528,7 +1528,7 @@
virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
if (dev->source.subsys.u.pci.backend
== VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- char *vfioGroupDev = virPCIDeviceGetVFIOGroupDev(pci);
+ char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
if (!vfioGroupDev) {
virPCIDeviceFree(pci);
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2980e22..51fad78 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1852,11 +1852,11 @@ cleanup:
return ret;
}
-/* virPCIDeviceGetVFIOGroupDev - return the name of the device used to
- * control this PCI device's group (e.g. "/dev/vfio/15")
+/* virPCIDeviceGetIOMMUGroupDev - return the name of the device used
+ * to control this PCI device's group (e.g. "/dev/vfio/15")
*/
char *
-virPCIDeviceGetVFIOGroupDev(virPCIDevicePtr dev)
+virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev)
{
char *devPath = NULL;
char *groupPath = NULL;
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 944aa09..89717b8 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -117,7 +117,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev,
virPCIDeviceFileActor actor,
void *opaque);
char *
-virPCIDeviceGetVFIOGroupDev(virPCIDevicePtr dev);
+virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev);
int virPCIDeviceIsAssignable(virPCIDevicePtr dev,
int strict_acs_check);
--
1.7.11.7
5:29 a.m.
New subject: [libvirt] [PATCHv2 02/12] pci: rename virPCIDeviceGetVFIOGroupDev to virPCIDeviceGetIOMMUGroupDev
On Mon, Jun 24, 2013 at 11:05:28PM -0400, Laine Stump wrote:
I realized after the fact that it's probably better in the long
run to
give this function a name that matches the name of the link used in
sysfs to hold the group (iommu_group).
I'm changing it now because I'm about to add several more functions
that deal with iommu groups.
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_cgroup.c | 4 ++--
src/security/security_apparmor.c | 2 +-
src/security/security_dac.c | 4 ++--
src/security/security_selinux.c | 4 ++--
src/util/virpci.c | 6 +++---
src/util/virpci.h | 2 +-
7 files changed, 12 insertions(+), 12 deletions(-)
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:05 p.m.
New subject: [libvirt] [PATCHv2 03/12] pci: make virPCIParseDeviceAddress public
This function has utility outside of virpci.c, so make it public.
---
src/libvirt_private.syms | 1 +
src/util/virpci.c | 2 +-
src/util/virpci.h | 2 ++
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 410e932..643f311 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1728,6 +1728,7 @@ virPCIGetVirtualFunctionIndex;
virPCIGetVirtualFunctionInfo;
virPCIGetVirtualFunctions;
virPCIIsVirtualFunction;
+virPCIParseDeviceAddress;
# util/virpidfile.h
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 51fad78..fc04cac 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2046,7 +2046,7 @@ logStrToLong_ui(char const *s,
return ret;
}
-static int
+int
virPCIParseDeviceAddress(char *address,
virPCIDeviceAddressPtr bdf)
{
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 89717b8..bcf1b81 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -152,6 +152,8 @@ int virPCIGetAddrString(unsigned int domain,
char **pciConfigAddr)
ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
+int virPCIParseDeviceAddress(char *address, virPCIDeviceAddressPtr bdf);
+
int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
char **pfname, int *vf_index);
--
1.7.11.7
5:30 a.m.
New subject: [libvirt] [PATCHv2 03/12] pci: make virPCIParseDeviceAddress public
On Mon, Jun 24, 2013 at 11:05:29PM -0400, Laine Stump wrote:
This function has utility outside of virpci.c, so make it public.
---
src/libvirt_private.syms | 1 +
src/util/virpci.c | 2 +-
src/util/virpci.h | 2 ++
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 410e932..643f311 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1728,6 +1728,7 @@ virPCIGetVirtualFunctionIndex;
virPCIGetVirtualFunctionInfo;
virPCIGetVirtualFunctions;
virPCIIsVirtualFunction;
+virPCIParseDeviceAddress;
# util/virpidfile.h
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 51fad78..fc04cac 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2046,7 +2046,7 @@ logStrToLong_ui(char const *s,
return ret;
}
-static int
+int
virPCIParseDeviceAddress(char *address,
virPCIDeviceAddressPtr bdf)
Given that the struct is 'virPCIDeviceAddressPtr' the method
name is really better as virPCIDeviceAddressParse'
{
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 89717b8..bcf1b81 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -152,6 +152,8 @@ int virPCIGetAddrString(unsigned int domain,
char **pciConfigAddr)
ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
+int virPCIParseDeviceAddress(char *address, virPCIDeviceAddressPtr bdf);
+
int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
char **pfname, int *vf_index);
ACK with the suggested rename
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:36 a.m.
New subject: [libvirt] [PATCHv2 03/12] pci: make virPCIParseDeviceAddress public
On 06/25/2013 06:30 AM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 11:05:29PM -0400, Laine Stump wrote:
> This function has utility outside of virpci.c, so make it public.
> ---
> src/libvirt_private.syms | 1 +
> src/util/virpci.c | 2 +-
> src/util/virpci.h | 2 ++
> 3 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 410e932..643f311 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1728,6 +1728,7 @@ virPCIGetVirtualFunctionIndex;
> virPCIGetVirtualFunctionInfo;
> virPCIGetVirtualFunctions;
> virPCIIsVirtualFunction;
> +virPCIParseDeviceAddress;
>
>
> # util/virpidfile.h
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 51fad78..fc04cac 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -2046,7 +2046,7 @@ logStrToLong_ui(char const *s,
> return ret;
> }
>
> -static int
> +int
> virPCIParseDeviceAddress(char *address,
> virPCIDeviceAddressPtr bdf)
Given that the struct is 'virPCIDeviceAddressPtr' the method
name is really better as virPCIDeviceAddressParse'
Yeah, I noticed that way after the fact (it was an existing function
that I decided to use).
> {
> diff --git a/src/util/virpci.h b/src/util/virpci.h
> index 89717b8..bcf1b81 100644
> --- a/src/util/virpci.h
> +++ b/src/util/virpci.h
> @@ -152,6 +152,8 @@ int virPCIGetAddrString(unsigned int domain,
> char **pciConfigAddr)
> ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
>
> +int virPCIParseDeviceAddress(char *address, virPCIDeviceAddressPtr bdf);
> +
> int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
> char **pfname, int *vf_index);
ACK with the suggested rename
Thanks. I'll make that change and push it.
10:05 p.m.
New subject: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions
Any device which belongs to an "IOMMU group" (used by vfio) will
have links to all devices of its group listed in
/sys/bus/pci/$device/iommu_group/devices;
/sys/bus/pci/$device/iommu_group is actually a link to
/sys/kernel/iommu_groups/$n, where $n is the group number (there
will be a corresponding device node at /dev/vfio/$n once the
devices are bound to the vfio-pci driver)
The following functions are added:
virPCIDeviceGetIOMMUGroupList
Gets a virPCIDeviceList with one virPCIDeviceList for each device
in the same IOMMU group as the provided virPCIDevice (a copy of the
original device object is included in the list.
virPCIIOMMUGroupIterate
Calls the function @actor once for each device in the group that
contains the given virPCIDeviceAddress.
virPCIGetIOMMUGroupAddresses
Fills in a virPCIDeviceAddressPtr * with an array of
virPCIDeviceAddress, one for each device in the iommu group of the
provided virPCIDeviceAddress (including a copy of the original).
virPCIGetIOMMUGroupNum
Returns the group number as an int (a valid group number will always
be 0 or greater). If there is no iommu_group link in the device's
directory (usually indicating that vfio isn't loaded), -2 will be
returned. On any real error, -1 will be returned.
---
src/libvirt_private.syms | 4 +
src/util/virpci.c | 217 +++++++++++++++++++++++++++++++++++++++++++++++
src/util/virpci.h | 15 +++-
3 files changed, 233 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 643f311..e906742 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1693,6 +1693,7 @@ virPCIDeviceDetach;
virPCIDeviceFileIterate;
virPCIDeviceFree;
virPCIDeviceGetIOMMUGroupDev;
+virPCIDeviceGetIOMMUGroupList;
virPCIDeviceGetManaged;
virPCIDeviceGetName;
virPCIDeviceGetRemoveSlot;
@@ -1722,11 +1723,14 @@ virPCIDeviceSetStubDriver;
virPCIDeviceSetUnbindFromStub;
virPCIDeviceSetUsedBy;
virPCIDeviceWaitForCleanup;
+virPCIGetIOMMUGroupAddresses;
+virPCIGetIOMMUGroupNum;
virPCIGetNetName;
virPCIGetPhysicalFunction;
virPCIGetVirtualFunctionIndex;
virPCIGetVirtualFunctionInfo;
virPCIGetVirtualFunctions;
+virPCIIOMMUGroupIterate;
virPCIIsVirtualFunction;
virPCIParseDeviceAddress;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index fc04cac..5209372 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1852,6 +1852,223 @@ cleanup:
return ret;
}
+
+/* virPCIIOMMUGroupIterate:
+ * Call @actor for all devices in the same iommu_group as orig
+ * (including orig itself) Even if there is no iommu_group for the
+ * device, call @actor once for orig.
+ */
+int
+virPCIIOMMUGroupIterate(virPCIDeviceAddressPtr orig,
+ virPCIDeviceAddressActor actor,
+ void *opaque)
+{
+ char *groupPath = NULL;
+ DIR *groupDir = NULL;
+ int ret = -1;
+ struct dirent *ent;
+
+ if (virAsprintf(&groupPath,
+ PCI_SYSFS "devices/%04x:%02x:%02x.%x/iommu_group/devices",
+ orig->domain, orig->bus, orig->slot, orig->function) <
0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (!(groupDir = opendir(groupPath))) {
+ /* just process the original device, nothing more */
+ ret = (actor)(orig, opaque);
+ goto cleanup;
+ }
+
+ while ((ent = readdir(groupDir)) != NULL) {
+ virPCIDeviceAddress newDev;
+
+ if (ent->d_name[0] == '.')
+ continue;
+
+ if (virPCIParseDeviceAddress(ent->d_name, &newDev) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Found invalid device link '%s' in
'%s'"),
+ ent->d_name, groupPath);
+ goto cleanup;
+ }
+
+ if ((actor)(&newDev, opaque) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ VIR_FREE(groupPath);
+ if (groupDir)
+ closedir(groupDir);
+ return ret;
+}
+
+
+static int
+virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
+{
+ int ret = -1;
+ virPCIDeviceListPtr groupList = opaque;
+ virPCIDevicePtr newDev;
+
+ if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus,
+ newDevAddr->slot, newDevAddr->function))) {
+ goto cleanup;
+ }
+
+ if (virPCIDeviceListAdd(groupList, newDev) < 0) {
+ goto cleanup;
+ }
+ newDev = NULL; /* it's now on the list */
+ ret = 0;
+cleanup:
+ virPCIDeviceFree(newDev);
+ return ret;
+}
+
+
+/*
+ * virPCIDeviceGetIOMMUGroupList - return a virPCIDeviceList containing
+ * all of the devices in the same iommu_group as @dev.
+ *
+ * Return the new list, or NULL on failure
+ */
+virPCIDeviceListPtr
+virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev)
+{
+ virPCIDeviceListPtr groupList = virPCIDeviceListNew();
+ virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
+ dev->slot, dev->function };
+
+ if (!groupList)
+ goto error;
+
+ if (virPCIIOMMUGroupIterate(&devAddr, virPCIDeviceGetIOMMUGroupAddOne,
+ groupList) < 0) {
+ goto error;
+ }
+ return groupList;
+
+error:
+ virObjectUnref(groupList);
+ return NULL;
+}
+
+
+typedef struct {
+ virPCIDeviceAddressPtr **iommuGroupDevices;
+ size_t *nIommuGroupDevices;
+} virPCIDeviceAddressList;
+typedef virPCIDeviceAddressList *virPCIDeviceAddressListPtr;
+
+static int
+virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
+{
+ int ret = -1;
+ virPCIDeviceAddressListPtr addrList = opaque;
+ virPCIDeviceAddressPtr copyAddr;
+
+ /* make a copy to insert onto the list */
+ if (VIR_ALLOC(copyAddr) < 0)
+ goto cleanup;
+
+ *copyAddr = *newDevAddr;
+
+ if (VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices,
+ *addrList->nIommuGroupDevices, copyAddr) < 0) {
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ VIR_FREE(copyAddr);
+ return ret;
+}
+
+
+/*
+ * virPCIGetIOMMUGroupAddresses - return a virPCIDeviceList containing
+ * all of the devices in the same iommu_group as @dev.
+ *
+ * Return the new list, or NULL on failure
+ */
+int
+virPCIGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
+ virPCIDeviceAddressPtr **iommuGroupDevices,
+ size_t *nIommuGroupDevices)
+{
+ int ret = -1;
+ virPCIDeviceAddressList addrList = { iommuGroupDevices,
+ nIommuGroupDevices };
+
+ if (virPCIIOMMUGroupIterate(devAddr,
+ virPCIGetIOMMUGroupAddressesAddOne,
+ &addrList) < 0) {
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ return ret;
+}
+
+
+/* virPCIGetIOMMUGroupNum - return the group number of this PCI
+ * device's iommu_group, or -2 if there is no iommu_group for the
+ * device (or -1 if there was any other error)
+ */
+int
+virPCIGetIOMMUGroupNum(virPCIDeviceAddressPtr addr)
+{
+ char *devName = NULL;
+ char *devPath = NULL;
+ char *groupPath = NULL;
+ const char *groupNumStr;
+ unsigned int groupNum;
+ int ret = -1;
+
+ if (virAsprintf(&devName, "%.4x:%.2x:%.2x.%.1x", addr->domain,
+ addr->bus, addr->slot, addr->function) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (virPCIFile(&devPath, devName, "iommu_group") < 0)
+ goto cleanup;
+ if (virFileIsLink(devPath) != 1) {
+ ret = -2;
+ goto cleanup;
+ }
+ if (virFileResolveLink(devPath, &groupPath) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to resolve device %s iommu_group symlink
%s"),
+ devName, devPath);
+ goto cleanup;
+ }
+
+ groupNumStr = last_component(groupPath);
+ if (virStrToLong_ui(groupNumStr, NULL, 10, &groupNum) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("device %s iommu_group symlink %s has "
+ "invalid group number %s"),
+ devName, groupPath, groupNumStr);
+ ret = -1;
+ goto cleanup;
+ }
+
+ ret = groupNum;
+cleanup:
+ VIR_FREE(devName);
+ VIR_FREE(devPath);
+ VIR_FREE(groupPath);
+ return ret;
+}
+
+
/* virPCIDeviceGetIOMMUGroupDev - return the name of the device used
* to control this PCI device's group (e.g. "/dev/vfio/15")
*/
diff --git a/src/util/virpci.h b/src/util/virpci.h
index bcf1b81..972f86b 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -112,12 +112,21 @@ int virPCIDeviceListFindIndex(virPCIDeviceListPtr list,
*/
typedef int (*virPCIDeviceFileActor)(virPCIDevicePtr dev,
const char *path, void *opaque);
-
int virPCIDeviceFileIterate(virPCIDevicePtr dev,
virPCIDeviceFileActor actor,
void *opaque);
-char *
-virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev);
+
+typedef int (*virPCIDeviceAddressActor)(virPCIDeviceAddress *addr,
+ void *opaque);
+int virPCIIOMMUGroupIterate(virPCIDeviceAddressPtr orig,
+ virPCIDeviceAddressActor actor,
+ void *opaque);
+virPCIDeviceListPtr virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev);
+int virPCIGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
+ virPCIDeviceAddressPtr **iommuGroupDevices,
+ size_t *nIommuGroupDevices);
+int virPCIGetIOMMUGroupNum(virPCIDeviceAddressPtr dev);
+char *virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev);
int virPCIDeviceIsAssignable(virPCIDevicePtr dev,
int strict_acs_check);
--
1.7.11.7
5:34 a.m.
New subject: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions
On Mon, Jun 24, 2013 at 11:05:30PM -0400, Laine Stump wrote:
Any device which belongs to an "IOMMU group" (used by vfio)
will
have links to all devices of its group listed in
/sys/bus/pci/$device/iommu_group/devices;
/sys/bus/pci/$device/iommu_group is actually a link to
/sys/kernel/iommu_groups/$n, where $n is the group number (there
will be a corresponding device node at /dev/vfio/$n once the
devices are bound to the vfio-pci driver)
The following functions are added:
Naming convention is normally <object><action>, which I would
interpret here as 'virPCIDeviceIOMMUGroup' as the object,
since you're passing a 'virPCIDevice' or 'virPCIDeviceAddress'
type as parameter.
virPCIDeviceGetIOMMUGroupList
Gets a virPCIDeviceList with one virPCIDeviceList for each device
in the same IOMMU group as the provided virPCIDevice (a copy of the
original device object is included in the list.
ack to this name
virPCIIOMMUGroupIterate
Calls the function @actor once for each device in the group that
contains the given virPCIDeviceAddress.
virPCIDeviceIOMMUGroupIterate
virPCIGetIOMMUGroupAddresses
Fills in a virPCIDeviceAddressPtr * with an array of
virPCIDeviceAddress, one for each device in the iommu group of the
provided virPCIDeviceAddress (including a copy of the original).
virPCIDeviceGetIOMMUGroupAddresses
virPCIGetIOMMUGroupNum
Returns the group number as an int (a valid group number will always
be 0 or greater). If there is no iommu_group link in the device's
directory (usually indicating that vfio isn't loaded), -2 will be
returned. On any real error, -1 will be returned.
virPCIDeviceGetIOMMUGroupNum
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:44 a.m.
New subject: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions
On 06/25/2013 06:34 AM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 11:05:30PM -0400, Laine Stump wrote:
> Any device which belongs to an "IOMMU group" (used by vfio) will
> have links to all devices of its group listed in
> /sys/bus/pci/$device/iommu_group/devices;
> /sys/bus/pci/$device/iommu_group is actually a link to
> /sys/kernel/iommu_groups/$n, where $n is the group number (there
> will be a corresponding device node at /dev/vfio/$n once the
> devices are bound to the vfio-pci driver)
>
> The following functions are added:
Naming convention is normally <object><action>, which I would
interpret here as 'virPCIDeviceIOMMUGroup' as the object,
since you're passing a 'virPCIDevice' or 'virPCIDeviceAddress'
type as parameter.
> virPCIDeviceGetIOMMUGroupList
>
> Gets a virPCIDeviceList with one virPCIDeviceList for each device
> in the same IOMMU group as the provided virPCIDevice (a copy of the
> original device object is included in the list.
ack to this name
> virPCIIOMMUGroupIterate
>
> Calls the function @actor once for each device in the group that
> contains the given virPCIDeviceAddress.
virPCIDeviceIOMMUGroupIterate
Well, actually it would be virPCIDeviceAddressIOMMUGroupIterate, which
is really quite a mouthful (and leads to very long lines). Due to the
extreme length, and based on seeing some other function that had just a
"virPCI" prefix, I chose the shorter name (although I secretly wondered
if you would raise an objection).
I definitely see the advantage of consistent naming, even if it's not
(yet) done consistently across all functions, so I'll change these names.
> virPCIGetIOMMUGroupAddresses
>
> Fills in a virPCIDeviceAddressPtr * with an array of
> virPCIDeviceAddress, one for each device in the iommu group of the
> provided virPCIDeviceAddress (including a copy of the original).
virPCIDeviceGetIOMMUGroupAddresses
virPCIDeviceAddressGetIOMMUGroupAddresses
> virPCIGetIOMMUGroupNum
>
> Returns the group number as an int (a valid group number will always
> be 0 or greater). If there is no iommu_group link in the device's
> directory (usually indicating that vfio isn't loaded), -2 will be
> returned. On any real error, -1 will be returned.
virPCIDeviceGetIOMMUGroupNum
virPCIDeviceAddressGetIOMMUGroupNum
Going through all this, I've noticed that we *still* have multiple "pci
address" objects. For example virDevicePCIAddress (defined in /conf,
includes a "multifunction" attribute) and virPCIDeviceAddress (defined
in /util, no "multifunction"), and that many larger objects that include
a PCI address just define the individual attributes instead of using one
of the predefined objects. This leads to code that constructs one flavor
of PCI address/device from another just to call a function that requires
the other flavor. That *really* needs cleaning up...
5:40 a.m.
New subject: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions
On Mon, Jun 24, 2013 at 11:05:30PM -0400, Laine Stump wrote:
Any device which belongs to an "IOMMU group" (used by vfio)
will
have links to all devices of its group listed in
/sys/bus/pci/$device/iommu_group/devices;
/sys/bus/pci/$device/iommu_group is actually a link to
/sys/kernel/iommu_groups/$n, where $n is the group number (there
will be a corresponding device node at /dev/vfio/$n once the
devices are bound to the vfio-pci driver)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index fc04cac..5209372 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1852,6 +1852,223 @@ cleanup:
return ret;
}
+
+/* virPCIIOMMUGroupIterate:
+ * Call @actor for all devices in the same iommu_group as orig
+ * (including orig itself) Even if there is no iommu_group for the
+ * device, call @actor once for orig.
+ */
+int
+virPCIIOMMUGroupIterate(virPCIDeviceAddressPtr orig,
+ virPCIDeviceAddressActor actor,
+ void *opaque)
+{
+ char *groupPath = NULL;
+ DIR *groupDir = NULL;
+ int ret = -1;
+ struct dirent *ent;
+
+ if (virAsprintf(&groupPath,
+ PCI_SYSFS
"devices/%04x:%02x:%02x.%x/iommu_group/devices",
+ orig->domain, orig->bus, orig->slot, orig->function)
< 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (!(groupDir = opendir(groupPath))) {
+ /* just process the original device, nothing more */
+ ret = (actor)(orig, opaque);
+ goto cleanup;
+ }
+
Since Eric isn't around to nit-pick, I'll do it :-)
Set 'errno = 0' here before the readdir() call
+ while ((ent = readdir(groupDir)) != NULL) {
+ virPCIDeviceAddress newDev;
+
+ if (ent->d_name[0] == '.')
+ continue;
+
+ if (virPCIParseDeviceAddress(ent->d_name, &newDev) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Found invalid device link '%s' in
'%s'"),
+ ent->d_name, groupPath);
+ goto cleanup;
+ }
+
+ if ((actor)(&newDev, opaque) < 0)
+ goto cleanup;
And here
errno = 0;
+ }
Then do
if (errno != 0) {
virReportSystemError(errno, _("Failed to read directory entry for
%s"), grouppath)
goto cleanup
}
that way you distinguish EOF from error when readdir() returns NULL.
/me makes a note that we should write a wrapper around readdir(),
since almost all our code has this wrong.
+
> + ret = 0;
+
> +cleanup:
> + VIR_FREE(groupPath);
> + if (groupDir)
> + closedir(groupDir);
> + return ret;
> +}
+
+
> +static int
> +virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
> +{
> + int ret = -1;
> + virPCIDeviceListPtr groupList = opaque;
> + virPCIDevicePtr newDev;
+
> + if (!(newDev =
virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus,
> + newDevAddr->slot, newDevAddr->function)))
{
> + goto cleanup;
+ }
+
> + if
(virPCIDeviceListAdd(groupList, newDev) < 0) {
> + goto cleanup;
+ }
> + newDev = NULL; /* it's now on the list */
> + ret = 0;
> +cleanup:
> + virPCIDeviceFree(newDev);
> + return ret;
> +}
+
+
> +/*
> + * virPCIDeviceGetIOMMUGroupList - return a virPCIDeviceList containing
> + * all of the devices in the same iommu_group as @dev.
> + *
> + * Return the new list, or NULL on failure
> + */
> +virPCIDeviceListPtr
> +virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev)
> +{
> + virPCIDeviceListPtr groupList = virPCIDeviceListNew();
> + virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
> + dev->slot, dev->function };
+
> + if (!groupList)
> + goto error;
+
> + if (virPCIIOMMUGroupIterate(&devAddr,
virPCIDeviceGetIOMMUGroupAddOne,
> + groupList) < 0) {
> + goto error;
+ }
Overkill with {} here
> +static int
> +virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
> +{
> + int ret = -1;
> + virPCIDeviceAddressListPtr addrList = opaque;
> + virPCIDeviceAddressPtr copyAddr;
+
> + /* make a copy to insert onto the list */
> + if (VIR_ALLOC(copyAddr) < 0)
> + goto cleanup;
+
> + *copyAddr = *newDevAddr;
+
> + if
(VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices,
> + *addrList->nIommuGroupDevices, copyAddr) < 0) {
> + goto cleanup;
+ }
Overkill with {}
Missing virReportOOMError() ?
+
> + ret = 0;
> +cleanup:
> + VIR_FREE(copyAddr);
> + return ret;
> +}
+
+
> +/*
> + * virPCIGetIOMMUGroupAddresses - return a virPCIDeviceList containing
> + * all of the devices in the same iommu_group as @dev.
> + *
> + * Return the new list, or NULL on failure
> + */
> +int
> +virPCIGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
> + virPCIDeviceAddressPtr **iommuGroupDevices,
> + size_t *nIommuGroupDevices)
> +{
> + int ret = -1;
> + virPCIDeviceAddressList addrList = { iommuGroupDevices,
> + nIommuGroupDevices };
+
> + if (virPCIIOMMUGroupIterate(devAddr,
> + virPCIGetIOMMUGroupAddressesAddOne,
> + &addrList) < 0) {
> + goto cleanup;
+ }
Overkill with {}
+
> + ret = 0;
> +cleanup:
> + return ret;
> +}
+
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:52 a.m.
New subject: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions
On 06/25/2013 06:40 AM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 11:05:30PM -0400, Laine Stump wrote:
> Any device which belongs to an "IOMMU group" (used by vfio) will
> have links to all devices of its group listed in
> /sys/bus/pci/$device/iommu_group/devices;
> /sys/bus/pci/$device/iommu_group is actually a link to
> /sys/kernel/iommu_groups/$n, where $n is the group number (there
> will be a corresponding device node at /dev/vfio/$n once the
> devices are bound to the vfio-pci driver)
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index fc04cac..5209372 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1852,6 +1852,223 @@ cleanup:
> return ret;
> }
>
> +
> +/* virPCIIOMMUGroupIterate:
> + * Call @actor for all devices in the same iommu_group as orig
> + * (including orig itself) Even if there is no iommu_group for the
> + * device, call @actor once for orig.
> + */
> +int
> +virPCIIOMMUGroupIterate(virPCIDeviceAddressPtr orig,
> + virPCIDeviceAddressActor actor,
> + void *opaque)
> +{
> + char *groupPath = NULL;
> + DIR *groupDir = NULL;
> + int ret = -1;
> + struct dirent *ent;
> +
> + if (virAsprintf(&groupPath,
> + PCI_SYSFS
"devices/%04x:%02x:%02x.%x/iommu_group/devices",
> + orig->domain, orig->bus, orig->slot, orig->function)
< 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (!(groupDir = opendir(groupPath))) {
> + /* just process the original device, nothing more */
> + ret = (actor)(orig, opaque);
> + goto cleanup;
> + }
> +
Since Eric isn't around to nit-pick, I'll do it :-)
Set 'errno = 0' here before the readdir() call
> + while ((ent = readdir(groupDir)) != NULL) {
> + virPCIDeviceAddress newDev;
> +
> + if (ent->d_name[0] == '.')
> + continue;
> +
> + if (virPCIParseDeviceAddress(ent->d_name, &newDev) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Found invalid device link '%s' in
'%s'"),
> + ent->d_name, groupPath);
> + goto cleanup;
> + }
> +
> + if ((actor)(&newDev, opaque) < 0)
> + goto cleanup;
And here
errno = 0;
> + }
Then do
if (errno != 0) {
virReportSystemError(errno, _("Failed to read directory entry for
%s"), grouppath)
goto cleanup
}
that way you distinguish EOF from error when readdir() returns NULL.
/me makes a note that we should write a wrapper around readdir(),
since almost all our code has this wrong.
Definitely. If I ever did know about this idiosyncracy of readdir() in
the past, I had certainly forgotten it.
> +
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(groupPath);
> + if (groupDir)
> + closedir(groupDir);
> + return ret;
> +}
> +
> +
> +static int
> +virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
> +{
> + int ret = -1;
> + virPCIDeviceListPtr groupList = opaque;
> + virPCIDevicePtr newDev;
> +
> + if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus,
> + newDevAddr->slot, newDevAddr->function)))
{
> + goto cleanup;
> + }
> +
> + if (virPCIDeviceListAdd(groupList, newDev) < 0) {
> + goto cleanup;
> + }
> + newDev = NULL; /* it's now on the list */
> + ret = 0;
> +cleanup:
> + virPCIDeviceFree(newDev);
> + return ret;
> +}
> +
> +
> +/*
> + * virPCIDeviceGetIOMMUGroupList - return a virPCIDeviceList containing
> + * all of the devices in the same iommu_group as @dev.
> + *
> + * Return the new list, or NULL on failure
> + */
> +virPCIDeviceListPtr
> +virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev)
> +{
> + virPCIDeviceListPtr groupList = virPCIDeviceListNew();
> + virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
> + dev->slot, dev->function };
> +
> + if (!groupList)
> + goto error;
> +
> + if (virPCIIOMMUGroupIterate(&devAddr, virPCIDeviceGetIOMMUGroupAddOne,
> + groupList) < 0) {
> + goto error;
> + }
Overkill with {} here
In addition to the necessary braces when the body is > 1 line, I also
like to have {} when the conditional itself is > 1 line (and I've been
doing that ever since I can remember). If you dislike it I can cease and
desist, though :-)
> +static int
> +virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
> +{
> + int ret = -1;
> + virPCIDeviceAddressListPtr addrList = opaque;
> + virPCIDeviceAddressPtr copyAddr;
> +
> + /* make a copy to insert onto the list */
> + if (VIR_ALLOC(copyAddr) < 0)
> + goto cleanup;
> +
> + *copyAddr = *newDevAddr;
> +
> + if (VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices,
> + *addrList->nIommuGroupDevices, copyAddr) < 0) {
> + goto cleanup;
> + }
Overkill with {}
Missing virReportOOMError() ?
Yes. I caught myself omitting one of those the other night, and at the
time thought to myself "I think I left one out somewhere else too...".
Maybe we should send *all* VIR_* macros down the same road as
VIR_STRDUP, and integrate OOM error logging into them.
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(copyAddr);
> + return ret;
> +}
> +
> +
> +/*
> + * virPCIGetIOMMUGroupAddresses - return a virPCIDeviceList containing
> + * all of the devices in the same iommu_group as @dev.
> + *
> + * Return the new list, or NULL on failure
> + */
> +int
> +virPCIGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
> + virPCIDeviceAddressPtr **iommuGroupDevices,
> + size_t *nIommuGroupDevices)
> +{
> + int ret = -1;
> + virPCIDeviceAddressList addrList = { iommuGroupDevices,
> + nIommuGroupDevices };
> +
> + if (virPCIIOMMUGroupIterate(devAddr,
> + virPCIGetIOMMUGroupAddressesAddOne,
> + &addrList) < 0) {
> + goto cleanup;
> + }
Overkill with {}
Same deal - the condition is 3 lines long and this gives me a visual
crush. It could just be because I'm accustomed to it though (I also used
to hate the K&R/libvirt style of putting the opening brace at the end of
the line, but I've gotten used to that too)
10:05 p.m.
New subject: [libvirt] [PATCHv2 05/12] nodedev: add iommuGroup to node device object
This includes adding it to the nodedev parser and formatter, docs, and
test.
---
docs/formatnode.html.in | 63 +++++++++++++++-
docs/schemas/nodedev.rng | 11 +++
src/conf/node_device_conf.c | 86 +++++++++++++++++++++-
src/conf/node_device_conf.h | 5 +-
src/node_device/node_device_udev.c | 21 +++++-
tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml | 16 ++++
tests/nodedevxml2xmltest.c | 1 +
7 files changed, 199 insertions(+), 4 deletions(-)
create mode 100644 tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index 11654e5..ad05a70 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -80,6 +80,36 @@
<dd>Vendor details from the device ROM, including an
attribute <code>id</code> with the hexadecimal vendor
id, and an optional text name of that vendor.</dd>
+ <dt><code>iommuGroup</code></dt>
+ <dd>
+ This optional element describes the "IOMMU group" this
+ device belongs to. If the element exists, it has a
+ mandatory <code>number</code> attribute which tells
+ the group number used for management of the group (all
+ devices in group "n" will be found in
+ "/sys/kernel/iommu_groups/n"). It will also have a
+ list of <code>address</code> subelements, each
+ containing the PCI address of a device in the same
+ group. The toplevel device will itself be included in
+ this list.
+ </dd>
+ <dt><code>capability</code></dt>
+ <dd>
+ This optional element can occur multiple times. If it
+ exists, it has a mandatory <code>type</code> attribute
+ which will be set to
+ either <code>physical_function</code>
+ or <code>virtual_functions</code>. If the type
+ is <code>physical_function</code>, there will be a
+ single <code>address</code> subelement which contains
+ the PCI address of the SRIOV Physical Function (PF)
+ that is the parent of this device (and this device is,
+ by implication, an SRIOV Virtual Function (VF)). If
+ the type is <code>virtual_functions</code>, then this
+ device is an SRIOV PF, and the capability element will
+ have a list of <code>address</code> subelements, one
+ for each VF on this PF.
+ </dd>
</dl>
</dd>
<dt><code>usb_device</code></dt>
@@ -232,7 +262,38 @@
<address>00:27:13:6a:fe:00</address>
<capability type='80203'/>
</capability>
-</device></pre>
+</device>
+
+<device>
+ <name>pci_0000_02_00_0</name>
+
<path>/sys/devices/pci0000:00/0000:00:04.0/0000:02:00.0</path>
+ <parent>pci_0000_00_04_0</parent>
+ <driver>
+ <name>igb</name>
+ </driver>
+ <capability type='pci'>
+ <domain>0</domain>
+ <bus>2</bus>
+ <slot>0</slot>
+ <function>0</function>
+ <product id='0x10c9'>82576 Gigabit Network
Connection</product>
+ <vendor id='0x8086'>Intel Corporation</vendor>
+ <capability type='virt_functions'>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x2'/>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x4'/>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x6'/>
+ <address domain='0x0000' bus='0x02' slot='0x11'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x11'
function='0x2'/>
+ <address domain='0x0000' bus='0x02' slot='0x11'
function='0x4'/>
+ </capability>
+ <iommuGroup number='12'>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x1'/>
+ </iommuGroup>
+ </capability>
+</device>
+ </pre>
</body>
</html>
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 1f5dcb9..d2bebff 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -144,6 +144,17 @@
</element>
</optional>
+ <optional>
+ <element name='iommuGroup'>
+ <attribute name='number'>
+ <ref name='unsignedInt'/>
+ </attribute>
+ <oneOrMore>
+ <ref name='address'/>
+ </oneOrMore>
+ </element>
+ </optional>
+
</define>
<define name='capusbdev'>
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 548cd8f..96742ef 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1,6 +1,7 @@
/*
* node_device_conf.c: config handling for node devices
*
+ * Copyright (C) 2009-2013 Red Hat, Inc.
* Copyright (C) 2008 Virtual Iron Software, Inc.
* Copyright (C) 2008 David F. Lively
*
@@ -31,6 +32,7 @@
#include "viralloc.h"
#include "virstring.h"
#include "node_device_conf.h"
+#include "device_conf.h"
#include "virxml.h"
#include "virbuffer.h"
#include "viruuid.h"
@@ -330,6 +332,20 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDefPtr def)
}
virBufferAddLit(&buf, " </capability>\n");
}
+ if (data->pci_dev.nIommuGroupDevices) {
+ virBufferAsprintf(&buf, " <iommuGroup
number='%d'>\n",
+ data->pci_dev.iommuGroupNumber);
+ for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) {
+ virBufferAsprintf(&buf,
+ " <address domain='0x%.4x'
bus='0x%.2x' "
+ "slot='0x%.2x'
function='0x%.1x'/>\n",
+ data->pci_dev.iommuGroupDevices[i]->domain,
+ data->pci_dev.iommuGroupDevices[i]->bus,
+ data->pci_dev.iommuGroupDevices[i]->slot,
+
data->pci_dev.iommuGroupDevices[i]->function);
+ }
+ virBufferAddLit(&buf, " </iommuGroup>\n");
+ }
break;
case VIR_NODE_DEV_CAP_USB_DEV:
virBufferAsprintf(&buf, " <bus>%d</bus>\n",
data->usb_dev.bus);
@@ -966,12 +982,71 @@ out:
}
static int
+virNodeDevCapPciDevIommuGroupParseXML(xmlXPathContextPtr ctxt,
+ xmlNodePtr iommuGroupNode,
+ union _virNodeDevCapData *data)
+{
+ xmlNodePtr origNode = ctxt->node;
+ xmlNodePtr *addrNodes = NULL;
+ char *numberStr = NULL;
+ int nAddrNodes, ii, ret = -1;
+ virPCIDeviceAddressPtr pciAddr = NULL;
+
+ ctxt->node = iommuGroupNode;
+
+ numberStr = virXMLPropString(iommuGroupNode, "number");
+ if (!numberStr) {
+ virReportError(VIR_ERR_XML_ERROR,
+ "%s", _("missing iommuGroup number
attribute"));
+ goto cleanup;
+ }
+ if (virStrToLong_ui(numberStr, NULL, 10,
+ &data->pci_dev.iommuGroupNumber) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("invalid iommuGroup number attribute '%s'"),
+ numberStr);
+ goto cleanup;
+ }
+
+ if ((nAddrNodes = virXPathNodeSet("./address", ctxt, &addrNodes)) <
0)
+ goto cleanup;
+
+ for (ii = 0; ii < nAddrNodes; ii++) {
+ virDevicePCIAddress addr = { 0, 0, 0, 0, 0 };
+ if (virDevicePCIAddressParseXML(addrNodes[ii], &addr) < 0)
+ goto cleanup;
+ if (VIR_ALLOC(pciAddr) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ pciAddr->domain = addr.domain;
+ pciAddr->bus = addr.bus;
+ pciAddr->slot = addr.slot;
+ pciAddr->function = addr.function;
+ if (VIR_APPEND_ELEMENT(data->pci_dev.iommuGroupDevices,
+ data->pci_dev.nIommuGroupDevices,
+ pciAddr) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+cleanup:
+ ctxt->node = origNode;
+ VIR_FREE(addrNodes);
+ VIR_FREE(pciAddr);
+ return ret;
+}
+
+
+static int
virNodeDevCapPciDevParseXML(xmlXPathContextPtr ctxt,
virNodeDeviceDefPtr def,
xmlNodePtr node,
union _virNodeDevCapData *data)
{
- xmlNodePtr orignode;
+ xmlNodePtr orignode, iommuGroupNode;
int ret = -1;
orignode = ctxt->node;
@@ -1016,6 +1091,12 @@ virNodeDevCapPciDevParseXML(xmlXPathContextPtr ctxt,
data->pci_dev.vendor_name = virXPathString("string(./vendor[1])",
ctxt);
data->pci_dev.product_name = virXPathString("string(./product[1])",
ctxt);
+ if ((iommuGroupNode = virXPathNode("./iommuGroup[1]", ctxt))) {
+ if (virNodeDevCapPciDevIommuGroupParseXML(ctxt, iommuGroupNode,
+ data) < 0) {
+ goto out;
+ }
+ }
ret = 0;
out:
ctxt->node = orignode;
@@ -1385,6 +1466,9 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
for (i = 0; i < data->pci_dev.num_virtual_functions; i++) {
VIR_FREE(data->pci_dev.virtual_functions[i]);
}
+ for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) {
+ VIR_FREE(data->pci_dev.iommuGroupDevices[i]);
+ }
break;
case VIR_NODE_DEV_CAP_USB_DEV:
VIR_FREE(data->usb_dev.product_name);
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 17240be..ec35da2 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -1,7 +1,7 @@
/*
* node_device_conf.h: config handling for node devices
*
- * Copyright (C) 2010-2011 Red Hat, Inc.
+ * Copyright (C) 2009-2013 Red Hat, Inc.
* Copyright (C) 2008 Virtual Iron Software, Inc.
* Copyright (C) 2008 David F. Lively
*
@@ -112,6 +112,9 @@ struct _virNodeDevCapsDef {
virPCIDeviceAddressPtr *virtual_functions;
unsigned int num_virtual_functions;
unsigned int flags;
+ virPCIDeviceAddressPtr *iommuGroupDevices;
+ size_t nIommuGroupDevices;
+ unsigned int iommuGroupNumber;
} pci_dev;
struct {
unsigned int bus;
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index b94be80..9bb4a17 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -421,7 +421,8 @@ static int udevProcessPCI(struct udev_device *device,
{
const char *syspath = NULL;
union _virNodeDevCapData *data = &def->caps->data;
- int ret = -1;
+ virPCIDeviceAddress addr;
+ int tmpGroup, ret = -1;
char *p;
int rc;
@@ -501,6 +502,24 @@ static int udevProcessPCI(struct udev_device *device,
else if (!rc && (data->pci_dev.num_virtual_functions > 0))
data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+ /* iommu group */
+ addr.domain = data->pci_dev.domain;
+ addr.bus = data->pci_dev.bus;
+ addr.slot = data->pci_dev.slot;
+ addr.function = data->pci_dev.function;
+ tmpGroup = virPCIGetIOMMUGroupNum(&addr);
+ if (tmpGroup == -1) {
+ /* error was already reported */
+ goto out;
+ /* -2 return means there is no iommu_group data */
+ } else if (tmpGroup >= 0) {
+ if (virPCIGetIOMMUGroupAddresses(&addr,
&data->pci_dev.iommuGroupDevices,
+ &data->pci_dev.nIommuGroupDevices) <
0) {
+ goto out;
+ }
+ data->pci_dev.iommuGroupNumber = tmpGroup;
+ }
+
ret = 0;
out:
diff --git a/tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
b/tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
new file mode 100644
index 0000000..eff8932
--- /dev/null
+++ b/tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
@@ -0,0 +1,16 @@
+<device>
+ <name>pci_0000_02_00_0</name>
+ <parent>pci_0000_00_04_0</parent>
+ <capability type='pci'>
+ <domain>0</domain>
+ <bus>2</bus>
+ <slot>0</slot>
+ <function>0</function>
+ <product id='0x10c9'>82576 Gigabit Network Connection</product>
+ <vendor id='0x8086'>Intel Corporation</vendor>
+ <iommuGroup number='12'>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x1'/>
+ </iommuGroup>
+ </capability>
+</device>
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index e81c617..ed49857 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -78,6 +78,7 @@ mymain(void)
DO_TEST("net_00_13_02_b9_f9_d3");
DO_TEST("net_00_15_58_2f_e9_55");
DO_TEST("pci_1002_71c4");
+ DO_TEST("pci_8086_10c9_sriov_pf");
DO_TEST("pci_8086_27c5_scsi_host_0");
DO_TEST("pci_8086_27c5_scsi_host_scsi_device_lun0");
DO_TEST("pci_8086_27c5_scsi_host_scsi_host");
--
1.7.11.7
5:36 a.m.
New subject: [libvirt] [PATCHv2 05/12] nodedev: add iommuGroup to node device object
On Mon, Jun 24, 2013 at 11:05:31PM -0400, Laine Stump wrote:
This includes adding it to the nodedev parser and formatter, docs,
and
test.
---
docs/formatnode.html.in | 63 +++++++++++++++-
docs/schemas/nodedev.rng | 11 +++
src/conf/node_device_conf.c | 86 +++++++++++++++++++++-
src/conf/node_device_conf.h | 5 +-
src/node_device/node_device_udev.c | 21 +++++-
tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml | 16 ++++
tests/nodedevxml2xmltest.c | 1 +
7 files changed, 199 insertions(+), 4 deletions(-)
create mode 100644 tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
ACK, but could you illustrate the XML syntax in the commit
message. I know its shown in the docs, but it makes life
easier when browsing 'git log'
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:53 p.m.
New subject: [libvirt] [PATCHv2 05/12] nodedev: add iommuGroup to node device object
I just realized that I had only implemented this for the udev nodeDevice
driver, but not the HAL driver. I can easily add the same code into the
HAL driver, but don't have any system to test building it on.
Should I put that code in untested, or leave the HAL driver without this
functionality? (Does anyone still use HAL?)
On 06/24/2013 11:05 PM, Laine Stump wrote:
This includes adding it to the nodedev parser and formatter, docs,
and
test.
---
docs/formatnode.html.in | 63 +++++++++++++++-
docs/schemas/nodedev.rng | 11 +++
src/conf/node_device_conf.c | 86 +++++++++++++++++++++-
src/conf/node_device_conf.h | 5 +-
src/node_device/node_device_udev.c | 21 +++++-
tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml | 16 ++++
tests/nodedevxml2xmltest.c | 1 +
7 files changed, 199 insertions(+), 4 deletions(-)
create mode 100644 tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index 11654e5..ad05a70 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -80,6 +80,36 @@
<dd>Vendor details from the device ROM, including an
attribute <code>id</code> with the hexadecimal vendor
id, and an optional text name of that vendor.</dd>
+ <dt><code>iommuGroup</code></dt>
+ <dd>
+ This optional element describes the "IOMMU group" this
+ device belongs to. If the element exists, it has a
+ mandatory <code>number</code> attribute which tells
+ the group number used for management of the group (all
+ devices in group "n" will be found in
+ "/sys/kernel/iommu_groups/n"). It will also have a
+ list of <code>address</code> subelements, each
+ containing the PCI address of a device in the same
+ group. The toplevel device will itself be included in
+ this list.
+ </dd>
+ <dt><code>capability</code></dt>
+ <dd>
+ This optional element can occur multiple times. If it
+ exists, it has a mandatory <code>type</code> attribute
+ which will be set to
+ either <code>physical_function</code>
+ or <code>virtual_functions</code>. If the type
+ is <code>physical_function</code>, there will be a
+ single <code>address</code> subelement which contains
+ the PCI address of the SRIOV Physical Function (PF)
+ that is the parent of this device (and this device is,
+ by implication, an SRIOV Virtual Function (VF)). If
+ the type is <code>virtual_functions</code>, then this
+ device is an SRIOV PF, and the capability element will
+ have a list of <code>address</code> subelements, one
+ for each VF on this PF.
+ </dd>
</dl>
</dd>
<dt><code>usb_device</code></dt>
@@ -232,7 +262,38 @@
<address>00:27:13:6a:fe:00</address>
<capability type='80203'/>
</capability>
-</device></pre>
+</device>
+
+<device>
+ <name>pci_0000_02_00_0</name>
+
<path>/sys/devices/pci0000:00/0000:00:04.0/0000:02:00.0</path>
+ <parent>pci_0000_00_04_0</parent>
+ <driver>
+ <name>igb</name>
+ </driver>
+ <capability type='pci'>
+ <domain>0</domain>
+ <bus>2</bus>
+ <slot>0</slot>
+ <function>0</function>
+ <product id='0x10c9'>82576 Gigabit Network
Connection</product>
+ <vendor id='0x8086'>Intel Corporation</vendor>
+ <capability type='virt_functions'>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x2'/>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x4'/>
+ <address domain='0x0000' bus='0x02' slot='0x10'
function='0x6'/>
+ <address domain='0x0000' bus='0x02' slot='0x11'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x11'
function='0x2'/>
+ <address domain='0x0000' bus='0x02' slot='0x11'
function='0x4'/>
+ </capability>
+ <iommuGroup number='12'>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x1'/>
+ </iommuGroup>
+ </capability>
+</device>
+ </pre>
</body>
</html>
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 1f5dcb9..d2bebff 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -144,6 +144,17 @@
</element>
</optional>
+ <optional>
+ <element name='iommuGroup'>
+ <attribute name='number'>
+ <ref name='unsignedInt'/>
+ </attribute>
+ <oneOrMore>
+ <ref name='address'/>
+ </oneOrMore>
+ </element>
+ </optional>
+
</define>
<define name='capusbdev'>
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 548cd8f..96742ef 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1,6 +1,7 @@
/*
* node_device_conf.c: config handling for node devices
*
+ * Copyright (C) 2009-2013 Red Hat, Inc.
* Copyright (C) 2008 Virtual Iron Software, Inc.
* Copyright (C) 2008 David F. Lively
*
@@ -31,6 +32,7 @@
#include "viralloc.h"
#include "virstring.h"
#include "node_device_conf.h"
+#include "device_conf.h"
#include "virxml.h"
#include "virbuffer.h"
#include "viruuid.h"
@@ -330,6 +332,20 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDefPtr def)
}
virBufferAddLit(&buf, " </capability>\n");
}
+ if (data->pci_dev.nIommuGroupDevices) {
+ virBufferAsprintf(&buf, " <iommuGroup
number='%d'>\n",
+ data->pci_dev.iommuGroupNumber);
+ for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) {
+ virBufferAsprintf(&buf,
+ " <address domain='0x%.4x'
bus='0x%.2x' "
+ "slot='0x%.2x'
function='0x%.1x'/>\n",
+ data->pci_dev.iommuGroupDevices[i]->domain,
+ data->pci_dev.iommuGroupDevices[i]->bus,
+ data->pci_dev.iommuGroupDevices[i]->slot,
+
data->pci_dev.iommuGroupDevices[i]->function);
+ }
+ virBufferAddLit(&buf, " </iommuGroup>\n");
+ }
break;
case VIR_NODE_DEV_CAP_USB_DEV:
virBufferAsprintf(&buf, " <bus>%d</bus>\n",
data->usb_dev.bus);
@@ -966,12 +982,71 @@ out:
}
static int
+virNodeDevCapPciDevIommuGroupParseXML(xmlXPathContextPtr ctxt,
+ xmlNodePtr iommuGroupNode,
+ union _virNodeDevCapData *data)
+{
+ xmlNodePtr origNode = ctxt->node;
+ xmlNodePtr *addrNodes = NULL;
+ char *numberStr = NULL;
+ int nAddrNodes, ii, ret = -1;
+ virPCIDeviceAddressPtr pciAddr = NULL;
+
+ ctxt->node = iommuGroupNode;
+
+ numberStr = virXMLPropString(iommuGroupNode, "number");
+ if (!numberStr) {
+ virReportError(VIR_ERR_XML_ERROR,
+ "%s", _("missing iommuGroup number
attribute"));
+ goto cleanup;
+ }
+ if (virStrToLong_ui(numberStr, NULL, 10,
+ &data->pci_dev.iommuGroupNumber) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("invalid iommuGroup number attribute '%s'"),
+ numberStr);
+ goto cleanup;
+ }
+
+ if ((nAddrNodes = virXPathNodeSet("./address", ctxt, &addrNodes)) <
0)
+ goto cleanup;
+
+ for (ii = 0; ii < nAddrNodes; ii++) {
+ virDevicePCIAddress addr = { 0, 0, 0, 0, 0 };
+ if (virDevicePCIAddressParseXML(addrNodes[ii], &addr) < 0)
+ goto cleanup;
+ if (VIR_ALLOC(pciAddr) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ pciAddr->domain = addr.domain;
+ pciAddr->bus = addr.bus;
+ pciAddr->slot = addr.slot;
+ pciAddr->function = addr.function;
+ if (VIR_APPEND_ELEMENT(data->pci_dev.iommuGroupDevices,
+ data->pci_dev.nIommuGroupDevices,
+ pciAddr) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+cleanup:
+ ctxt->node = origNode;
+ VIR_FREE(addrNodes);
+ VIR_FREE(pciAddr);
+ return ret;
+}
+
+
+static int
virNodeDevCapPciDevParseXML(xmlXPathContextPtr ctxt,
virNodeDeviceDefPtr def,
xmlNodePtr node,
union _virNodeDevCapData *data)
{
- xmlNodePtr orignode;
+ xmlNodePtr orignode, iommuGroupNode;
int ret = -1;
orignode = ctxt->node;
@@ -1016,6 +1091,12 @@ virNodeDevCapPciDevParseXML(xmlXPathContextPtr ctxt,
data->pci_dev.vendor_name = virXPathString("string(./vendor[1])",
ctxt);
data->pci_dev.product_name = virXPathString("string(./product[1])",
ctxt);
+ if ((iommuGroupNode = virXPathNode("./iommuGroup[1]", ctxt))) {
+ if (virNodeDevCapPciDevIommuGroupParseXML(ctxt, iommuGroupNode,
+ data) < 0) {
+ goto out;
+ }
+ }
ret = 0;
out:
ctxt->node = orignode;
@@ -1385,6 +1466,9 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
for (i = 0; i < data->pci_dev.num_virtual_functions; i++) {
VIR_FREE(data->pci_dev.virtual_functions[i]);
}
+ for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) {
+ VIR_FREE(data->pci_dev.iommuGroupDevices[i]);
+ }
break;
case VIR_NODE_DEV_CAP_USB_DEV:
VIR_FREE(data->usb_dev.product_name);
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 17240be..ec35da2 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -1,7 +1,7 @@
/*
* node_device_conf.h: config handling for node devices
*
- * Copyright (C) 2010-2011 Red Hat, Inc.
+ * Copyright (C) 2009-2013 Red Hat, Inc.
* Copyright (C) 2008 Virtual Iron Software, Inc.
* Copyright (C) 2008 David F. Lively
*
@@ -112,6 +112,9 @@ struct _virNodeDevCapsDef {
virPCIDeviceAddressPtr *virtual_functions;
unsigned int num_virtual_functions;
unsigned int flags;
+ virPCIDeviceAddressPtr *iommuGroupDevices;
+ size_t nIommuGroupDevices;
+ unsigned int iommuGroupNumber;
} pci_dev;
struct {
unsigned int bus;
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index b94be80..9bb4a17 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -421,7 +421,8 @@ static int udevProcessPCI(struct udev_device *device,
{
const char *syspath = NULL;
union _virNodeDevCapData *data = &def->caps->data;
- int ret = -1;
+ virPCIDeviceAddress addr;
+ int tmpGroup, ret = -1;
char *p;
int rc;
@@ -501,6 +502,24 @@ static int udevProcessPCI(struct udev_device *device,
else if (!rc && (data->pci_dev.num_virtual_functions > 0))
data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+ /* iommu group */
+ addr.domain = data->pci_dev.domain;
+ addr.bus = data->pci_dev.bus;
+ addr.slot = data->pci_dev.slot;
+ addr.function = data->pci_dev.function;
+ tmpGroup = virPCIGetIOMMUGroupNum(&addr);
+ if (tmpGroup == -1) {
+ /* error was already reported */
+ goto out;
+ /* -2 return means there is no iommu_group data */
+ } else if (tmpGroup >= 0) {
+ if (virPCIGetIOMMUGroupAddresses(&addr,
&data->pci_dev.iommuGroupDevices,
+ &data->pci_dev.nIommuGroupDevices) <
0) {
+ goto out;
+ }
+ data->pci_dev.iommuGroupNumber = tmpGroup;
+ }
+
ret = 0;
out:
diff --git a/tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
b/tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
new file mode 100644
index 0000000..eff8932
--- /dev/null
+++ b/tests/nodedevschemadata/pci_8086_10c9_sriov_pf.xml
@@ -0,0 +1,16 @@
+<device>
+ <name>pci_0000_02_00_0</name>
+ <parent>pci_0000_00_04_0</parent>
+ <capability type='pci'>
+ <domain>0</domain>
+ <bus>2</bus>
+ <slot>0</slot>
+ <function>0</function>
+ <product id='0x10c9'>82576 Gigabit Network Connection</product>
+ <vendor id='0x8086'>Intel Corporation</vendor>
+ <iommuGroup number='12'>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x0'/>
+ <address domain='0x0000' bus='0x02' slot='0x00'
function='0x1'/>
+ </iommuGroup>
+ </capability>
+</device>
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index e81c617..ed49857 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -78,6 +78,7 @@ mymain(void)
DO_TEST("net_00_13_02_b9_f9_d3");
DO_TEST("net_00_15_58_2f_e9_55");
DO_TEST("pci_1002_71c4");
+ DO_TEST("pci_8086_10c9_sriov_pf");
DO_TEST("pci_8086_27c5_scsi_host_0");
DO_TEST("pci_8086_27c5_scsi_host_scsi_device_lun0");
DO_TEST("pci_8086_27c5_scsi_host_scsi_host");
4:33 a.m.
New subject: [libvirt] [PATCHv2 05/12] nodedev: add iommuGroup to node device object
On Wed, Jun 26, 2013 at 12:53:47AM -0400, Laine Stump wrote:
I just realized that I had only implemented this for the udev
nodeDevice
driver, but not the HAL driver. I can easily add the same code into the
HAL driver, but don't have any system to test building it on.
Should I put that code in untested, or leave the HAL driver without this
functionality? (Does anyone still use HAL?)
HAL is dead, so no system which is new enough to support vfio
will be using HAL. As such you can ignore it for anything that
is related to vfio IMHO
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:05 p.m.
New subject: [libvirt] [PATCHv2 06/12] pci: eliminate repetitive path constructions in virPCIDeviceBindToStub
The same strings were being re-created multiple times just to save
declaring a new variable. In the meantime, the use of the generic
variable names led to confusion when trying to follow the code. This
patch creates strings for:
stubDriverName (was called "driver" in original args)
stubDriverPath ("/sys/bus/pci/drivers/${stubDriverName}")
driverLink ("${device}/driver")
oldDriverName (the final component of path linked to by
"${device}/driver")
oldDriverPath ("/sys/bus/pci/drivers/${oldDriverName}")
then re-uses them as necessary.
---
src/util/virpci.c | 73 ++++++++++++++++++++++++++++---------------------------
1 file changed, 37 insertions(+), 36 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 5209372..128f721 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1068,22 +1068,30 @@ cleanup:
static int
-virPCIDeviceBindToStub(virPCIDevicePtr dev, const char *driver)
+virPCIDeviceBindToStub(virPCIDevicePtr dev,
+ const char *stubDriverName)
{
int result = -1;
- char *drvdir = NULL;
- char *path = NULL;
int reprobe = false;
- /* check whether the device is already bound to a driver */
- if (virPCIDriverDir(&drvdir, driver) < 0 ||
- virPCIFile(&path, dev->name, "driver") < 0) {
+ char *stubDriverPath = NULL;
+ char *driverLink = NULL;
+ char *oldDriverPath = NULL;
+ char *oldDriverName = NULL;
+ char *path = NULL; /* reused for different purposes */
+
+ if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 ||
+ virPCIFile(&driverLink, dev->name, "driver") < 0 ||
+ virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath,
+ &oldDriverName) < 0) {
goto cleanup;
}
- if (virFileExists(path)) {
- if (virFileLinkPointsTo(path, drvdir)) {
- /* The device is already bound to pci-stub */
+ if (virFileExists(driverLink)) {
+ if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
+ /* The device is already bound to the correct driver */
+ VIR_DEBUG("Device %s is already bound to %s",
+ dev->name, stubDriverName);
result = 0;
goto cleanup;
}
@@ -1098,26 +1106,21 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, const char *driver)
* is triggered for such a device, it will also be immediately
* bound by the stub.
*/
- if (virPCIDriverFile(&path, driver, "new_id") < 0) {
+ if (virPCIDriverFile(&path, stubDriverName, "new_id") < 0) {
goto cleanup;
}
if (virFileWriteStr(path, dev->id, 0) < 0) {
virReportSystemError(errno,
_("Failed to add PCI device ID '%s' to
%s"),
- dev->id, driver);
+ dev->id, stubDriverName);
goto cleanup;
}
/* check whether the device is bound to pci-stub when we write dev->id to
- * new_id.
+ * ${stubDriver}/new_id.
*/
- if (virPCIDriverDir(&drvdir, driver) < 0 ||
- virPCIFile(&path, dev->name, "driver") < 0) {
- goto remove_id;
- }
-
- if (virFileLinkPointsTo(path, drvdir)) {
+ if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
dev->unbind_from_stub = true;
dev->remove_slot = true;
goto remove_id;
@@ -1144,33 +1147,29 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, const char *driver)
/* If the device isn't already bound to pci-stub, try binding it now.
*/
- if (virPCIDriverDir(&drvdir, driver) < 0 ||
- virPCIFile(&path, dev->name, "driver") < 0) {
- goto remove_id;
- }
-
- if (!virFileLinkPointsTo(path, drvdir)) {
+ if (!virFileLinkPointsTo(driverLink, stubDriverPath)) {
/* Xen's pciback.ko wants you to use new_slot first */
- if (virPCIDriverFile(&path, driver, "new_slot") < 0) {
+ if (virPCIDriverFile(&path, stubDriverName, "new_slot") < 0) {
goto remove_id;
}
if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0)
{
virReportSystemError(errno,
- _("Failed to add slot for PCI device '%s'
to %s"),
- dev->name, driver);
+ _("Failed to add slot for "
+ "PCI device '%s' to %s"),
+ dev->name, stubDriverName);
goto remove_id;
}
dev->remove_slot = true;
- if (virPCIDriverFile(&path, driver, "bind") < 0) {
+ if (virPCIDriverFile(&path, stubDriverName, "bind") < 0) {
goto remove_id;
}
if (virFileWriteStr(path, dev->name, 0) < 0) {
virReportSystemError(errno,
_("Failed to bind PCI device '%s' to
%s"),
- dev->name, driver);
+ dev->name, stubDriverName);
goto remove_id;
}
dev->unbind_from_stub = true;
@@ -1180,11 +1179,11 @@ remove_id:
/* If 'remove_id' exists, remove the device id from pci-stub's dynamic
* ID table so that 'drivers_probe' works below.
*/
- if (virPCIDriverFile(&path, driver, "remove_id") < 0) {
+ if (virPCIDriverFile(&path, stubDriverName, "remove_id") < 0) {
/* We do not remove PCI ID from pci-stub, and we cannot reprobe it */
if (dev->reprobe) {
VIR_WARN("Could not remove PCI ID '%s' from %s, and the device
"
- "cannot be probed again.", dev->id, driver);
+ "cannot be probed again.", dev->id, stubDriverName);
}
dev->reprobe = false;
goto cleanup;
@@ -1193,12 +1192,12 @@ remove_id:
if (virFileExists(path) && virFileWriteStr(path, dev->id, 0) < 0) {
virReportSystemError(errno,
_("Failed to remove PCI ID '%s' from
%s"),
- dev->id, driver);
+ dev->id, stubDriverName);
/* remove PCI ID from pci-stub failed, and we cannot reprobe it */
if (dev->reprobe) {
VIR_WARN("Failed to remove PCI ID '%s' from %s, and the device
"
- "cannot be probed again.", dev->id, driver);
+ "cannot be probed again.", dev->id, stubDriverName);
}
dev->reprobe = false;
goto cleanup;
@@ -1207,12 +1206,14 @@ remove_id:
result = 0;
cleanup:
- VIR_FREE(drvdir);
+ VIR_FREE(stubDriverPath);
+ VIR_FREE(driverLink);
+ VIR_FREE(oldDriverPath);
+ VIR_FREE(oldDriverName);
VIR_FREE(path);
- if (result < 0) {
+ if (result < 0)
virPCIDeviceUnbindFromStub(dev);
- }
return result;
}
--
1.7.11.7
5:41 a.m.
New subject: [libvirt] [PATCHv2 06/12] pci: eliminate repetitive path constructions in virPCIDeviceBindToStub
On Mon, Jun 24, 2013 at 11:05:32PM -0400, Laine Stump wrote:
The same strings were being re-created multiple times just to save
declaring a new variable. In the meantime, the use of the generic
variable names led to confusion when trying to follow the code. This
patch creates strings for:
stubDriverName (was called "driver" in original args)
stubDriverPath ("/sys/bus/pci/drivers/${stubDriverName}")
driverLink ("${device}/driver")
oldDriverName (the final component of path linked to by
"${device}/driver")
oldDriverPath ("/sys/bus/pci/drivers/${oldDriverName}")
then re-uses them as necessary.
---
src/util/virpci.c | 73 ++++++++++++++++++++++++++++---------------------------
1 file changed, 37 insertions(+), 36 deletions(-)
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:05 p.m.
New subject: [libvirt] [PATCHv2 07/12] pci: update stubDriver name in virPCIDeviceBindToStub
If the device is bound to a stub driver different from what is saved
in the virPCIDevice's stubDriver attribute, update it.
---
src/util/virpci.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 128f721..cbade1b 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1079,6 +1079,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
char *oldDriverPath = NULL;
char *oldDriverName = NULL;
char *path = NULL; /* reused for different purposes */
+ const char *newDriverName = NULL;
if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 ||
virPCIFile(&driverLink, dev->name, "driver") < 0 ||
@@ -1092,6 +1093,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
/* The device is already bound to the correct driver */
VIR_DEBUG("Device %s is already bound to %s",
dev->name, stubDriverName);
+ newDriverName = stubDriverName;
result = 0;
goto cleanup;
}
@@ -1203,6 +1205,7 @@ remove_id:
goto cleanup;
}
+ newDriverName = stubDriverName;
result = 0;
cleanup:
@@ -1212,6 +1215,11 @@ cleanup:
VIR_FREE(oldDriverName);
VIR_FREE(path);
+ if (newDriverName &&
+ STRNEQ_NULLABLE(dev->stubDriver, newDriverName)) {
+ VIR_FREE(dev->stubDriver);
+ result = VIR_STRDUP(dev->stubDriver, newDriverName);
+ }
if (result < 0)
virPCIDeviceUnbindFromStub(dev);
--
1.7.11.7
5:43 a.m.
New subject: [libvirt] [PATCHv2 07/12] pci: update stubDriver name in virPCIDeviceBindToStub
On Mon, Jun 24, 2013 at 11:05:33PM -0400, Laine Stump wrote:
If the device is bound to a stub driver different from what is saved
in the virPCIDevice's stubDriver attribute, update it.
---
src/util/virpci.c | 8 ++++++++
1 file changed, 8 insertions(+)
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:05 p.m.
New subject: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO
I just learned that VFIO resets PCI devices when they are assigned to
guests / returned to the host, so it is redundant for libvirt to reset
the devices. This patch inhibits calling virPCIDeviceReset to devices
that will be/were assigned using VFIO.
---
src/qemu/qemu_hostdev.c | 4 ++++
src/qemu/qemu_hotplug.c | 5 +++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dfe39c6..d7d54d7 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
* can safely reset them */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
+ continue;
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0)
goto reattachdevs;
@@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
+ continue;
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0) {
virErrorPtr err = virGetLastError();
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 18f5fa5..46875ad 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
if (pci) {
activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci);
if (activePci &&
- virPCIDeviceReset(activePci, driver->activePciHostdevs,
- driver->inactivePciHostdevs) == 0) {
+ (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
+ virPCIDeviceReset(activePci, driver->activePciHostdevs,
+ driver->inactivePciHostdevs) == 0)) {
qemuReattachPciDevice(activePci, driver);
ret = 0;
} else {
--
1.7.11.7
10:17 p.m.
New subject: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO
I didn't notice that Martin had already ACKed this patch earlier today.
I just pushed it, so no further review is needed (although it would
still be good to get 07/12 reviewed and pushed to be sure this patch has
proper information)
On 06/24/2013 11:05 PM, Laine Stump wrote:
I just learned that VFIO resets PCI devices when they are assigned
to
guests / returned to the host, so it is redundant for libvirt to reset
the devices. This patch inhibits calling virPCIDeviceReset to devices
that will be/were assigned using VFIO.
---
src/qemu/qemu_hostdev.c | 4 ++++
src/qemu/qemu_hotplug.c | 5 +++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dfe39c6..d7d54d7 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
* can safely reset them */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
+ continue;
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0)
goto reattachdevs;
@@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
+ continue;
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0) {
virErrorPtr err = virGetLastError();
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 18f5fa5..46875ad 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
if (pci) {
activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci);
if (activePci &&
- virPCIDeviceReset(activePci, driver->activePciHostdevs,
- driver->inactivePciHostdevs) == 0) {
+ (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
+ virPCIDeviceReset(activePci, driver->activePciHostdevs,
+ driver->inactivePciHostdevs) == 0)) {
qemuReattachPciDevice(activePci, driver);
ret = 0;
} else {
5:44 a.m.
New subject: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO
On Mon, Jun 24, 2013 at 11:05:34PM -0400, Laine Stump wrote:
I just learned that VFIO resets PCI devices when they are assigned
to
guests / returned to the host, so it is redundant for libvirt to reset
the devices. This patch inhibits calling virPCIDeviceReset to devices
that will be/were assigned using VFIO.
---
src/qemu/qemu_hostdev.c | 4 ++++
src/qemu/qemu_hotplug.c | 5 +++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dfe39c6..d7d54d7 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
* can safely reset them */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
+ continue;
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0)
goto reattachdevs;
@@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
+ continue;
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0) {
virErrorPtr err = virGetLastError();
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 18f5fa5..46875ad 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
if (pci) {
activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci);
if (activePci &&
- virPCIDeviceReset(activePci, driver->activePciHostdevs,
- driver->inactivePciHostdevs) == 0) {
+ (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
+ virPCIDeviceReset(activePci, driver->activePciHostdevs,
+ driver->inactivePciHostdevs) == 0)) {
I'm guessing that virPCIDeviceGetStubDriver() isn't returning 'pci-stub'
here, right ? Otherwise you'd would have used the same pattern as earlier
qemuReattachPciDevice(activePci, driver);
ret = 0;
} else {
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:06 p.m.
New subject: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO
On 06/25/2013 06:44 AM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 11:05:34PM -0400, Laine Stump wrote:
> I just learned that VFIO resets PCI devices when they are assigned to
> guests / returned to the host, so it is redundant for libvirt to reset
> the devices. This patch inhibits calling virPCIDeviceReset to devices
> that will be/were assigned using VFIO.
> ---
> src/qemu/qemu_hostdev.c | 4 ++++
> src/qemu/qemu_hotplug.c | 5 +++--
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index dfe39c6..d7d54d7 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> * can safely reset them */
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
> + continue;
> if (virPCIDeviceReset(dev, driver->activePciHostdevs,
> driver->inactivePciHostdevs) < 0)
> goto reattachdevs;
> @@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
>
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
> + continue;
> if (virPCIDeviceReset(dev, driver->activePciHostdevs,
> driver->inactivePciHostdevs) < 0) {
> virErrorPtr err = virGetLastError();
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 18f5fa5..46875ad 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
> if (pci) {
> activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci);
> if (activePci &&
> - virPCIDeviceReset(activePci, driver->activePciHostdevs,
> - driver->inactivePciHostdevs) == 0) {
> + (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
> + virPCIDeviceReset(activePci, driver->activePciHostdevs,
> + driver->inactivePciHostdevs) == 0)) {
I'm guessing that virPCIDeviceGetStubDriver() isn't returning 'pci-stub'
here, right ? Otherwise you'd would have used the same pattern as earlier
No, the stub driver should be set correctly at this time[*]. It was just
a case of having multiple options for learning the answer in this case,
and choosing the one that's more efficient.
In the other cases where I need to decide whether or not to call
virPCIDeviceReset, I don't have access to the virDomainHostdevDef, only
to a virPCIDevice that was created from the virDomainHostdevDef, so I
use the less efficient method of a strcmp of the stubDriverName.
[*] When the device is initially attached to the guest, It's set from
the hostdev right at the time the virPCIDevice is created based on
config data in virDomainHostdevDef. That virPCIDevice is placed in the
activePciHostdevs list and is what is found in "activePci" in the above
code chunk. In the case that libvirtd is restarted, again the
virPCIDevice that is in the activePciHostdevs list is created based on
the virDomainHostdevDef in the domain's config, so it will have the
correct stubDriverName.
12:09 p.m.
New subject: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO
On Tue, Jun 25, 2013 at 01:06:55PM -0400, Laine Stump wrote:
On 06/25/2013 06:44 AM, Daniel P. Berrange wrote:
> On Mon, Jun 24, 2013 at 11:05:34PM -0400, Laine Stump wrote:
>> I just learned that VFIO resets PCI devices when they are assigned to
>> guests / returned to the host, so it is redundant for libvirt to reset
>> the devices. This patch inhibits calling virPCIDeviceReset to devices
>> that will be/were assigned using VFIO.
>> ---
>> src/qemu/qemu_hostdev.c | 4 ++++
>> src/qemu/qemu_hotplug.c | 5 +++--
>> 2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
>> index dfe39c6..d7d54d7 100644
>> --- a/src/qemu/qemu_hostdev.c
>> +++ b/src/qemu/qemu_hostdev.c
>> @@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
>> * can safely reset them */
>> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev),
"vfio-pci"))
>> + continue;
>> if (virPCIDeviceReset(dev, driver->activePciHostdevs,
>> driver->inactivePciHostdevs) < 0)
>> goto reattachdevs;
>> @@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr
driver,
>>
>> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev),
"vfio-pci"))
>> + continue;
>> if (virPCIDeviceReset(dev, driver->activePciHostdevs,
>> driver->inactivePciHostdevs) < 0) {
>> virErrorPtr err = virGetLastError();
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 18f5fa5..46875ad 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
>> if (pci) {
>> activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci);
>> if (activePci &&
>> - virPCIDeviceReset(activePci, driver->activePciHostdevs,
>> - driver->inactivePciHostdevs) == 0) {
>> + (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO
||
>> + virPCIDeviceReset(activePci, driver->activePciHostdevs,
>> + driver->inactivePciHostdevs) == 0)) {
> I'm guessing that virPCIDeviceGetStubDriver() isn't returning
'pci-stub'
> here, right ? Otherwise you'd would have used the same pattern as earlier
No, the stub driver should be set correctly at this time[*]. It was just
a case of having multiple options for learning the answer in this case,
and choosing the one that's more efficient.
Ok, so where I was going here, was that if the the virPCIDevicePtr
object has enough info, then could we make virPCIDeviceReset()
look for stub == vfio-pci, and make itself a nop. This avoids the
risk of one caller forgetting to do the check itself.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:28 p.m.
New subject: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO
On 06/25/2013 01:09 PM, Daniel P. Berrange wrote:
On Tue, Jun 25, 2013 at 01:06:55PM -0400, Laine Stump wrote:
> On 06/25/2013 06:44 AM, Daniel P. Berrange wrote:
>> On Mon, Jun 24, 2013 at 11:05:34PM -0400, Laine Stump wrote:
>>> I just learned that VFIO resets PCI devices when they are assigned to
>>> guests / returned to the host, so it is redundant for libvirt to reset
>>> the devices. This patch inhibits calling virPCIDeviceReset to devices
>>> that will be/were assigned using VFIO.
>>> ---
>>> src/qemu/qemu_hostdev.c | 4 ++++
>>> src/qemu/qemu_hotplug.c | 5 +++--
>>> 2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
>>> index dfe39c6..d7d54d7 100644
>>> --- a/src/qemu/qemu_hostdev.c
>>> +++ b/src/qemu/qemu_hostdev.c
>>> @@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr
driver,
>>> * can safely reset them */
>>> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>>> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>>> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev),
"vfio-pci"))
>>> + continue;
>>> if (virPCIDeviceReset(dev, driver->activePciHostdevs,
>>> driver->inactivePciHostdevs) < 0)
>>> goto reattachdevs;
>>> @@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr
driver,
>>>
>>> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>>> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>>> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev),
"vfio-pci"))
>>> + continue;
>>> if (virPCIDeviceReset(dev, driver->activePciHostdevs,
>>> driver->inactivePciHostdevs) < 0) {
>>> virErrorPtr err = virGetLastError();
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index 18f5fa5..46875ad 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
>>> if (pci) {
>>> activePci = virPCIDeviceListSteal(driver->activePciHostdevs,
pci);
>>> if (activePci &&
>>> - virPCIDeviceReset(activePci, driver->activePciHostdevs,
>>> - driver->inactivePciHostdevs) == 0) {
>>> + (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO
||
>>> + virPCIDeviceReset(activePci, driver->activePciHostdevs,
>>> + driver->inactivePciHostdevs) == 0)) {
>> I'm guessing that virPCIDeviceGetStubDriver() isn't returning
'pci-stub'
>> here, right ? Otherwise you'd would have used the same pattern as earlier
> No, the stub driver should be set correctly at this time[*]. It was just
> a case of having multiple options for learning the answer in this case,
> and choosing the one that's more efficient.
Ok, so where I was going here, was that if the the virPCIDevicePtr
object has enough info, then could we make virPCIDeviceReset()
look for stub == vfio-pci, and make itself a nop. This avoids the
risk of one caller forgetting to do the check itself.
Interesting idea, and it will work as long as:
1) we only reset the device while it's bound to the stub driver (not,
for example, just after it's been unbound from vfio-pci and bound to the
host driver).
and
2) we really never want to reset a device that is bound to vfio-pci.
I just double-checked, and as far as I can see, both of these are true,
so we can do it. I'll make a patch to do that as soon as I finish
cleaning up the existing patches.
10:05 p.m.
New subject: [libvirt] [PATCHv2 09/12] pci: virPCIDeviceListAddCopy API
Make a copy of the device and add the copy to the
list. (virPCIDeviceListAdd() adds the original object to the list
instead).
---
src/libvirt_private.syms | 1 +
src/util/virpci.c | 17 +++++++++++++++++
src/util/virpci.h | 1 +
3 files changed, 19 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e906742..bf9bd12 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1703,6 +1703,7 @@ virPCIDeviceGetUnbindFromStub;
virPCIDeviceGetUsedBy;
virPCIDeviceIsAssignable;
virPCIDeviceListAdd;
+virPCIDeviceListAddCopy;
virPCIDeviceListCount;
virPCIDeviceListDel;
virPCIDeviceListFind;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index cbade1b..378b4f3 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1705,6 +1705,23 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list,
return 0;
}
+
+/* virPCIDeviceListAddCopy - add a *copy* of the device to this list */
+int
+virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev)
+{
+ virPCIDevicePtr copy = virPCIDeviceCopy(dev);
+
+ if (!copy)
+ return -1;
+ if (virPCIDeviceListAdd(list, copy) < 0) {
+ virPCIDeviceFree(copy);
+ return -1;
+ }
+ return 0;
+}
+
+
virPCIDevicePtr
virPCIDeviceListGet(virPCIDeviceListPtr list,
int idx)
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 972f86b..5f80de3 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -83,6 +83,7 @@ void virPCIDeviceReattachInit(virPCIDevice *dev);
virPCIDeviceListPtr virPCIDeviceListNew(void);
int virPCIDeviceListAdd(virPCIDeviceListPtr list,
virPCIDevicePtr dev);
+int virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev);
virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr list,
int idx);
int virPCIDeviceListCount(virPCIDeviceListPtr list);
--
1.7.11.7
5:45 a.m.
New subject: [libvirt] [PATCHv2 09/12] pci: virPCIDeviceListAddCopy API
On Mon, Jun 24, 2013 at 11:05:35PM -0400, Laine Stump wrote:
Make a copy of the device and add the copy to the
list. (virPCIDeviceListAdd() adds the original object to the list
instead).
---
src/libvirt_private.syms | 1 +
src/util/virpci.c | 17 +++++++++++++++++
src/util/virpci.h | 1 +
3 files changed, 19 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e906742..bf9bd12 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1703,6 +1703,7 @@ virPCIDeviceGetUnbindFromStub;
virPCIDeviceGetUsedBy;
virPCIDeviceIsAssignable;
virPCIDeviceListAdd;
+virPCIDeviceListAddCopy;
virPCIDeviceListCount;
virPCIDeviceListDel;
virPCIDeviceListFind;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index cbade1b..378b4f3 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1705,6 +1705,23 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list,
return 0;
}
+
+/* virPCIDeviceListAddCopy - add a *copy* of the device to this list */
+int
+virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev)
+{
+ virPCIDevicePtr copy = virPCIDeviceCopy(dev);
+
+ if (!copy)
+ return -1;
+ if (virPCIDeviceListAdd(list, copy) < 0) {
+ virPCIDeviceFree(copy);
+ return -1;
+ }
+ return 0;
+}
+
+
virPCIDevicePtr
virPCIDeviceListGet(virPCIDeviceListPtr list,
int idx)
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 972f86b..5f80de3 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -83,6 +83,7 @@ void virPCIDeviceReattachInit(virPCIDevice *dev);
virPCIDeviceListPtr virPCIDeviceListNew(void);
int virPCIDeviceListAdd(virPCIDeviceListPtr list,
virPCIDevicePtr dev);
+int virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev);
virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr list,
int idx);
int virPCIDeviceListCount(virPCIDeviceListPtr list);
ACK, though as a followup, it'd be nice to actually eliminate the
virPCIDeviceListAdd variant, since APIs stealing ownership of passed
in parameters have been a good source of bugs historically.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:06 p.m.
New subject: [libvirt] [PATCHv2 09/12] pci: virPCIDeviceListAddCopy API
On 06/25/2013 06:45 AM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 11:05:35PM -0400, Laine Stump wrote:
> Make a copy of the device and add the copy to the
> list. (virPCIDeviceListAdd() adds the original object to the list
> instead).
> ---
> src/libvirt_private.syms | 1 +
> src/util/virpci.c | 17 +++++++++++++++++
> src/util/virpci.h | 1 +
> 3 files changed, 19 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e906742..bf9bd12 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1703,6 +1703,7 @@ virPCIDeviceGetUnbindFromStub;
> virPCIDeviceGetUsedBy;
> virPCIDeviceIsAssignable;
> virPCIDeviceListAdd;
> +virPCIDeviceListAddCopy;
> virPCIDeviceListCount;
> virPCIDeviceListDel;
> virPCIDeviceListFind;
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index cbade1b..378b4f3 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1705,6 +1705,23 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list,
> return 0;
> }
>
> +
> +/* virPCIDeviceListAddCopy - add a *copy* of the device to this list */
> +int
> +virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev)
> +{
> + virPCIDevicePtr copy = virPCIDeviceCopy(dev);
> +
> + if (!copy)
> + return -1;
> + if (virPCIDeviceListAdd(list, copy) < 0) {
> + virPCIDeviceFree(copy);
> + return -1;
> + }
> + return 0;
> +}
> +
> +
> virPCIDevicePtr
> virPCIDeviceListGet(virPCIDeviceListPtr list,
> int idx)
> diff --git a/src/util/virpci.h b/src/util/virpci.h
> index 972f86b..5f80de3 100644
> --- a/src/util/virpci.h
> +++ b/src/util/virpci.h
> @@ -83,6 +83,7 @@ void virPCIDeviceReattachInit(virPCIDevice *dev);
> virPCIDeviceListPtr virPCIDeviceListNew(void);
> int virPCIDeviceListAdd(virPCIDeviceListPtr list,
> virPCIDevicePtr dev);
> +int virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev);
> virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr list,
> int idx);
> int virPCIDeviceListCount(virPCIDeviceListPtr list);
ACK, though as a followup, it'd be nice to actually eliminate the
virPCIDeviceListAdd variant, since APIs stealing ownership of passed
in parameters have been a good source of bugs historically.
Yes, and a source of confusion (which I guess is a good part of the
cause of the bugs :-)
The *really* ugly part of all the virPCIDeviceList code is all the
places that an object isn't even stolen, but is placed on multiple lists
simultaneously.
10:05 p.m.
New subject: [libvirt] [PATCHv2 10/12] pci: eliminate leak in OOM condition
The "fix" I pushed a few commits ago would still leak a virPCIDevice
in case of an OOM error. Although it's inconsequential in practice,
this patch satisfies my OCD.
---
src/util/virpci.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 378b4f3..515feea 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1268,11 +1268,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
/* Add *a copy of* the dev into list inactiveDevs, if
* it's not already there.
*/
- if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) {
- virPCIDevicePtr copy = virPCIDeviceCopy(dev);
-
- if ((!copy) || virPCIDeviceListAdd(inactiveDevs, copy) < 0)
- return -1;
+ if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) &&
+ virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {
+ return -1;
}
return 0;
--
1.7.11.7
5:46 a.m.
New subject: [libvirt] [PATCHv2 10/12] pci: eliminate leak in OOM condition
On Mon, Jun 24, 2013 at 11:05:36PM -0400, Laine Stump wrote:
The "fix" I pushed a few commits ago would still leak a
virPCIDevice
in case of an OOM error. Although it's inconsequential in practice,
this patch satisfies my OCD.
---
src/util/virpci.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 378b4f3..515feea 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1268,11 +1268,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
/* Add *a copy of* the dev into list inactiveDevs, if
* it's not already there.
*/
- if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) {
- virPCIDevicePtr copy = virPCIDeviceCopy(dev);
-
- if ((!copy) || virPCIDeviceListAdd(inactiveDevs, copy) < 0)
- return -1;
+ if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) &&
+ virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {
+ return -1;
}
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:05 p.m.
New subject: [libvirt] [PATCHv2 11/12] pci: fix dangling pointer in qemuDomainReAttachHostdevDevices
(This isn't as bad as it sounds - it's only a problem in case of an
OOM error.)
qemuGetActivePciHostDeviceList() had been creating a list that
contained pointers to objects that were also on the activePciHostdevs
list. In case of an OOM error, this newly created list would be
virObjectUnref'ed, which would cause everything on the list to be
freed. But all of those objects would still be on the
activePciHostdevs list, which could have very bad consequences if that
list was ever again accessed.
The solution used here is to populate the new list with *copies* of
the objects from the original list. It turns out that on return from
qemuGetActivePciHostDeviceList(), the caller would almost immediately
go through all the device objects and "steal" them (i.e. remove the
pointer from the list but not delete it) all from either one list or
the other; we now instead just *delete* (remove from the list and
free) each device from one list or the other, so in the end we have
the same state.
---
src/qemu/qemu_hostdev.c | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index d7d54d7..09ac6ad 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -86,6 +86,12 @@ qemuGetPciHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
}
/*
+ * qemuGetActivePciHostDeviceList - make a new list with a *copy* of
+ * every virPCIDevice object that is found on the activePciHostdevs
+ * list *and* is in the hostdev list for this domain.
+ *
+ * Return the new list, or NULL if there was a failure.
+ *
* Pre-condition: driver->activePciHostdevs is locked
*/
static virPCIDeviceListPtr
@@ -101,7 +107,7 @@ qemuGetActivePciHostDeviceList(virQEMUDriverPtr driver,
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
- virPCIDevicePtr dev;
+ virDevicePCIAddressPtr addr;
virPCIDevicePtr activeDev;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
@@ -109,24 +115,14 @@ qemuGetActivePciHostDeviceList(virQEMUDriverPtr driver,
if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
continue;
- dev = virPCIDeviceNew(hostdev->source.subsys.u.pci.addr.domain,
- hostdev->source.subsys.u.pci.addr.bus,
- hostdev->source.subsys.u.pci.addr.slot,
- hostdev->source.subsys.u.pci.addr.function);
- if (!dev) {
+ addr = &hostdev->source.subsys.u.pci.addr;
+ activeDev = virPCIDeviceListFindByIDs(driver->activePciHostdevs,
+ addr->domain, addr->bus,
+ addr->slot, addr->function);
+ if (activeDev && virPCIDeviceListAddCopy(list, activeDev) < 0) {
virObjectUnref(list);
return NULL;
}
-
- if ((activeDev = virPCIDeviceListFind(driver->activePciHostdevs, dev))) {
- if (virPCIDeviceListAdd(list, activeDev) < 0) {
- virPCIDeviceFree(dev);
- virObjectUnref(list);
- return NULL;
- }
- }
-
- virPCIDeviceFree(dev);
}
return list;
@@ -1084,20 +1080,24 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
virPCIDevicePtr activeDev = NULL;
- /* Never delete the dev from list driver->activePciHostdevs
- * if it's used by other domain.
+ /* delete the copy of the dev from pcidevs if it's used by
+ * other domain. Or delete it from activePciHostDevs if it had
+ * been used by this domain.
*/
activeDev = virPCIDeviceListFind(driver->activePciHostdevs, dev);
if (activeDev &&
STRNEQ_NULLABLE(name, virPCIDeviceGetUsedBy(activeDev))) {
- virPCIDeviceListSteal(pcidevs, dev);
+ virPCIDeviceListDel(pcidevs, dev);
continue;
}
- /* virObjectUnref() will take care of freeing the dev. */
- virPCIDeviceListSteal(driver->activePciHostdevs, dev);
+ virPCIDeviceListDel(driver->activePciHostdevs, dev);
}
+ /* At this point, any device that had been used by the guest is in
+ * pcidevs, but has been removed from activePciHostdevs.
+ */
+
/*
* For SRIOV net host devices, unset mac and port profile before
* reset and reattach device
--
1.7.11.7
10:05 p.m.
New subject: [libvirt] [PATCHv2 12/12] qemu: fix infinite loop in OOM error path
A loop in qemuPrepareHostdevPCIDevices() intended to cycle through all
the objects on the list pcidevs was doing "while (listcount > 0)", but
nothing in the body of the loop was reducing the size of the list - it
was instead removing items from a *different* list. It has now been
safely changed to a for() loop.
---
src/qemu/qemu_hostdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 09ac6ad..404939e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -638,8 +638,8 @@ inactivedevs:
/* Only steal all the devices from driver->activePciHostdevs. We will
* free them in virObjectUnref().
*/
- while (virPCIDeviceListCount(pcidevs) > 0) {
- virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, 0);
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+ virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
virPCIDeviceListSteal(driver->activePciHostdevs, dev);
}
--
1.7.11.7
5:47 a.m.
New subject: [libvirt] [PATCHv2 12/12] qemu: fix infinite loop in OOM error path
On Mon, Jun 24, 2013 at 11:05:38PM -0400, Laine Stump wrote:
A loop in qemuPrepareHostdevPCIDevices() intended to cycle through
all
the objects on the list pcidevs was doing "while (listcount > 0)", but
nothing in the body of the loop was reducing the size of the list - it
was instead removing items from a *different* list. It has now been
safely changed to a for() loop.
---
src/qemu/qemu_hostdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 09ac6ad..404939e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -638,8 +638,8 @@ inactivedevs:
/* Only steal all the devices from driver->activePciHostdevs. We will
* free them in virObjectUnref().
*/
- while (virPCIDeviceListCount(pcidevs) > 0) {
- virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, 0);
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+ virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
virPCIDeviceListSteal(driver->activePciHostdevs, dev);
}
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:42 p.m.
On 06/24/2013 11:05 PM, Laine Stump wrote:
This is what remains of yesterday's VFIO groups patchset that is
considered still useful, but wasn't yet ACKed (I've pushed the 5 that
were ACKed). In addition, I found a few more bugs in the
virPCIDeviceList handling and have included patches for those. There
are a couple of these that would be really nice to get into 1.0.7, one
in particular which is new functionality, but has 4 prerequisites:
The most important patch here from a functionality point of view is
PATCH 05/12, which adds the <iommuGroup> element to the output of
"virsh nodedev-dumpxml". This is essential for users/management
applications to have adequate information to know which devices need
to be detached from host drivers. (Unfortunately, all of the patches
prior to 05/12 are required for 05/12 to cleanly apply.)
Okay, I've pushed all except 04/12 (which needed changes) and 05/12
(which requires 04/12 to be pushed first). I made the changes to 04/12
and reposted.
https://www.redhat.com/archives/libvir-list/2013-June/msg01090.html
4167
days inactive
4168
days old
37 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Laine Stump