9:46 a.m.
Interface Pools and Passthrough mode:
Current Method:
The passthrough mode uses a macvtap a direct connection to connect each guest to the
network. The physical interface to be used is picked from among those listed in
<interface> sub elements of the <forward> element.
The current specification for <forward> extends to allow 0 or more <interface>
sub-elements:
Example:
<forward mode='passthrough' dev='eth10'/>
<interface dev='eth10'/>
<interface dev='eth12'/>
<interface dev='eth18'/>
<interface dev='eth20'/>
</forward>
However with an ethernet card with 64 VF's or more, the above method gets tedious on
the system.
On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are
multiple non-contiguous ranges.
Proposed Method:
The 5 patches provided along with this introductory e-mail
i) Introduce a structure to store the state of all the virtual functions attached to each
physical function
ii) Find a free virtual function given the physical function.
The forward specification will hence only mention the physical function as the interface
sub element:
Example:
<forward mode='passthrough' dev='eth2'/>
<interface dev='eth2'/>
</forward>
Shradha Shah (5):
Moving the declaration of _pciDevice and _pciDeviceList to pci.h
Added function pciSysfsFile to enable access to the PCI SYSFS files.
Adding functions virNetDevGetVirtualFunctions and virNetDevGetNetName
virNetDevGetVirtualFunctions: Gets the BDF of all the Virtual
Functions given a physical function virNetDevGetNetName: Gets
the interface name of the PCI Device.
Addition of a new device structure to store the state of a Virtual
Function Modifications to the Physical Device Structure to
store state of its Virtual Functions
If a system has 64 or more VF's, it is quite tedious to mention each
VF in the interface pool. The following modification find a
Free VF given a Physical Function when mode=passthrough. We
also save the state of each VF. Hence modifications to
networkAllocateActualDevice, networkNotifyActualDevice and
networkReleaseActualDevice were required. The following
patch does just that.
src/conf/network_conf.c | 19 ++++-
src/conf/network_conf.h | 10 ++
src/network/bridge_driver.c | 208 +++++++++++++++++++++++++++++++++++++------
src/util/pci.c | 52 ++++-------
src/util/pci.h | 36 ++++++++
src/util/virnetdev.c | 89 ++++++++++++++++++
src/util/virnetdev.h | 10 ++
7 files changed, 359 insertions(+), 65 deletions(-)
--
1.7.4.4
9:48 a.m.
New subject: [libvirt] [PATCH 1/5] Moving the declaration of _pciDevice and _pciDeviceList to pci.h
---
src/util/pci.c | 35 -----------------------------------
src/util/pci.h | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index cd82b43..857078d 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -45,45 +45,10 @@
# define MODPROBE "modprobe"
#endif
-#define PCI_SYSFS "/sys/bus/pci/"
-#define PCI_ID_LEN 10 /* "XXXX XXXX" */
-#define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */
-
#define SRIOV_FOUND 0
#define SRIOV_NOT_FOUND 1
#define SRIOV_ERROR -1
-struct _pciDevice {
- unsigned domain;
- unsigned bus;
- unsigned slot;
- unsigned function;
-
- char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */
- char id[PCI_ID_LEN]; /* product vendor */
- char *path;
- const char *used_by; /* The domain which uses the device */
- int fd;
-
- unsigned initted;
- unsigned pcie_cap_pos;
- unsigned pci_pm_cap_pos;
- unsigned has_flr : 1;
- unsigned has_pm_reset : 1;
- unsigned managed : 1;
-
- /* used by reattach function */
- unsigned unbind_from_stub : 1;
- unsigned remove_slot : 1;
- unsigned reprobe : 1;
-};
-
-struct _pciDeviceList {
- unsigned count;
- pciDevice **devs;
-};
-
-
/* For virReportOOMError() and virReportSystemError() */
#define VIR_FROM_THIS VIR_FROM_NONE
diff --git a/src/util/pci.h b/src/util/pci.h
index 76e37e3..8e47fc2 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -24,9 +24,43 @@
# include "internal.h"
+#define PCI_SYSFS "/sys/bus/pci/"
+#define PCI_ID_LEN 10 /* "XXXX XXXX" */
+#define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */
+
typedef struct _pciDevice pciDevice;
typedef struct _pciDeviceList pciDeviceList;
+struct _pciDevice {
+ unsigned domain;
+ unsigned bus;
+ unsigned slot;
+ unsigned function;
+
+ char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */
+ char id[PCI_ID_LEN]; /* product vendor */
+ char *path;
+ const char *used_by; /* The domain which uses the device */
+ int fd;
+
+ unsigned initted;
+ unsigned pcie_cap_pos;
+ unsigned pci_pm_cap_pos;
+ unsigned has_flr : 1;
+ unsigned has_pm_reset : 1;
+ unsigned managed : 1;
+
+ /* used by reattach function */
+ unsigned unbind_from_stub : 1;
+ unsigned remove_slot : 1;
+ unsigned reprobe : 1;
+};
+
+struct _pciDeviceList {
+ unsigned count;
+ pciDevice **devs;
+};
+
struct pci_config_address {
unsigned int domain;
unsigned int bus;
--
1.7.4.4
1:56 p.m.
New subject: [libvirt] [PATCH 1/5] Moving the declaration of _pciDevice and _pciDeviceList to pci.h
On Tue, Nov 29, 2011 at 03:48:29PM +0000, Shradha Shah wrote:
---
src/util/pci.c | 35 -----------------------------------
src/util/pci.h | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index cd82b43..857078d 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -45,45 +45,10 @@
# define MODPROBE "modprobe"
#endif
-#define PCI_SYSFS "/sys/bus/pci/"
-#define PCI_ID_LEN 10 /* "XXXX XXXX" */
-#define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */
-
#define SRIOV_FOUND 0
#define SRIOV_NOT_FOUND 1
#define SRIOV_ERROR -1
-struct _pciDevice {
- unsigned domain;
- unsigned bus;
- unsigned slot;
- unsigned function;
-
- char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */
- char id[PCI_ID_LEN]; /* product vendor */
- char *path;
- const char *used_by; /* The domain which uses the device */
- int fd;
-
- unsigned initted;
- unsigned pcie_cap_pos;
- unsigned pci_pm_cap_pos;
- unsigned has_flr : 1;
- unsigned has_pm_reset : 1;
- unsigned managed : 1;
-
- /* used by reattach function */
- unsigned unbind_from_stub : 1;
- unsigned remove_slot : 1;
- unsigned reprobe : 1;
-};
-
-struct _pciDeviceList {
- unsigned count;
- pciDevice **devs;
-};
-
-
/* For virReportOOMError() and virReportSystemError() */
#define VIR_FROM_THIS VIR_FROM_NONE
diff --git a/src/util/pci.h b/src/util/pci.h
index 76e37e3..8e47fc2 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -24,9 +24,43 @@
# include "internal.h"
+#define PCI_SYSFS "/sys/bus/pci/"
+#define PCI_ID_LEN 10 /* "XXXX XXXX" */
+#define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */
+
typedef struct _pciDevice pciDevice;
typedef struct _pciDeviceList pciDeviceList;
+struct _pciDevice {
+ unsigned domain;
+ unsigned bus;
+ unsigned slot;
+ unsigned function;
+
+ char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */
+ char id[PCI_ID_LEN]; /* product vendor */
+ char *path;
+ const char *used_by; /* The domain which uses the device */
+ int fd;
+
+ unsigned initted;
+ unsigned pcie_cap_pos;
+ unsigned pci_pm_cap_pos;
+ unsigned has_flr : 1;
+ unsigned has_pm_reset : 1;
+ unsigned managed : 1;
+
+ /* used by reattach function */
+ unsigned unbind_from_stub : 1;
+ unsigned remove_slot : 1;
+ unsigned reprobe : 1;
+};
+
+struct _pciDeviceList {
+ unsigned count;
+ pciDevice **devs;
+};
I could have misread something, but looking at the following 4 patches,
I don't see any code which requires direct access to the internals of
the 'struct pciDevice' type. If there is such a need I've missed, then
it would be desirable to just add more setter/getter APIs in the pci.h,
and keep 'struct pciDevice' private to pci.c
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 :|
9:48 a.m.
New subject: [libvirt] [PATCH 2/5] Added function pciSysfsFile to enable access to the PCI SYSFS files.
---
src/util/pci.c | 17 +++++++++++++++++
src/util/pci.h | 2 ++
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index 857078d..89a9d12 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -39,6 +39,7 @@
#include "command.h"
#include "virterror_internal.h"
#include "virfile.h"
+#include "virnetdev.h"
/* avoid compilation breakage on some systems */
#ifndef MODPROBE
@@ -1991,6 +1992,22 @@ out:
return ret;
}
+ /*
+ * Returns a path to the PCI sysfs file given the BDF of the PCI function
+ */
+
+int
+pciSysfsFile(char **pci_sysfs_device_link, char *pciDeviceName)
+{
+ if (virAsprintf(pci_sysfs_device_link, PCI_SYSFS "devices/%s",
+ pciDeviceName) < 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ return 0;
+}
+
/*
* Returns the network device name of a pci device
*/
diff --git a/src/util/pci.h b/src/util/pci.h
index 8e47fc2..3800d98 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -143,4 +143,6 @@ int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
+int pciSysfsFile(char **pci_sysfs_device_link, char *pciDeviceName);
+
#endif /* __VIR_PCI_H__ */
--
1.7.4.4
1:58 p.m.
New subject: [libvirt] [PATCH 2/5] Added function pciSysfsFile to enable access to the PCI SYSFS files.
On Tue, Nov 29, 2011 at 03:48:52PM +0000, Shradha Shah wrote:
---
src/util/pci.c | 17 +++++++++++++++++
src/util/pci.h | 2 ++
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index 857078d..89a9d12 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -39,6 +39,7 @@
#include "command.h"
#include "virterror_internal.h"
#include "virfile.h"
+#include "virnetdev.h"
I think this can be removed.
@@ -1991,6 +1992,22 @@ out:
return ret;
}
+ /*
+ * Returns a path to the PCI sysfs file given the BDF of the PCI function
+ */
+
+int
+pciSysfsFile(char **pci_sysfs_device_link, char *pciDeviceName)
+{
+ if (virAsprintf(pci_sysfs_device_link, PCI_SYSFS "devices/%s",
+ pciDeviceName) < 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ return 0;
+}
+
/*
* Returns the network device name of a pci device
*/
diff --git a/src/util/pci.h b/src/util/pci.h
index 8e47fc2..3800d98 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -143,4 +143,6 @@ int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
+int pciSysfsFile(char **pci_sysfs_device_link, char *pciDeviceName);
My preference would be to swap the order of these two args, so the
out parameter is last. Functionally though, the patch is fine.
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 :|
9:15 a.m.
New subject: [libvirt] [PATCH] Changed the order of arguments of function pciSysfsFile
---
src/util/pci.c | 3 +--
src/util/pci.h | 2 +-
src/util/virnetdev.c | 2 +-
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index 89a9d12..311a847 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -39,7 +39,6 @@
#include "command.h"
#include "virterror_internal.h"
#include "virfile.h"
-#include "virnetdev.h"
/* avoid compilation breakage on some systems */
#ifndef MODPROBE
@@ -1997,7 +1996,7 @@ out:
*/
int
-pciSysfsFile(char **pci_sysfs_device_link, char *pciDeviceName)
+pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link)
{
if (virAsprintf(pci_sysfs_device_link, PCI_SYSFS "devices/%s",
pciDeviceName) < 0) {
diff --git a/src/util/pci.h b/src/util/pci.h
index 3800d98..30ff6ff 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -143,6 +143,6 @@ int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
-int pciSysfsFile(char **pci_sysfs_device_link, char *pciDeviceName);
+int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link);
#endif /* __VIR_PCI_H__ */
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index d07d8fa..0b3f91a 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1017,7 +1017,7 @@ virNetDevGetNetName(struct pci_config_address *vf, char **vfname)
dev = pciGetDevice(vf->domain, vf->bus, vf->slot, vf->function);
if (dev != NULL) {
- if (pciSysfsFile(&pci_sysfs_device_link, dev->name) < 0) {
+ if (pciSysfsFile(dev->name, &pci_sysfs_device_link) < 0) {
virReportSystemError(ENOSYS, "%s",
_("Failed to get PCI SYSFS file"));
goto out;
--
1.7.4.4
9:49 a.m.
New subject: [libvirt] [PATCH 3/5] Adding functions virNetDevGetVirtualFunctions and virNetDevGetNetName virNetDevGetVirtualFunctions: Gets the BDF of all the Virtual Functions given a physical function virNetDevGetNetName: Gets the interface name of the PCI Device.
---
src/util/virnetdev.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virnetdev.h | 10 ++++++
2 files changed, 99 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 86196a1..d07d8fa 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -968,6 +968,76 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char
*ifname,
}
/**
+ * virNetDevGetVirtualFunctions:
+ *
+ * @pfname : name of the physical function interface name
+ * @virt_fns: array that will hold the pointers to the virtual_functions
+ * @num_virt_fns: number of virtual functions
+ *
+ * Returns 0 on success and -1 on failure
+ */
+
+int
+virNetDevGetVirtualFunctions(const char *pfname,
+ struct pci_config_address ***virt_fns,
+ unsigned int *num_virt_fns)
+{
+ int ret = -1;
+ char *pf_sysfs_device_link = NULL;
+
+
+ if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") <
0)
+ return ret;
+
+ if (pciGetVirtualFunctions(pf_sysfs_device_link, virt_fns,
+ num_virt_fns) < 0)
+ goto out;
+ ret = 0;
+
+out:
+ VIR_FREE(pf_sysfs_device_link);
+ return ret;
+}
+
+/**
+ * virNetDevGetNetName:
+ *
+ * @vf : pci address (BDF) of the Virtual Function
+ * @vfname: interface name of the virtual function returned by this function
+ *
+ * Returns 0 on success and -1 on failure
+ */
+
+int
+virNetDevGetNetName(struct pci_config_address *vf, char **vfname)
+{
+ int ret = -1;
+ pciDevice *dev = NULL;
+ char *pci_sysfs_device_link = NULL;
+
+ dev = pciGetDevice(vf->domain, vf->bus, vf->slot, vf->function);
+ if (dev != NULL) {
+ if (pciSysfsFile(&pci_sysfs_device_link, dev->name) < 0) {
+ virReportSystemError(ENOSYS, "%s",
+ _("Failed to get PCI SYSFS file"));
+ goto out;
+ }
+
+ if (pciDeviceNetName(pci_sysfs_device_link, vfname) < 0) {
+ virReportSystemError(ENOSYS, "%s",
+ _("Failed to get interface name of the VF"));
+ goto out;
+ }
+ }
+ ret = 0;
+
+out:
+ VIR_FREE(pci_sysfs_device_link);
+ pciFreeDevice(dev);
+ return ret;
+}
+
+/**
* virNetDevIsVirtualFunction:
* @ifname : name of the interface
*
@@ -1055,6 +1125,25 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
}
#else /* !__linux__ */
int
+virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED,
+ struct pci_config_address ***virt_fns ATTRIBUTE_UNUSED,
+ unsigned int *num_virt_fns ATTRIBUTE_UNUSED)
+{
+ virReportSystemError(ENOSYS, "%s",
+ _("Unable to get virtual functions on this
platfornm"));
+ return -1;
+}
+
+int
+virNetDevGetNetName(struct pci_config_address *vf ATTRIBUTE_UNUSED,
+ char **vfname ATTRIBUTE_UNUSED)
+{
+ virReportSystemError(ENOSYS, "%s",
+ _("Unable to get Device name on this platfornm"));
+ return -1;
+}
+
+int
virNetDevIsVirtualFunction(const char *ifname ATTRIBUTE_UNUSED)
{
virReportSystemError(ENOSYS, "%s",
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 13ba1da..4d1573a 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -24,6 +24,7 @@
# define __VIR_NETDEV_H__
# include "virsocketaddr.h"
+# include "pci.h"
int virNetDevExists(const char *brname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
@@ -99,4 +100,13 @@ int virNetDevGetVirtualFunctionIndex(const char *pfname, const char
*vfname,
int virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virNetDevGetVirtualFunctions(const char *pfname,
+ struct pci_config_address ***virt_fns,
+ unsigned int *num_virt_fns)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+
+int virNetDevGetNetName(struct pci_config_address *vf, char **vfname)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
#endif /* __VIR_NETDEV_H__ */
--
1.7.4.4
2:01 p.m.
New subject: [libvirt] [PATCH 3/5] Adding functions virNetDevGetVirtualFunctions and virNetDevGetNetName virNetDevGetVirtualFunctions: Gets the BDF of all the Virtual Functions given a physical function virNetDevGetNetName: Gets the interface name of the PCI Device.
On Tue, Nov 29, 2011 at 03:49:05PM +0000, Shradha Shah wrote:
---
src/util/virnetdev.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virnetdev.h | 10 ++++++
2 files changed, 99 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 86196a1..d07d8fa 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -968,6 +968,76 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char
*ifname,
}
/**
+ * virNetDevGetVirtualFunctions:
+ *
+ * @pfname : name of the physical function interface name
+ * @virt_fns: array that will hold the pointers to the virtual_functions
+ * @num_virt_fns: number of virtual functions
+ *
+ * Returns 0 on success and -1 on failure
+ */
+
+int
+virNetDevGetVirtualFunctions(const char *pfname,
+ struct pci_config_address ***virt_fns,
+ unsigned int *num_virt_fns)
+{
+ int ret = -1;
+ char *pf_sysfs_device_link = NULL;
+
+
+ if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") <
0)
+ return ret;
+
+ if (pciGetVirtualFunctions(pf_sysfs_device_link, virt_fns,
+ num_virt_fns) < 0)
+ goto out;
+ ret = 0;
+
+out:
+ VIR_FREE(pf_sysfs_device_link);
+ return ret;
+}
+
+/**
+ * virNetDevGetNetName:
+ *
+ * @vf : pci address (BDF) of the Virtual Function
+ * @vfname: interface name of the virtual function returned by this function
+ *
+ * Returns 0 on success and -1 on failure
+ */
+
+int
+virNetDevGetNetName(struct pci_config_address *vf, char **vfname)
+{
+ int ret = -1;
+ pciDevice *dev = NULL;
+ char *pci_sysfs_device_link = NULL;
+
+ dev = pciGetDevice(vf->domain, vf->bus, vf->slot, vf->function);
+ if (dev != NULL) {
+ if (pciSysfsFile(&pci_sysfs_device_link, dev->name) < 0) {
+ virReportSystemError(ENOSYS, "%s",
+ _("Failed to get PCI SYSFS file"));
+ goto out;
+ }
+
+ if (pciDeviceNetName(pci_sysfs_device_link, vfname) < 0) {
+ virReportSystemError(ENOSYS, "%s",
+ _("Failed to get interface name of the VF"));
+ goto out;
+ }
+ }
+ ret = 0;
+
+out:
+ VIR_FREE(pci_sysfs_device_link);
+ pciFreeDevice(dev);
+ return ret;
+}
+
+/**
* virNetDevIsVirtualFunction:
* @ifname : name of the interface
*
@@ -1055,6 +1125,25 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
}
#else /* !__linux__ */
int
+virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED,
+ struct pci_config_address ***virt_fns ATTRIBUTE_UNUSED,
+ unsigned int *num_virt_fns ATTRIBUTE_UNUSED)
+{
+ virReportSystemError(ENOSYS, "%s",
+ _("Unable to get virtual functions on this
platfornm"));
+ return -1;
+}
+
+int
+virNetDevGetNetName(struct pci_config_address *vf ATTRIBUTE_UNUSED,
+ char **vfname ATTRIBUTE_UNUSED)
+{
+ virReportSystemError(ENOSYS, "%s",
+ _("Unable to get Device name on this platfornm"));
+ return -1;
+}
+
+int
virNetDevIsVirtualFunction(const char *ifname ATTRIBUTE_UNUSED)
{
virReportSystemError(ENOSYS, "%s",
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 13ba1da..4d1573a 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -24,6 +24,7 @@
# define __VIR_NETDEV_H__
# include "virsocketaddr.h"
+# include "pci.h"
int virNetDevExists(const char *brname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
@@ -99,4 +100,13 @@ int virNetDevGetVirtualFunctionIndex(const char *pfname, const char
*vfname,
int virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virNetDevGetVirtualFunctions(const char *pfname,
+ struct pci_config_address ***virt_fns,
+ unsigned int *num_virt_fns)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+
+int virNetDevGetNetName(struct pci_config_address *vf, char **vfname)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
Rather than expose the 'struct pci_config_address' in the virnetdev.h
header file, I think it would be desirable to merge the virNetDevGetNameName
code into virNetDevGetVirtualFunctions, so that it just directly returns the
list of VF devices. eg
int virNetDevGetVirtualFunctions(const char *pfname,
char **vfnames,
unsigned int *nvfnames)
The callers all seem to call virNetDevGetNetName on the results of
virNetDevGetVirtualFunctions, so this merge would simplify code in
the callers somewhat.
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 :|
9:49 a.m.
New subject: [libvirt] [PATCH 4/5] Addition of a new device structure to store the state of a Virtual Function Modifications to the Physical Device Structure to store state of its Virtual Functions
---
src/conf/network_conf.c | 19 ++++++++++++++++++-
src/conf/network_conf.h | 10 ++++++++++
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 10afcde..d30d757 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -43,6 +43,7 @@
#include "buf.h"
#include "c-ctype.h"
#include "virfile.h"
+#include "logging.h"
#define MAX_BRIDGE_ID 256
#define VIR_FROM_THIS VIR_FROM_NETWORK
@@ -103,6 +104,12 @@ virNetworkForwardIfDefClear(virNetworkForwardIfDefPtr def)
VIR_FREE(def->dev);
}
+static void
+virNetworkForwardVfDefClear(virNetworkForwardVfDefPtr def)
+{
+ VIR_FREE(def->dev);
+}
+
static void virNetworkIpDefClear(virNetworkIpDefPtr def)
{
int ii;
@@ -144,7 +151,7 @@ static void virNetworkDNSDefFree(virNetworkDNSDefPtr def)
void virNetworkDefFree(virNetworkDefPtr def)
{
- int ii;
+ int ii, jj;
if (!def)
return;
@@ -154,6 +161,10 @@ void virNetworkDefFree(virNetworkDefPtr def)
VIR_FREE(def->domain);
for (ii = 0 ; ii < def->nForwardIfs && def->forwardIfs ; ii++) {
+ for (jj = 0 ; jj < (def->forwardIfs[ii]).nForwardVfs ; jj++) {
+ virNetworkForwardVfDefClear(&(def->forwardIfs[ii].forwardVfs[jj]));
+ }
+ VIR_FREE(def->forwardIfs[ii].forwardVfs);
virNetworkForwardIfDefClear(&def->forwardIfs[ii]);
}
VIR_FREE(def->forwardIfs);
@@ -979,6 +990,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
if (forwardDev) {
def->forwardIfs[0].usageCount = 0;
+ def->forwardIfs[0].vfs_in_use_count = 0;
+ def->forwardIfs[0].nForwardVfs = 0;
+ def->forwardIfs[0].forwardVfs = NULL;
def->forwardIfs[0].dev = forwardDev;
forwardDev = NULL;
def->nForwardIfs++;
@@ -1011,6 +1025,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
def->forwardIfs[ii].dev = forwardDev;
forwardDev = NULL;
def->forwardIfs[ii].usageCount = 0;
+ def->forwardIfs[ii].vfs_in_use_count = 0;
+ def->forwardIfs[ii].nForwardVfs = 0;
+ def->forwardIfs[ii].forwardVfs = NULL;
def->nForwardIfs++;
}
}
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 1be20f8..d1dba94 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -112,11 +112,21 @@ struct _virNetworkIpDef {
virSocketAddr bootserver;
};
+typedef struct _virNetworkForwardVfDef virNetworkForwardVfDef;
+typedef virNetworkForwardVfDef *virNetworkForwardVfDefPtr;
+struct _virNetworkForwardVfDef {
+ char *dev;
+ int usageCount;
+};
+
typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr;
struct _virNetworkForwardIfDef {
char *dev; /* name of device */
int usageCount; /* how many guest interfaces are bound to this device? */
+ int vfs_in_use_count; /*if SRIOV, Are any of the Vf's on the dev in use? */
+ size_t nForwardVfs;
+ virNetworkForwardVfDefPtr forwardVfs;
};
typedef struct _virPortGroupDef virPortGroupDef;
--
1.7.4.4
9:49 a.m.
New subject: [libvirt] [PATCH 5/5] If a system has 64 or more VF's, it is quite tedious to mention each VF in the interface pool. The following modification find a Free VF given a Physical Function when mode=passthrough. We also save the state of each VF. Hence modifications to networkAllocateActualDevice, networkNotifyActualDevice and networkReleaseActualDevice were required. The following patch does just that.
---
src/network/bridge_driver.c | 208 +++++++++++++++++++++++++++++++++++++------
1 files changed, 179 insertions(+), 29 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c49c25b..a2e3119 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -55,6 +55,7 @@
#include "memory.h"
#include "uuid.h"
#include "iptables.h"
+#include "pci.h"
#include "logging.h"
#include "dnsmasq.h"
#include "configmake.h"
@@ -2823,8 +2824,11 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
goto cleanup;
} else {
int ii;
- virNetworkForwardIfDefPtr dev = NULL;
-
+ virNetworkForwardVfDefPtr dev = NULL;
+ unsigned int vf_found = 0;
+ unsigned int num_virt_fns = 0;
+ struct pci_config_address **virt_fns = NULL;
+
/* pick an interface from the pool */
/* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
@@ -2832,32 +2836,105 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
* 0. Other modes can share, so just search for the one with
* the lowest usageCount.
*/
- if ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) ||
- ((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))) {
- /* pick first dev with 0 usageCount */
+ if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) {
+ if (netdef->nForwardIfs == 1) {
+ if (netdef->forwardIfs[0].vfs_in_use_count == 0) {
+ if ((virNetDevGetVirtualFunctions(netdef->forwardIfs[0].dev,
+ &virt_fns, &num_virt_fns))
< 0){
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get Virtual functions on
%s"),
+ netdef->forwardIfs[0].dev);
+ goto out;
+ }
+
+ netdef->forwardIfs[0].nForwardVfs = num_virt_fns;
+
+ if ((VIR_ALLOC_N(netdef->forwardIfs[0].forwardVfs,
+ netdef->forwardIfs[0].nForwardVfs)) < 0)
{
+ virReportOOMError();
+ goto out;
+ }
+
+ for (ii = 0; ii < netdef->forwardIfs[0].nForwardVfs; ii++)
{
+ if ((virNetDevGetNetName(virt_fns[ii],
+
&netdef->forwardIfs[0].forwardVfs[ii].dev)) < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get Interface
name"));
+ goto out;
+ }
+ netdef->forwardIfs[0].forwardVfs[ii].usageCount = 0;
+ }
+ }
+
+ for (ii = 0; ii < netdef->forwardIfs[0].nForwardVfs; ii++) {
+ if (netdef->forwardIfs[0].forwardVfs[ii].usageCount == 0) {
+ netdef->forwardIfs[0].vfs_in_use_count++;
+ dev = &netdef->forwardIfs[0].forwardVfs[ii];
+ vf_found = 1;
+ break;
+ }
+ }
+ /* If No Vf's are present or if Vf's are in use
+ * check whether the PF is free and assign PF
+ * to dev
+ */
+ if(vf_found == 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("No Free Vf's on %s, checking if PF
is free"),
+ netdef->forwardIfs[0].dev);
+ if (netdef->forwardIfs[0].usageCount == 0) {
+ dev = (virNetworkForwardVfDef
*)&netdef->forwardIfs[0];
+ }
+ }
+ vf_found = 0;
+ }
+ /* If more than 1 ForwardIfs are present check usagecount of each
+ * find the one that is free
+ */
+ else {
+ for (ii = 0; ii < netdef->nForwardIfs; ii++) {
+ if (netdef->forwardIfs[ii].usageCount == 0) {
+ dev = (virNetworkForwardVfDef
*)&netdef->forwardIfs[ii];
+ break;
+ }
+ }
+ }
+ }
+ 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)) {
+
+
+ /* pick first dev with 0 usageCount */
+
for (ii = 0; ii < netdef->nForwardIfs; ii++) {
if (netdef->forwardIfs[ii].usageCount == 0) {
- dev = &netdef->forwardIfs[ii];
+ dev = (virNetworkForwardVfDef *)&netdef->forwardIfs[ii];
break;
}
}
- } else {
+ }
+ else {
/* pick least used dev */
- dev = &netdef->forwardIfs[0];
+ dev = (virNetworkForwardVfDef *)&netdef->forwardIfs[0];
for (ii = 1; ii < netdef->nForwardIfs; ii++) {
if (netdef->forwardIfs[ii].usageCount < dev->usageCount)
- dev = &netdef->forwardIfs[ii];
+ dev = (virNetworkForwardVfDef *)&netdef->forwardIfs[ii];
}
}
+
+ out:
+ for (ii = 0; ii < num_virt_fns; ii++)
+ VIR_FREE(virt_fns[ii]);
+ VIR_FREE(virt_fns);
+
/* dev points at the physical device we want to use */
if (!dev) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
_("network '%s' requires exclusive access
to interfaces, but none are available"),
- netdef->name);
+ netdef->name);
goto cleanup;
}
iface->data.network.actual->data.direct.linkdev = strdup(dev->dev);
@@ -2871,7 +2948,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
dev->dev, dev->usageCount);
}
}
-
+
ret = 0;
cleanup:
if (network)
@@ -2935,17 +3012,55 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
netdef->name);
goto cleanup;
} else {
- int ii;
- virNetworkForwardIfDefPtr dev = NULL;
-
+ int ii, jj, isVf;
+ virNetworkForwardVfDefPtr dev = NULL;
+ char *pfDeviceName;
+
/* find the matching interface in the pool and increment its usageCount */
+
+ isVf = virNetDevIsVirtualFunction(actualDev);
- for (ii = 0; ii < netdef->nForwardIfs; ii++) {
- if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) {
- dev = &netdef->forwardIfs[ii];
- break;
+ if (isVf == 1) {
+ /*If Vf get PF : pciGetPhysicalFunction */
+ if ((virNetDevGetPhysicalFunction(actualDev, &pfDeviceName)) < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get Physical functions for
%s"),
+ actualDev);
+ goto out;
+ }
+
+ /* Find the matching PF from the list of netdef->forwardIfs
+ * Search through the ForwardVfs list of the PF to find the VF
+ */
+ for (ii = 0; ii < netdef->nForwardIfs; ii++) {
+ if (STREQ(pfDeviceName, netdef->forwardIfs[ii].dev)) {
+ for (jj = 0; jj < netdef->forwardIfs[ii].nForwardVfs; jj++) {
+ if (STREQ(actualDev,
netdef->forwardIfs[ii].forwardVfs[jj].dev)) {
+ dev = &netdef->forwardIfs[ii].forwardVfs[jj];
+ break;
+ }
+ }
+ }
}
}
+ else if (isVf == 0) {
+ /*If the actual dev is a PF do the following code*/
+ for (ii = 0; ii < netdef->nForwardIfs; ii++) {
+ if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) {
+ dev = (virNetworkForwardVfDef *)&netdef->forwardIfs[ii];
+ break;
+ }
+ }
+ }
+ else {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not find whether %s is PF or VF"),
+ actualDev);
+ goto out;
+ }
+
+ out:
+
/* dev points at the physical device we want to use */
if (!dev) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2953,7 +3068,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
netdef->name, actualDev);
goto cleanup;
}
-
+
/* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
* exclusive access to a device, so current usageCount must be
* 0 in those cases.
@@ -3036,15 +3151,50 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
netdef->name);
goto cleanup;
} else {
- int ii;
- virNetworkForwardIfDefPtr dev = NULL;
-
- for (ii = 0; ii < netdef->nForwardIfs; ii++) {
- if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) {
- dev = &netdef->forwardIfs[ii];
- break;
+ int ii, jj, isVf;
+ virNetworkForwardVfDefPtr dev = NULL;
+ char *pfDeviceName;
+
+ isVf = virNetDevIsVirtualFunction(actualDev);
+
+ if (isVf == 1) {
+ /*If Vf get PF */
+ if ((virNetDevGetPhysicalFunction(actualDev, &pfDeviceName)) < 0) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get Physical functions for
%s"),
+ actualDev);
+ goto out;
}
+
+ /* Find the matching PF from the list of netdef->forwardIfs
+ * search through the ForwardVfs list of the PF to find the VF
+ */
+ for (ii = 0; ii < netdef->nForwardIfs; ii++) {
+ if (STREQ(pfDeviceName, netdef->forwardIfs[ii].dev)) {
+ for (jj = 0; jj < netdef->forwardIfs[ii].nForwardVfs; jj++) {
+ if (STREQ(actualDev,
netdef->forwardIfs[ii].forwardVfs[jj].dev)) {
+ dev = &netdef->forwardIfs[ii].forwardVfs[jj];
+ break;
+ }
+ }
+ }
+ }
+ }
+ else if (isVf == 0) {
+ for (ii = 0; ii < netdef->nForwardIfs; ii++) {
+ if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) {
+ dev = (virNetworkForwardVfDef *)&netdef->forwardIfs[ii];
+ break;
+ }
+ }
+ }
+ else {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not find whether %s is PF or VF"),
+ actualDev);
+ goto out;
}
+ out:
/* dev points at the physical device we've been using */
if (!dev) {
networkReportError(VIR_ERR_INTERNAL_ERROR,
--
1.7.4.4
1:53 p.m.
On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
Interface Pools and Passthrough mode:
Current Method:
The passthrough mode uses a macvtap a direct connection to connect each guest to the
network. The physical interface to be used is picked from among those listed in
<interface> sub elements of the <forward> element.
The current specification for <forward> extends to allow 0 or more
<interface> sub-elements:
Example:
<forward mode='passthrough' dev='eth10'/>
<interface dev='eth10'/>
<interface dev='eth12'/>
<interface dev='eth18'/>
<interface dev='eth20'/>
</forward>
However with an ethernet card with 64 VF's or more, the above method gets tedious on
the system.
On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are
multiple non-contiguous ranges.
Proposed Method:
The 5 patches provided along with this introductory e-mail
i) Introduce a structure to store the state of all the virtual functions attached to each
physical function
ii) Find a free virtual function given the physical function.
The forward specification will hence only mention the physical function as the interface
sub element:
Example:
<forward mode='passthrough' dev='eth2'/>
<interface dev='eth2'/>
</forward>
I can see what you mean about it being tedious to construct the config with
all 64 (or more) VF's listed, but this proposed change has a couple of issues
First of all, the change you describe would be a semantic change in the
network XML, which would break compatibility with previous releases of
libvirt. Since we consider XML to be poart of our long term ABI stability
guarantee I don't think we can do the change.
Ignoring the ABI issue, I'm concerned that as we get PFs with an increasingly
large number of VFs, we may well *not* want to associate all VFs with a single
virtual network definition. eg, we might wna to put 32 VFs in one network and
32 VFs in another network. Or if we have 2 PFs, we might want to interleave
VFs from several PFs across virtual networks. If all we can do is list the
PF in the XML, we loose significant flexibility in how VFs are assigned.
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 :|
7:29 p.m.
On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
> Interface Pools and Passthrough mode:
>
> Current Method:
> The passthrough mode uses a macvtap a direct connection to connect each guest to the
network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>
> The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
> Example:
> <forward mode='passthrough' dev='eth10'/>
> <interface dev='eth10'/>
> <interface dev='eth12'/>
> <interface dev='eth18'/>
> <interface dev='eth20'/>
> </forward>
>
> However with an ethernet card with 64 VF's or more, the above method gets tedious
on the system.
>
> On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there
are multiple non-contiguous ranges.
Heh. You've gone through some of the same thought process I went through
when I wrote the original code :-)
>
> Proposed Method:
> The 5 patches provided along with this introductory e-mail
>
> i) Introduce a structure to store the state of all the virtual functions attached to
each physical function
> ii) Find a free virtual function given the physical function.
>
> The forward specification will hence only mention the physical function as the
interface sub element:
> Example:
> <forward mode='passthrough' dev='eth2'/>
> <interface dev='eth2'/>
> </forward>
I can see what you mean about it being tedious to construct the config with
all 64 (or more) VF's listed, but this proposed change has a couple of issues
First of all, the change you describe would be a semantic change in the
network XML, which would break compatibility with previous releases of
libvirt. Since we consider XML to be poart of our long term ABI stability
guarantee I don't think we can do the change.
Ignoring the ABI issue, I'm concerned that as we get PFs with an increasingly
large number of VFs, we may well *not* want to associate all VFs with a single
virtual network definition. eg, we might wna to put 32 VFs in one network and
32 VFs in another network. Or if we have 2 PFs, we might want to interleave
VFs from several PFs across virtual networks. If all we can do is list the
PF in the XML, we loose significant flexibility in how VFs are assigned.
My first concern too when I saw the patch was the semantic change (but
also the loss of flexibility), which is obviously a no-go. It's a
convenient capability to have though, so it would be nice to get it in
somehow. What if we allowed including all the VFs associated with a PF
by adding an extra attribute? e.g.:
<interface dev='eth10' type='sriov'/>
(or whatever is more appropriate in place of "sriov"). Or possibly a
different element type could be used:
<pf dev='eth10'/>
(didn't want to spend time thinking of a better name than "pf"...).
At the time the network is created, this would cause libvirt to get the
list of all VFs for the given PF and put them into the pool. This could
be used instead of, or in combination with, the existing <interface
dev='eth1'/> form. Thus the existing semantics would be preserved, the
flexibility of specifying individual devices would be retained, and the
desired convenience of adding all VFs of a PF with a single line would
be added.
10:30 a.m.
On 11/30/2011 01:29 AM, Laine Stump wrote:
On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
> On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>> Interface Pools and Passthrough mode:
>>
>> Current Method:
>> The passthrough mode uses a macvtap a direct connection to connect each guest to
the network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>>
>> The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>> Example:
>> <forward mode='passthrough' dev='eth10'/>
>> <interface dev='eth10'/>
>> <interface dev='eth12'/>
>> <interface dev='eth18'/>
>> <interface dev='eth20'/>
>> </forward>
>>
>> However with an ethernet card with 64 VF's or more, the above method gets
tedious on the system.
>>
>> On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when
there are multiple non-contiguous ranges.
Heh. You've gone through some of the same thought process I went through when I wrote
the original code :-)
>>
>> Proposed Method:
>> The 5 patches provided along with this introductory e-mail
>>
>> i) Introduce a structure to store the state of all the virtual functions attached
to each physical function
>> ii) Find a free virtual function given the physical function.
>>
>> The forward specification will hence only mention the physical function as the
interface sub element:
>> Example:
>> <forward mode='passthrough' dev='eth2'/>
>> <interface dev='eth2'/>
>> </forward>
> I can see what you mean about it being tedious to construct the config with
> all 64 (or more) VF's listed, but this proposed change has a couple of issues
>
> First of all, the change you describe would be a semantic change in the
> network XML, which would break compatibility with previous releases of
> libvirt. Since we consider XML to be poart of our long term ABI stability
> guarantee I don't think we can do the change.
>
> Ignoring the ABI issue, I'm concerned that as we get PFs with an increasingly
> large number of VFs, we may well *not* want to associate all VFs with a single
> virtual network definition. eg, we might wna to put 32 VFs in one network and
> 32 VFs in another network. Or if we have 2 PFs, we might want to interleave
> VFs from several PFs across virtual networks. If all we can do is list the
> PF in the XML, we loose significant flexibility in how VFs are assigned.
My first concern too when I saw the patch was the semantic change (but also the loss of
flexibility), which is obviously a no-go. It's a convenient capability to have though,
so it would be nice to get it in somehow. What if we allowed including all the VFs
associated with a PF by adding an extra attribute? e.g.:
<interface dev='eth10' type='sriov'/>
(or whatever is more appropriate in place of "sriov"). Or possibly a different
element type could be used:
<pf dev='eth10'/>
(didn't want to spend time thinking of a better name than "pf"...).
At the time the network is created, this would cause libvirt to get the list of all VFs
for the given PF and put them into the pool. This could be used instead of, or in
combination with, the existing <interface dev='eth1'/> form. Thus the
existing semantics would be preserved, the flexibility of specifying individual devices
would be retained, and the desired convenience of adding all VFs of a PF with a single
line would be added.
I completely agree with this method, I can work on this cause next week. May I ask which
method you would suggest I go forward with,
1) <interface dev='eth10' type='sriov'/>
2) Or possibly a different element type could be used: <pf dev='eth10'/>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Laine and Daniel,
Many thanks for reviewing my patch series.
I do understand the point of loss of flexibility when we want to interleave Vfs from
several PFs across virtual networks.
Regarding the changes to the network semantics, I am a little bit confused,
1) We do not require a change to the XML schema in order to support the patch series
2) Is the change required to support the modifications to the structures
'virNetworkForwardVfDef' and 'virNetworkForwardIfDef'? OR
3) Would the change be required because of the compatibility issue with interface pools?
If the concern is the compatibility issue, the patch series I have submitted takes this
into consideration and will still work as before if interface pools are mentioned instead
of just the PF.
I completely agree with Laine's suggestion in the previous thread, I can work on this
cause next week. May I ask for suggestions as to which method I should go forward with,
1) <interface dev='eth10' type='sriov'/>
2) Or possibly a different element type could be used: <pf dev='eth10'/>
Many Thanks,
Regards,
Shradha Shah
1:31 p.m.
On 12/02/2011 11:30 AM, Shradha Shah wrote:
On 11/30/2011 01:29 AM, Laine Stump wrote:
> On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
>> On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>>> Interface Pools and Passthrough mode:
>>>
>>> Current Method:
>>> The passthrough mode uses a macvtap a direct connection to connect each guest
to the network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>>>
>>> The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>>> Example:
>>> <forward mode='passthrough' dev='eth10'/>
>>> <interface dev='eth10'/>
>>> <interface dev='eth12'/>
>>> <interface dev='eth18'/>
>>> <interface dev='eth20'/>
>>> </forward>
>>>
>>> However with an ethernet card with 64 VF's or more, the above method gets
tedious on the system.
>>>
>>> On the other hand, just parameterizing a string (eth%d) is inadequate, eg,
when there are multiple non-contiguous ranges.
>
> Heh. You've gone through some of the same thought process I went through when I
wrote the original code :-)
>
>
>>> Proposed Method:
>>> The 5 patches provided along with this introductory e-mail
>>>
>>> i) Introduce a structure to store the state of all the virtual functions
attached to each physical function
>>> ii) Find a free virtual function given the physical function.
>>>
>>> The forward specification will hence only mention the physical function as
the interface sub element:
>>> Example:
>>> <forward mode='passthrough' dev='eth2'/>
>>> <interface dev='eth2'/>
>>> </forward>
>> I can see what you mean about it being tedious to construct the config with
>> all 64 (or more) VF's listed, but this proposed change has a couple of
issues
>>
>> First of all, the change you describe would be a semantic change in the
>> network XML, which would break compatibility with previous releases of
>> libvirt. Since we consider XML to be poart of our long term ABI stability
>> guarantee I don't think we can do the change.
>>
>> Ignoring the ABI issue, I'm concerned that as we get PFs with an
increasingly
>> large number of VFs, we may well *not* want to associate all VFs with a single
>> virtual network definition. eg, we might wna to put 32 VFs in one network and
>> 32 VFs in another network. Or if we have 2 PFs, we might want to interleave
>> VFs from several PFs across virtual networks. If all we can do is list the
>> PF in the XML, we loose significant flexibility in how VFs are assigned.
> My first concern too when I saw the patch was the semantic change (but also the loss
of flexibility), which is obviously a no-go. It's a convenient capability to have
though, so it would be nice to get it in somehow. What if we allowed including all the VFs
associated with a PF by adding an extra attribute? e.g.:
>
> <interface dev='eth10' type='sriov'/>
>
> (or whatever is more appropriate in place of "sriov"). Or possibly a
different element type could be used:
>
> <pf dev='eth10'/>
>
> (didn't want to spend time thinking of a better name than "pf"...).
>
> At the time the network is created, this would cause libvirt to get the list of all
VFs for the given PF and put them into the pool. This could be used instead of, or in
combination with, the existing<interface dev='eth1'/> form. Thus the
existing semantics would be preserved, the flexibility of specifying individual devices
would be retained, and the desired convenience of adding all VFs of a PF with a single
line would be added.
>
I completely agree with this method, I can work on this cause next week. May I ask which
method you would suggest I go forward with,
1)<interface dev='eth10' type='sriov'/>
2) Or possibly a different element type could be used:<pf dev='eth10'/>
I don't have a preference. I was hoping that Dan (or somebody else)
would :-) (Also, don't infer from my mail that this suggestion is the
way to go - my own ideas have been shot down on libvir-list before as well.)
Laine and Daniel,
Many thanks for reviewing my patch series.
I do understand the point of loss of flexibility when we want to interleave Vfs from
several PFs across virtual networks.
Regarding the changes to the network semantics, I am a little bit confused,
1) We do not require a change to the XML schema in order to support the patch series
Additions to the schema are fine.
2) Is the change required to support the modifications to the
structures 'virNetworkForwardVfDef' and 'virNetworkForwardIfDef'? OR
3) Would the change be required because of the compatibility issue with interface pools?
The problem we're concerned about is that the meaning of:
<forward mode='passthrough'>
<interface dev='eth0'/>
</forward>
changes. With existing code, that means "a pool with a single ethernet
interface named eth0"; with your patches, it means "a pool with several
ethernet interfaces ethX-ethY (which are the VFs associated with the PF
named eth0). This isn't acceptable because libvirt guarantees 100% API
stability across releases.
It would be acceptable, though (in my opinion) to have the new behavior
occur based on the presence of an extra attribute, or a different element.
If the concern is the compatibility issue, the patch series I have submitted takes this
into consideration and will still work as before if interface pools are mentioned instead
of just the PF.
Not having an SRIOV-capable NIC to toy with (yet! I have one on order),
I was unable to directly try out your code, and didn't go through it
line by line, but based on your description it seems that an interface
definition that is currently valid (my example above) would have a
different interpretation after your patches are applied. Is that not the
case?
I completely agree with Laine's suggestion in the previous
thread, I can work on this cause next week. May I ask for suggestions as to which method I
should go forward with,
1)<interface dev='eth10' type='sriov'/>
2) Or possibly a different element type could be used:<pf dev='eth10'/>
You may want to wait until Monday to see if Dan weighs in on this one
way or the other (or with a better idea) (he's traveling at the moment).
5:37 a.m.
On Tue, Nov 29, 2011 at 08:29:35PM -0500, Laine Stump wrote:
On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
>On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>>Interface Pools and Passthrough mode:
>>
>>Current Method:
>>The passthrough mode uses a macvtap a direct connection to connect each guest to
the network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>>
>>The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>>Example:
>><forward mode='passthrough' dev='eth10'/>
>><interface dev='eth10'/>
>><interface dev='eth12'/>
>><interface dev='eth18'/>
>><interface dev='eth20'/>
>></forward>
>>
>>However with an ethernet card with 64 VF's or more, the above method gets
tedious on the system.
>Ignoring the ABI issue, I'm concerned that as we get PFs with an increasingly
>large number of VFs, we may well *not* want to associate all VFs with a single
>virtual network definition. eg, we might wna to put 32 VFs in one network and
>32 VFs in another network. Or if we have 2 PFs, we might want to interleave
>VFs from several PFs across virtual networks. If all we can do is list the
>PF in the XML, we loose significant flexibility in how VFs are assigned.
My first concern too when I saw the patch was the semantic change
(but also the loss of flexibility), which is obviously a no-go. It's
a convenient capability to have though, so it would be nice to get
it in somehow. What if we allowed including all the VFs associated
with a PF by adding an extra attribute? e.g.:
<interface dev='eth10' type='sriov'/>
This feels a little bit wrong to me.
(or whatever is more appropriate in place of "sriov"). Or
possibly a
different element type could be used:
<pf dev='eth10'/>
I like this idea, because it is providing additional useful info,
rather than changing existing elements, so it is maximally
compatible.
(didn't want to spend time thinking of a better name than
"pf"...).
At the time the network is created, this would cause libvirt to get
the list of all VFs for the given PF and put them into the pool.
This could be used instead of, or in combination with, the existing
<interface dev='eth1'/> form. Thus the existing semantics would be
preserved, the flexibility of specifying individual devices would be
retained, and the desired convenience of adding all VFs of a PF with
a single line would be added.
IIUC, what you're suggesting is the following behaviour:
* Explicit interface list. App inputs:
<forward mode='passthrough'>
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'>
<pf dev='eth0'/>
</forward>
libvirt expands XML to be
<forward mode='passthrough'>
<pf dev='eth0'/>
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
This is good because all previous info is still intact
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 :|
5:54 a.m.
On 12/05/2011 11:37 AM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 08:29:35PM -0500, Laine Stump wrote:
> On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
>> On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>>> Interface Pools and Passthrough mode:
>>>
>>> Current Method:
>>> The passthrough mode uses a macvtap a direct connection to connect each guest
to the network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>>>
>>> The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>>> Example:
>>> <forward mode='passthrough' dev='eth10'/>
>>> <interface dev='eth10'/>
>>> <interface dev='eth12'/>
>>> <interface dev='eth18'/>
>>> <interface dev='eth20'/>
>>> </forward>
>>>
>>> However with an ethernet card with 64 VF's or more, the above method gets
tedious on the system.
>
>> Ignoring the ABI issue, I'm concerned that as we get PFs with an
increasingly
>> large number of VFs, we may well *not* want to associate all VFs with a single
>> virtual network definition. eg, we might wna to put 32 VFs in one network and
>> 32 VFs in another network. Or if we have 2 PFs, we might want to interleave
>> VFs from several PFs across virtual networks. If all we can do is list the
>> PF in the XML, we loose significant flexibility in how VFs are assigned.
>
> My first concern too when I saw the patch was the semantic change
> (but also the loss of flexibility), which is obviously a no-go. It's
> a convenient capability to have though, so it would be nice to get
> it in somehow. What if we allowed including all the VFs associated
> with a PF by adding an extra attribute? e.g.:
>
> <interface dev='eth10' type='sriov'/>
This feels a little bit wrong to me.
> (or whatever is more appropriate in place of "sriov"). Or possibly a
> different element type could be used:
>
> <pf dev='eth10'/>
I like this idea, because it is providing additional useful info,
rather than changing existing elements, so it is maximally
compatible.
> (didn't want to spend time thinking of a better name than "pf"...).
>
> At the time the network is created, this would cause libvirt to get
> the list of all VFs for the given PF and put them into the pool.
> This could be used instead of, or in combination with, the existing
> <interface dev='eth1'/> form. Thus the existing semantics would be
> preserved, the flexibility of specifying individual devices would be
> retained, and the desired convenience of adding all VFs of a PF with
> a single line would be added.
IIUC, what you're suggesting is the following behaviour:
* Explicit interface list. App inputs:
<forward mode='passthrough'>
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'>
<pf dev='eth0'/>
</forward>
libvirt expands XML to be
<forward mode='passthrough'>
<pf dev='eth0'/>
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
This is good because all previous info is still intact
Regards,
Daniel
Daniel and Laine,
I agree with the above mentioned suggestion and I will re-submit a set of patches using
your suggestions.
Many thanks for your input.
Regards,
Shradha
1 p.m.
On 12/05/2011 06:37 AM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 08:29:35PM -0500, Laine Stump wrote:
> On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
>> On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>>> Interface Pools and Passthrough mode:
>>>
>>> Current Method:
>>> The passthrough mode uses a macvtap a direct connection to connect each guest
to the network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>>>
>>> The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>>> Example:
>>> <forward mode='passthrough' dev='eth10'/>
>>> <interface dev='eth10'/>
>>> <interface dev='eth12'/>
>>> <interface dev='eth18'/>
>>> <interface dev='eth20'/>
>>> </forward>
>>>
>>> However with an ethernet card with 64 VF's or more, the above method gets
tedious on the system.
>> Ignoring the ABI issue, I'm concerned that as we get PFs with an
increasingly
>> large number of VFs, we may well *not* want to associate all VFs with a single
>> virtual network definition. eg, we might wna to put 32 VFs in one network and
>> 32 VFs in another network. Or if we have 2 PFs, we might want to interleave
>> VFs from several PFs across virtual networks. If all we can do is list the
>> PF in the XML, we loose significant flexibility in how VFs are assigned.
> My first concern too when I saw the patch was the semantic change
> (but also the loss of flexibility), which is obviously a no-go. It's
> a convenient capability to have though, so it would be nice to get
> it in somehow. What if we allowed including all the VFs associated
> with a PF by adding an extra attribute? e.g.:
>
> <interface dev='eth10' type='sriov'/>
This feels a little bit wrong to me.
> (or whatever is more appropriate in place of "sriov"). Or possibly a
> different element type could be used:
>
> <pf dev='eth10'/>
I like this idea, because it is providing additional useful info,
rather than changing existing elements, so it is maximally
compatible.
> (didn't want to spend time thinking of a better name than "pf"...).
>
> At the time the network is created, this would cause libvirt to get
> the list of all VFs for the given PF and put them into the pool.
> This could be used instead of, or in combination with, the existing
> <interface dev='eth1'/> form. Thus the existing semantics would be
> preserved, the flexibility of specifying individual devices would be
> retained, and the desired convenience of adding all VFs of a PF with
> a single line would be added.
IIUC, what you're suggesting is the following behaviour:
* Explicit interface list. App inputs:
<forward mode='passthrough'>
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'>
<pf dev='eth0'/>
</forward>
libvirt expands XML to be
<forward mode='passthrough'>
<pf dev='eth0'/>
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
This is good because all previous info is still intact
I actually hadn't thought of modifying the XML and displaying it in
net-dumpxml or (netdumpxml --inactive), which is what I think you may be
implying here. This would have the advantage of making a management
application's job easier when displaying status (available interfaces,
etc), but could lead to confusion when a host's hardware was changed
(since there would be no detectable difference between dev elements that
were entered by hand, and those that were automatically derived from a
pf element). Also, it would end up cluttering up the config file again,
which is part of what this is trying to avoid (although eliminating the
need to type in all N vf names is the primary concern).
Unless we come up with a way of differentiating between auto-generated
<interface> elements (including keeping track of the parent <pf>) and
those entered by hand, I think the XML itself shouldn't be changed, but
only the contents of the interface pool in memory.
(I'm not necessarily suggesting it, but one way of differentiating
auto-generated entries would be with an additional "parent='eth0'" (or
"pf='eth0'") attribute).
4:16 a.m.
On Mon, Dec 05, 2011 at 02:00:52PM -0500, Laine Stump wrote:
On 12/05/2011 06:37 AM, Daniel P. Berrange wrote:
>On Tue, Nov 29, 2011 at 08:29:35PM -0500, Laine Stump wrote:
>>On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
>>>On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>>>>Interface Pools and Passthrough mode:
>>>>
>>>>Current Method:
>>>>The passthrough mode uses a macvtap a direct connection to connect each
guest to the network. The physical interface to be used is picked from among those listed
in<interface> sub elements of the<forward> element.
>>>>
>>>>The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>>>>Example:
>>>><forward mode='passthrough' dev='eth10'/>
>>>><interface dev='eth10'/>
>>>><interface dev='eth12'/>
>>>><interface dev='eth18'/>
>>>><interface dev='eth20'/>
>>>></forward>
>>>>
>>>>However with an ethernet card with 64 VF's or more, the above method
gets tedious on the system.
>>>Ignoring the ABI issue, I'm concerned that as we get PFs with an
increasingly
>>>large number of VFs, we may well *not* want to associate all VFs with a
single
>>>virtual network definition. eg, we might wna to put 32 VFs in one network
and
>>>32 VFs in another network. Or if we have 2 PFs, we might want to interleave
>>>VFs from several PFs across virtual networks. If all we can do is list the
>>>PF in the XML, we loose significant flexibility in how VFs are assigned.
>>My first concern too when I saw the patch was the semantic change
>>(but also the loss of flexibility), which is obviously a no-go. It's
>>a convenient capability to have though, so it would be nice to get
>>it in somehow. What if we allowed including all the VFs associated
>>with a PF by adding an extra attribute? e.g.:
>>
>><interface dev='eth10' type='sriov'/>
>This feels a little bit wrong to me.
>
>>(or whatever is more appropriate in place of "sriov"). Or possibly a
>>different element type could be used:
>>
>><pf dev='eth10'/>
>I like this idea, because it is providing additional useful info,
>rather than changing existing elements, so it is maximally
>compatible.
>
>>(didn't want to spend time thinking of a better name than
"pf"...).
>>
>>At the time the network is created, this would cause libvirt to get
>>the list of all VFs for the given PF and put them into the pool.
>>This could be used instead of, or in combination with, the existing
>><interface dev='eth1'/> form. Thus the existing semantics would
be
>>preserved, the flexibility of specifying individual devices would be
>>retained, and the desired convenience of adding all VFs of a PF with
>>a single line would be added.
>IIUC, what you're suggesting is the following behaviour:
>
> * Explicit interface list. App inputs:
>
> <forward mode='passthrough'>
> <interface dev='eth10'/>
> <interface dev='eth11'/>
> <interface dev='eth12'/>
> <interface dev='eth13'/>
> </forward>
>
> libvirt does not change XML
>
> * Automatically interface list from PF. App inputs:
>
> <forward mode='passthrough'>
> <pf dev='eth0'/>
> </forward>
>
> libvirt expands XML to be
>
> <forward mode='passthrough'>
> <pf dev='eth0'/>
> <interface dev='eth10'/>
> <interface dev='eth11'/>
> <interface dev='eth12'/>
> <interface dev='eth13'/>
> </forward>
>
>This is good because all previous info is still intact
I actually hadn't thought of modifying the XML and displaying it in
net-dumpxml or (netdumpxml --inactive), which is what I think you
may be implying here. This would have the advantage of making a
management application's job easier when displaying status
(available interfaces, etc), but could lead to confusion when a
host's hardware was changed (since there would be no detectable
difference between dev elements that were entered by hand, and those
that were automatically derived from a pf element). Also, it would
end up cluttering up the config file again, which is part of what
this is trying to avoid (although eliminating the need to type in
all N vf names is the primary concern).
Unless we come up with a way of differentiating between
auto-generated <interface> elements (including keeping track of the
parent <pf>) and those entered by hand, I think the XML itself
shouldn't be changed, but only the contents of the interface pool in
memory.
As with domains, every network has both an active and inactive
XML config. When the network is not running, we should only be
showing the user provided <interface> elements. Only once you
start the network, do we automatically fill in <interface>
elements based on <pf>. So if we add a flag for virNetworkGetXMLDesc()
like VIR_NETWORK_XML_INACTIVE, then you can distinguish by comparing
the live XML to the inactive XML.
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 :|
6:41 a.m.
On 12/06/2011 10:16 AM, Daniel P. Berrange wrote:
On Mon, Dec 05, 2011 at 02:00:52PM -0500, Laine Stump wrote:
> On 12/05/2011 06:37 AM, Daniel P. Berrange wrote:
>> On Tue, Nov 29, 2011 at 08:29:35PM -0500, Laine Stump wrote:
>>> On 11/29/2011 02:53 PM, Daniel P. Berrange wrote:
>>>> On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
>>>>> Interface Pools and Passthrough mode:
>>>>>
>>>>> Current Method:
>>>>> The passthrough mode uses a macvtap a direct connection to connect
each guest to the network. The physical interface to be used is picked from among those
listed in<interface> sub elements of the<forward> element.
>>>>>
>>>>> The current specification for<forward> extends to allow 0 or
more<interface> sub-elements:
>>>>> Example:
>>>>> <forward mode='passthrough' dev='eth10'/>
>>>>> <interface dev='eth10'/>
>>>>> <interface dev='eth12'/>
>>>>> <interface dev='eth18'/>
>>>>> <interface dev='eth20'/>
>>>>> </forward>
>>>>>
>>>>> However with an ethernet card with 64 VF's or more, the above
method gets tedious on the system.
>>>> Ignoring the ABI issue, I'm concerned that as we get PFs with an
increasingly
>>>> large number of VFs, we may well *not* want to associate all VFs with a
single
>>>> virtual network definition. eg, we might wna to put 32 VFs in one network
and
>>>> 32 VFs in another network. Or if we have 2 PFs, we might want to
interleave
>>>> VFs from several PFs across virtual networks. If all we can do is list
the
>>>> PF in the XML, we loose significant flexibility in how VFs are assigned.
>>> My first concern too when I saw the patch was the semantic change
>>> (but also the loss of flexibility), which is obviously a no-go. It's
>>> a convenient capability to have though, so it would be nice to get
>>> it in somehow. What if we allowed including all the VFs associated
>>> with a PF by adding an extra attribute? e.g.:
>>>
>>> <interface dev='eth10' type='sriov'/>
>> This feels a little bit wrong to me.
>>
>>> (or whatever is more appropriate in place of "sriov"). Or possibly
a
>>> different element type could be used:
>>>
>>> <pf dev='eth10'/>
>> I like this idea, because it is providing additional useful info,
>> rather than changing existing elements, so it is maximally
>> compatible.
>>
>>> (didn't want to spend time thinking of a better name than
"pf"...).
>>>
>>> At the time the network is created, this would cause libvirt to get
>>> the list of all VFs for the given PF and put them into the pool.
>>> This could be used instead of, or in combination with, the existing
>>> <interface dev='eth1'/> form. Thus the existing semantics
would be
>>> preserved, the flexibility of specifying individual devices would be
>>> retained, and the desired convenience of adding all VFs of a PF with
>>> a single line would be added.
>> IIUC, what you're suggesting is the following behaviour:
>>
>> * Explicit interface list. App inputs:
>>
>> <forward mode='passthrough'>
>> <interface dev='eth10'/>
>> <interface dev='eth11'/>
>> <interface dev='eth12'/>
>> <interface dev='eth13'/>
>> </forward>
>>
>> libvirt does not change XML
>>
>> * Automatically interface list from PF. App inputs:
>>
>> <forward mode='passthrough'>
>> <pf dev='eth0'/>
>> </forward>
>>
>> libvirt expands XML to be
>>
>> <forward mode='passthrough'>
>> <pf dev='eth0'/>
>> <interface dev='eth10'/>
>> <interface dev='eth11'/>
>> <interface dev='eth12'/>
>> <interface dev='eth13'/>
>> </forward>
>>
>> This is good because all previous info is still intact
>
>
> I actually hadn't thought of modifying the XML and displaying it in
> net-dumpxml or (netdumpxml --inactive), which is what I think you
> may be implying here. This would have the advantage of making a
> management application's job easier when displaying status
> (available interfaces, etc), but could lead to confusion when a
> host's hardware was changed (since there would be no detectable
> difference between dev elements that were entered by hand, and those
> that were automatically derived from a pf element). Also, it would
> end up cluttering up the config file again, which is part of what
> this is trying to avoid (although eliminating the need to type in
> all N vf names is the primary concern).
>
> Unless we come up with a way of differentiating between
> auto-generated <interface> elements (including keeping track of the
> parent <pf>) and those entered by hand, I think the XML itself
> shouldn't be changed, but only the contents of the interface pool in
> memory.
As with domains, every network has both an active and inactive
XML config. When the network is not running, we should only be
showing the user provided <interface> elements. Only once you
start the network, do we automatically fill in <interface>
elements based on <pf>. So if we add a flag for virNetworkGetXMLDesc()
like VIR_NETWORK_XML_INACTIVE, then you can distinguish by comparing
the live XML to the inactive XML.
I agree that an inactive XML should show only the user provided data.
But in the case of active XML, would it be wise to display the entire VF pool in the XML?
I think this would clutter the config file when a NIC supports 127 VF's per port like
in the case of Solarflare.
Also, the free Vf's are discovered only after virNetworkDefParseXML and
networkAllocateActualDevice (in mode Passthrough). Is there a way of modifying the active
XML at this point?
Shradha
Daniel
1:55 p.m.
On Tue, Nov 29, 2011 at 03:46:13PM +0000, Shradha Shah wrote:
Shradha Shah (5):
Moving the declaration of _pciDevice and _pciDeviceList to pci.h
Added function pciSysfsFile to enable access to the PCI SYSFS files.
Adding functions virNetDevGetVirtualFunctions and virNetDevGetNetName
virNetDevGetVirtualFunctions: Gets the BDF of all the Virtual
Functions given a physical function virNetDevGetNetName: Gets
the interface name of the PCI Device.
Addition of a new device structure to store the state of a Virtual
Function Modifications to the Physical Device Structure to
store state of its Virtual Functions
If a system has 64 or more VF's, it is quite tedious to mention each
VF in the interface pool. The following modification find a
Free VF given a Physical Function when mode=passthrough. We
also save the state of each VF. Hence modifications to
networkAllocateActualDevice, networkNotifyActualDevice and
networkReleaseActualDevice were required. The following
patch does just that.
BTW, to make 'git send-email' happy when generating Subject: lines
and these patch summaries, make sure the commit message consists of
a short single line summary, then a blank line, then the longer
multiple line description
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 :|
4765
days inactive
4772
days old
19 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Shradha Shah