[libvirt] [PATCH 0/2] qemu: support setting vlan tag for <interface
This would have been just one small patch, but there was a deficiency in the low level functions to save and restore pre-existing state of a virtual function. I have tested these patches on a RHEL6 machine with an SR-IOV adapter, and it properly handled the save/set .. [attach-to-guest] .. [detach] .. restore cycle both when the original state of the VF had a vlan tag and not, and when the guest was requesting a vlan tag, and not. (Now I'm wondering if this would also work for macvtap "passthrough" mode when the adapter used is a VF - it looks like the only thing stopping it is that the code to save / set / restore in that mode uses the device name of the vf, rather than referring to it as <physical device> vf <n>; since only VFs understand setting the vlan tag, the lower level code just assumes it can't be done)
When a network device that is a VF of an SR-IOV card was assigned to a guest using <interface type='hostdev'>, only the MAC address was being saved/restored, but the VLAN tag was left untouched. Up to now we haven't actually used vlan tags on SR-IOV devices, so the guest would have used whatever was set, and left it the same at the end. The patch following this one will hook up the <vlan> element from the interface config, so save/restore of the device state needs to also include the vlan tag. MAC address is being saved as a simple ASCII string in a file named for the device under /var/run. The VLAN tag is now just added at the end of that file, after a newline. It might be nicer if the file was XML (in case it ever gets more complicated) but at the moment there's nothing else on the horizon, and this makes backward compatibility easier. --- src/util/virnetdev.c | 56 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index f1ee0a4..25bdf01 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1533,33 +1533,41 @@ virNetDevReplaceVfConfig(const char *pflinkdev, int vf, int vlanid, const char *stateDir) { + int ret = -1; virMacAddr oldmac; int oldvlanid = -1; char *path = NULL; char macstr[VIR_MAC_STRING_BUFLEN]; + char *fileData = NULL; int ifindex = -1; if (virNetDevGetVfConfig(pflinkdev, vf, &oldmac, &oldvlanid) < 0) - return -1; + goto cleanup; if (virAsprintf(&path, "%s/%s_vf%d", stateDir, pflinkdev, vf) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - virMacAddrFormat(&oldmac, macstr); - if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) { - virReportSystemError(errno, _("Unable to preserve mac for pf = %s," - " vf = %d"), pflinkdev, vf); - VIR_FREE(path); - return -1; + if (virAsprintf(&fileData, "%s\n%d\n", + virMacAddrFormat(&oldmac, macstr), oldvlanid) < 0) { + virReportOOMError(); + goto cleanup; + } + if (virFileWriteStr(path, fileData, O_CREAT|O_TRUNC|O_WRONLY) < 0) { + virReportSystemError(errno, _("Unable to preserve mac/vlan tag " + "for pf = %s, vf = %d"), pflinkdev, vf); + goto cleanup; } - VIR_FREE(path); - - return virNetDevSetVfConfig(pflinkdev, ifindex, vf, true, + ret = virNetDevSetVfConfig(pflinkdev, ifindex, vf, true, macaddress, vlanid, NULL); + +cleanup: + VIR_FREE(path); + VIR_FREE(fileData); + return ret; } static int @@ -1567,8 +1575,9 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf, const char *stateDir) { int rc = -1; - char *macstr = NULL; char *path = NULL; + char *fileData = NULL; + char *vlan = NULL; virMacAddr oldmac; int vlanid = -1; int ifindex = -1; @@ -1579,14 +1588,29 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf, return rc; } - if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0) { + if (virFileReadAll(path, 128, &fileData) < 0) { goto cleanup; } - if (virMacAddrParse(macstr, &oldmac) != 0) { + if ((vlan = strchr(fileData, '\n'))) { + char *endptr; + + *vlan++ = 0; /* NULL terminate the mac address */ + if (*vlan) { + if ((virStrToLong_i(vlan, &endptr, 10, &vlanid) < 0) || + (endptr && *endptr != '\n' && *endptr != 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse vlan tag from '%s'"), + vlan); + goto cleanup; + } + } + } + + if (virMacAddrParse(fileData, &oldmac) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse MAC address from '%s'"), - macstr); + fileData); goto cleanup; } @@ -1597,7 +1621,7 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf, cleanup: VIR_FREE(path); - VIR_FREE(macstr); + VIR_FREE(fileData); return rc; } -- 1.7.11.4
On 2012年08月16日 12:34, Laine Stump wrote:
When a network device that is a VF of an SR-IOV card was assigned to a guest using<interface type='hostdev'>, only the MAC address was being saved/restored, but the VLAN tag was left untouched. Up to now we haven't actually used vlan tags on SR-IOV devices, so the guest would have used whatever was set, and left it the same at the end.
The patch following this one will hook up the<vlan> element from the interface config, so save/restore of the device state needs to also include the vlan tag.
MAC address is being saved as a simple ASCII string in a file named for the device under /var/run. The VLAN tag is now just added at the end of that file, after a newline. It might be nicer if the file was XML (in case it ever gets more complicated) but at the moment there's nothing else on the horizon, and this makes backward compatibility easier. --- src/util/virnetdev.c | 56 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index f1ee0a4..25bdf01 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1533,33 +1533,41 @@ virNetDevReplaceVfConfig(const char *pflinkdev, int vf, int vlanid, const char *stateDir) { + int ret = -1; virMacAddr oldmac; int oldvlanid = -1; char *path = NULL; char macstr[VIR_MAC_STRING_BUFLEN]; + char *fileData = NULL; int ifindex = -1;
if (virNetDevGetVfConfig(pflinkdev, vf,&oldmac,&oldvlanid)< 0) - return -1; + goto cleanup;
if (virAsprintf(&path, "%s/%s_vf%d", stateDir, pflinkdev, vf)< 0) { virReportOOMError(); - return -1; + goto cleanup; }
- virMacAddrFormat(&oldmac, macstr); - if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY)< 0) { - virReportSystemError(errno, _("Unable to preserve mac for pf = %s," - " vf = %d"), pflinkdev, vf); - VIR_FREE(path); - return -1; + if (virAsprintf(&fileData, "%s\n%d\n", + virMacAddrFormat(&oldmac, macstr), oldvlanid)< 0) { + virReportOOMError(); + goto cleanup; + } + if (virFileWriteStr(path, fileData, O_CREAT|O_TRUNC|O_WRONLY)< 0) { + virReportSystemError(errno, _("Unable to preserve mac/vlan tag " + "for pf = %s, vf = %d"), pflinkdev, vf); + goto cleanup; }
- VIR_FREE(path); - - return virNetDevSetVfConfig(pflinkdev, ifindex, vf, true, + ret = virNetDevSetVfConfig(pflinkdev, ifindex, vf, true, macaddress, vlanid, NULL); + +cleanup: + VIR_FREE(path); + VIR_FREE(fileData); + return ret; }
static int @@ -1567,8 +1575,9 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf, const char *stateDir) { int rc = -1; - char *macstr = NULL; char *path = NULL; + char *fileData = NULL; + char *vlan = NULL; virMacAddr oldmac; int vlanid = -1; int ifindex = -1; @@ -1579,14 +1588,29 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf, return rc; }
- if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN,&macstr)< 0) { + if (virFileReadAll(path, 128,&fileData)< 0) { goto cleanup; }
- if (virMacAddrParse(macstr,&oldmac) != 0) { + if ((vlan = strchr(fileData, '\n'))) { + char *endptr; + + *vlan++ = 0; /* NULL terminate the mac address */
s/terminate/terminates/
+ if (*vlan) { + if ((virStrToLong_i(vlan,&endptr, 10,&vlanid)< 0) || + (endptr&& *endptr != '\n'&& *endptr != 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse vlan tag from '%s'"), + vlan); + goto cleanup; + } + } + } + + if (virMacAddrParse(fileData,&oldmac) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse MAC address from '%s'"), - macstr); + fileData); goto cleanup; }
@@ -1597,7 +1621,7 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf,
cleanup: VIR_FREE(path); - VIR_FREE(macstr); + VIR_FREE(fileData);
return rc; }
Looks good for me, ACK.
On 08/16/2012 02:34 AM, Osier Yang wrote:
- if (virMacAddrParse(macstr,&oldmac) != 0) { + if ((vlan = strchr(fileData, '\n'))) { + char *endptr; + + *vlan++ = 0; /* NULL terminate the mac address */
s/terminate/terminates/
Actually, 'terminate' is correct - this is a comment describing what the assignment is good for (convert arbitrary contents into 0 in order to NUL-terminate the vlan variable), not the vlan format itself (a valid vlan exists when NUL terminates the string). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The underlying function to set the vlan tag of an SR-IOV network device was already in place (although an extra patch to save/restore the original vlan tag was needed), and recent patches added the ability to configure a vlan tag. This patch just ties those two together. An SR-IOV device doesn't support vlan trunking, so if anyone tries to configure more than a single tag, or set the trunk flag, and error is logged. --- src/qemu/qemu_hostdev.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 7619fd0..46c84b5 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -301,6 +301,7 @@ qemuDomainHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, char *stateDir) { char *linkdev = NULL; + virNetDevVlanPtr vlan; virNetDevVPortProfilePtr virtPort; int ret = -1; int vf = -1; @@ -319,19 +320,46 @@ qemuDomainHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, if (qemuDomainHostdevNetDevice(hostdev, &linkdev, &vf) < 0) return ret; + vlan = virDomainNetGetActualVlan(hostdev->parent.data.net); virtPort = virDomainNetGetActualVirtPortProfile( hostdev->parent.data.net); - if (virtPort) + if (virtPort) { + if (vlan) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("direct setting of the vlan tag is not allowed " + "for hostdev devices using %s mode"), + virNetDevVPortTypeToString(virtPort->virtPortType)); + goto cleanup; + } ret = qemuDomainHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort, &hostdev->parent.data.net->mac, uuid, port_profile_associate); - else - /* Set only mac */ + } else { + /* Set only mac and vlan */ + if (vlan) { + if (vlan->nTags != 1 || vlan->trunk) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("vlan trunking is not supported " + "by SR-IOV network devices")); + goto cleanup; + } + if (vf == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("vlan can only be set for SR-IOV VFs, but " + "%s is not a VF"), linkdev); + goto cleanup; + } + vlanid = vlan->tag[0]; + } else if (vf >= 0) { + vlanid = 0; /* assure any current vlan tag is reset */ + } + ret = virNetDevReplaceNetConfig(linkdev, vf, - &hostdev->parent.data.net->mac, vlanid, - stateDir); + &hostdev->parent.data.net->mac, + vlanid, stateDir); + } +cleanup: VIR_FREE(linkdev); - return ret; } -- 1.7.11.4
On 2012年08月16日 12:34, Laine Stump wrote:
The underlying function to set the vlan tag of an SR-IOV network device was already in place (although an extra patch to save/restore the original vlan tag was needed), and recent patches added the ability to configure a vlan tag. This patch just ties those two together.
An SR-IOV device doesn't support vlan trunking, so if anyone tries to configure more than a single tag, or set the trunk flag, and error is logged. --- src/qemu/qemu_hostdev.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 7619fd0..46c84b5 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -301,6 +301,7 @@ qemuDomainHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, char *stateDir) { char *linkdev = NULL; + virNetDevVlanPtr vlan; virNetDevVPortProfilePtr virtPort; int ret = -1; int vf = -1; @@ -319,19 +320,46 @@ qemuDomainHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, if (qemuDomainHostdevNetDevice(hostdev,&linkdev,&vf)< 0) return ret;
+ vlan = virDomainNetGetActualVlan(hostdev->parent.data.net); virtPort = virDomainNetGetActualVirtPortProfile( hostdev->parent.data.net); - if (virtPort) + if (virtPort) { + if (vlan) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("direct setting of the vlan tag is not allowed " + "for hostdev devices using %s mode"), + virNetDevVPortTypeToString(virtPort->virtPortType)); + goto cleanup; + } ret = qemuDomainHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort,&hostdev->parent.data.net->mac, uuid, port_profile_associate); - else - /* Set only mac */ + } else { + /* Set only mac and vlan */ + if (vlan) { + if (vlan->nTags != 1 || vlan->trunk) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("vlan trunking is not supported " + "by SR-IOV network devices")); + goto cleanup; + } + if (vf == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("vlan can only be set for SR-IOV VFs, but " + "%s is not a VF"), linkdev); + goto cleanup; + } + vlanid = vlan->tag[0]; + } else if (vf>= 0) { + vlanid = 0; /* assure any current vlan tag is reset */ + } + ret = virNetDevReplaceNetConfig(linkdev, vf, -&hostdev->parent.data.net->mac, vlanid, - stateDir); +&hostdev->parent.data.net->mac, + vlanid, stateDir); + } +cleanup: VIR_FREE(linkdev); - return ret; }
ACK.
participants (3)
-
Eric Blake -
Laine Stump -
Osier Yang