In Tegra SoC, IOMMU can set some mapping attribute for each page, for exmaple, READable, and WRITEable. We'd like to use this feature *newly* for robustness, where read-only pages can be protected from being overwritten by wrong operation. We have been using IOMMU via DMA mapping API(ARM). DMA mapping API currently doesn't use "prot" parameter when calling IOMMU API. So this series tries to pass "struct dma_attrs *attrs" via "int prot" in IOMMU API. I'm not so sure if this implementation is right or not because:
- Casting (struct dma_attrs *) to (int) in DMA API doesn't look nice. - IOMMU layer needs to cast (int) back to (struct dma_attrs *) again, which can be considered as violation of layers.
If you have any implementations/suggestions, it would be really helpful.
This series isn't applied cleanly but this is posted to request for comments.
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
arch/arm/mm/dma-mapping.c | 34 +++++++++++++++++++++------------- drivers/iommu/tegra-smmu.c | 41 +++++++++++++++++++++++++++++------------ include/linux/dma-attrs.h | 1 + 3 files changed, 51 insertions(+), 25 deletions(-)
This patch adds DMA_ATTR_READ_ONLY attribute to the DMA-mapping subsystem.
This sets mapping attribute read-only.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- include/linux/dma-attrs.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h index f272809..5d7371c 100644 --- a/include/linux/dma-attrs.h +++ b/include/linux/dma-attrs.h @@ -18,6 +18,7 @@ enum dma_attr { DMA_ATTR_NO_KERNEL_MAPPING, DMA_ATTR_SKIP_CPU_SYNC, DMA_ATTR_SKIP_FREE_IOVA, + DMA_ATTR_READ_ONLY, DMA_ATTR_MAX, };
Pass DMA attribute as IOMMU property, which can be proccessed in the backend implementation of IOMMU. For example, DMA_ATTR_READ_ONLY can be translated into each IOMMU H/W implementaion.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/mm/dma-mapping.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 4152ed6..cbc6768 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1254,7 +1254,8 @@ err: */ static dma_addr_t ____iommu_create_mapping(struct device *dev, dma_addr_t *req, - struct page **pages, size_t size) + struct page **pages, size_t size, + struct dma_attrs *attrs) { struct dma_iommu_mapping *mapping = dev->archdata.mapping; unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT; @@ -1280,7 +1281,7 @@ ____iommu_create_mapping(struct device *dev, dma_addr_t *req, break;
len = (j - i) << PAGE_SHIFT; - ret = iommu_map(mapping->domain, iova, phys, len, 0); + ret = iommu_map(mapping->domain, iova, phys, len, (int)attrs); if (ret < 0) goto fail; iova += len; @@ -1294,9 +1295,10 @@ fail: }
static dma_addr_t -__iommu_create_mapping(struct device *dev, struct page **pages, size_t size) +__iommu_create_mapping(struct device *dev, struct page **pages, size_t size, + struct dma_attrs *attrs) { - return ____iommu_create_mapping(dev, NULL, pages, size); + return ____iommu_create_mapping(dev, NULL, pages, size, attrs); }
static int __iommu_remove_mapping(struct device *dev, dma_addr_t iova, size_t size) @@ -1332,7 +1334,7 @@ static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs) }
static void *__iommu_alloc_atomic(struct device *dev, size_t size, - dma_addr_t *handle) + dma_addr_t *handle, struct dma_attrs *attrs) { struct page *page; void *addr; @@ -1341,7 +1343,7 @@ static void *__iommu_alloc_atomic(struct device *dev, size_t size, if (!addr) return NULL;
- *handle = __iommu_create_mapping(dev, &page, size); + *handle = __iommu_create_mapping(dev, &page, size, attrs); if (*handle == DMA_ERROR_CODE) goto err_mapping;
@@ -1378,17 +1380,20 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size, size = PAGE_ALIGN(size);
if (gfp & GFP_ATOMIC) - return __iommu_alloc_atomic(dev, size, handle); + + return __iommu_alloc_atomic(dev, size, handle, attrs);
pages = __iommu_alloc_buffer(dev, size, gfp); if (!pages) return NULL;
if (*handle == DMA_ERROR_CODE) - *handle = __iommu_create_mapping(dev, pages, size); + *handle = __iommu_create_mapping(dev, pages, size, attrs); else - *handle = ____iommu_create_mapping(dev, handle, pages, size); + *handle = ____iommu_create_mapping(dev, handle, pages, size, + attrs);
+ *handle = __iommu_create_mapping(dev, pages, size, attrs); if (*handle == DMA_ERROR_CODE) goto err_buffer;
@@ -1513,7 +1518,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
skip_cmaint: count = size >> PAGE_SHIFT; - ret = iommu_map_sg(mapping->domain, iova_base, sg, count, 0); + ret = iommu_map_sg(mapping->domain, iova_base, sg, count, (int)attrs); if (WARN_ON(ret < 0)) goto fail;
@@ -1716,7 +1721,8 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p if (dma_addr == DMA_ERROR_CODE) return dma_addr;
- ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0); + ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, + (int)attrs); if (ret < 0) goto fail;
@@ -1756,7 +1762,8 @@ static dma_addr_t arm_iommu_map_page_at(struct device *dev, struct page *page, if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs)) __dma_page_cpu_to_dev(page, offset, size, dir);
- ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0); + ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, + (int)attrs); if (ret < 0) return DMA_ERROR_CODE;
@@ -1778,7 +1785,8 @@ static dma_addr_t arm_iommu_map_pages(struct device *dev, struct page **pages, __dma_page_cpu_to_dev(pages[i], 0, PAGE_SIZE, dir); }
- ret = iommu_map_pages(mapping->domain, dma_handle, pages, count, 0); + ret = iommu_map_pages(mapping->domain, dma_handle, pages, count, + (int)attrs); if (ret < 0) return DMA_ERROR_CODE;
Hi,
It would be better to define a prot flag bit in iommu API and convert the attrs to prot flag bit in dma-mapping aPI before calling the iommu API.
On Thu, Jun 20, 2013 at 11:19 AM, Hiroshi Doyu hdoyu@nvidia.com wrote:
Pass DMA attribute as IOMMU property, which can be proccessed in the backend implementation of IOMMU. For example, DMA_ATTR_READ_ONLY can be translated into each IOMMU H/W implementaion.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
arch/arm/mm/dma-mapping.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 4152ed6..cbc6768 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1254,7 +1254,8 @@ err: */ static dma_addr_t ____iommu_create_mapping(struct device *dev, dma_addr_t *req,
struct page **pages, size_t size)
struct page **pages, size_t size,struct dma_attrs *attrs){ struct dma_iommu_mapping *mapping = dev->archdata.mapping; unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT; @@ -1280,7 +1281,7 @@ ____iommu_create_mapping(struct device *dev, dma_addr_t *req, break;
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len, 0);
ret = iommu_map(mapping->domain, iova, phys, len, (int)attrs);
Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
if (ret < 0) goto fail; iova += len;@@ -1294,9 +1295,10 @@ fail: }
static dma_addr_t -__iommu_create_mapping(struct device *dev, struct page **pages, size_t size) +__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
struct dma_attrs *attrs){
return ____iommu_create_mapping(dev, NULL, pages, size);
return ____iommu_create_mapping(dev, NULL, pages, size, attrs);}
static int __iommu_remove_mapping(struct device *dev, dma_addr_t iova, size_t size) @@ -1332,7 +1334,7 @@ static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs) }
static void *__iommu_alloc_atomic(struct device *dev, size_t size,
dma_addr_t *handle)
dma_addr_t *handle, struct dma_attrs *attrs){ struct page *page; void *addr; @@ -1341,7 +1343,7 @@ static void *__iommu_alloc_atomic(struct device *dev, size_t size, if (!addr) return NULL;
*handle = __iommu_create_mapping(dev, &page, size);
*handle = __iommu_create_mapping(dev, &page, size, attrs); if (*handle == DMA_ERROR_CODE) goto err_mapping;@@ -1378,17 +1380,20 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size, size = PAGE_ALIGN(size);
if (gfp & GFP_ATOMIC)
return __iommu_alloc_atomic(dev, size, handle);
return __iommu_alloc_atomic(dev, size, handle, attrs); pages = __iommu_alloc_buffer(dev, size, gfp); if (!pages) return NULL; if (*handle == DMA_ERROR_CODE)
*handle = __iommu_create_mapping(dev, pages, size);
*handle = __iommu_create_mapping(dev, pages, size, attrs); else
*handle = ____iommu_create_mapping(dev, handle, pages, size);
*handle = ____iommu_create_mapping(dev, handle, pages, size,attrs);*handle = __iommu_create_mapping(dev, pages, size, attrs); if (*handle == DMA_ERROR_CODE) goto err_buffer;@@ -1513,7 +1518,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
skip_cmaint: count = size >> PAGE_SHIFT;
ret = iommu_map_sg(mapping->domain, iova_base, sg, count, 0);
ret = iommu_map_sg(mapping->domain, iova_base, sg, count, (int)attrs);
Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
if (WARN_ON(ret < 0)) goto fail;@@ -1716,7 +1721,8 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p if (dma_addr == DMA_ERROR_CODE) return dma_addr;
ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len,(int)attrs);
Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
if (ret < 0) goto fail;@@ -1756,7 +1762,8 @@ static dma_addr_t arm_iommu_map_page_at(struct device *dev, struct page *page, if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs)) __dma_page_cpu_to_dev(page, offset, size, dir);
ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len,(int)attrs);
Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
if (ret < 0) return DMA_ERROR_CODE;@@ -1778,7 +1785,8 @@ static dma_addr_t arm_iommu_map_pages(struct device *dev, struct page **pages, __dma_page_cpu_to_dev(pages[i], 0, PAGE_SIZE, dir); }
ret = iommu_map_pages(mapping->domain, dma_handle, pages, count, 0);
ret = iommu_map_pages(mapping->domain, dma_handle, pages, count,(int)attrs);
Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
if (ret < 0) return DMA_ERROR_CODE;-- 1.8.1.5
Linaro-mm-sig mailing list Linaro-mm-sig@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
Hi Nishanth,
Nishanth Peethambaran nishanth.p@gmail.com wrote @ Thu, 20 Jun 2013 10:07:00 +0200:
It would be better to define a prot flag bit in iommu API and convert the attrs to prot flag bit in dma-mapping aPI before calling the iommu API.
That's the 1st option.
On Thu, Jun 20, 2013 at 11:19 AM, Hiroshi Doyu hdoyu@nvidia.com wrote:
....
@@ -1280,7 +1281,7 @@ ____iommu_create_mapping(struct device *dev, dma_addr_t *req, break;
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len, 0);
ret = iommu_map(mapping->domain, iova, phys, len, (int)attrs);Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
Both DMA_ATTR_READ_ONLY and IOMMU_READ are just logical bit in their layers respectively and eventually it's converted to H/W dependent bit.
If IOMMU is considered as one of specific case of DMA, sharing dma_attr between IOMMU and DMA may not be so bad. IIRC, ARM: dma-mapping API was implemented based on this concept(?).
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index d8f98b1..161a1b0 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -755,7 +755,7 @@ int iommu_domain_has_cap(struct iommu_domain *domain, EXPORT_SYMBOL_GPL(iommu_domain_has_cap);
int iommu_map(struct iommu_domain *domain, unsigned long iova, - phys_addr_t paddr, size_t size, int prot) + phys_addr_t paddr, size_t size, struct dma_attr *attrs) { unsigned long orig_iova = iova; unsigned int min_pagesz;
Hi Hiroshi,
My preference would be to keep the iommu prot flags separate from dma-mapping attrs. dma-attrs have options which are not related to iommu - like having ARM kernel mapping. iommu.h is at a much lower level where we can define prot flags which are useful for iommu page-table management.
Thinking again on it, I feel the translation of dma-attr should happen when dma-mapping code makes calls to " iommu mapping" API. "iommu mapping" API which does iova management and make iommu API calls could be taken out from dma-mapping.c, for which I see discussion already happening for arm64.
If devices allocate via dma-mapping API, physical mem allocation, iova allocation and iommu mapping happens internally. Devices may allocate physical memory using any allocator (say ION including carveout area), use "iommu mapping" API which will do iova allocation and iommu mapping. The prot flags could be useful in this case as well - not sure whether we would need dma-attrs here.
- Nishanth Peethambaran
- Nishanth Peethambaran +91-9448074166
On Thu, Jun 20, 2013 at 1:54 PM, Hiroshi Doyu hdoyu@nvidia.com wrote:
Hi Nishanth,
Nishanth Peethambaran nishanth.p@gmail.com wrote @ Thu, 20 Jun 2013 10:07:00 +0200:
It would be better to define a prot flag bit in iommu API and convert the attrs to prot flag bit in dma-mapping aPI before calling the iommu API.
That's the 1st option.
On Thu, Jun 20, 2013 at 11:19 AM, Hiroshi Doyu hdoyu@nvidia.com wrote:
....
@@ -1280,7 +1281,7 @@ ____iommu_create_mapping(struct device *dev, dma_addr_t *req, break;
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len, 0);
ret = iommu_map(mapping->domain, iova, phys, len, (int)attrs);Use dma_get_attr and translate the READ_ONLY attr to a new READ_ONLY prot flag bit which needs to be defined in iommu.h
Both DMA_ATTR_READ_ONLY and IOMMU_READ are just logical bit in their layers respectively and eventually it's converted to H/W dependent bit.
If IOMMU is considered as one of specific case of DMA, sharing dma_attr between IOMMU and DMA may not be so bad. IIRC, ARM: dma-mapping API was implemented based on this concept(?).
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index d8f98b1..161a1b0 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -755,7 +755,7 @@ int iommu_domain_has_cap(struct iommu_domain *domain, EXPORT_SYMBOL_GPL(iommu_domain_has_cap);
int iommu_map(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot)
phys_addr_t paddr, size_t size, struct dma_attr *attrs){ unsigned long orig_iova = iova; unsigned int min_pagesz;
Nishanth Peethambaran nishanth.p@gmail.com wrote @ Thu, 20 Jun 2013 10:55:32 +0200:
Thinking again on it, I feel the translation of dma-attr should happen when dma-mapping code makes calls to " iommu mapping" API. "iommu mapping" API which does iova management and make iommu API calls could be taken out from dma-mapping.c, for which I see discussion already happening for arm64.
According to the discussion ARM64:dma-mapping, I got an implression that IOVA management part in ARM:dma-mapping API would be a new common module(ie: /lib/iommu-helper.c), but still IOMMU API itself would reamin as primitive as what they are.
+--------------------------------- | DMA mapping API | +----------------- +--------------+-----------+ | DMA mapping API($ARCH) | IOMMU API | IOVA MGT | +------------------ +--------------+-----------+ |IOMMU API(H/W)| +--------------+ | IOMMU H/W | +--------------+
If devices allocate via dma-mapping API, physical mem allocation, iova allocation and iommu mapping happens internally. Devices may allocate physical memory using any allocator (say ION including carveout area), use "iommu mapping" API which will do iova allocation and iommu mapping. The prot flags could be useful in this case as well - not sure whether we would need dma-attrs here.
I haven't followed ION recently, but can't ION backed by DMA mapping API instead of using IOMMU API directly?
On Thursday 20 June 2013, Hiroshi Doyu wrote:
If devices allocate via dma-mapping API, physical mem allocation, iova allocation and iommu mapping happens internally. Devices may allocate physical memory using any allocator (say ION including carveout area), use "iommu mapping" API which will do iova allocation and iommu mapping. The prot flags could be useful in this case as well - not sure whether we would need dma-attrs here.
I haven't followed ION recently, but can't ION backed by DMA mapping API instead of using IOMMU API directly?
For a GPU with an IOMMU, you typically want per-process IOMMU contexts, which are only available when using the IOMMU API directly, as the dma-mapping abstraction uses only one context for kernel space.
Arnd
Arnd Bergmann arnd@arndb.de wrote @ Thu, 20 Jun 2013 12:13:13 +0200:
On Thursday 20 June 2013, Hiroshi Doyu wrote:
If devices allocate via dma-mapping API, physical mem allocation, iova allocation and iommu mapping happens internally. Devices may allocate physical memory using any allocator (say ION including carveout area), use "iommu mapping" API which will do iova allocation and iommu mapping. The prot flags could be useful in this case as well - not sure whether we would need dma-attrs here.
I haven't followed ION recently, but can't ION backed by DMA mapping API instead of using IOMMU API directly?
For a GPU with an IOMMU, you typically want per-process IOMMU contexts, which are only available when using the IOMMU API directly, as the dma-mapping abstraction uses only one context for kernel space.
Yes, we had some experiment for switching IOMMU context with DMA mapping API. We needed to add some new DMA mapping API, and didn't look so nice at that time. What do you think to introduce multiple context or switching context in dma-mapping abstruction?
On Thursday 20 June 2013, Hiroshi Doyu wrote:
Arnd Bergmann arnd@arndb.de wrote @ Thu, 20 Jun 2013 12:13:13 +0200:
On Thursday 20 June 2013, Hiroshi Doyu wrote:
If devices allocate via dma-mapping API, physical mem allocation, iova allocation and iommu mapping happens internally. Devices may allocate physical memory using any allocator (say ION including carveout area), use "iommu mapping" API which will do iova allocation and iommu mapping. The prot flags could be useful in this case as well - not sure whether we would need dma-attrs here.
I haven't followed ION recently, but can't ION backed by DMA mapping API instead of using IOMMU API directly?
For a GPU with an IOMMU, you typically want per-process IOMMU contexts, which are only available when using the IOMMU API directly, as the dma-mapping abstraction uses only one context for kernel space.
Yes, we had some experiment for switching IOMMU context with DMA mapping API. We needed to add some new DMA mapping API, and didn't look so nice at that time. What do you think to introduce multiple context or switching context in dma-mapping abstruction?
My feeling is that drivers with the need for multiple contexts are better off using the iommu API, since that is what it was made for. The dma-mapping abstraction really tries to hide the bus address assignment, while users with multiple contexts typically also want to control the bus addresses.
Arnd
On Thu, Jun 20, 2013 at 4:27 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 20 June 2013, Hiroshi Doyu wrote:
Arnd Bergmann arnd@arndb.de wrote @ Thu, 20 Jun 2013 12:13:13 +0200:
On Thursday 20 June 2013, Hiroshi Doyu wrote:
If devices allocate via dma-mapping API, physical mem allocation, iova allocation and iommu mapping happens internally. Devices may allocate physical memory using any allocator (say ION including carveout area), use "iommu mapping" API which will do iova allocation and iommu mapping. The prot flags could be useful in this case as well - not sure whether we would need dma-attrs here.
I haven't followed ION recently, but can't ION backed by DMA mapping API instead of using IOMMU API directly?
For a GPU with an IOMMU, you typically want per-process IOMMU contexts, which are only available when using the IOMMU API directly, as the dma-mapping abstraction uses only one context for kernel space.
Yes, we had some experiment for switching IOMMU context with DMA mapping API. We needed to add some new DMA mapping API, and didn't look so nice at that time. What do you think to introduce multiple context or switching context in dma-mapping abstruction?
My feeling is that drivers with the need for multiple contexts are better off using the iommu API, since that is what it was made for. The dma-mapping abstraction really tries to hide the bus address assignment, while users with multiple contexts typically also want to control the bus addresses.
Arnd
ION is more of a physical memory allocator supporting buddy pages as well as memory reserved at boot-time. DMA type of heap is only one of the types of heap. For system heap, ION provides an sg_table which device will have to map it using iommu API to get dma_address for the device. Even for DMA type of heap, each device will have to map the pages to its own iommu as Arnd mentioned.
- Nishanth Peethambaran
Support read-only mapping via struct dma_attrs.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- drivers/iommu/tegra-smmu.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index fab1f19..3aff4cd 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -862,12 +862,13 @@ static size_t __smmu_iommu_unmap_largepage(struct smmu_as *as, dma_addr_t iova) }
static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova, - unsigned long pfn) + unsigned long pfn, int prot) { struct smmu_device *smmu = as->smmu; unsigned long *pte; unsigned int *count; struct page *page; + int attrs = as->pte_attr;
pte = locate_pte(as, iova, true, &page, &count); if (WARN_ON(!pte)) @@ -875,7 +876,11 @@ static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova,
if (*pte == _PTE_VACANT(iova)) (*count)++; - *pte = SMMU_PFN_TO_PTE(pfn, as->pte_attr); + + if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot)) + attrs &= ~_WRITABLE; + + *pte = SMMU_PFN_TO_PTE(pfn, attrs); FLUSH_CPU_DCACHE(pte, page, sizeof(*pte)); flush_ptc_and_tlb(smmu, as, iova, pte, page, 0); put_signature(as, iova, pfn); @@ -883,23 +888,27 @@ static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova, }
static int __smmu_iommu_map_page(struct smmu_as *as, dma_addr_t iova, - phys_addr_t pa) + phys_addr_t pa, int prot) { unsigned long pfn = __phys_to_pfn(pa);
- return __smmu_iommu_map_pfn(as, iova, pfn); + return __smmu_iommu_map_pfn(as, iova, pfn, prot); }
static int __smmu_iommu_map_largepage(struct smmu_as *as, dma_addr_t iova, - phys_addr_t pa) + phys_addr_t pa, int prot) { unsigned long pdn = SMMU_ADDR_TO_PDN(iova); unsigned long *pdir = (unsigned long *)page_address(as->pdir_page); + int attrs = _PDE_ATTR;
if (pdir[pdn] != _PDE_VACANT(pdn)) return -EINVAL;
- pdir[pdn] = SMMU_ADDR_TO_PDN(pa) << 10 | _PDE_ATTR; + if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot)) + attrs &= ~_WRITABLE; + + pdir[pdn] = SMMU_ADDR_TO_PDN(pa) << 10 | attrs; FLUSH_CPU_DCACHE(&pdir[pdn], as->pdir_page, sizeof pdir[pdn]); flush_ptc_and_tlb(as->smmu, as, iova, &pdir[pdn], as->pdir_page, 1);
@@ -912,7 +921,8 @@ static int smmu_iommu_map(struct iommu_domain *domain, unsigned long iova, struct smmu_as *as = domain->priv; unsigned long flags; int err; - int (*fn)(struct smmu_as *as, dma_addr_t iova, phys_addr_t pa); + int (*fn)(struct smmu_as *as, dma_addr_t iova, phys_addr_t pa, + int prot);
dev_dbg(as->smmu->dev, "[%d] %08lx:%08x\n", as->asid, iova, pa);
@@ -929,7 +939,7 @@ static int smmu_iommu_map(struct iommu_domain *domain, unsigned long iova, }
spin_lock_irqsave(&as->lock, flags); - err = fn(as, iova, pa); + err = fn(as, iova, pa, prot); spin_unlock_irqrestore(&as->lock, flags); return err; } @@ -943,6 +953,10 @@ static int smmu_iommu_map_pages(struct iommu_domain *domain, unsigned long iova, unsigned long *pdir = page_address(as->pdir_page); int err = 0; bool flush_all = (total > SZ_512) ? true : false; + int attrs = as->pte_attr; + + if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot)) + attrs &= ~_WRITABLE;
spin_lock_irqsave(&as->lock, flags);
@@ -977,8 +991,7 @@ static int smmu_iommu_map_pages(struct iommu_domain *domain, unsigned long iova, if (*pte == _PTE_VACANT(iova + i * PAGE_SIZE)) (*rest)++;
- *pte = SMMU_PFN_TO_PTE(page_to_pfn(pages[i]), - as->pte_attr); + *pte = SMMU_PFN_TO_PTE(page_to_pfn(pages[i]), attrs); }
pte = &ptbl[ptn]; @@ -1010,6 +1023,10 @@ static int smmu_iommu_map_sg(struct iommu_domain *domain, unsigned long iova, bool flush_all = (nents * PAGE_SIZE > SZ_512) ? true : false; struct smmu_as *as = domain->priv; struct smmu_device *smmu = as->smmu; + int attrs = as->pte_attr; + + if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot)) + attrs &= ~_WRITABLE;
for (count = 0, s = sgl; count < nents; s = sg_next(s)) { phys_addr_t phys = page_to_phys(sg_page(s)); @@ -1053,7 +1070,7 @@ static int smmu_iommu_map_sg(struct iommu_domain *domain, unsigned long iova, (*rest)++; }
- *pte = SMMU_PFN_TO_PTE(pfn + i, as->pte_attr); + *pte = SMMU_PFN_TO_PTE(pfn + i, attrs); }
pte = &ptbl[ptn]; @@ -1191,7 +1208,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain, struct page *page;
page = as->smmu->avp_vector_page; - __smmu_iommu_map_pfn(as, 0, page_to_pfn(page)); + __smmu_iommu_map_pfn(as, 0, page_to_pfn(page), 0);
pr_debug("Reserve "page zero" \ for AVP vectors using a common dummy\n");
On Thu, Jun 20, 2013 at 2:49 PM, Hiroshi Doyu hdoyu@nvidia.com wrote:
Support read-only mapping via struct dma_attrs.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
drivers/iommu/tegra-smmu.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index fab1f19..3aff4cd 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -862,12 +862,13 @@ static size_t __smmu_iommu_unmap_largepage(struct smmu_as *as, dma_addr_t iova) }
static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova,
unsigned long pfn)
unsigned long pfn, int prot)
Can you find 'prot' is used at other arch? In previous patch, you cast it as 'int' but below code cast it as 'struct dma_attr' again. doesn't it better to use 'struct dma_attr' as parameter to avoid double cast? Of course you have to modify existing APIs to use 'struct dma_attr'.
Thank you, Kyungmin Park
{ struct smmu_device *smmu = as->smmu; unsigned long *pte; unsigned int *count; struct page *page;
int attrs = as->pte_attr; pte = locate_pte(as, iova, true, &page, &count); if (WARN_ON(!pte))@@ -875,7 +876,11 @@ static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova,
if (*pte == _PTE_VACANT(iova)) (*count)++;
*pte = SMMU_PFN_TO_PTE(pfn, as->pte_attr);
if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot))attrs &= ~_WRITABLE;*pte = SMMU_PFN_TO_PTE(pfn, attrs); FLUSH_CPU_DCACHE(pte, page, sizeof(*pte)); flush_ptc_and_tlb(smmu, as, iova, pte, page, 0); put_signature(as, iova, pfn);@@ -883,23 +888,27 @@ static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova, }
static int __smmu_iommu_map_page(struct smmu_as *as, dma_addr_t iova,
phys_addr_t pa)
phys_addr_t pa, int prot){ unsigned long pfn = __phys_to_pfn(pa);
return __smmu_iommu_map_pfn(as, iova, pfn);
return __smmu_iommu_map_pfn(as, iova, pfn, prot);}
static int __smmu_iommu_map_largepage(struct smmu_as *as, dma_addr_t iova,
phys_addr_t pa)
phys_addr_t pa, int prot){ unsigned long pdn = SMMU_ADDR_TO_PDN(iova); unsigned long *pdir = (unsigned long *)page_address(as->pdir_page);
int attrs = _PDE_ATTR; if (pdir[pdn] != _PDE_VACANT(pdn)) return -EINVAL;
pdir[pdn] = SMMU_ADDR_TO_PDN(pa) << 10 | _PDE_ATTR;
if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot))attrs &= ~_WRITABLE;pdir[pdn] = SMMU_ADDR_TO_PDN(pa) << 10 | attrs; FLUSH_CPU_DCACHE(&pdir[pdn], as->pdir_page, sizeof pdir[pdn]); flush_ptc_and_tlb(as->smmu, as, iova, &pdir[pdn], as->pdir_page, 1);@@ -912,7 +921,8 @@ static int smmu_iommu_map(struct iommu_domain *domain, unsigned long iova, struct smmu_as *as = domain->priv; unsigned long flags; int err;
int (*fn)(struct smmu_as *as, dma_addr_t iova, phys_addr_t pa);
int (*fn)(struct smmu_as *as, dma_addr_t iova, phys_addr_t pa,int prot); dev_dbg(as->smmu->dev, "[%d] %08lx:%08x\n", as->asid, iova, pa);@@ -929,7 +939,7 @@ static int smmu_iommu_map(struct iommu_domain *domain, unsigned long iova, }
spin_lock_irqsave(&as->lock, flags);
err = fn(as, iova, pa);
err = fn(as, iova, pa, prot); spin_unlock_irqrestore(&as->lock, flags); return err;} @@ -943,6 +953,10 @@ static int smmu_iommu_map_pages(struct iommu_domain *domain, unsigned long iova, unsigned long *pdir = page_address(as->pdir_page); int err = 0; bool flush_all = (total > SZ_512) ? true : false;
int attrs = as->pte_attr;if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot))attrs &= ~_WRITABLE; spin_lock_irqsave(&as->lock, flags);@@ -977,8 +991,7 @@ static int smmu_iommu_map_pages(struct iommu_domain *domain, unsigned long iova, if (*pte == _PTE_VACANT(iova + i * PAGE_SIZE)) (*rest)++;
*pte = SMMU_PFN_TO_PTE(page_to_pfn(pages[i]),as->pte_attr);
*pte = SMMU_PFN_TO_PTE(page_to_pfn(pages[i]), attrs); } pte = &ptbl[ptn];@@ -1010,6 +1023,10 @@ static int smmu_iommu_map_sg(struct iommu_domain *domain, unsigned long iova, bool flush_all = (nents * PAGE_SIZE > SZ_512) ? true : false; struct smmu_as *as = domain->priv; struct smmu_device *smmu = as->smmu;
int attrs = as->pte_attr;if (dma_get_attr(DMA_ATTR_READ_ONLY, (struct dma_attrs *)prot))attrs &= ~_WRITABLE; for (count = 0, s = sgl; count < nents; s = sg_next(s)) { phys_addr_t phys = page_to_phys(sg_page(s));@@ -1053,7 +1070,7 @@ static int smmu_iommu_map_sg(struct iommu_domain *domain, unsigned long iova, (*rest)++; }
*pte = SMMU_PFN_TO_PTE(pfn + i, as->pte_attr);
*pte = SMMU_PFN_TO_PTE(pfn + i, attrs); } pte = &ptbl[ptn];@@ -1191,7 +1208,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain, struct page *page;
page = as->smmu->avp_vector_page;
__smmu_iommu_map_pfn(as, 0, page_to_pfn(page));
__smmu_iommu_map_pfn(as, 0, page_to_pfn(page), 0); pr_debug("Reserve \"page zero\" \ for AVP vectors using a common dummy\n");-- 1.8.1.5
Linaro-mm-sig mailing list Linaro-mm-sig@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
Hi Kyungmin,
Kyungmin Park kmpark@infradead.org wrote @ Thu, 20 Jun 2013 08:50:14 +0200:
On Thu, Jun 20, 2013 at 2:49 PM, Hiroshi Doyu hdoyu@nvidia.com wrote:
Support read-only mapping via struct dma_attrs.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
drivers/iommu/tegra-smmu.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index fab1f19..3aff4cd 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -862,12 +862,13 @@ static size_t __smmu_iommu_unmap_largepage(struct smmu_as *as, dma_addr_t iova) }
static int __smmu_iommu_map_pfn(struct smmu_as *as, dma_addr_t iova,
unsigned long pfn)
unsigned long pfn, int prot)Can you find 'prot' is used at other arch? In previous patch, you cast
{amd,intel}_iommu_map() take the IOMMU standard flags below and translate it into each H/W dependent bits.
#define IOMMU_READ (1) #define IOMMU_WRITE (2) #define IOMMU_CACHE (4) /* DMA cache coherency */
The others like OMAP/MSM pass their H/W dependent bits all the way. I think that they don't use IOMMU via DMA mapping API.
it as 'int' but below code cast it as 'struct dma_attr' again. doesn't it better to use 'struct dma_attr' as parameter to avoid double cast? Of course you have to modify existing APIs to use 'struct dma_attr'.
If DMA mapping API is considered as the standard frontend of IOMMU(API), that may be an option.
Hi Hiroshi,
On Thu, Jun 20, 2013 at 06:49:41AM +0100, Hiroshi Doyu wrote:
In Tegra SoC, IOMMU can set some mapping attribute for each page, for exmaple, READable, and WRITEable. We'd like to use this feature *newly* for robustness, where read-only pages can be protected from being overwritten by wrong operation. We have been using IOMMU via DMA mapping API(ARM). DMA mapping API currently doesn't use "prot" parameter when calling IOMMU API. So this series tries to pass "struct dma_attrs *attrs" via "int prot" in IOMMU API. I'm not so sure if this implementation is right or not because:
- Casting (struct dma_attrs *) to (int) in DMA API doesn't look nice.
- IOMMU layer needs to cast (int) back to (struct dma_attrs *) again, which can be considered as violation of layers.
If you have any implementations/suggestions, it would be really helpful.
This series isn't applied cleanly but this is posted to request for comments.
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
These patches don't seem to have made it to the linux-arm-kernel list (either in my inbox or the list archive). Could you try and resend them please?
Also: how do they interact with my patch here?:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175124.html
Will
Hi Will,
Will Deacon will.deacon@arm.com wrote @ Thu, 20 Jun 2013 11:31:52 +0200:
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
These patches don't seem to have made it to the linux-arm-kernel list (either in my inbox or the list archive). Could you try and resend them please?
I'm sorry but linux-arm-kernel list someitmes requires me moderators approval. I don't know why. I'll find a way to work around.
Here are links for the original post:
http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005860.html http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005861.html http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005862.html http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005863.html
Also: how do they interact with my patch here?:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175124.html
This conversion looks reasonable enough, but some platform may need more attrs to pass IOMMU like OMAP.
#define IOMMU_FLAG (IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_8)
How can we deal with those cases?
On Thu, Jun 20, 2013 at 10:59:29AM +0100, Hiroshi Doyu wrote:
Hi Will,
Will Deacon will.deacon@arm.com wrote @ Thu, 20 Jun 2013 11:31:52 +0200:
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
These patches don't seem to have made it to the linux-arm-kernel list (either in my inbox or the list archive). Could you try and resend them please?
I'm sorry but linux-arm-kernel list someitmes requires me moderators approval. I don't know why. I'll find a way to work around.
I guess it doesn't like the threading. It works better if you have "[PATCH" in the subject.
Catalin Marinas catalin.marinas@arm.com wrote @ Thu, 20 Jun 2013 12:11:10 +0200:
On Thu, Jun 20, 2013 at 10:59:29AM +0100, Hiroshi Doyu wrote:
Hi Will,
Will Deacon will.deacon@arm.com wrote @ Thu, 20 Jun 2013 11:31:52 +0200:
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
These patches don't seem to have made it to the linux-arm-kernel list (either in my inbox or the list archive). Could you try and resend them please?
I'm sorry but linux-arm-kernel list someitmes requires me moderators approval. I don't know why. I'll find a way to work around.
I guess it doesn't like the threading. It works better if you have "[PATCH" in the subject.
It seems to work. I did with "[PATCH" and --no-thread. Thanks.
On Thu, Jun 20, 2013 at 10:59:29AM +0100, Hiroshi Doyu wrote:
Hi Will,
Hi Hiroshi,
Will Deacon will.deacon@arm.com wrote @ Thu, 20 Jun 2013 11:31:52 +0200:
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
These patches don't seem to have made it to the linux-arm-kernel list (either in my inbox or the list archive). Could you try and resend them please?
I'm sorry but linux-arm-kernel list someitmes requires me moderators approval. I don't know why. I'll find a way to work around.
Strange. It would definitely be good to get that fixed!
Here are links for the original post:
http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005860.html http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005861.html http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005862.html http://lists.linuxfoundation.org/pipermail/iommu/2013-June/005863.html
Thanks, but I can't reply inline to those :)
Also: how do they interact with my patch here?:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175124.html
This conversion looks reasonable enough, but some platform may need more attrs to pass IOMMU like OMAP.
#define IOMMU_FLAG (IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_8)
How can we deal with those cases?
Well, if we have weird and wonderful flags like this, which are IOMMU-specific, then I'd suggest short-circuiting the flags altogether and putting them inside somewhere like dev_archdata.iommu.
Will
Hello,
On 6/20/2013 7:49 AM, Hiroshi Doyu wrote:
In Tegra SoC, IOMMU can set some mapping attribute for each page, for exmaple, READable, and WRITEable. We'd like to use this feature *newly* for robustness, where read-only pages can be protected from being overwritten by wrong operation. We have been using IOMMU via DMA mapping API(ARM). DMA mapping API currently doesn't use "prot" parameter when calling IOMMU API. So this series tries to pass "struct dma_attrs *attrs" via "int prot" in IOMMU API. I'm not so sure if this implementation is right or not because:
- Casting (struct dma_attrs *) to (int) in DMA API doesn't look nice.
- IOMMU layer needs to cast (int) back to (struct dma_attrs *) again, which can be considered as violation of layers.
If you have any implementations/suggestions, it would be really helpful.
This series isn't applied cleanly but this is posted to request for comments.
Using DMA attributes for this seems to be a bad idea. The dma direction parameter is much more appropriate. Will Deacon recently posted a patch which does it right, see:
https://git.linaro.org/gitweb?p=people/mszyprowski/linux-dma-mapping.git%3Ba...
Hiroshi Doyu (3): common: DMA-mapping: add DMA_ATTR_READ_ONLY attribute ARM: dma-mapping: Pass DMA attrs as IOMMU prot iommu/tegra: smmu: Support read-only mapping
arch/arm/mm/dma-mapping.c | 34 +++++++++++++++++++++------------- drivers/iommu/tegra-smmu.c | 41 +++++++++++++++++++++++++++++------------ include/linux/dma-attrs.h | 1 + 3 files changed, 51 insertions(+), 25 deletions(-)
Best regards
On Fri, Jun 21, 2013 at 09:17:59AM +0200, Marek Szyprowski wrote:
Using DMA attributes for this seems to be a bad idea. The dma direction parameter is much more appropriate. Will Deacon recently posted a patch which does it right, see:
https://git.linaro.org/gitweb?p=people/mszyprowski/linux-dma-mapping.git%3Ba...
Agreed, that is one usecase the dma-direction parameter was made for. In particular:
DMA_FROM_DEVICE -> Write only mapping DMA_TO_DEVICE -> Read only mapping DMA_BIDIRECTIONAL -> Read/Write mapping
So no need to use the dma attributes for that.
Joerg
On Fri, 21 Jun 2013 18:03:44 +0200 Joerg Roedel joro@8bytes.org wrote:
On Fri, Jun 21, 2013 at 09:17:59AM +0200, Marek Szyprowski wrote:
Using DMA attributes for this seems to be a bad idea. The dma direction parameter is much more appropriate. Will Deacon recently posted a patch which does it right, see:
https://git.linaro.org/gitweb?p=people/mszyprowski/linux-dma-mapping.git%3Ba...
Agreed, that is one usecase the dma-direction parameter was made for. In particular:
DMA_FROM_DEVICE -> Write only mapping DMA_TO_DEVICE -> Read only mapping DMA_BIDIRECTIONAL -> Read/Write mapping
So no need to use the dma attributes for that.
Ok, thanks. One more question, IOMMU H/W sometimes supports more platform specific attributes than READ/WRITE. For example, in OMAP,
#define IOMMU_FLAG (IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_8)
Is there any way to deal with those platform specific attrs from DMA mapping API POV?
On Mon, Jun 24, 2013 at 08:17:59AM +0300, Hiroshi Doyu wrote:
Ok, thanks. One more question, IOMMU H/W sometimes supports more platform specific attributes than READ/WRITE. For example, in OMAP,
#define IOMMU_FLAG (IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_8)
Is there any way to deal with those platform specific attrs from DMA mapping API POV?
Depends on the kind of flag and whether you want to make it changeable from the DMA-API. The AMD IOMMU for example has a flag in the page-tables to force PCI DMA coherency. This is always set by the driver.
For other parameters that should be changeable and don't fit into the dma_direction parameter in some way the use of dma_attr would make sense.
Joerg
linaro-mm-sig@lists.linaro.org
-
Arnd Bergmann -
Catalin Marinas -
Hiroshi Doyu -
Joerg Roedel -
Kyungmin Park -
Marek Szyprowski -
Nishanth Peethambaran -
Will Deacon