[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/8] Hostdev-hybrid mode requires a direct linkdev and direct mode.



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 redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]