Re: [libvirt] [PATCH V4 4/6] libxl: support creating domain with VF assignment from a pool

>>> On 3/29/2016 at 08:05 AM, in message <56F9C6D0.5000702(a)suse.com&gt;, Jim Fehlig <jfehlig(a)suse.com&gt; 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(a)suse.com&gt; >> --- >> 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.

> 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); + }

> 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.