On Wed, Mar 4, 2015 at 6:45 PM, Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: > 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. Thanks for taking time to explain. That is also my understanding on those flags. > > 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 > I understand the issue on the API here. I will go for extending the iommu API with iommu_domain_capable, except if you have any reason to push for the second solution. _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
