On Fri, 2010-05-21 at 16:46 -0700, Chris Wright wrote:
* Stefan Berger (stefanb@linux.vnet.ibm.com) wrote:
Below is David Alan's original patch with lots of changes.
In particular, it now parses the following two XML descriptions, one for 802.1Qbg and 802.1Qbh and stored the data internally. The actual triggering of the switch setup protocol has not been implemented here but the relevant code to do that should go into the functions associatePortProfileId() and disassociatePortProfileId().
For future work, have you thought about preassociate on migration?
You mean first to do a preassociate and then after migration has succeeded a (full) associate. I had thought about it. I believe the current code would need to know what the actual cause was for it being called. We currently handle it I believe correctly for - 'direct' type of interface creation and destruction - resume after a suspend Now if we wanted to pre-associate on migration on the destination host, we would need to know that the function was called due to a migration operation in order not to call the associate function but rather the pre-associate one. The I believe upon failure of migration the disassociate would still be call-able on the target host, but in addition we would need to call the associate function after the migration was successful. We're not doing this. I suppose also that nothing would be allowed to go wrong once one switches from pre-associate to associate in such a scenario since the migration succeeded and a failure to associate couldn't be handled gracefully anymore (by failing migration). [...]
Some of the changes from V1 to V2: - parser and XML generator have been separated into their own functions so they can be re-used elsewhere (passthrough case for example) - Adapted XML parser and generator support the above shown type (802.1Qbg, 802.1Qbh). - Adapted schema to above XML - Adapted test XML to above XML - Passing through the VM's UUID which seems to be necessary for 802.1Qbh -- sorry no host UUID - adding virtual function ID to association function, in case it's necessary to use (for SR-IOV)
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
This is looking like pretty complete infrastructure to me. Have you been able generate test (dis)associate messages through this framework w/ the vsi patches? Pending that (I can see you are able to at least generate debug messages showing the thing work), and some minor nits below.
I have been able to communicate with a self-hacked dummy server for 802.1Qbg that receives a netlink message and responds with a netlink message via unicast to libvirt. With that server I am currently sending the response immediately to the RTM_SETLINK message, which I believe isn't what the kernel is doing. Rather it should send it back upon RTM_GETLINK. If that server is missing or indicates failure to start, I am timing out and fail the start of the VM. The disassociate part is in the path of the tear down code and is being invoked as well. I haven't tested much with that for 802.1Qbg.
Acked-by: Chris Wright <chrisw@redhat.com>
From a945107f047c7cd71f9c1b74fd74c47d8cdc3670 Mon Sep 17 00:00:00 2001 From: David Allan <dallan@redhat.com> Date: Fri, 12 Mar 2010 13:25:04 -0500 Subject: [PATCH 1/1] POC of port profile id support
* Modified schema per DanPB's feedback * Added test for modified schema --- docs/schemas/domain.rng | 69 ++++++++++++++ src/conf/domain_conf.c | 155 +++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 35 +++++++ src/qemu/qemu_conf.c | 18 +-- src/qemu/qemu_conf.h | 5 - src/qemu/qemu_driver.c | 17 +-- src/util/macvtap.c | 151 +++++++++++++++++++++++++++----- src/util/macvtap.h | 10 +- tests/domainschemadata/portprofile.xml | 36 +++++++ 9 files changed, 446 insertions(+), 50 deletions(-) create mode 100644 tests/domainschemadata/portprofile.xml
Index: libvirt-acl/src/conf/domain_conf.c =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.c +++ libvirt-acl/src/conf/domain_conf.c @@ -242,6 +242,11 @@ VIR_ENUM_IMPL(virDomainNetdevMacvtap, VI "private", "bridge")
+VIR_ENUM_IMPL(virVirtualPort, VIR_VIRTUALPORT_TYPE_LAST, + "none", + "802.1Qbg", + "802.1Qbh") + VIR_ENUM_IMPL(virDomainClockOffset, VIR_DOMAIN_CLOCK_OFFSET_LAST, "utc", "localtime", @@ -1807,6 +1812,145 @@ cleanup: }
+static void
static int so we can capture an error?
Ok.
+virVirtualPortProfileDefParseXML(xmlNodePtr node, + virVirtualPortProfileDefPtr virtPort) +{ + char *virtPortType; + char *virtPortManagerID = NULL; + char *virtPortTypeID = NULL; + char *virtPortTypeIDVersion = NULL; + char *virtPortInstanceID = NULL; + char *virtPortProfileID = NULL; + xmlNodePtr cur = node->children;
int ret = -1;
+ + virtPortType = virXMLPropString(node, "type"); + if (!virtPortType) + return; + + while (cur != NULL) { + if (xmlStrEqual(cur->name, BAD_CAST "parameters")) { + + virtPortManagerID = virXMLPropString(cur, "managerid"); + virtPortTypeID = virXMLPropString(cur, "typeid"); + virtPortTypeIDVersion = virXMLPropString(cur, "typeidversion"); + virtPortInstanceID = virXMLPropString(cur, "instanceid"); + virtPortProfileID = virXMLPropString(cur, "profileid"); + + break;
s/break/goto out/
+ } + + cur = cur->next; + } + + virtPort->virtPortType = VIR_VIRTUALPORT_NONE; + + switch (virVirtualPortTypeFromString(virtPortType)) { + + case VIR_VIRTUALPORT_8021QBG: + if (virtPortManagerID != NULL && virtPortTypeID != NULL && + virtPortTypeIDVersion != NULL) { + unsigned int val; + + if ((virStrToLong_ui(virtPortManagerID, NULL, 10, &val) && + virStrToLong_ui(virtPortManagerID, NULL, 16, &val) ) ||
Why not a baseless strtol (0 instead of 10 and 16)? And I think this should give some virDomainReportError() so that we can figure out what's happening when the xml is incorrect.
Done.
Ditto for parsing the whole thing...
+ val > 0xff) + break;
s/break/goto out/
Ok.
+ + virtPort->u.virtPort8021Qbg.managerID = (uint8_t)val; + + if ((virStrToLong_ui(virtPortTypeID, NULL, 10, &val) && + virStrToLong_ui(virtPortTypeID, NULL, 16, &val) ) || + val > 0xffffff) + break; + + virtPort->u.virtPort8021Qbg.typeID = (uint32_t)val; + + if ((virStrToLong_ui(virtPortTypeIDVersion, NULL, 10, &val) && + virStrToLong_ui(virtPortTypeIDVersion, NULL, 16, &val) ) || + val > 0xff) + break; + + virtPort->u.virtPort8021Qbg.typeIDVersion = (uint8_t)val; + + if (virtPortInstanceID != NULL) { + if (virUUIDParse(virtPortInstanceID, virtPort->u.virtPort8021Qbg.instanceID)) + break; + } else { + if (virUUIDGenerate(virtPort->u.virtPort8021Qbg.instanceID)) + break; + } + + virtPort->virtPortType = VIR_VIRTUALPORT_8021QBG; + } + break; + + case VIR_VIRTUALPORT_8021QBH: + if (virtPortProfileID != NULL) { + if (virStrcpyStatic(virtPort->u.virtPort8021Qbh.profileID, + virtPortProfileID) != NULL) + virtPort->virtPortType = VIR_VIRTUALPORT_8021QBH; + } + break; + + + default: + case VIR_VIRTUALPORT_NONE: + case VIR_VIRTUALPORT_TYPE_LAST: + break;
error cases, no? goto out
Ok. However, I'd still allow the omission of <virtualport/> so I can start a macvtap without having to have a (future) switch.
+ } +
ret = 0;
out:
+ VIR_FREE(virtPortManagerID); + VIR_FREE(virtPortTypeID); + VIR_FREE(virtPortTypeIDVersion); + VIR_FREE(virtPortInstanceID); + VIR_FREE(virtPortProfileID); + VIR_FREE(virtPortType);
return ret;
Ok.
+} + + +static void +virVirtualPortProfileFormat(virBufferPtr buf, virVirtualPortProfileDefPtr virtPort, + const char *indent) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (virtPort->virtPortType == VIR_VIRTUALPORT_NONE) + return; + + virBufferVSprintf(buf, "%s<virtualport type='%s'>\n", + indent, virVirtualPortTypeToString(virtPort->virtPortType)); + + switch (virtPort->virtPortType) { + case VIR_VIRTUALPORT_NONE: + case VIR_VIRTUALPORT_TYPE_LAST: + break; + + case VIR_VIRTUALPORT_8021QBG: + virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID, + uuidstr); + virBufferVSprintf(buf, + "%s <parameters managerid='%d' typeid='%d' " + "typeidversion='%d' instanceid='%s'/>\n", + indent, + virtPort->u.virtPort8021Qbg.managerID, + virtPort->u.virtPort8021Qbg.typeID, + virtPort->u.virtPort8021Qbg.typeIDVersion, + uuidstr); + break; + + case VIR_VIRTUALPORT_8021QBH: + virBufferVSprintf(buf, + "%s <parameters profileid='%s'/>\n", + indent, + virtPort->u.virtPort8021Qbh.profileID); + break; + } + + virBufferVSprintf(buf, "%s</virtualport>\n", indent); +} + + /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition * @return 0 on success, -1 on failure @@ -1832,6 +1976,8 @@ virDomainNetDefParseXML(virCapsPtr caps, char *devaddr = NULL; char *mode = NULL; virNWFilterHashTablePtr filterparams = NULL; + virVirtualPortProfileDef virtPort; + bool virtPortParsed = false;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -1873,6 +2019,11 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlStrEqual(cur->name, BAD_CAST "source")) { dev = virXMLPropString(cur, "dev"); mode = virXMLPropString(cur, "mode"); + } else if ((virtPortParsed == false) && + (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) && + xmlStrEqual(cur->name, BAD_CAST "virtualport")) { + virVirtualPortProfileDefParseXML(cur, &virtPort); + virtPortParsed = true; } else if ((network == NULL) && ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) || (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) || @@ -2048,6 +2199,8 @@ virDomainNetDefParseXML(virCapsPtr caps, } else def->data.direct.mode = VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA;
+ def->data.direct.virtPortProfile = virtPort; + def->data.direct.linkdev = dev; dev = NULL;
@@ -5140,6 +5293,8 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferVSprintf(buf, " mode='%s'", virDomainNetdevMacvtapTypeToString(def->data.direct.mode)); virBufferAddLit(buf, "/>\n"); + virVirtualPortProfileFormat(buf, &def->data.direct.virtPortProfile, + " "); break;
case VIR_DOMAIN_NET_TYPE_USER: Index: libvirt-acl/src/conf/domain_conf.h =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.h +++ libvirt-acl/src/conf/domain_conf.h @@ -259,6 +259,39 @@ enum virDomainNetdevMacvtapType { };
+enum virVirtualPortType { + VIR_VIRTUALPORT_NONE, + VIR_VIRTUALPORT_8021QBG, + VIR_VIRTUALPORT_8021QBH, + + VIR_VIRTUALPORT_TYPE_LAST, +}; + +# ifdef IFLA_VF_PORT_PROFILE_MAX +# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX IFLA_VF_PORT_PROFILE_MAX +# else +# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX 40 +# endif + +/* profile data for macvtap (VEPA) */ +typedef struct _virVirtualPortProfileDef virVirtualPortProfileDef; +typedef virVirtualPortProfileDef *virVirtualPortProfileDefPtr; +struct _virVirtualPortProfileDef { + enum virVirtualPortType virtPortType; + union { + struct { + uint8_t managerID; + uint32_t typeID; // 24 bit valid + uint8_t typeIDVersion; + unsigned char instanceID[VIR_UUID_BUFLEN]; + } virtPort8021Qbg; + struct { + char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX]; + } virtPort8021Qbh; + } u; +}; + + /* Stores the virtual network interface configuration */ typedef struct _virDomainNetDef virDomainNetDef; typedef virDomainNetDef *virDomainNetDefPtr; @@ -290,6 +323,7 @@ struct _virDomainNetDef { struct { char *linkdev; int mode; + virVirtualPortProfileDef virtPortProfile; } direct; } data; char *ifname; @@ -1089,6 +1123,7 @@ VIR_ENUM_DECL(virDomainSeclabel) VIR_ENUM_DECL(virDomainClockOffset)
VIR_ENUM_DECL(virDomainNetdevMacvtap) +VIR_ENUM_DECL(virVirtualPort)
VIR_ENUM_DECL(virDomainTimerName) VIR_ENUM_DECL(virDomainTimerTrack) Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -43,6 +43,7 @@
# include "util.h" # include "memory.h" +# include "logging.h" # include "macvtap.h" # include "interface.h" # include "conf/domain_conf.h" @@ -57,6 +58,16 @@ # define MACVTAP_NAME_PREFIX "macvtap" # define MACVTAP_NAME_PATTERN "macvtap%d"
+ +static int associatePortProfileId(const char *macvtap_ifname, + const virVirtualPortProfileDefPtr virtPort, + int vf, + const unsigned char *vmuuid); + +static int disassociatePortProfileId(const char *macvtap_ifname, + const virVirtualPortProfileDefPtr virtPort); + + static int nlOpen(void) { int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE); @@ -567,39 +578,38 @@ configMacvtapTap(int tapfd, int vnet_hdr
return 0; } - - /** * openMacvtapTap: * Create an instance of a macvtap device and open its tap character * device. * @tgifname: Interface name that the macvtap is supposed to have. May * be NULL if this function is supposed to choose a name - * @macaddress: The MAC address for the macvtap device - * @linkdev: The interface name of the NIC to connect to the external bridge - * @mode_str: String describing the mode. Valid are 'bridge', 'vepa' and - * 'private'. + * @net: pointer to the virDomainNetDef object describing the direct + * type if an interface * @res_ifname: Pointer to a string pointer where the actual name of the * interface will be stored into if everything succeeded. It is up * to the caller to free the string. + * @vnet_hdr: Whether to enable IFF_VNET_HDR on the interface + * @vmuuid: The (raw) UUID of the VM * * Returns file descriptor of the tap device in case of success, * negative value otherwise with error reported. * + * Open a macvtap device and trigger the switch setup protocol + * if valid port profile parameters were provided. */ int openMacvtapTap(const char *tgifname, - const unsigned char *macaddress, - const char *linkdev, - int mode, + virDomainNetDefPtr net,
Any reason to keep tgifname now that you're passing full virDomainNetDefPtr?
True. Next would be to hanlde the **res_ifname in this function, but that I'd leave to a separate patch.
char **res_ifname, - int vnet_hdr) + int vnet_hdr, + const unsigned char *vmuuid) { const char *type = "macvtap"; int c, rc; char ifname[IFNAMSIZ]; int retries, do_retry = 0; - uint32_t macvtapMode = macvtapModeFromInt(mode); + uint32_t macvtapMode = macvtapModeFromInt(net->data.direct.mode); const char *cr_ifname; int ifindex;
@@ -616,7 +626,7 @@ openMacvtapTap(const char *tgifname, return -1; } cr_ifname = tgifname; - rc = link_add(type, macaddress, 6, tgifname, linkdev, + rc = link_add(type, net->mac, 6, tgifname, net->data.direct.linkdev, macvtapMode, &do_retry); if (rc) return -1; @@ -626,7 +636,8 @@ create_name: for (c = 0; c < 8192; c++) { snprintf(ifname, sizeof(ifname), MACVTAP_NAME_PATTERN, c); if (ifaceGetIndex(false, ifname, &ifindex) == ENODEV) { - rc = link_add(type, macaddress, 6, ifname, linkdev, + rc = link_add(type, net->mac, 6, ifname, + net->data.direct.linkdev, macvtapMode, &do_retry); if (rc == 0) break; @@ -639,6 +650,14 @@ create_name: cr_ifname = ifname; }
+ if (associatePortProfileId(cr_ifname, + &net->data.direct.virtPortProfile, + -1, + vmuuid) != 0) { + rc = -1; + goto link_del_exit; + } + rc = ifaceUp(cr_ifname); if (rc != 0) { virReportSystemError(errno, @@ -647,7 +666,7 @@ create_name: "MAC address"), cr_ifname); rc = -1; - goto link_del_exit; + goto disassociate_exit; }
rc = openTap(cr_ifname, 10); @@ -656,14 +675,18 @@ create_name: if (configMacvtapTap(rc, vnet_hdr) < 0) { close(rc); rc = -1; - goto link_del_exit; + goto disassociate_exit; } *res_ifname = strdup(cr_ifname); } else - goto link_del_exit; + goto disassociate_exit;
return rc;
+disassociate_exit: + disassociatePortProfileId(cr_ifname, + &net->data.direct.virtPortProfile); + link_del_exit: link_del(cr_ifname);
@@ -673,14 +696,102 @@ link_del_exit:
/** * delMacvtap: - * @ifname : The name of the macvtap interface + * @net: pointer to virDomainNetDef object * - * Delete an interface given its name. + * Delete an interface given its name. Disassociate + * it with the switch if port profile parameters + * were provided. */ void -delMacvtap(const char *ifname) +delMacvtap(virDomainNetDefPtr net) { - link_del(ifname); + if (net->ifname) { + disassociatePortProfileId(net->ifname, + &net->data.direct.virtPortProfile); + link_del(net->ifname); + } }
#endif + + +/** + * associatePortProfile + * + * @macvtap_ifname: The name of the macvtap device + * @virtPort: pointer to the object holding port profile parameters + * @vf: virtual function number, -1 if to be ignored + * @vmuuid : the UUID of the virtual machine + * + * Associate a port on a swtich with a profile. This function + * may notify a kernel driver or an external daemon to run + * the setup protocol. If profile parameters were not supplied + * by the user, then this function returns without doing + * anything. + * + * Returns 0 in case of success, != 0 otherwise with error + * having been reported. + */ +static int +associatePortProfileId(const char *macvtap_ifname, + const virVirtualPortProfileDefPtr virtPort, + int vf, + const unsigned char *vmuuid) +{ + int rc = 0; + VIR_DEBUG("Associating port profile '%p' on link device '%s'", + virtPort, macvtap_ifname); + (void)vf; + (void)vmuuid; + + switch (virtPort->virtPortType) { + case VIR_VIRTUALPORT_NONE: + case VIR_VIRTUALPORT_TYPE_LAST: + break;
If we capture these on parsing and error out, then these could be invalid here.
They would not even appear here. Problem is just I cannot even test anything with macvtap anymore without having a profile which then in turn requires either a daemon or an enic kernel driver.
+ + case VIR_VIRTUALPORT_8021QBG: + + break; + + case VIR_VIRTUALPORT_8021QBH: + + break; + } + + return rc; +} + + +/** + * disassociatePortProfile + * + * @macvtap_ifname: The name of the macvtap device + * @virtPort: point to object holding port profile parameters + * + * Returns 0 in case of success, != 0 otherwise with error + * having been reported. + */ +static int +disassociatePortProfileId(const char *macvtap_ifname, + const virVirtualPortProfileDefPtr virtPort) +{ + int rc = 0; + VIR_DEBUG("Disassociating port profile id '%p' on link device '%s' ", + virtPort, macvtap_ifname); + + switch (virtPort->virtPortType) { + case VIR_VIRTUALPORT_NONE: + case VIR_VIRTUALPORT_TYPE_LAST: + break;
If we capture these on parsing and error out, then these could be invalid here.
If I do that then please send me hardware :-) I'll post v8 after some testing - while I still can :-) Stefan