Re: [libvirt] [PATCH] qemu: set macvtap physdevs online when macvtap is set online

On 04/15/2015 02:59 PM, John Ferlan wrote: > On 04/13/2015 01:44 PM, Laine Stump wrote: >> A further fix for: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1113474 >> >> Since there is no possibility that any type of macvtap will work if >> the parent physdev it's attached to is offline, we should bring the >> physdev online at the same time as the macvtap. When taking the >> macvtap offline, it's also necessary to take the physdev offline for >> macvtap passthrough mode (because the physdev has the same MAC address >> as the macvtap device, so could potentially cause problems with >> misdirected packets during migration, as outlined in commits 829770 >> and 879c13). We can't set the physdev offline for other macvtap modes >> 1) because there may be other macvtap devices attached to the same >> physdev in the other modes whereas passthrough mode is exclusive to >> one macvtap at a time, and 2) there's no practical reason to do so >> anyway. >> --- >> src/qemu/qemu_hotplug.c | 8 +++----- >> src/qemu/qemu_interface.c | 29 +++++++++++++++++++++++++++-- >> 2 files changed, 30 insertions(+), 7 deletions(-) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 2f0549e..9be2ea3 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -3931,11 +3931,9 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, >> VIR_WARN("cannot clear bandwidth setting for device : %s", >> detach->ifname); >> >> - /* deactivate the tap/macvtap device on the host (currently this >> - * isn't necessary, as everything done in >> - * qemuInterfaceStopDevice() is made meaningless when the device >> - * is deleted anyway, but in the future it may be important, and >> - * doesn't hurt anything for now) >> + /* deactivate the tap/macvtap device on the host, which could also >> + * affect the parent device (e.g. macvtap passthrough mode sets >> + * the parent device offline) >> */ >> ignore_value(qemuInterfaceStopDevice(detach)); >> >> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c >> index 201a7dd..01226ac 100644 >> --- a/src/qemu/qemu_interface.c >> +++ b/src/qemu/qemu_interface.c >> @@ -63,7 +63,20 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net) >> goto cleanup; >> } >> break; >> - case VIR_DOMAIN_NET_TYPE_DIRECT: >> + >> + case VIR_DOMAIN_NET_TYPE_DIRECT: { >> + const char *physdev = virDomainNetGetActualDirectDev(net); >> + bool isOnline = true; > Seems unnecessary to set = true, since virNetDevGetIFFlag (what > virNetDevGetOnline calls) will set either True or False on success. This is some defensive programming to protect against physdev being NULL (which *should* be impossible for type='direct', but not verifiably so without a much larger context). The idea is that I never call virNetDevGetOnline at all unless physdev != NULL. So if physdev was NULL, I want isOnline to be set to true so I won't try to "virNetDevSetOnline(NULL, true);". If I was more trustful of the code (much of which was probably written or reviewed by me) I wouldn't need this. > Doesn't hurt... Your call though. > >> + >> + /* set the physdev online if necessary. It may already be up, >> + * in which case we shouldn't re-up it just in case that causes >> + * some sort of "blip" in the physdev's status. >> + */ >> + if (physdev && virNetDevGetOnline(physdev, &isOnline) < 0) >> + goto cleanup; >> + if (!isOnline && virNetDevSetOnline(physdev, true) < 0) >> + goto cleanup; >> + > No issue putting it online in order for us to use it; however, if it is > online before we got here, shouldn't we know that for Stop? Yes and no. On one hand, it's nice to put everything back exactly how we found it. On the other, when an interface is being used for either hostdev passthrough or macvtap passthrough, it can't be used for anything else, so we know that at the time we started using it it was unused, and immediately after we're finished it will be unused, and ifup'ing an interface that isn't in use only leads to grief (for example, I believe certain versions of NetworkManager like to notice things like this and start up dhclient on the interface). Also there is the hostdev passthrough usage to consider - in that case, we detach the device from the host net driver, then reattach it; when we reattach it always ends up offline, so this behavior in macvtap passthrough is no different. I considered modifying the replace/restoreconfig functions to save and restore the online status of the interface so that all types of networking use would terminate with the device in exactly the same state in which it was found. Unfortunately, when doing hostdev passthrough the replace/restoreconfig functions are both called while the VF is detached from the host driver, and although there is an IFLA_VF_LINK_STATE in the IFLA_VF_INFO stuff, it is only implemented for a small subset of drivers (not including the most common ones - igbvf and ixgbvf) and it can only be used to *set* the state, but not to get it. So that's out. Another thing that I discovered while looking into this is that the other macvtap modes simply don't work at all on a VF; the VF driver prevents promiscuous mode (supposedly for security reasons), but the other macvtap modes don't work properly without it. And a VF isn't really very useful as a host netdev either - you don't get any extra bandwidth, for example, because it's using the same physical port as the PF. So if you want to use macvtap bridge or private mode, you need to just use the PF (I still haven't figured out vepa/802.1Qbg). >> /* macvtap devices share their MAC address with the guest >> * domain, and if they are set online prior to the domain CPUs >> * being started, the host may send out traffic from this >> @@ -79,6 +92,7 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net) >> if (virNetDevSetOnline(net->ifname, true) < 0) >> goto cleanup; >> break; >> + } >> >> case VIR_DOMAIN_NET_TYPE_USER: >> case VIR_DOMAIN_NET_TYPE_ETHERNET: >> @@ -146,7 +160,9 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net) >> } >> break; >> >> - case VIR_DOMAIN_NET_TYPE_DIRECT: >> + case VIR_DOMAIN_NET_TYPE_DIRECT: { >> + const char *physdev = virDomainNetGetActualDirectDev(net); >> + >> /* macvtap interfaces need to be marked !IFF_UP (ie "down") to >> * prevent any host-generated traffic sent from this interface >> * from putting bad info into the arp caches of other machines >> @@ -154,7 +170,16 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net) >> */ >> if (virNetDevSetOnline(net->ifname, false) < 0) >> goto cleanup; >> + >> + /* also mark the physdev down for passthrough macvtap, as the >> + * physdev has the same MAC address as the macvtap device. >> + */ >> + if (virDomainNetGetActualDirectMode(net) == >> + VIR_NETDEV_MACVLAN_MODE_PASSTHRU && >> + physdev && virNetDevSetOnline(physdev, false) < 0) >> + goto cleanup; > Because if we were online at Start and didn't need to put it online, but > then this code puts it offline, what affect does that have on something > else that may have been relying on it being online... If there was something the host was using the interface for before we started using it, it's going to have to be setup again when we're done, so it will require some other intervention anyway, and the device could be put online at that time. How about this - I'll make a separate patch that modifies the replace/restore config functions to save and restore the online status of the VF only for the cases that we can also access it directly as a netdev. Then it will be preserved properly for the cases when we are able. I want to make it a separate patch so that it can be cleanly reverted if we later decide that is actually a *bad* thing to do, though.

> John >> break; >> + } >> >> case VIR_DOMAIN_NET_TYPE_USER: >> case VIR_DOMAIN_NET_TYPE_ETHERNET: >>