Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, 2015-03-04 at 17:07 +0100, Baptiste Reynal wrote:
> This patch series makes the VFIO_IOMMU_TYPE1 driver buildable on ARM, so it
> may be used with ARM SMMUs. It also adds support for the IOMMU_NOEXEC flag
> supported by SMMUs adhering to the ARM SMMU specification so the VFIO user can
> specify whether the target memory can be executed by the device behind the
> SMMU.

Ok, so the differences between IOMMU_CACHE and IOMMU_NOEXEC are twofold.
First is that when the IOMMU is IOMMU_CAP_CACHE_COHERENCY we always use
IOMMU_CACHE for all mappings within the domain.  This is not a user
provided mapping flag.  Second, if a device is added behind an IOMMU
that is not IOMMU_CAP_CACHE_COHERENCY capable, this simply means that it
needs to be managed as a separate domain within the same container.
vfio_domains_have_iommu_cache() reports true only if all domains within
the container support, and are therefore using, IOMMU_CACHE.

On the other hand, IOMMU_NOEXEC is a feature, but not a global mapping
flag for a domain.  There are potentially some mappings with it within a
domain and some without, just like the IOMMU_READ/IOMMU_WRITE.  It's
therefore a per-DMA mapping flag as you've implemented in patch 4.
Second, as a user specified flag, support for it is mandatory.  If a
device is added behind an IOMMU that does not support NOEXEC and NOEXEC
mappings are in use, the device must be rejected from addition to the
container.

Hopefully that's correct, because I think that's what the code does.
The problem I have with the code is that I don't think it's acceptable
to call iommu_ops.capable() directly.  This is breaking an abstraction
of the IOMMU API.  The obvious problem with that is:

bool iommu_capable(struct bus_type *bus, enum iommu_cap cap)

I assume you're calling the op directly because of bus_type, but that's
no excuse to misuse the API.  Our choice is either to try to extend the
API, maybe create:

bool iommu_domain_capable(struct iommu_domain *domain, enum iommu_cap cap)

Or to use the available function by either caching the value when we do
have a bus_type available to us, or storing or discovering bus_type.  It
doesn't seem that terrible to cache it on the domain.  Be careful though
that if a device imposing lack of NOEXEC on a domain is removed, the
ability to do NOEXEC mappings should be reinstated.  That would be easy
if we use separate vfio_domains to manage NOEXEC capable vs NOEXEC
in-capable devices (like we do CACHE vs no-CACHE), but there's some
additional overhead to do that since it implies a separate iommu_domain.
Thanks,

Alex