| Message ID | cd11bc7851dbe46db6d14821a942678047331913.1597931876.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Convert arch/arm to use iommu-dma | expand |
On Thu, Aug 20, 2020 at 04:08:32PM +0100, Robin Murphy wrote: > Now that arch/arm is wired up for default domains and iommu-dma, > implement the corresponding driver-side support for DMA domains. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/tegra-smmu.c | 37 +++++++++++++++++++++---------------- > 1 file changed, 21 insertions(+), 16 deletions(-) We can't do that yet because it will currently still break for use-cases such as display where we don't properly set up identity mappings during boot. The result is that the dma-iommu code will enable translations before the driver gets a chance to set up any mappings and if the display controller was left on by the bootloader, scanning out a splash screen, this causes faults between the point where dma-iommu is being set up for the display controller and where the display controller starts mapping its own buffers (rather than the ones mapped by the bootloader). That said, I do have a series that I've been carrying around for longer than I've wanted that does exactly this for Tegra SMMU and I'd prefer if you could drop this particular change from your series so that I can keep working on resolving the identity mapping issues first. Thierry
On 2020-08-27 16:45, Thierry Reding wrote: > On Thu, Aug 20, 2020 at 04:08:32PM +0100, Robin Murphy wrote: >> Now that arch/arm is wired up for default domains and iommu-dma, >> implement the corresponding driver-side support for DMA domains. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/iommu/tegra-smmu.c | 37 +++++++++++++++++++++---------------- >> 1 file changed, 21 insertions(+), 16 deletions(-) > > We can't do that yet because it will currently still break for use-cases > such as display where we don't properly set up identity mappings during > boot. The result is that the dma-iommu code will enable translations > before the driver gets a chance to set up any mappings and if the > display controller was left on by the bootloader, scanning out a splash > screen, this causes faults between the point where dma-iommu is being > set up for the display controller and where the display controller > starts mapping its own buffers (rather than the ones mapped by the > bootloader). Rest assured that I understand the situation all too well ;) As with tegra-gart, the unspoken point here is that since tegra-smmu implements of_xlate(), then arm_setup_iommu_dma_ops() must already be causing the exact same problem, no? This patch only seeks to move any existing behaviour over to the common backend, regardless of whether it was ever really appropriate in the first place. > That said, I do have a series that I've been carrying around for longer > than I've wanted that does exactly this for Tegra SMMU and I'd prefer if > you could drop this particular change from your series so that I can > keep working on resolving the identity mapping issues first. That would mean you'd see a functional change from the final patch, wherein nothing would ever be able to get translation unless drivers do their own explicit IOMMU API management. If you definitely want that change then OK, but it would still be nice to do it "properly" with IOMMU_DOMAIN_IDENTITY support, rather than just forcibly failing default domain allocation. I'm having a go at reworking the tegra-gart patch in that direction, so I can certainly try it for tegra-smmu as well. Robin.
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 124c8848ab7e..8e276eac84d9 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -5,6 +5,7 @@ #include <linux/bitops.h> #include <linux/debugfs.h> +#include <linux/dma-iommu.h> #include <linux/err.h> #include <linux/iommu.h> #include <linux/kernel.h> @@ -278,7 +279,7 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) { struct tegra_smmu_as *as; - if (type != IOMMU_DOMAIN_UNMANAGED) + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA) return NULL; as = kzalloc(sizeof(*as), GFP_KERNEL); @@ -288,25 +289,19 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); - if (!as->pd) { - kfree(as); - return NULL; - } + if (!as->pd) + goto out_free_as; as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); - if (!as->count) { - __free_page(as->pd); - kfree(as); - return NULL; - } + if (!as->count) + goto out_free_all; as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); - if (!as->pts) { - kfree(as->count); - __free_page(as->pd); - kfree(as); - return NULL; - } + if (!as->pts) + goto out_free_all; + + if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(&as->domain)) + goto out_free_all; /* setup aperture */ as->domain.geometry.aperture_start = 0; @@ -314,12 +309,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) as->domain.geometry.force_aperture = true; return &as->domain; + +out_free_all: + kfree(as->pts); + kfree(as->count); + __free_page(as->pd); +out_free_as: + kfree(as); + return NULL; } static void tegra_smmu_domain_free(struct iommu_domain *domain) { struct tegra_smmu_as *as = to_smmu_as(domain); + iommu_put_dma_cookie(domain); + /* TODO: free page directory and page tables */ WARN_ON_ONCE(as->use_count);
Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/tegra-smmu.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)