[libvirt] [PATCH 0/5] Support forward mode='hostdev' and interface pools
This patch series supports the forward mode='hostdev'. The functionality of this mode is the same as interface type='hostdev' but with the added benefit of using interface pools. The patch series also contains a patch to support use of interface names and PCI device addresses interchangeably in a network xml, and return the appropriate one in actualDevice when networkAllocateActualDevice is called. This functionality is not supported for any other forward mode except hostdev. Currently this patch series also does not support USB hostdev passthrough. At the top level managed attribute can be specified for a pf dev or an interface dev (with identical results as when it's specified for a hostdev Shradha Shah (5): Code to return interface name or pci_addr of the VF in actualDevice Moved the code to create implicit interface pool from PF to a new function Introduce forward mode='hostdev' for network XML in order to use the functionality of interface pools. Forward Mode Hostdev Implementation Supporting managed option for forward devs when using HOSTDEV mode docs/schemas/network.rng | 1 + src/conf/network_conf.c | 119 +++++++++++++++- src/conf/network_conf.h | 13 ++- src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 325 +++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.c | 14 ++ src/util/pci.c | 7 +- src/util/pci.h | 3 + src/util/virnetdev.c | 134 +++++++++++++++++- src/util/virnetdev.h | 19 +++ 10 files changed, 568 insertions(+), 70 deletions(-) -- 1.7.4.4
The network pool should be able to keep track of both, network device names nad PCI addresses, and return the appropriate one in the actualDevice when networkAllocateActualDevice is called. Signed-off-by: Shradha Shah <sshah@solarflare.com> --- src/conf/network_conf.c | 55 ++++++++++++++++++++++++++++++++++++++++++- src/conf/network_conf.h | 10 +++++++- src/network/bridge_driver.c | 47 ++++++++++++++++++++++++++++++++++++ src/util/pci.c | 5 +++- src/util/virnetdev.c | 6 +--- 5 files changed, 116 insertions(+), 7 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6515efe..8fcba16 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -98,6 +98,12 @@ virPortGroupDefClear(virPortGroupDefPtr def) } static void +virNetworkForwardPfDefClear(virNetworkForwardPfDefPtr def) +{ + VIR_FREE(def->dev); +} + +static void virNetworkForwardIfDefClear(virNetworkForwardIfDefPtr def) { VIR_FREE(def->dev); @@ -163,7 +169,7 @@ void virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->domain); for (ii = 0 ; ii < def->nForwardPfs && def->forwardPfs ; ii++) { - virNetworkForwardIfDefClear(&def->forwardPfs[ii]); + virNetworkForwardPfDefClear(&def->forwardPfs[ii]); } VIR_FREE(def->forwardPfs); @@ -925,6 +931,44 @@ error: return result; } +/* Function to compare strings with wildcard strings*/ +/* When editing this function also edit the one in src/network/bridge_driver.c*/ +static int +wildcmp(const char *wild, const char *string) +{ +/* Written by Jack Handy - <A href="mailto:jakkhandy@hotmail.com">jakkhandy@hotmail.com</A>*/ + const char *cp = NULL, *mp = NULL; + + while ((*string) && (*wild != '*')) { + if ((*wild != *string) && (*wild != '?')) { + return 0; + } + wild++; + string++; + } + + while (*string) { + if (*wild == '*') { + if (!*++wild) { + return 1; + } + mp = wild; + cp = string+1; + } else if ((*wild == *string) || (*wild == '?')) { + wild++; + string++; + } else { + wild = mp; + string = cp++; + } + } + + while (*wild == '*') { + wild++; + } + return !*wild; +} + static virNetworkDefPtr virNetworkDefParseXML(xmlXPathContextPtr ctxt) { @@ -1170,6 +1214,15 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->nForwardIfs++; } } + int i; + for (i = 0; i < nForwardIfs; i++) { + if (wildcmp("????:??:??.?", def->forwardIfs[i].dev)) { + def->forwardIfs[i].isPciAddr = true; + } else { + def->forwardIfs[i].isPciAddr = false; + } + } + VIR_FREE(forwardDev); VIR_FREE(forwardPfNodes); VIR_FREE(forwardIfNodes); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 4339a69..b205cb0 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -133,6 +133,14 @@ typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; struct _virNetworkForwardIfDef { char *dev; /* name of device */ int usageCount; /* how many guest interfaces are bound to this device? */ + bool isPciAddr; /* Differentiate a VF based on interface name or pci addr*/ +}; + +typedef struct _virNetworkForwardPfDef virNetworkForwardPfDef; +typedef virNetworkForwardPfDef *virNetworkForwardPfDefPtr; +struct _virNetworkForwardPfDef { + char *dev; /* name of device */ + int usageCount; /* how many guest interfaces are bound to this device? */ }; typedef struct _virPortGroupDef virPortGroupDef; @@ -163,7 +171,7 @@ struct _virNetworkDef { * interfaces), they will be listed here. */ size_t nForwardPfs; - virNetworkForwardIfDefPtr forwardPfs; + virNetworkForwardPfDefPtr forwardPfs; size_t nForwardIfs; virNetworkForwardIfDefPtr forwardIfs; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d82212f..41e3a49 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -87,6 +87,43 @@ struct network_driver { char *logDir; }; +/* Function to compare strings with wildcard strings*/ +/* When editing this function also edit the one in src/conf/network_conf.c*/ +static int +wildcmp(const char *wild, const char *string) +{ + // Written by Jack Handy - <A href="mailto:jakkhandy@hotmail.com">jakkhandy@otmail.com</A> + const char *cp = NULL, *mp = NULL; + + while ((*string) && (*wild != '*')) { + if ((*wild != *string) && (*wild != '?')) { + return 0; + } + wild++; + string++; + } + + while (*string) { + if (*wild == '*') { + if (!*++wild) { + return 1; + } + mp = wild; + cp = string+1; + } else if ((*wild == *string) || (*wild == '?')) { + wild++; + string++; + } else { + wild = mp; + string = cp++; + } + } + + while (*wild == '*') { + wild++; + } + return !*wild; +} static void networkDriverLock(struct network_driver *driver) { @@ -2918,6 +2955,16 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) goto cleanup; } netdef->forwardIfs[ii].usageCount = 0; + if (wildcmp("????:??:??.?",netdef->forwardIfs[ii].dev)) + netdef->forwardIfs[ii].isPciAddr = true; + else + netdef->forwardIfs[ii].isPciAddr = false; + + if (netdef->forwardIfs[ii].isPciAddr == true) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Passthrough mode does not work with PCI addresses and needs Interface names")); + goto cleanup; + } } } diff --git a/src/util/pci.c b/src/util/pci.c index b3be405..88e4ac5 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -2113,8 +2113,11 @@ pciDeviceNetName(char *device_link_sysfs_path, char **netname) } dir = opendir(pcidev_sysfs_net_path); - if (dir == NULL) + if (dir == NULL) { + VIR_DEBUG("No interface name"); + ret = 1; goto out; + } while ((entry = readdir(dir))) { if (STREQ(entry->d_name, ".") || diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index d53352f..a328294 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -989,7 +989,6 @@ virNetDevGetVirtualFunctions(const char *pfname, char *pf_sysfs_device_link = NULL; char *pci_sysfs_device_link = NULL; struct pci_config_address **virt_fns; - char *pciConfigAddr; if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) return ret; @@ -1009,12 +1008,12 @@ virNetDevGetVirtualFunctions(const char *pfname, virt_fns[i]->bus, virt_fns[i]->slot, virt_fns[i]->function, - &pciConfigAddr) < 0) { + &((*vfname)[i])) < 0) { virReportSystemError(ENOSYS, "%s", _("Failed to get PCI Config Address String")); goto cleanup; } - if (pciSysfsFile(pciConfigAddr, &pci_sysfs_device_link) < 0) { + if (pciSysfsFile((*vfname)[i], &pci_sysfs_device_link) < 0) { virReportSystemError(ENOSYS, "%s", _("Failed to get PCI SYSFS file")); goto cleanup; @@ -1037,7 +1036,6 @@ cleanup: VIR_FREE(virt_fns); VIR_FREE(pf_sysfs_device_link); VIR_FREE(pci_sysfs_device_link); - VIR_FREE(pciConfigAddr); return ret; } -- 1.7.4.4
On Fri, Jun 08, 2012 at 04:29:14PM +0100, Shradha Shah wrote:
@@ -925,6 +931,44 @@ error: return result; }
+/* Function to compare strings with wildcard strings*/ +/* When editing this function also edit the one in src/network/bridge_driver.c*/ +static int +wildcmp(const char *wild, const char *string) +{ +/* Written by Jack Handy - <A href="mailto:jakkhandy@hotmail.com">jakkhandy@hotmail.com</A>*/
Isn't this just reimplementing the Glibc 'fnmatch' function that we already use ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/11/2012 11:50 AM, Daniel P. Berrange wrote:
On Fri, Jun 08, 2012 at 04:29:14PM +0100, Shradha Shah wrote:
@@ -925,6 +931,44 @@ error: return result; }
+/* Function to compare strings with wildcard strings*/ +/* When editing this function also edit the one in src/network/bridge_driver.c*/ +static int +wildcmp(const char *wild, const char *string) +{ +/* Written by Jack Handy - <A href="mailto:jakkhandy@hotmail.com">jakkhandy@hotmail.com</A>*/ Isn't this just reimplementing the Glibc 'fnmatch' function that we already use ?
Beyond that, this is being used to support another case of crowding multiple data items into a single attribute, which we actively discourage (see the current thread about dhcpsnoop :-). Instead of re-purposing the singular char *dev, it would be better to follow the example of, e.g., virDomainDeviceInfo, and have a union like this: int type; union { virDomainDevicePCIAddress pci; char *dev; } addr; or just put both directly in the struct if you might want both to be populated at the same time for some reason (maybe you want to derive all the PCI addresses from the netdev name, or vice versa, and cache them here. Or maybe you don't, I don't have an opinion). The XML for this would then look something like this: <network> <name>hostdev-network</name> <forward mode="hostdev"> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network> Or maybe, for consistency with source addresses in domain device definitions, it should be: <network> <name>hostdev-network</name> <forward mode="hostdev"> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network> (anyone else have an opinion on this?) As a preparation for this, you should probably have a prerequisite patch that renames virDomainDevicePCIAddress to virDevicePCIAddress, moves it to the new file device_conf.h, and also moves virDomainDevicePCIAddressParseXML into device_conf.c (renaming it on the way). Similarly, the part of virDomainHostdevSourceFormat that formats a pci address could/should be moved into its own function in device_conf.c. (I'm suggesting the name "device_conf.c" rather than "pci_device_conf.c" because I'm thinking it might be good to rename/move the other virDomainDevice*Address structs and functions to the same place. Before taking the time to make this reorganization, it might be good to get feedback from some other people.)
On 06/11/2012 01:14 PM, Laine Stump wrote:
On 06/11/2012 11:50 AM, Daniel P. Berrange wrote:
On Fri, Jun 08, 2012 at 04:29:14PM +0100, Shradha Shah wrote:
@@ -925,6 +931,44 @@ error: return result; }
+/* Function to compare strings with wildcard strings*/ +/* When editing this function also edit the one in src/network/bridge_driver.c*/ +static int +wildcmp(const char *wild, const char *string) +{ +/* Written by Jack Handy - <A href="mailto:jakkhandy@hotmail.com">jakkhandy@hotmail.com</A>*/ Isn't this just reimplementing the Glibc 'fnmatch' function that we already use ? Beyond that, this is being used to support another case of crowding multiple data items into a single attribute, which we actively discourage (see the current thread about dhcpsnoop :-).
Instead of re-purposing the singular char *dev, it would be better to follow the example of, e.g., virDomainDeviceInfo, and have a union like this:
int type; union { virDomainDevicePCIAddress pci; char *dev; } addr;
or just put both directly in the struct if you might want both to be populated at the same time for some reason (maybe you want to derive all the PCI addresses from the netdev name, or vice versa, and cache them here. Or maybe you don't, I don't have an opinion).
The XML for this would then look something like this:
<network> <name>hostdev-network</name> <forward mode="hostdev"> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network>
Or maybe, for consistency with source addresses in domain device definitions, it should be:
<network> <name>hostdev-network</name> <forward mode="hostdev"> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network>
Also, about the "managed" attribute you introduced in PATCH 5/5 (which I suggested you should add at the same time as the rest of the new XML): Do you really think anyone will ever want some of the interfaces to be managed='yes' and some managed='no'? If you rework the XML as I suggest above, you would be able to reuse the same function to parse and format pci addresses; if you add a "managed" attribute to each address element, you would no longer be able to do that. Alternately, if you just add a single managed attribute to <forward>, the <address> sub-element would remain identical to the <address> in the domain devices' <source> element, so the parse/format functions could be shared (and the consistency would also make mixups less likely). Something like this: <network> <name>hostdev-network</name> <forward mode="hostdev" managed="yes"> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network You lose some flexibility, but are able to re-use more code. If it never makes sense to mix managed and un-managed (I don't know enough about the topic to have the answer to that), then the lost flexibility is meaningless.
(anyone else have an opinion on this?)
The same question holds for this new addition :-)
As a preparation for this, you should probably have a prerequisite patch that renames virDomainDevicePCIAddress to virDevicePCIAddress, moves it to the new file device_conf.h, and also moves virDomainDevicePCIAddressParseXML into device_conf.c (renaming it on the way). Similarly, the part of virDomainHostdevSourceFormat that formats a pci address could/should be moved into its own function in device_conf.c.
(I'm suggesting the name "device_conf.c" rather than "pci_device_conf.c" because I'm thinking it might be good to rename/move the other virDomainDevice*Address structs and functions to the same place. Before taking the time to make this reorganization, it might be good to get feedback from some other people.)
On Tue, Jun 12, 2012 at 12:03:02PM -0400, Laine Stump wrote:
On 06/11/2012 01:14 PM, Laine Stump wrote:
On 06/11/2012 11:50 AM, Daniel P. Berrange wrote:
On Fri, Jun 08, 2012 at 04:29:14PM +0100, Shradha Shah wrote:
@@ -925,6 +931,44 @@ error: return result; }
+/* Function to compare strings with wildcard strings*/ +/* When editing this function also edit the one in src/network/bridge_driver.c*/ +static int +wildcmp(const char *wild, const char *string) +{ +/* Written by Jack Handy - <A href="mailto:jakkhandy@hotmail.com">jakkhandy@hotmail.com</A>*/ Isn't this just reimplementing the Glibc 'fnmatch' function that we already use ? Beyond that, this is being used to support another case of crowding multiple data items into a single attribute, which we actively discourage (see the current thread about dhcpsnoop :-).
Instead of re-purposing the singular char *dev, it would be better to follow the example of, e.g., virDomainDeviceInfo, and have a union like this:
int type; union { virDomainDevicePCIAddress pci; char *dev; } addr;
or just put both directly in the struct if you might want both to be populated at the same time for some reason (maybe you want to derive all the PCI addresses from the netdev name, or vice versa, and cache them here. Or maybe you don't, I don't have an opinion).
The XML for this would then look something like this:
<network> <name>hostdev-network</name> <forward mode="hostdev"> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <interface type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network>
Or maybe, for consistency with source addresses in domain device definitions, it should be:
<network> <name>hostdev-network</name> <forward mode="hostdev"> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network>
Also, about the "managed" attribute you introduced in PATCH 5/5 (which I suggested you should add at the same time as the rest of the new XML):
Do you really think anyone will ever want some of the interfaces to be managed='yes' and some managed='no'? If you rework the XML as I suggest above, you would be able to reuse the same function to parse and format pci addresses; if you add a "managed" attribute to each address element, you would no longer be able to do that.
Alternately, if you just add a single managed attribute to <forward>, the <address> sub-element would remain identical to the <address> in the domain devices' <source> element, so the parse/format functions could be shared (and the consistency would also make mixups less likely). Something like this:
<network> <name>hostdev-network</name> <forward mode="hostdev" managed="yes"> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x01'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x02'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x03'/> </forward> </network
You lose some flexibility, but are able to re-use more code. If it never makes sense to mix managed and un-managed (I don't know enough about the topic to have the answer to that), then the lost flexibility is meaningless.
(anyone else have an opinion on this?)
The same question holds for this new addition :-)
Yes, I tend to agree with your suggestions that we should model the PCI addresses fully and not have their encoded in a string. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This makes the code reusable Signed-off-by: Shradha Shah <sshah@solarflare.com> --- src/network/bridge_driver.c | 109 ++++++++++++++++++++++++++----------------- 1 files changed, 66 insertions(+), 43 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 41e3a49..8540003 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2761,6 +2761,60 @@ int networkRegister(void) { * "backend" function table. */ +/* networkCreateInterfacePool: + * @netdef: the original NetDef from the network + * + * Creates an implicit interface pool of VF's when a PF dev is given + */ +static int +networkCreateInterfacePool(virNetworkDefPtr netdef) { + unsigned int num_virt_fns = 0; + char **vfname = NULL; + int ret = -1, ii = 0; + + if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, + &vfname, &num_virt_fns)) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get Virtual functions on %s"), + netdef->forwardPfs->dev); + goto finish; + } + + if (num_virt_fns == 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("No Vf's present on SRIOV PF %s"), + netdef->forwardPfs->dev); + goto finish; + } + + if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) { + virReportOOMError(); + goto finish; + } + + netdef->nForwardIfs = num_virt_fns; + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + netdef->forwardIfs[ii].dev = strdup(vfname[ii]); + if (!netdef->forwardIfs[ii].dev) { + virReportOOMError(); + goto finish; + } + netdef->forwardIfs[ii].usageCount = 0; + if (wildcmp("????:??:??.?", netdef->forwardIfs[ii].dev)) + netdef->forwardIfs[ii].isPciAddr = true; + else + netdef->forwardIfs[ii].isPciAddr = false; + } + + ret = 0; +finish: + for (ii = 0; ii < num_virt_fns; ii++) + VIR_FREE(vfname[ii]); + VIR_FREE(vfname); + return ret; +} + /* networkAllocateActualDevice: * @iface: the original NetDef from the domain * @@ -2779,8 +2833,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network; virNetworkDefPtr netdef; virPortGroupDefPtr portgroup; - unsigned int num_virt_fns = 0; - char **vfname = NULL; int ii; int ret = -1; @@ -2858,7 +2910,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) || (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) { virNetDevVPortProfilePtr virtport = NULL; - + int rc = -1; /* <forward type='bridge|private|vepa|passthrough'> are all * VIR_DOMAIN_NET_TYPE_DIRECT. */ @@ -2926,57 +2978,31 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) */ if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) { if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) { - if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, - &vfname, &num_virt_fns)) < 0) { + if((rc = networkCreateInterfacePool(netdef)) < 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get Virtual functions on %s"), - netdef->forwardPfs->dev); + _("Could not create Interface Pool from PF")); goto cleanup; } - - if (num_virt_fns == 0) { + } + + /*if dev names are pci addrs in passthrough mode: error*/ + for (ii = 0; ii <netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].isPciAddr == true) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("No Vf's present on SRIOV PF %s"), - netdef->forwardPfs->dev); + _("Passthrough mode does not work with PCI addresses and needs Interface names")); goto cleanup; } - - if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) { - virReportOOMError(); - goto cleanup; - } - - netdef->nForwardIfs = num_virt_fns; - - for (ii = 0; ii < netdef->nForwardIfs; ii++) { - netdef->forwardIfs[ii].dev = strdup(vfname[ii]); - if (!netdef->forwardIfs[ii].dev) { - virReportOOMError(); - goto cleanup; - } - netdef->forwardIfs[ii].usageCount = 0; - if (wildcmp("????:??:??.?",netdef->forwardIfs[ii].dev)) - netdef->forwardIfs[ii].isPciAddr = true; - else - netdef->forwardIfs[ii].isPciAddr = false; - - if (netdef->forwardIfs[ii].isPciAddr == true) { - networkReportError(VIR_ERR_INTERNAL_ERROR, - _("Passthrough mode does not work with PCI addresses and needs Interface names")); - goto cleanup; - } - } } - + /* pick first dev with 0 usageCount */ for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (netdef->forwardIfs[ii].usageCount == 0) { dev = &netdef->forwardIfs[ii]; break; - } + } } - } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + }else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && iface->data.network.actual->data.direct.virtPortProfile && (iface->data.network.actual->data.direct.virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBH)) { @@ -3017,9 +3043,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) ret = 0; cleanup: - for (ii = 0; ii < num_virt_fns; ii++) - VIR_FREE(vfname[ii]); - VIR_FREE(vfname); if (network) virNetworkObjUnlock(network); if (ret < 0) { -- 1.7.4.4
On 06/08/2012 11:29 AM, Shradha Shah wrote:
This makes the code reusable
Rather than adding new code, then moving that new code around along with existing code that needs to be moved, it would be cleaner and easier to follow if you first moved the existing code, then introduced the new code (directly in the place that it will eventually live).
Signed-off-by: Shradha Shah <sshah@solarflare.com> --- src/network/bridge_driver.c | 109 ++++++++++++++++++++++++++----------------- 1 files changed, 66 insertions(+), 43 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 41e3a49..8540003 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2761,6 +2761,60 @@ int networkRegister(void) { * "backend" function table. */
+/* networkCreateInterfacePool: + * @netdef: the original NetDef from the network + * + * Creates an implicit interface pool of VF's when a PF dev is given + */ +static int +networkCreateInterfacePool(virNetworkDefPtr netdef) { + unsigned int num_virt_fns = 0; + char **vfname = NULL; + int ret = -1, ii = 0; + + if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, + &vfname, &num_virt_fns)) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get Virtual functions on %s"), + netdef->forwardPfs->dev); + goto finish; + } + + if (num_virt_fns == 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("No Vf's present on SRIOV PF %s"), + netdef->forwardPfs->dev); + goto finish; + } + + if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) { + virReportOOMError(); + goto finish; + } + + netdef->nForwardIfs = num_virt_fns; + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + netdef->forwardIfs[ii].dev = strdup(vfname[ii]); + if (!netdef->forwardIfs[ii].dev) { + virReportOOMError(); + goto finish; + } + netdef->forwardIfs[ii].usageCount = 0; + if (wildcmp("????:??:??.?", netdef->forwardIfs[ii].dev)) + netdef->forwardIfs[ii].isPciAddr = true; + else + netdef->forwardIfs[ii].isPciAddr = false; + } + + ret = 0; +finish: + for (ii = 0; ii < num_virt_fns; ii++) + VIR_FREE(vfname[ii]); + VIR_FREE(vfname); + return ret; +} + /* networkAllocateActualDevice: * @iface: the original NetDef from the domain * @@ -2779,8 +2833,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network; virNetworkDefPtr netdef; virPortGroupDefPtr portgroup; - unsigned int num_virt_fns = 0; - char **vfname = NULL; int ii; int ret = -1;
@@ -2858,7 +2910,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) || (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) { virNetDevVPortProfilePtr virtport = NULL; - + int rc = -1; /* <forward type='bridge|private|vepa|passthrough'> are all * VIR_DOMAIN_NET_TYPE_DIRECT. */ @@ -2926,57 +2978,31 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) */ if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) { if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) { - if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, - &vfname, &num_virt_fns)) < 0) { + if((rc = networkCreateInterfacePool(netdef)) < 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get Virtual functions on %s"), - netdef->forwardPfs->dev); + _("Could not create Interface Pool from PF")); goto cleanup; } - - if (num_virt_fns == 0) { + } + + /*if dev names are pci addrs in passthrough mode: error*/ + for (ii = 0; ii <netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].isPciAddr == true) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("No Vf's present on SRIOV PF %s"), - netdef->forwardPfs->dev); + _("Passthrough mode does not work with PCI addresses and needs Interface names")); goto cleanup; } - - if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) { - virReportOOMError(); - goto cleanup; - } - - netdef->nForwardIfs = num_virt_fns; - - for (ii = 0; ii < netdef->nForwardIfs; ii++) { - netdef->forwardIfs[ii].dev = strdup(vfname[ii]); - if (!netdef->forwardIfs[ii].dev) { - virReportOOMError(); - goto cleanup; - } - netdef->forwardIfs[ii].usageCount = 0; - if (wildcmp("????:??:??.?",netdef->forwardIfs[ii].dev)) - netdef->forwardIfs[ii].isPciAddr = true; - else - netdef->forwardIfs[ii].isPciAddr = false; - - if (netdef->forwardIfs[ii].isPciAddr == true) { - networkReportError(VIR_ERR_INTERNAL_ERROR, - _("Passthrough mode does not work with PCI addresses and needs Interface names")); - goto cleanup; - } - } } - + /* pick first dev with 0 usageCount */
for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (netdef->forwardIfs[ii].usageCount == 0) { dev = &netdef->forwardIfs[ii]; break; - } + } } - } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + }else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && iface->data.network.actual->data.direct.virtPortProfile && (iface->data.network.actual->data.direct.virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBH)) { @@ -3017,9 +3043,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
ret = 0; cleanup: - for (ii = 0; ii < num_virt_fns; ii++) - VIR_FREE(vfname[ii]); - VIR_FREE(vfname); if (network) virNetworkObjUnlock(network); if (ret < 0) {
This new forward mode sets up a PCI network device to be assigned to the guest with PCI passthrough. The PCI network device is chosen from an interface pool. Currently there is no support for USB devices when using this forward mode. Example XML: Network XML: <network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <pf dev="eth2"/> </forward> </network> The above xml leads to the creation of an implicit interface pool and a free network device is chosen from the pool. This patch would also work if an interface pool is given explicitly. The interface dev value can be an interface name or a PCI device address. Example XML: Network XML: <network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <interface dev="0000:04:00.1"/> <interface dev="0000:04:00.2"/> <interface dev="0000:04:00.3"/> </forward> </network> The MAC address would be provided in the domain xml. Signed-off-by: Shradha Shah <sshah@solarflare.com> --- docs/schemas/network.rng | 1 + src/conf/network_conf.c | 3 ++- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 6 ++++-- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 2ae879e..a0046f1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -82,6 +82,7 @@ <value>passthrough</value> <value>private</value> <value>vepa</value> + <value>hostdev</value> </choice> </attribute> </optional> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8fcba16..6b346c3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -51,7 +51,7 @@ VIR_ENUM_DECL(virNetworkForward) VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, - "none", "nat", "route", "bridge", "private", "vepa", "passthrough" ) + "none", "nat", "route", "bridge", "private", "vepa", "passthrough", "hostdev") #define virNetworkReportError(code, ...) \ virReportErrorHelper(VIR_FROM_NETWORK, code, __FILE__, \ @@ -1251,6 +1251,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: if (def->bridge) { virNetworkReportError(VIR_ERR_XML_ERROR, _("bridge name not allowed in %s mode (network '%s'"), diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index b205cb0..d473c71 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -45,6 +45,7 @@ enum virNetworkForwardType { VIR_NETWORK_FORWARD_PRIVATE, VIR_NETWORK_FORWARD_VEPA, VIR_NETWORK_FORWARD_PASSTHROUGH, + VIR_NETWORK_FORWARD_HOSTDEV, VIR_NETWORK_FORWARD_LAST, }; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8540003..cc53551 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1974,7 +1974,7 @@ networkStartNetworkExternal(struct network_driver *driver ATTRIBUTE_UNUSED, virNetworkObjPtr network ATTRIBUTE_UNUSED) { /* put anything here that needs to be done each time a network of - * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is started. On + * type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is started. On * failure, undo anything you've done, and return -1. On success * return 0. */ @@ -1985,7 +1985,7 @@ static int networkShutdownNetworkExternal(struct network_driver *driver ATTRIBUT virNetworkObjPtr network ATTRIBUTE_UNUSED) { /* put anything here that needs to be done each time a network of - * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is shutdown. On + * type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is shutdown. On * failure, undo anything you've done, and return -1. On success * return 0. */ @@ -2016,6 +2016,7 @@ networkStartNetwork(struct network_driver *driver, case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: ret = networkStartNetworkExternal(driver, network); break; } @@ -2075,6 +2076,7 @@ static int networkShutdownNetwork(struct network_driver *driver, case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: ret = networkShutdownNetworkExternal(driver, network); break; } -- 1.7.4.4
On 06/08/2012 11:29 AM, Shradha Shah wrote:
This new forward mode sets up a PCI network device to be assigned to the guest with PCI passthrough. The PCI network device is chosen from an interface pool. Currently there is no support for USB devices when using this forward mode.
Example XML: Network XML: <network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <pf dev="eth2"/> </forward> </network>
See my comment on PATCH 1/5 for my suggestion on changing the XML format.
The above xml leads to the creation of an implicit interface pool and a free network device is chosen from the pool. This patch would also work if an interface pool is given explicitly. The interface dev value can be an interface name or a PCI device address.
Example XML: Network XML: <network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <interface dev="0000:04:00.1"/> <interface dev="0000:04:00.2"/> <interface dev="0000:04:00.3"/> </forward> </network>
The MAC address would be provided in the domain xml.
Signed-off-by: Shradha Shah <sshah@solarflare.com> --- docs/schemas/network.rng | 1 + src/conf/network_conf.c | 3 ++- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 6 ++++-- 4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 2ae879e..a0046f1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -82,6 +82,7 @@ <value>passthrough</value> <value>private</value> <value>vepa</value> + <value>hostdev</value> </choice> </attribute> </optional> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8fcba16..6b346c3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -51,7 +51,7 @@ VIR_ENUM_DECL(virNetworkForward)
VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, - "none", "nat", "route", "bridge", "private", "vepa", "passthrough" ) + "none", "nat", "route", "bridge", "private", "vepa", "passthrough", "hostdev")
#define virNetworkReportError(code, ...) \ virReportErrorHelper(VIR_FROM_NETWORK, code, __FILE__, \ @@ -1251,6 +1251,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: if (def->bridge) { virNetworkReportError(VIR_ERR_XML_ERROR, _("bridge name not allowed in %s mode (network '%s'"), diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index b205cb0..d473c71 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -45,6 +45,7 @@ enum virNetworkForwardType { VIR_NETWORK_FORWARD_PRIVATE, VIR_NETWORK_FORWARD_VEPA, VIR_NETWORK_FORWARD_PASSTHROUGH, + VIR_NETWORK_FORWARD_HOSTDEV,
VIR_NETWORK_FORWARD_LAST, }; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8540003..cc53551 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1974,7 +1974,7 @@ networkStartNetworkExternal(struct network_driver *driver ATTRIBUTE_UNUSED, virNetworkObjPtr network ATTRIBUTE_UNUSED) { /* put anything here that needs to be done each time a network of - * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is started. On + * type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is started. On * failure, undo anything you've done, and return -1. On success * return 0. */ @@ -1985,7 +1985,7 @@ static int networkShutdownNetworkExternal(struct network_driver *driver ATTRIBUT virNetworkObjPtr network ATTRIBUTE_UNUSED) { /* put anything here that needs to be done each time a network of - * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is shutdown. On + * type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is shutdown. On * failure, undo anything you've done, and return -1. On success * return 0. */ @@ -2016,6 +2016,7 @@ networkStartNetwork(struct network_driver *driver, case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: ret = networkStartNetworkExternal(driver, network); break; } @@ -2075,6 +2076,7 @@ static int networkShutdownNetwork(struct network_driver *driver, case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: ret = networkShutdownNetworkExternal(driver, network); break; }
This patch chooses a free network device from the interface pool and creates a PCI HostDef to be passed to the guest, when forward mode is "hostdev". networkNotifyActualDevice and networkReleaseActualDevice are modified accordingly. Signed-off-by: Shradha Shah <sshah@solarflare.com> --- src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 180 +++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_command.c | 14 ++++ src/util/pci.c | 2 +- src/util/pci.h | 3 + src/util/virnetdev.c | 128 ++++++++++++++++++++++++++++++ src/util/virnetdev.h | 19 +++++ 7 files changed, 325 insertions(+), 24 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index afb308d..5b8ab0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1261,6 +1261,9 @@ virNetDevGetVLanID; virNetDevGetVirtualFunctionIndex; virNetDevGetVirtualFunctionInfo; virNetDevGetVirtualFunctions; +virNetDevParsePciConfigAddress; +virNetDevGetDeviceAddrString; +virNetDevGetPciAddrFromName; virNetDevIsOnline; virNetDevIsVirtualFunction; virNetDevLinkDump; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index cc53551..691ab07 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2835,6 +2835,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network; virNetworkDefPtr netdef; virPortGroupDefPtr portgroup; + virNetDevVPortProfilePtr virtport = NULL; + virNetworkForwardIfDefPtr dev = NULL; int ii; int ret = -1; @@ -2906,12 +2908,95 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) virReportOOMError(); goto cleanup; } - + } else if (netdef->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) { + int rc = -1; + if (!iface->data.network.actual + && (VIR_ALLOC(iface->data.network.actual) < 0)) { + virReportOOMError(); + goto cleanup; + } + iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_HOSTDEV; + if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) { + if((rc = networkCreateInterfacePool(netdef)) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not create Interface Pool from PF")); + goto cleanup; + } + } + /* pick first dev with 0 usageCount */ + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].usageCount == 0) { + dev = &netdef->forwardIfs[ii]; + break; + } + } + if (!dev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' requires exclusive access to interfaces, but none are available"), + netdef->name); + goto cleanup; + } + + iface->data.network.actual->data.hostdev.def.parent.type = VIR_DOMAIN_DEVICE_NET; + iface->data.network.actual->data.hostdev.def.parent.data.net = iface; + iface->data.network.actual->data.hostdev.def.info = &iface->info; + iface->data.network.actual->data.hostdev.def.mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; + iface->data.network.actual->data.hostdev.def.managed = 1; + iface->data.network.actual->data.hostdev.def.source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; + + if (dev->isPciAddr == true) { + virDomainDevicePCIAddressPtr addr = &iface->data.network.actual->data.hostdev.def.source.subsys.u.pci; + if (virNetDevParsePciConfigAddress(dev->dev, + &addr->domain, + &addr->bus, + &addr->slot, + &addr->function) < 0) { + goto cleanup; + } + } + else if (dev->isPciAddr == false) { + virDomainDevicePCIAddressPtr addr = &iface->data.network.actual->data.hostdev.def.source.subsys.u.pci; + char *device_pci_addr = NULL; + if (virNetDevGetPciAddrFromName(dev->dev, + &device_pci_addr) < 0) { + goto cleanup; + } + if (virNetDevParsePciConfigAddress(device_pci_addr, + &addr->domain, + &addr->bus, + &addr->slot, + &addr->function) < 0) { + goto cleanup; + } + VIR_FREE(device_pci_addr); + } + dev->usageCount++; + VIR_DEBUG("Using physical device %s, usageCount %d", + dev->dev, dev->usageCount); + + if (iface->data.network.virtPortProfile) { + virtport = iface->data.network.virtPortProfile; + } else { + if (portgroup) + virtport = portgroup->virtPortProfile; + else + virtport = netdef->virtPortProfile; + } + if (virtport) { + if (VIR_ALLOC(iface->data.network.actual->data.hostdev.virtPortProfile) < 0) { + virReportOOMError(); + goto cleanup; + } + /* There are no pointers in a virtualPortProfile, so a shallow copy + * is sufficient + */ + *iface->data.network.actual->data.direct.virtPortProfile = *virtport; + } } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) || (netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) || (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) || (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) { - virNetDevVPortProfilePtr virtport = NULL; int rc = -1; /* <forward type='bridge|private|vepa|passthrough'> are all * VIR_DOMAIN_NET_TYPE_DIRECT. @@ -2969,7 +3054,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) netdef->name); goto cleanup; } else { - virNetworkForwardIfDefPtr dev = NULL; /* pick an interface from the pool */ @@ -3070,14 +3154,16 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) struct network_driver *driver = driverState; virNetworkObjPtr network; virNetworkDefPtr netdef; - const char *actualDev; + virDomainHostdevDefPtr def = NULL; + const char *actualDev = NULL; int ret = -1; if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) return 0; if (!iface->data.network.actual || - (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) { + ((virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT) && + (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_HOSTDEV))) { VIR_DEBUG("Nothing to claim from network %s", iface->data.network.name); return 0; } @@ -3092,23 +3178,45 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) goto cleanup; } - actualDev = virDomainNetGetActualDirectDev(iface); - if (!actualDev) { - networkReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("the interface uses a direct mode, but has no source dev")); - goto cleanup; + if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_DIRECT) { + actualDev = virDomainNetGetActualDirectDev(iface); + if (!actualDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("the interface uses a direct mode, but has no source dev")); + goto cleanup; + } + } + + if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + def = virDomainNetGetActualHostdev(iface); + if (!def) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("the interface uses a hostdev mode, but has no hostdev")); + goto cleanup; + } } - netdef = network->def; if (netdef->nForwardIfs == 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), + _("network '%s' uses a direct/hostdev mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; } else { int ii; virNetworkForwardIfDefPtr dev = NULL; - + + virDomainDevicePCIAddressPtr addr = &(def->source.subsys.u.pci); + if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (virNetDevGetDeviceAddrString(addr->domain, + addr->bus, + addr->slot, + addr->function, + &actualDev) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not get device string for hostdev")); + goto cleanup; + } + } /* find the matching interface in the pool and increment its usageCount */ for (ii = 0; ii < netdef->nForwardIfs; ii++) { @@ -3125,12 +3233,12 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) goto cleanup; } - /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require - * exclusive access to a device, so current usageCount must be + /* PASSTHROUGH mode, HOSTDEV mode and PRIVATE Mode + 802.1Qbh both * require exclusive access to a device, so current usageCount must be * 0 in those cases. */ if ((dev->usageCount > 0) && ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || + (netdef->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) || ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && iface->data.network.actual->data.direct.virtPortProfile && (iface->data.network.actual->data.direct.virtPortProfile->virtPortType @@ -3170,14 +3278,16 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) struct network_driver *driver = driverState; virNetworkObjPtr network = NULL; virNetworkDefPtr netdef; - const char *actualDev; + virDomainHostdevDefPtr def = NULL; + const char *actualDev = NULL; int ret = -1; if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) return 0; if (!iface->data.network.actual || - (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) { + ((virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT) && + (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_HOSTDEV))) { VIR_DEBUG("Nothing to release to network %s", iface->data.network.name); ret = 0; goto cleanup; @@ -3193,23 +3303,47 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) goto cleanup; } - actualDev = virDomainNetGetActualDirectDev(iface); - if (!actualDev) { - networkReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("the interface uses a direct mode, but has no source dev")); - goto cleanup; + if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_DIRECT) { + actualDev = virDomainNetGetActualDirectDev(iface); + if (!actualDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("the interface uses a direct mode, but has no source dev")); + goto cleanup; + } + } + + if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + def = virDomainNetGetActualHostdev(iface); + if (!def) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("the interface uses a hostdev mode, but has no hostdev")); + goto cleanup; + } } netdef = network->def; if (netdef->nForwardIfs == 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), + _("network '%s' uses a direct/hostdev mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; } else { int ii; virNetworkForwardIfDefPtr dev = NULL; + virDomainDevicePCIAddressPtr addr = &(def->source.subsys.u.pci); + if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (virNetDevGetDeviceAddrString(addr->domain, + addr->bus, + addr->slot, + addr->function, + &actualDev) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not get device string for hostdev")); + goto cleanup; + } + } + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { dev = &netdef->forwardIfs[ii]; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 117542f..ea1c9c5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4897,6 +4897,20 @@ qemuBuildCommandLine(virConnectPtr conn, * code here that adds the newly minted hostdev to the * hostdevs array). */ + if (qemuAssignDeviceHostdevAlias(def, + virDomainNetGetActualHostdev(net), + (def->nhostdevs-1)) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not assign alias to Net Hostdev")); + goto error; + } + + if (virDomainHostdevInsert(def, + virDomainNetGetActualHostdev(net)) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Hostdev not inserted into the array")); + goto error; + } continue; } diff --git a/src/util/pci.c b/src/util/pci.c index 88e4ac5..6dd9953 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1812,7 +1812,7 @@ logStrToLong_ui(char const *s, return ret; } -static int +int pciParsePciConfigAddress(char *address, struct pci_config_address *bdf) { diff --git a/src/util/pci.h b/src/util/pci.h index b71bb12..8d85558 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -105,6 +105,9 @@ int pciGetVirtualFunctions(const char *sysfs_path, struct pci_config_address ***virtual_functions, unsigned int *num_virtual_functions); +int pciParsePciConfigAddress(char *address, + struct pci_config_address *bdf); + int pciDeviceIsVirtualFunction(const char *vf_sysfs_device_link); int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link, diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a328294..dad6035 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -38,6 +38,7 @@ #ifdef __linux__ # include <linux/sockios.h> +# include <linux/ethtool.h> # include <linux/if_vlan.h> #elif !defined(AF_PACKET) # undef HAVE_STRUCT_IFREQ @@ -463,6 +464,65 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) return rc; } +#if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ) +/** + * virNetDevGetPciAddrFromName: + * @device_pci_addr : string containing the BDF of the device + * @domain : pointer to the integer domain + * @bus : pointer to the integer bus + * @slot : pointer to integer slot + * @function: pointer to integer function + * + * Parses the PCI config address string. + * + * Returns 0 if success and -1 if error + * + */ +int +virNetDevGetPciAddrFromName(const char *ifname, + char **device_pci_addr) +{ + int ret = -1; + int fd = -1; + struct ifreq ifr; + struct ethtool_drvinfo drvinfo; + + memset(&drvinfo, 0, sizeof(drvinfo)); + + drvinfo.cmd = ETHTOOL_GDRVINFO; + + if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + ifr.ifr_ifru.ifru_data = (void *)&drvinfo; + + if (ioctl(fd, SIOCETHTOOL, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot get driver info on '%s'"), + ifname); + goto error; + } + + if (virAsprintf(device_pci_addr, "%s", drvinfo.bus_info) < 0) { + virReportOOMError(); + goto error; + } + + ret = 0; +error: + return ret; +} +#else +int virNetDevGetPciAddrFromName(const char* ifname ATTRIBUTE_UNUSED, + char **device_pci_addr ATTRIBUTE_UNUSED) +{ + virReportSystemError(errno, + _("Cannot get driver info on '%s'"), + ifname); + return -1; +} +#endif + #if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ) /** * virNetDevSetName: @@ -1040,6 +1100,74 @@ cleanup: } /** + * virNetDevGetDeviceAddrString: + * @domain : pointer to the integer domain + * @bus : pointer to the integer bus + * @slot : pointer to integer slot + * @function: pointer to integer function + * @addr: pointer to the memory allocated to hold the BDF string + * + * Creates the PCI config address string. + * + * Returns 0 if success and -1 if error + * + */ +int +virNetDevGetDeviceAddrString(unsigned domain, + unsigned bus, + unsigned slot, + unsigned function, + const char **addr) +{ + int ret = -1; + + if (pciGetDeviceAddrString(domain, bus, slot, function, (char **)addr) < 0) + goto error; + + ret = 0; + +error: + return ret; +} + +/** + * virNetDevParsePciConfigAddress: + * @device_pci_addr : string containing the BDF of the device + * @domain : pointer to the integer domain + * @bus : pointer to the integer bus + * @slot : pointer to integer slot + * @function: pointer to integer function + * + * Parses the PCI config address string. + * + * Returns 0 if success and -1 if error + * + */ +int +virNetDevParsePciConfigAddress(const char *device_pci_addr, + unsigned int *domain, + unsigned int *bus, + unsigned int *slot, + unsigned int *function) +{ + int ret = -1; + struct pci_config_address device; + + if (pciParsePciConfigAddress((char *)device_pci_addr, &device) != 0) { + goto error; + } + + *domain = device.domain; + *bus = device.bus; + *slot = device.slot; + *function = device.function; + + ret = 0; +error: + return ret; +} + +/** * virNetDevIsVirtualFunction: * @ifname : name of the interface * diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 660d2db..4248a85 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -106,6 +106,25 @@ int virNetDevGetVirtualFunctions(const char *pfname, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int virNetDevGetDeviceAddrString(unsigned domain, + unsigned bus, + unsigned slot, + unsigned function, + const char **addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; + +int virNetDevParsePciConfigAddress(const char *device_pci_addr, + unsigned int *domain, + unsigned int *bus, + unsigned int *slot, + unsigned int *function) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +int virNetDevGetPciAddrFromName(const char *ifname, + char **device_pci_addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + int virNetDevLinkDump(const char *ifname, int ifindex, struct nlattr **tb, unsigned char **recvbuf, -- 1.7.4.4
This patch supports the managed option in a network xml for network devices. This option is used for pci network devices when forward mode=hostdev. Hence the example network xml would be: <network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <pf dev="eth2" managed='yes'/> </forward> </network> OR <network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <interface dev="0000:04:00.1" managed='yes'/> <interface dev="0000:04:00.2" managed='yes'/> <interface dev="0000:04:00.3" managed='yes'/> </forward> </network> Signed-off-by: Shradha Shah <sshah@solarflare.com> --- src/conf/network_conf.c | 61 ++++++++++++++++++++++++++++++++++++++++-- src/conf/network_conf.h | 2 + src/network/bridge_driver.c | 5 +++- 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6b346c3..18e4ee3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -986,6 +986,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) char *forwardDev = NULL; xmlNodePtr save = ctxt->node; xmlNodePtr bandwidthNode = NULL; + char *managed = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -1128,6 +1129,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } forwardDev = virXPathString("string(./@dev)", ctxt); + managed = virXPathString("string(./managed)", ctxt); /* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); @@ -1151,6 +1153,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } + if (managed) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("A managed field should not be used in this location when using a SRIOV PF")); + goto error; + } + forwardDev = virXMLPropString(*forwardPfNodes, "dev"); if (!forwardDev) { virNetworkReportError(VIR_ERR_XML_ERROR, @@ -1159,9 +1167,16 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } + managed = virXMLPropString(*forwardPfNodes, "managed"); + if(managed != NULL) { + if (STREQ(managed, "yes")) + def->forwardPfs->managed = 1; + } + def->forwardPfs->usageCount = 0; def->forwardPfs->dev = forwardDev; forwardDev = NULL; + managed = NULL; def->nForwardPfs++; } else if (nForwardPfs > 1) { virNetworkReportError(VIR_ERR_XML_ERROR, @@ -1170,6 +1185,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } if (nForwardIfs > 0 || forwardDev) { int ii; + unsigned int managedvalue = 0; /* allocate array to hold all the portgroups */ if (VIR_ALLOC_N(def->forwardIfs, MAX(nForwardIfs, 1)) < 0) { @@ -1181,12 +1197,24 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->forwardIfs[0].usageCount = 0; def->forwardIfs[0].dev = forwardDev; forwardDev = NULL; + if (managed != NULL) { + if (STREQ(managed, "yes")) + def->forwardIfs[0].managed = 1; + } + managed = NULL; def->nForwardIfs++; } /* parse each forwardIf */ for (ii = 0; ii < nForwardIfs; ii++) { forwardDev = virXMLPropString(forwardIfNodes[ii], "dev"); + managed = virXMLPropString(forwardIfNodes[ii], "managed"); + if (managed != NULL) { + if (STREQ(managed, "yes")) + managedvalue = 1; + else + managedvalue = 0; + } if (!forwardDev) { virNetworkReportError(VIR_ERR_XML_ERROR, _("Missing required dev attribute in network '%s' forward interface element"), @@ -1204,12 +1232,22 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) forwardDev, def->name); goto error; } + + if (managedvalue != def->forwardIfs[0].managed) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("managed field of forward dev must match that of the first interface element dev in network '%s'"), + def->name); + goto error; + } + VIR_FREE(managed); VIR_FREE(forwardDev); continue; } def->forwardIfs[ii].dev = forwardDev; + def->forwardIfs[ii].managed = managedvalue; forwardDev = NULL; + managed = NULL; def->forwardIfs[ii].usageCount = 0; def->nForwardIfs++; } @@ -1224,6 +1262,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } VIR_FREE(forwardDev); + VIR_FREE(managed); VIR_FREE(forwardPfNodes); VIR_FREE(forwardIfNodes); @@ -1541,15 +1580,31 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) (def->nForwardIfs || def->nForwardPfs) ? "" : "/"); /* For now, hard-coded to at most 1 forwardPfs */ - if (def->nForwardPfs) - virBufferEscapeString(&buf, " <pf dev='%s'/>\n", + if (def->nForwardPfs) { + virBufferEscapeString(&buf, " <pf dev='%s'", def->forwardPfs[0].dev); + if (def->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) { + if (def->forwardPfs[0].managed == 1) + virBufferAddLit(&buf, " managed='yes'"); + else + virBufferAddLit(&buf, " managed='no'"); + } + virBufferAddLit(&buf, "/>\n"); + } + if (def->nForwardIfs && (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) { for (ii = 0; ii < def->nForwardIfs; ii++) { - virBufferEscapeString(&buf, " <interface dev='%s'/>\n", + virBufferEscapeString(&buf, " <interface dev='%s'", def->forwardIfs[ii].dev); + if (def->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) { + if (def->forwardIfs[ii].managed == 1) + virBufferAddLit(&buf, " managed='yes'"); + else + virBufferAddLit(&buf, " managed='no'"); + } + virBufferAddLit(&buf, "/>\n"); } } if (def->nForwardPfs || def->nForwardIfs) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index d473c71..4276934 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -135,6 +135,7 @@ struct _virNetworkForwardIfDef { char *dev; /* name of device */ int usageCount; /* how many guest interfaces are bound to this device? */ bool isPciAddr; /* Differentiate a VF based on interface name or pci addr*/ + unsigned int managed:1; }; typedef struct _virNetworkForwardPfDef virNetworkForwardPfDef; @@ -142,6 +143,7 @@ typedef virNetworkForwardPfDef *virNetworkForwardPfDefPtr; struct _virNetworkForwardPfDef { char *dev; /* name of device */ int usageCount; /* how many guest interfaces are bound to this device? */ + unsigned int managed:1; }; typedef struct _virPortGroupDef virPortGroupDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 691ab07..a2293e6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2807,6 +2807,9 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) { netdef->forwardIfs[ii].isPciAddr = true; else netdef->forwardIfs[ii].isPciAddr = false; + + if (netdef->forwardPfs[0].managed == 1) + netdef->forwardIfs[ii].managed = 1; } ret = 0; @@ -2942,7 +2945,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) iface->data.network.actual->data.hostdev.def.parent.data.net = iface; iface->data.network.actual->data.hostdev.def.info = &iface->info; iface->data.network.actual->data.hostdev.def.mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; - iface->data.network.actual->data.hostdev.def.managed = 1; + iface->data.network.actual->data.hostdev.def.managed = dev->managed; iface->data.network.actual->data.hostdev.def.source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; if (dev->isPciAddr == true) { -- 1.7.4.4
On 06/08/2012 11:30 AM, Shradha Shah wrote:
This patch supports the managed option in a network xml for network devices. This option is used for pci network devices when forward mode=hostdev.
Hence the example network xml would be:
<network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <pf dev="eth2" managed='yes'/> </forward> </network>
OR
<network> <name>direct-network</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8f</uuid> <forward mode="hostdev"> <interface dev="0000:04:00.1" managed='yes'/> <interface dev="0000:04:00.2" managed='yes'/> <interface dev="0000:04:00.3" managed='yes'/> </forward> </network>
This functionality should be included with the initial patch rather than as a later add-on.
Signed-off-by: Shradha Shah <sshah@solarflare.com> --- src/conf/network_conf.c | 61 ++++++++++++++++++++++++++++++++++++++++-- src/conf/network_conf.h | 2 + src/network/bridge_driver.c | 5 +++- 3 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6b346c3..18e4ee3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -986,6 +986,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) char *forwardDev = NULL; xmlNodePtr save = ctxt->node; xmlNodePtr bandwidthNode = NULL; + char *managed = NULL;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -1128,6 +1129,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) }
forwardDev = virXPathString("string(./@dev)", ctxt); + managed = virXPathString("string(./managed)", ctxt);
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); @@ -1151,6 +1153,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
+ if (managed) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("A managed field should not be used in this location when using a SRIOV PF")); + goto error; + } + forwardDev = virXMLPropString(*forwardPfNodes, "dev"); if (!forwardDev) { virNetworkReportError(VIR_ERR_XML_ERROR, @@ -1159,9 +1167,16 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
+ managed = virXMLPropString(*forwardPfNodes, "managed"); + if(managed != NULL) { + if (STREQ(managed, "yes")) + def->forwardPfs->managed = 1; + } + def->forwardPfs->usageCount = 0; def->forwardPfs->dev = forwardDev; forwardDev = NULL; + managed = NULL; def->nForwardPfs++; } else if (nForwardPfs > 1) { virNetworkReportError(VIR_ERR_XML_ERROR, @@ -1170,6 +1185,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } if (nForwardIfs > 0 || forwardDev) { int ii; + unsigned int managedvalue = 0;
/* allocate array to hold all the portgroups */ if (VIR_ALLOC_N(def->forwardIfs, MAX(nForwardIfs, 1)) < 0) { @@ -1181,12 +1197,24 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->forwardIfs[0].usageCount = 0; def->forwardIfs[0].dev = forwardDev; forwardDev = NULL; + if (managed != NULL) { + if (STREQ(managed, "yes")) + def->forwardIfs[0].managed = 1; + } + managed = NULL; def->nForwardIfs++; }
/* parse each forwardIf */ for (ii = 0; ii < nForwardIfs; ii++) { forwardDev = virXMLPropString(forwardIfNodes[ii], "dev"); + managed = virXMLPropString(forwardIfNodes[ii], "managed"); + if (managed != NULL) { + if (STREQ(managed, "yes")) + managedvalue = 1; + else + managedvalue = 0; + } if (!forwardDev) { virNetworkReportError(VIR_ERR_XML_ERROR, _("Missing required dev attribute in network '%s' forward interface element"), @@ -1204,12 +1232,22 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) forwardDev, def->name); goto error; } + + if (managedvalue != def->forwardIfs[0].managed) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("managed field of forward dev must match that of the first interface element dev in network '%s'"), + def->name); + goto error; + } + VIR_FREE(managed); VIR_FREE(forwardDev); continue; }
def->forwardIfs[ii].dev = forwardDev; + def->forwardIfs[ii].managed = managedvalue; forwardDev = NULL; + managed = NULL; def->forwardIfs[ii].usageCount = 0; def->nForwardIfs++; } @@ -1224,6 +1262,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) }
VIR_FREE(forwardDev); + VIR_FREE(managed); VIR_FREE(forwardPfNodes); VIR_FREE(forwardIfNodes);
@@ -1541,15 +1580,31 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) (def->nForwardIfs || def->nForwardPfs) ? "" : "/");
/* For now, hard-coded to at most 1 forwardPfs */ - if (def->nForwardPfs) - virBufferEscapeString(&buf, " <pf dev='%s'/>\n", + if (def->nForwardPfs) { + virBufferEscapeString(&buf, " <pf dev='%s'", def->forwardPfs[0].dev);
+ if (def->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) { + if (def->forwardPfs[0].managed == 1) + virBufferAddLit(&buf, " managed='yes'"); + else + virBufferAddLit(&buf, " managed='no'"); + } + virBufferAddLit(&buf, "/>\n"); + } + if (def->nForwardIfs && (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) { for (ii = 0; ii < def->nForwardIfs; ii++) { - virBufferEscapeString(&buf, " <interface dev='%s'/>\n", + virBufferEscapeString(&buf, " <interface dev='%s'", def->forwardIfs[ii].dev); + if (def->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) { + if (def->forwardIfs[ii].managed == 1) + virBufferAddLit(&buf, " managed='yes'"); + else + virBufferAddLit(&buf, " managed='no'"); + } + virBufferAddLit(&buf, "/>\n"); } } if (def->nForwardPfs || def->nForwardIfs) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index d473c71..4276934 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -135,6 +135,7 @@ struct _virNetworkForwardIfDef { char *dev; /* name of device */ int usageCount; /* how many guest interfaces are bound to this device? */ bool isPciAddr; /* Differentiate a VF based on interface name or pci addr*/ + unsigned int managed:1; };
typedef struct _virNetworkForwardPfDef virNetworkForwardPfDef; @@ -142,6 +143,7 @@ typedef virNetworkForwardPfDef *virNetworkForwardPfDefPtr; struct _virNetworkForwardPfDef { char *dev; /* name of device */ int usageCount; /* how many guest interfaces are bound to this device? */ + unsigned int managed:1; };
typedef struct _virPortGroupDef virPortGroupDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 691ab07..a2293e6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2807,6 +2807,9 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) { netdef->forwardIfs[ii].isPciAddr = true; else netdef->forwardIfs[ii].isPciAddr = false; + + if (netdef->forwardPfs[0].managed == 1) + netdef->forwardIfs[ii].managed = 1; }
ret = 0; @@ -2942,7 +2945,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) iface->data.network.actual->data.hostdev.def.parent.data.net = iface; iface->data.network.actual->data.hostdev.def.info = &iface->info; iface->data.network.actual->data.hostdev.def.mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; - iface->data.network.actual->data.hostdev.def.managed = 1; + iface->data.network.actual->data.hostdev.def.managed = dev->managed; iface->data.network.actual->data.hostdev.def.source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
if (dev->isPciAddr == true) {
Hello all, I have actually based these patches of v0.9.12. Top of tree libvirt had some errors due to which I was not able to use virsh and test these patches so I based them off the last release. But these patches should work equivalently well with top of tree. Many Thanks, Regards, Shradha Shah On 06/08/2012 04:26 PM, Shradha Shah wrote:
This patch series supports the forward mode='hostdev'. The functionality of this mode is the same as interface type='hostdev' but with the added benefit of using interface pools.
The patch series also contains a patch to support use of interface names and PCI device addresses interchangeably in a network xml, and return the appropriate one in actualDevice when networkAllocateActualDevice is called. This functionality is not supported for any other forward mode except hostdev.
Currently this patch series also does not support USB hostdev passthrough.
At the top level managed attribute can be specified for a pf dev or an interface dev (with identical results as when it's specified for a hostdev
Shradha Shah (5): Code to return interface name or pci_addr of the VF in actualDevice Moved the code to create implicit interface pool from PF to a new function Introduce forward mode='hostdev' for network XML in order to use the functionality of interface pools. Forward Mode Hostdev Implementation Supporting managed option for forward devs when using HOSTDEV mode
docs/schemas/network.rng | 1 + src/conf/network_conf.c | 119 +++++++++++++++- src/conf/network_conf.h | 13 ++- src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 325 +++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.c | 14 ++ src/util/pci.c | 7 +- src/util/pci.h | 3 + src/util/virnetdev.c | 134 +++++++++++++++++- src/util/virnetdev.h | 19 +++ 10 files changed, 568 insertions(+), 70 deletions(-)
On 06/08/2012 12:06 PM, Shradha Shah wrote:
Hello all,
I have actually based these patches of v0.9.12.
Top of tree libvirt had some errors due to which I was not able to use virsh and test these patches so I based them off the last release.
Do you still have the same problem with the latest from git? It will be really nice to have this functionality in - I've considered the type='hostdev' support incomplete without it. Some general comments beyond (or reinforcing) those I already made in responses to the individual patches: * You have no documentation (should be added to docs/formatnetwork.html.in and docs/formatdomain.html.in), which really needs to be in the same patch that enables the new functionality. * Likewise I didn't see any new test case (tests/networkxml2xml*). * In general, I think it's useful to arrange multiple patches like this: * One or more patches that re-factor existing code (without causing any functional change) to prepare for the new code. * A patch that adds the new XML parsing/formatting, updates the RNG, and adds xml2xml tests. * A patch that connects the new functionality to the backend (in this case, much of that is already done, since the "<interface type='hostdev'>" patches were written such that networkAllocationActualDevice is called at the appropriate place and the actualdevice object is queried when appropriate (obviously there will be bugs in this, since it couldn't be properly tested until now). You may find that it useful to split the 3rd item into 2 parts: 1) a patch that updates the network driver to properly utilize the new attributes/elements that are now in virNetworkDef (i.e. allowing the pf element to populate all the devs/addresses) and 2) A patch that fixes any latent bugs in the qemu driver utilizing the returned dev/address). I guess in the end there are 4 rules of thumb I try to think of when I'm putting together a patch series (others probably have their own set of personal rules): 1) try to make each patch as easy to understand as possible, while avoiding having a large number of related, yet trivial, patches. 2) unrelated changes (or changes that could be useful separately from the rest of the series, e.g. general purpose utility functions) should be in separate patches. 3) try to avoid code churn - if something will move, move it once; when adding new code, the patch that introduces it should introduce it where you want it to be at the end of the series. 4) above anything else, it is a requirement that make check (and other existing libvirt operation) work correctly at any step in applying the patches. Sometimes this will require temporarily putting in a small bit of stub code, omitting/modifying a test, or even combining two patches that you would rather have separated, but it is necessary in order for git bisect to be useful.
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Shradha Shah