What is described seems reasonable; however, I'm hoping Laine could
provide some context here as to whether the change is good since he's
the original author of the change (see commit id '7a600cf7'). I've added
him to the CC list....
On 02/12/2015 04:17 AM, Zhang Bo wrote:
> If we assign a VF, which has already been used by an active guest, to another
> guest, and try to start the 2nd guest later on, the 2nd guest would not
> start, and the VF won't work anymore.
>
> Steps to reproduce the problem:
> 1 Assign a VF to guest A, and start the guest. The VF works fine.
> 2 Assign the VF to guest B, and try to start guest B. guest B can't start.
> 3 Guest A's network becomes unreachable, because its VF now doesn't work.
>
> Reasons for this problem is:
> 1 When we start guest B, libvirtd checks whether the VF is already used
> by another guest, if so, qemuPrepareHostDevices() returns with failure.
> 2 Then, libvirtd calls qemuProcessStop() to cleanup resourses, which would
s/resourses/resources
> restore the VFs of guest B. Specifically, it reads
> /var/run/libvirt/hostdevmgr/ethX_vfX to get the VF's original MAC/VLAN,
> and set it back to current VF.
> 3 As that the VF is still in use by guest A, libvirtd just set its MAC/VLAN
> to another value, the VF doesn't work anymore.
>
> Detailed flow:
> qemuProcessStart
> \___qemuPrepareHostDevices(if it fails, goto cleanup)
> \ \_qemuPrepareHostdevPCIDevices
> \ \_____virHostdevPreparePCIDevices
> \ \____LOOP1:virPCIDeviceListFind
> \ (whether the device is in use by another active guest)
> \ if the VF has been assigned to, qemuPrepareHostDevices() fails
> \___cleanup:
> \__qemuProcessStop
> \____qemuDomainReAttachHostDevices
> \____qemuDomainReAttachHostdevDevices
> \____virHostdevReAttachPCIDevices
> \_____virHostdevNetConfigRestore
> (it gets MAC/VLAN form /var/run/libvirt/hostdevmgr/ethX_vfX,
> and set it back to the VF, making the VF unusable)
>
> This patch checks whether the VF is already in use before restoring it.
>
> Signed-off-by: Zhang Bo <oscar.zhangbo@xxxxxxxxxx>
> Signed-off-by: Zhuang Yanying <zhuangyanying@xxxxxxxxxx>
> ---
> src/util/virhostdev.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 9678e2b..ee19400 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -816,9 +816,21 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
> * For SRIOV net host devices, unset mac and port profile before
> * reset and reattach device
> */
> - for (i = 0; i < nhostdevs; i++)
> + for (i = 0; i < nhostdevs; i++){
^
Need a space
> + virPCIDevicePtr devNotInuse = NULL;
> + virDevicePCIAddressPtr addr = NULL;
> + virDomainHostdevDefPtr hostdev = hostdevs[i];
> + addr = &hostdev->source.subsys.u.pci.addr;
> + devNotInuse = virPCIDeviceListFindByIDs(pcidevs,
> + addr->domain, addr->bus,
> + addr->slot, addr->function);
> + if (!devNotInuse) {
> + continue;
> + }
No need for the { } here.
Use of TAB's not SPACE's for all new code
These would have been resolved if you had used 'make syntax-check'
before sending.
> +
> virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir,
Could have gone with just hostdev here as P1
John
> oldStateDir);
> + }
>
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list