<div dir="ltr">
<div bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div>On 2015年03月31日 17:46, Martin Kletzander
wrote:<br>
</div>
<blockquote type="cite">On
Thu, Mar 26, 2015 at 10:23:47PM +0800, Huanle Han wrote:
<br>
<blockquote type="cite">Fix for such a case:
<br>
1. Domain A and B xml contain the same SRIOV net
hostdev(<interface
<br>
type='hostdev' /> with same pci address).
<br>
2. virsh start A (Successfully, and configure the SRIOV net with
<br>
custom mac)
<br>
3. virsh start B (Fail because of the hostdev used by domain A
or other
<br>
reason.)
<br>
In step 3, 'virHostdevNetConfigRestore' is called for the
hostdev
<br>
which is still used by domain A. It makes the mac/vlan of the
SRIOV net
<br>
change.
<br>
<br>
</blockquote>
<br>
Makes perfect sense, thanks for finding that out.
<br>
<br>
<blockquote type="cite">Code Change in this fix:
<br>
1. As the pci used by other domain have been removed from
<br>
'pcidevs' in previous loop, we only restore the nic config for
<br>
the hostdev still in 'pcidevs'(used by this domain)
<br>
2. wrap a function 'virHostdevIsPCINetDevice', which detect
whether the
<br>
hostdev is a pci net device or not.
<br>
3. update the comments to make it more clear
<br>
<br>
Signed-off-by: Huanle Han <a href="mailto:hanxueluo@gmail.com" target="_blank"><hanxueluo@gmail.com></a>
<br>
---
<br>
src/util/virhostdev.c | 52
++++++++++++++++++++++++++++++++++++---------------
<br>
1 file changed, 37 insertions(+), 15 deletions(-)
<br>
<br>
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
<br>
index 83f567d..b04bae2 100644
<br>
--- a/src/util/virhostdev.c
<br>
+++ b/src/util/virhostdev.c
<br>
@@ -350,6 +350,14 @@ virHostdevNetDevice(virDomainHostdevDefPtr
hostdev, char **linkdev,
<br>
return ret;
<br>
}
<br>
<br>
+static int
<br>
+virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev)
<br>
+{
<br>
+ return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
&&
<br>
+ hostdev->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
<br>
+ hostdev->parent.type == VIR_DOMAIN_DEVICE_NET
&&
<br>
+ hostdev-><a href="http://parent.data.net" target="_blank">parent.data.net</a>;
<br>
+}
<br>
<br>
static int
<br>
virHostdevNetConfigVirtPortProfile(const char *linkdev, int vf,
<br>
@@ -481,10 +489,7 @@
virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev,
<br>
/* This is only needed for PCI devices that have been
defined
<br>
* using <interface type='hostdev'>. For all others,
it is a NOP.
<br>
*/
<br>
- if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
<br>
- hostdev->source.subsys.type !=
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI ||
<br>
- hostdev->parent.type != VIR_DOMAIN_DEVICE_NET ||
<br>
- !hostdev-><a href="http://parent.data.net" target="_blank">parent.data.net</a>)
<br>
+ if (!virHostdevIsPCINetDevice(hostdev))
<br>
return 0;
<br>
<br>
isvf = virHostdevIsVirtualFunction(hostdev);
<br>
@@ -781,19 +786,20 @@
virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
<br>
goto cleanup;
<br>
}
<br>
<br>
- /* Again 4 loops; mark all devices as inactive before reset
<br>
+ /* Here are 4 loops; mark all devices as inactive before
reset
<br>
* them and reset all the devices before re-attach.
<br>
* Attach mac and port profile parameters to devices
<br>
*/
<br>
+
<br>
+ /* Loop 1: delete the copy of the dev from pcidevs if it's
used by
<br>
+ * other domain. Or delete it from activePCIHostDevs if it
had
<br>
+ * been used by this domain.
<br>
+ */
<br>
i = 0;
<br>
while (i < virPCIDeviceListCount(pcidevs)) {
<br>
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
<br>
virPCIDevicePtr activeDev = NULL;
<br>
<br>
- /* delete the copy of the dev from pcidevs if it's used
by
<br>
- * other domain. Or delete it from activePCIHostDevs if
it had
<br>
- * been used by this domain.
<br>
- */
<br>
activeDev =
virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev);
<br>
if (activeDev) {
<br>
const char *usedby_drvname;
<br>
@@ -814,14 +820,27 @@
virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
<br>
* pcidevs, but has been removed from activePCIHostdevs.
<br>
*/
<br>
<br>
- /*
<br>
- * For SRIOV net host devices, unset mac and port profile
before
<br>
- * reset and reattach device
<br>
+ /* Loop 2: before reset and reattach device,
<br>
+ * unset mac and port profile for SRIOV net host devices
used by this domain
<br>
*/
<br>
</blockquote>
<br>
The comments were formatted as a sentence, it would be nice to
keep it
<br>
that way.
<br>
</blockquote>
Is comment as below OK?<br>
/* <br>
* Loop 2: For SRIOV net host devices used by this domain, unset
mac<br>
* and port profile before reset and reattach device<br>
*/<br>
<br>
<blockquote type="cite">
<blockquote type="cite">- for (i = 0; i < nhostdevs; i++)
<br>
- virHostdevNetConfigRestore(hostdevs[i],
hostdev_mgr->stateDir,
<br>
- oldStateDir);
<br>
+ for (i = 0; i < nhostdevs; i++) {
<br>
+ virDomainHostdevDefPtr hostdev = hostdevs[i];
<br>
<br>
+ if (virHostdevIsPCINetDevice(hostdev)) {
<br>
+ virDomainHostdevSubsysPCIPtr pcisrc =
&hostdev->source.subsys.u.pci;
<br>
+ virPCIDevicePtr dev = NULL;
<br>
+ dev = virPCIDeviceNew(pcisrc->addr.domain,
pcisrc->addr.bus,
<br>
+ pcisrc->addr.slot,
pcisrc->addr.function);
<br>
+
<br>
</blockquote>
<br>
The daemon will crash if this function returns NULL. There should
be
<br>
check for that, but it shouldn't be fatal, so we can clean up as
much
<br>
as possible (see Loop 3 for example on how to handle that).
<br>
</blockquote>
Is this OK? <br>
if (dev) {<br>
if (virPCIDeviceListFind(pcidevs, dev)) {<br>
virHostdevNetConfigRestore(hostdev,
hostdev_mgr->stateDir,<br>
oldStateDir);<br>
}<br>
} else {<br>
virErrorPtr err = virGetLastError();<br>
VIR_ERROR(_("Failed to new PCI device: %s"),<br>
err ? err->message : _("unknown
error"));<br>
virResetError(err);<br>
}<br>
virPCIDeviceFree(dev);<br>
<br>
<blockquote type="cite">
<blockquote type="cite">+ if
(virPCIDeviceListFind(pcidevs, dev)) {
<br>
+ virHostdevNetConfigRestore(hostdev,
hostdev_mgr->stateDir,
<br>
+ oldStateDir);
<br>
+ }
<br>
+ virPCIDeviceFree(dev);
<br>
+ }
<br>
+ }
<br>
+
<br>
+ /* Loop 3: reset pci device used by this domain */
<br>
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
<br>
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
<br>
<br>
@@ -834,6 +853,9 @@
virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
<br>
}
<br>
}
<br>
<br>
+ /* Loop 4: reattach pci devices used by this domain
<br>
+ * and steal all the devices from pcidevs
<br>
+ */
<br>
while (virPCIDeviceListCount(pcidevs) > 0) {
<br>
virPCIDevicePtr dev =
virPCIDeviceListStealIndex(pcidevs, 0);
<br>
virHostdevReattachPCIDevice(dev, hostdev_mgr);
<br>
--
<br>
1.9.1
<br>
<br>
</blockquote>
<br>
Other than that, the patch looks fine.
<br>
<br>
Martin
<br>
</blockquote>
<br>
</div>
</div>