From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1 Date: Fri, 6 Mar 2015 10:41:48 +0000 Message-ID: <20150306104148.GC22377@arm.com> References: <1425485274-5709-1-git-send-email-b.reynal@virtualopensystems.com> <1425491104.5200.268.camel@redhat.com> <54F893C3.6020006@linaro.org> <1425589915.5200.336.camel@redhat.com> <54F96D96.8040207@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <54F96D96.8040207-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Eric Auger Cc: "tech-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org" , "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" , "kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org" List-Id: iommu@lists.linux-foundation.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