[libvirt] [PATCH v3 1/4] virutil: Introduce virHostHasIOMMU

[Michal Privoznik]

On 05/31/2018 02:30 PM, Filip Alac wrote:
> ---
>  src/conf/capabilities.c |  6 ++++++
>  src/conf/capabilities.h |  3 +++
>  src/util/virutil.c      | 28 ++++++++++++++++++++++++++++
>  src/util/virutil.h      |  2 ++
>  4 files changed, 39 insertions(+)

It would be better if you'd merge this and next patch into one and call
it "Move out IOMMU detection into a separate function", or something
similar. The reason is that at no point the code is duplicated, where as
with this approach it is (even if for a short time).

> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index dd2fc77f91..ba19d5db8c 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -1743,3 +1743,9 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>      virBitmapFree(cpus);
>      return ret;
>  }
> +
> +void
> +virCapabilitiesHostInitIOMMU(virCapsPtr caps)
> +{
> +    caps->host.iommu = virHostHasIOMMU();
> +}

This does not belong here. The proper order is to prepare everything and
introduce new feature after that. Imagine that virHostHasIOMMU() will be
used later, in a commit that fixes a bug. Now, downstream maintainers
would probably want to backport that particular fix and since it uses
virHostHasIOMMU() they will also backport this commit. But at the same
time they would be backporting a new feature which is something you
shouldn't do (the whole point of backporting commits is to stabilize
code base and introducing new features goes against that).

> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index f0a06a24df..fe1b9ea455 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -183,6 +183,7 @@ struct _virCapsHost {
>      int nPagesSize;             /* size of pagesSize array */
>      unsigned int *pagesSize;    /* page sizes support on the system */
>      unsigned char host_uuid[VIR_UUID_BUFLEN];
> +    bool iommu;
>  };
>  
>  typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
> @@ -327,4 +328,6 @@ void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr);
>  
>  int virCapabilitiesInitCaches(virCapsPtr caps);
>  
> +void virCapabilitiesHostInitIOMMU(virCapsPtr caps);
> +
>  #endif /* __VIR_CAPABILITIES_H */
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index bb4474acd5..7edcda0ee7 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2090,3 +2090,31 @@ virMemoryMaxValue(bool capped)
>      else
>          return LLONG_MAX;
>  }
> +
> +bool
> +virHostHasIOMMU(void)
> +{
> +    DIR *iommuDir = NULL;
> +    struct dirent *iommuGroup = NULL;
> +    bool ret = false;
> +    int direrr;
> +
> +    /* condition 1 - /sys/kernel/iommu_groups/ contains entries */
> +    if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0)
> +        goto cleanup;
> +
> +    while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) {
> +        /* assume we found a group */
> +        break;
> +    }
> +
> +    if (direrr < 0 || !iommuGroup)
> +        goto cleanup;
> +    /* okay, iommu is on and recognizes groups */
> +
> +    ret = true;
> +
> + cleanup:
> +    VIR_DIR_CLOSE(iommuDir);
> +    return ret;
> +}

Since this is copied from qemuHostdevHostSupportsPassthroughVFIO() the
correct patch would be where you're moving this part out of there into here.

> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index be0f6b0ea8..1ba9635bd9 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -216,6 +216,8 @@ unsigned long long virMemoryLimitTruncate(unsigned long long value);
>  bool virMemoryLimitIsSet(unsigned long long value);
>  unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
>  
> +bool virHostHasIOMMU(void);
> +

This patch misses libvirt_private.syms adjustment. Again, long story
short, one patch needs to contain one semantic change. And looking into
next patch (where you have the syms change) does not follow the rule.

Michal