[libvirt] [PATCH 3/8] Hostdev-hybrid mode requires a direct linkdev and direct mode.
Shradha Shah
sshah at solarflare.com
Thu Sep 13 11:45:02 UTC 2012
On 09/11/2012 08:15 PM, Laine Stump wrote:
> (not a full review yet, but a couple of things I noticed in passing)
>
> On 09/07/2012 12:15 PM, Shradha Shah wrote:
>> In this mode the guest contains a Virtual network device along with a
>> SRIOV VF passed through to the guest as a pci device.
>> ---
>> src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++++++++--
>> src/conf/domain_conf.h | 5 +++++
>> src/libvirt_private.syms | 1 +
>> src/util/pci.c | 2 +-
>> src/util/pci.h | 2 ++
>> src/util/virnetdev.c | 40 ++++++++++++++++++++++++++++++++++++++++
>> src/util/virnetdev.h | 6 ++++++
>> 7 files changed, 91 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index d8ab40c..c59ea00 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -1022,6 +1022,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
>> virDomainHostdevDefClear(&def->data.hostdev.def);
>> break;
>> case VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID:
>> + VIR_FREE(def->data.hostdev.linkdev);
>> virDomainHostdevDefClear(&def->data.hostdev.def);
>> break;
>> default:
>> @@ -1077,6 +1078,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>> break;
>>
>> case VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID:
>> + VIR_FREE(def->data.hostdev.linkdev);
>> virDomainHostdevDefClear(&def->data.hostdev.def);
>> break;
>>
>> @@ -4687,6 +4689,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
>> char *mode = NULL;
>> char *linkstate = NULL;
>> char *addrtype = NULL;
>> + char *pfname = NULL;
>> virNWFilterHashTablePtr filterparams = NULL;
>> virDomainActualNetDefPtr actual = NULL;
>> xmlNodePtr oldnode = ctxt->node;
>> @@ -5024,6 +5027,27 @@ virDomainNetDefParseXML(virCapsPtr caps,
>> hostdev, flags) < 0) {
>> goto error;
>> }
>> +
>> + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
>> + if (virNetDevGetPhysicalFunctionFromVfPciAddr(hostdev->source.subsys.u.pci.domain,
>> + hostdev->source.subsys.u.pci.bus,
>> + hostdev->source.subsys.u.pci.slot,
>> + hostdev->source.subsys.u.pci.function,
>> + &pfname) < 0) {
>
> It's problematic to have this call in a parsing function - it requires
> the actual hardware to be present on the machine doing the parse. For
> example, doing this in the parse causes the qemuxml2xml and qemuxml2argv
> tests to fail on my system, because my hardware doesn't match yours:
>
>
>
> One last thing - after applying the entire series, I'm getting the
> following failures in make check:
>
>
> VIR_TEST_DEBUG=2 ./qemuxml2xmltest
> TEST: qemuxml2xmltest
> [...]
> 7) QEMU XML-2-XML net-hostdevhybrid ...
> libvir: Domain Config error : internal error Could not get Physical
> Function of the hostdev
>
> ...
>
>
> VIR_TEST_DEBUG=2 ./qemuxml2argvtest
> TEST: qemuxml2argvtest
> [...]
> 113) QEMU XML-2-ARGV net-hostdevhybrid
> ... libvir: Domain Config error : internal error Could not get Physical
> Function of the hostdev
>
>
>
> I couldn't find any other uses of network device "ioctl" type calls in
> the conf directory. I think we at least frown on doing that in
> parsing/formatting, and may even forbid it.
>
> At any rate, You need to not do this here. Instead, perhaps you can
> leave it empty if it's not given explicitly in the input XML, and fill
> it in in the caller when appropriate?
Thanks for the review Laine. I had actually encountered this issue when I was working on
the patches but at that moment I could not think of a workaround.
Thanks for the suggestion. I will do the needful.
>
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Could not get Physical Function of the hostdev"));
>> + goto error;
>> + }
>> + }
>> + if (pfname != NULL)
>> + def->data.hostdev.linkdev = strdup(pfname);
>> + else {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Linkdev is required in %s mode"),
>> + virDomainNetTypeToString(def->type));
>> + goto error;
>> + }
>> + def->data.hostdev.mode = VIR_NETDEV_MACVLAN_MODE_BRIDGE;
>> break;
>>
>> case VIR_DOMAIN_NET_TYPE_USER:
>> @@ -14664,11 +14688,16 @@ virDomainNetGetActualDirectDev(virDomainNetDefPtr iface)
>> {
>> if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT)
>> return iface->data.direct.linkdev;
>> + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID)
>> + return iface->data.hostdev.linkdev;
>> if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
>> return NULL;
>> if (!iface->data.network.actual)
>> return NULL;
>> - return iface->data.network.actual->data.direct.linkdev;
>> + if (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID)
>> + return iface->data.network.actual->data.hostdev.linkdev;
>> + else
>> + return iface->data.network.actual->data.direct.linkdev;
>> }
>>
>> int
>> @@ -14676,11 +14705,16 @@ virDomainNetGetActualDirectMode(virDomainNetDefPtr iface)
>> {
>> if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT)
>> return iface->data.direct.mode;
>> + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID)
>> + return iface->data.hostdev.mode;
>> if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
>> return 0;
>> if (!iface->data.network.actual)
>> return 0;
>> - return iface->data.network.actual->data.direct.mode;
>> + if (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID)
>> + return iface->data.network.actual->data.hostdev.mode;
>> + else
>> + return iface->data.network.actual->data.direct.mode;
>> }
>>
>> virDomainHostdevDefPtr
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 156eb32..171dd70 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -44,6 +44,7 @@
>> # include "virnetdevopenvswitch.h"
>> # include "virnetdevbandwidth.h"
>> # include "virnetdevvlan.h"
>> +# include "virnetdev.h"
>> # include "virobject.h"
>> # include "device_conf.h"
>>
>> @@ -778,6 +779,8 @@ struct _virDomainActualNetDef {
>> int mode; /* enum virMacvtapMode from util/macvtap.h */
>> } direct;
>> struct {
>> + char *linkdev;
>> + int mode;
>
> It appears that the mode is always "bridge", so I don't see any point in
> including it here.
>
>
>> virDomainHostdevDef def;
>> } hostdev;
>> } data;
>> @@ -833,6 +836,8 @@ struct _virDomainNetDef {
>> int mode; /* enum virMacvtapMode from util/macvtap.h */
>> } direct;
>> struct {
>> + char *linkdev;
>> + int mode;
>> virDomainHostdevDef def;
>> } hostdev;
>> } data;
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 10af063..2d06cc3 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1399,6 +1399,7 @@ virNetDevGetMTU;
>> virNetDevGetPhysicalFunction;
>> virNetDevGetVLanID;
>> virNetDevGetVirtualFunctionIndex;
>> +virNetDevGetPhysicalFunctionFromVfPciAddr;
>> virNetDevGetVirtualFunctionInfo;
>> virNetDevGetVirtualFunctions;
>> virNetDevIsOnline;
>> diff --git a/src/util/pci.c b/src/util/pci.c
>> index 0742d07..ba8fffc 100644
>> --- a/src/util/pci.c
>> +++ b/src/util/pci.c
>> @@ -802,7 +802,7 @@ pciDriverFile(char **buffer, const char *driver, const char *file)
>> return 0;
>> }
>>
>> -static int
>> +int
>> pciDeviceFile(char **buffer, const char *device, const char *file)
>> {
>> VIR_FREE(*buffer);
>> diff --git a/src/util/pci.h b/src/util/pci.h
>> index 8bbab07..936fee4 100644
>> --- a/src/util/pci.h
>> +++ b/src/util/pci.h
>> @@ -116,6 +116,8 @@ int pciConfigAddressToSysfsFile(struct pci_config_address *dev,
>>
>> int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
>>
>> +int pciDeviceFile(char **buffer, const char *device, const char *file);
>> +
>> int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link)
>> ATTRIBUTE_RETURN_CHECK;
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index f622f5d..4d4fcb1 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1097,6 +1097,46 @@ virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname,
>> }
>>
>> /**
>> + * virNetDevGetPhyscialFunctionFromVfPciAddr
>> + *
>> + * @domain : Domain part of VF PCI addr
>> + * @bus : Bus part of VF PCI addr
>> + * @slot : Slot part of VF PCI addr
>> + * @function : Function part of VF PCI addr
>> + * @pfname : Contains sriov physical function for Vf upon
>> + * successful return
>> + *
>> + * Returns 0 on success, -1 on failure
>> + *
>> + */
>> +int
>> +virNetDevGetPhysicalFunctionFromVfPciAddr(unsigned domain,
>> + unsigned bus,
>> + unsigned slot,
>> + unsigned function,
>> + char **pfname)
>> +{
>> + char *pciConfigAddr = NULL;
>> + char *physfn_sysfs_path = NULL;
>> + int ret = -1;
>> +
>> + if (pciGetDeviceAddrString(domain, bus, slot, function,
>> + &pciConfigAddr) < 0) {
>> + goto cleanup;
>> + }
>> + if (pciDeviceFile(&physfn_sysfs_path, pciConfigAddr, "physfn") < 0) {
>> + goto cleanup;
>> + }
>> + ret = pciDeviceNetName(physfn_sysfs_path, pfname);
>> +
>> +cleanup:
>> + VIR_FREE(pciConfigAddr);
>> + VIR_FREE(physfn_sysfs_path);
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> * virNetDevGetPhysicalFunction
>> *
>> * @ifname : name of the physical function interface name
>> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
>> index 705ad9c..2e102f2 100644
>> --- a/src/util/virnetdev.h
>> +++ b/src/util/virnetdev.h
>> @@ -99,6 +99,12 @@ int virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname,
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>> ATTRIBUTE_RETURN_CHECK;
>>
>> +int virNetDevGetPhysicalFunctionFromVfPciAddr(unsigned domain, unsigned bus,
>> + unsigned slot, unsigned function,
>> + char **pfname)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>> + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
>> +
>> int virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list