[RFC 0/3] How to pass IOMMU map attr via DMA API?

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Hiroshi Doyu]

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(-)

-- 
1.8.1.5

[Linaro-mm-sig] [RFC 3/3] iommu/tegra: smmu: Support read-only mapping

[Kyungmin Park]

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 at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

[Linaro-mm-sig] [RFC 3/3] iommu/tegra: smmu: Support read-only mapping

[Hiroshi Doyu]

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.

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Nishanth Peethambaran]

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 at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Hiroshi Doyu]

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;

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Nishanth Peethambaran]

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;

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Hiroshi Doyu]

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?

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Arnd Bergmann]

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

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Hiroshi Doyu]

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?

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Arnd Bergmann]

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

[Linaro-mm-sig] [RFC 2/3] ARM: dma-mapping: Pass DMA attrs as IOMMU prot

[Nishanth Peethambaran]

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

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Will Deacon]

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

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Hiroshi Doyu]

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?

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Catalin Marinas]

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

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Hiroshi Doyu]

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.

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Will Deacon]

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

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Marek Szyprowski]

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;a=commit;h=8fc3749bd31d139db58f874e093255fe62505968


> 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
-- 
Marek Szyprowski
Samsung R&D Institute Poland

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Joerg Roedel]

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;a=commit;h=8fc3749bd31d139db58f874e093255fe62505968

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

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Hiroshi Doyu]

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;a=commit;h=8fc3749bd31d139db58f874e093255fe62505968
> 
> 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?

[RFC 0/3] How to pass IOMMU map attr via DMA API?

[Joerg Roedel]

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