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

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

Hi Eric,

On Fri, Mar 06, 2015 at 09:04:22AM +0000, Eric Auger wrote:
> Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
> not a direct dependency of vfio-platform driver although I understand
> Baptiste needs it to test with PL330 device. On my side I can test
> vfio-platform driver without it, assigning the Midway Calxeda xgmac.

Realistically, I don't expect anybody using device passthrough for
practical applications to be using a PL330. Your xgmac example is far
more compelling.

> Also if I am not wrong the title of the series now does not really
> reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
> type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
> supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.
> 
> However with this issue of IOMMU_CACHE always set along with arm-smmu,
> there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
> since the integrated pieces are not functional.

Well, I'm still trying to understand exactly what is happening in your case:

  - Is the xgmac coherent or not? Does it have a "dma-coherent" property?
  - Are you installing the SMMU page tables at stage-1 or stage-2?
  - If it *is* coherent, then we should use IOMMU_CACHE mappings for the
    DMA buffers and ensure that the guest knows it is coherent (by
    preserving the "dma-coherent" flag).
  - If it is *not* coherent, then the behaviour of IOMMU_CACHE depends
    on the stage of translation:

    * Stage-1: we will make the transactions cacheable, and you'll need
      to tell the guest that the device is actually cache coherent
    * Stage-2: IOMMU_CACHE won't actually have any effect, so everything
      should work as non-coherent.

In other words, I think you're probably just telling the guest the wrong
thing.

> On top of the dma-coherent property of the *master*, should not we also
> query the cache-coherent property of the interconnect downstream to the
> smmu?

I don't think so... "dma-coherent" should only be set on a master if the
interconnect is properly configured. It's supposed to be a "If you DMA now,
it will snoop the CPU caches" flag as opposed to "If you write a random
selection of MMIO registers in the SoC, then this device will be coherent".

> How can we progress quickly on this topic? is it acceptable to return
> false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack?

No; pretending that a device is not coherent when it actually is can lead
to corruption of DMA buffers due to unnecessary cache invalidation.

> As a longer term solution, would it make sense to add a user flag at VFIO
> user API level to turn the IOMMU_CACHE on?

I think userspace certainly needs a way to figure out if a device is
coherent or not, otherwise it can't generate the correct device-tree
properties for something like a KVM guest but the IOMMU_* setting should
remain in the kernel IMO. Similarly for things like MSI pages, which would
need to be mapped as device memory on ARM -- that should be exposed as a
higher level "please map my MSI page here" ioctl as opposed to requiring
userspace to supply the correct memory attributes.

Will