<div dir="ltr"><br><div><div><br><div><div><div class="gmail_extra"><div class="gmail_quote">2015-04-02 1:35 GMT+08:00 Laine Stump <span dir="ltr"><<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<div>On 04/01/2015 12:15 PM, Huanle Han
wrote:<br>
</div>
<blockquote type="cite">
<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>
</blockquote>
</div>
</div>
</blockquote>
<br></span>
Someone else encountered this last month too, and posted a patch:<br>
<br>
<a href="https://www.redhat.com/archives/libvir-list/2015-February/msg00394.html" target="_blank">https://www.redhat.com/archives/libvir-list/2015-February/msg00394.html</a><br>
<br>
The patch was NACKed because I didn't think the fix was correct
(although it did work)<br>
<br>
<a href="https://www.redhat.com/archives/libvir-list/2015-February/msg00976.html" target="_blank">https://www.redhat.com/archives/libvir-list/2015-February/msg00976.html</a><br>
<br>
I don't have the time to do a close analysis of this patch right
now, but from the comments it sounds like it addresses the concern
that I had with the previous patch (that it wouldn't be messing with
any devices that were currently not in use by any domain, and hadn't
yet been reached in the setup part). I wanted to point that out now
so that whoever looks at the next version of this patch checks for
that.<br>
<br></div></blockquote><div><div><div>active virPCIDevice has member variable 'used_by_drvname' and
'used_by_domname' to mark its user. According to
’virHostdevPreparePCIDevices‘, only the pci is successfully prepared,
will the member 'used_by_XXX' be assigned.<br><br></div><div>The domain PCI state when reattach:<br></div><div>1.
If qemuProcessStart() fails before pci
setup(virHostdevPreparePCIDevices()), the pci will either be not active,
or be active but used by others.<br>2. if qemuProcessStart() fails
during pci setup , virHostdevPreparePCIDevices rollbacks the change. The
result will be the same with 1. <br></div><div>3. if qemuProcessStart() fails after pci setup, the pci will be active and used by this domain.<br><br></div><div>In fucntion virHostdevReAttachPCIDevices,<br>1. pcidevs is from virHostdevGetActivePCIHostDeviceList(). That is say,
pci in pcidevs is active ( used/prepared by any domains). <br>2.
In loop 1, the pci used by
other domains is removed from the pcidevs. After loop, all pcis in
pcidevs are used/prepared by this domain. <br>3. The following reattach operation is on these 'pcidevs', not the pci used by nobody or other domains.<br></div><div><br></div>So I don't think the problem you said exists. This solution is OK.<br></div>Am I right? <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<br>
<blockquote type="cite"><div><div class="h5">
<div dir="ltr">
<div bgcolor="#FFFFFF" text="#000000">
<blockquote type="cite"> <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>
<br>
<fieldset></fieldset>
<br>
</div></div><span class=""><font color="#888888"><pre>--
libvir-list mailing list
<a href="mailto:libvir-list@redhat.com" target="_blank">libvir-list@redhat.com</a>
<a href="https://www.redhat.com/mailman/listinfo/libvir-list" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a></pre>
</font></span></blockquote>
<br>
</div>
</blockquote></div><br></div></div></div></div></div></div>