On 03/30/2016 03:38 AM, Chun Yan Liu wrote:

...

...

...

On 3/29/2016 at 08:05 AM, in message <56F9C6D0.5000702@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: On 03/21/2016 02:11 AM, Chunyan Liu wrote: Add codes to support creating domain with network defition of assigning SRIOV VF from a pool.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_domain.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 3 +++ 2 files changed, 53 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c8d09b1..d11bf3a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -36,6 +36,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "xen_common.h" +#include "network/bridge_driver.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -764,6 +765,10 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, if (net->ifname && STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); + + /* cleanup actual device */ + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(vm->def, net); } }

@@ -960,6 +965,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) } }

+static int +libxlNetworkPrepareDevices(virDomainDefPtr def) +{ + int ret = -1; + size_t i; + + for (i = 0; i < def->nnets; i++) { + virDomainNetDefPtr net = def->nets[i]; + int actualType; + + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (networkAllocateActualDevice(def, net) < 0) + goto cleanup; + + actualType = virDomainNetGetActualType(net); + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + /* Each type='hostdev' network device must also have a + * corresponding entry in the hostdevs array. For netdevs + * that are hardcoded as type='hostdev', this is already + * done by the parser, but for those allocated from a + * network / determined at runtime, we need to do it + * separately. + */ + virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net); + virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; + + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; + + if (virDomainHostdevInsert(def, hostdev) < 0) + goto cleanup; + } + } + ret = 0; + cleanup: + return ret;

Nothing to cleanup here. I think we should just return -1 on failure paths and 0 on success.

...

+}

/* * Start a domain through libxenlight. @@ -1036,6 +1083,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, vm, true) < 0) goto cleanup;

+ if (libxlNetworkPrepareDevices(vm->def) < 0) + goto cleanup;

I accessed a machine to test this series and have found a few problems.

1. When attaching an SR-IOV vf from a pool, the attach appears successful. I see that xen's pciback driver is bound to the device in the host. I didn't see the device in the guest (could be a guest problem), For pv guest, from testing, it is shown in guest. For HVM, seems xl pci-attach has the same result.

Yes, I'm seeing the same thing. Looks like a bug in libxl or qemu.

...

nor in dumpxml or /var/run/libvirt/libxl/<dom-name>.xml. Ah, there is a mistake during a review change to original patch 1/6. Didn't notice that earlier.

In libxlDomainAttachNetDevice: The if (!ret) is needed, maybe cleanup is not proper. Since for actual type is hostdev case, after a successful libxlDomainAttachHostDevice, we need to update vm->def->nets too.

cleanup: libxl_device_nic_dispose(&nic); - out: - if (!ret) + if (!ret) { vm->def->nets[vm->def->nnets++] = net; + } else { + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(vm->def, net); + }

Similar for libxDomainDetachNetDevice cleanup: Original is still needed.

cleanup: libxl_device_nic_dispose(&nic); - if (!ret) + if (!ret) { + networkReleaseActualDevice(vm->def, detach); virDomainNetRemove(vm->def, detachidx); + }

Yikes! I should have looked at the code closer before making those changes - lesson learned. I sent a small series to fix it https://www.redhat.com/archives/libvir-list/2016-March/msg01470.html

...

Not surprisingly, this causes problems with device-detach

# virsh detach-device dom sriov-pool-vif.xml error: Failed to detach device from sriov-pool-vif.xml error: operation failed: no device matching mac address 00:16:3e:7a:35:df found

2. After starting a domain containing an interface from sr-iov vf pool, the interface xml is changed from type='network' to type='hostdev'. E.g. before creating domain

<interface type='network'> <source network='passthrough'/> <mac address='00:16:3e:7a:35:df'/> </interface>

after creating domain

<interface type='hostdev' managed='yes'> <mac address='00:16:3e:7a:35:df'/> <driver name='xen'/> <source> <address type='pci' domain='0x0000' bus='0x0a' slot='0x11' function='0x1'/> </source> </interface>

Maybe this is intended behavior, but it seems odd. This is intended.

...

3. I started a domain containing an interface from sr-iov vf pool. Looks good. I then tried 'virsh detach-device ...', which never returned until keepalive timeout. The device was removed from the guest, but libvirtd got hung up in the process. Oddly, I wasn't able to interrupt libvirtd with gdb to see what its threads were doing.

This is libxl problem. With xl pci-detach, it has the same issue.

Yep, another bug in xen-unstable libxl. With the small fixes in the series I mentioned above, this patch looks and tests good :-). Since it has been around for quite some time, I've asked for permission to commit it even though we are in 1.3.3 freeze. Regards, Jim