Re: [RFC 0/5] OMAP groundwork for IOMMU-based DMA API

2011-12-05 Thread Roedel, Joerg 
On Mon, Dec 05, 2011 at 04:21:21PM +0200, Ohad Ben-Cohen wrote:
 Hi Joerg,
 
 On Tue, Nov 15, 2011 at 12:39 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  On Sun, Sep 25, 2011 at 06:58:52AM -0400, Ohad Ben-Cohen wrote:
  Ohad Ben-Cohen (5):
    ARM: dev_archdata: add private iommu extension
    ARM: OMAP: omap_device: add a method to set iommu private archdata
    ARM: OMAP: iommu: declare a private iommu binding struct
    ARM: OMAP3: bind omap3isp_device to its iommu device
    iommu/omap: eliminate the public omap_find_iommu_device() method
 
  It doesn't apply cleanly, can you please rebase the code, collect the
  Acked-by's and resend?
 
 I have collected Tony's and Laurent's Acked-by's and amended them to
 the commit logs.
 
 I have also made sure it applies cleanly to a recent 3.2, and tested
 it with OMAP4/remoteproc. I couldn't test it with omap3isp though,
 because I'm gated by (what looks like a) 3.2 omap3isp regression (see
 http://www.spinics.net/lists/linux-omap/msg60510.html). I did test it
 of course with omap3isp and 3.1, so I don't expect any omap3isp/iommu
 issues with these patches. Anyway, when the omap3isp issue will be
 resolved I'll make sure there isn't any iommu regression.
 
 Please pull, thanks a lot!
 
 The following changes since commit cfcfc9eca2bcbd26a8e206baeb005b055dbf8e37:
 
   Linux 3.2-rc2 (2011-11-15 15:02:59 -0200)
 
 are available in the git repository at:
   git://git.kernel.org/pub/scm/linux/kernel/git/ohad/linux.git for-joerg
 
 Ohad Ben-Cohen (3):
   ARM: OMAP: iommu: declare a private iommu binding struct
   ARM: OMAP3: bind omap3isp_device to its iommu device
   iommu/omap: eliminate the public omap_find_iommu_device() method
 
  arch/arm/mach-omap2/devices.c   |7 
  arch/arm/plat-omap/include/plat/iommu.h |   31 +++--
  arch/arm/plat-omap/include/plat/iovmm.h |   12 +++---
  drivers/iommu/omap-iommu.c  |   58 
 ++-
  drivers/iommu/omap-iovmm.c  |   31 +++--
  drivers/media/video/omap3isp/isp.c  |   30 +++-
  drivers/media/video/omap3isp/isp.h  |2 -
  drivers/media/video/omap3isp/ispccdc.c  |   18 +-
  drivers/media/video/omap3isp/ispstat.c  |8 ++--
  drivers/media/video/omap3isp/ispvideo.c |4 +-
  10 files changed, 107 insertions(+), 94 deletions(-)

Pulled into arm/omap, thanks Ohad.

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/5] OMAP groundwork for IOMMU-based DMA API

2011-11-15 Thread Roedel, Joerg 
Hi Ohad,

On Sun, Sep 25, 2011 at 06:58:52AM -0400, Ohad Ben-Cohen wrote:
 Ohad Ben-Cohen (5):
   ARM: dev_archdata: add private iommu extension
   ARM: OMAP: omap_device: add a method to set iommu private archdata
   ARM: OMAP: iommu: declare a private iommu binding struct
   ARM: OMAP3: bind omap3isp_device to its iommu device
   iommu/omap: eliminate the public omap_find_iommu_device() method

It doesn't apply cleanly, can you please rebase the code, collect the
Acked-by's and resend?

Thanks,

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/5] OMAP groundwork for IOMMU-based DMA API

2011-11-15 Thread Roedel, Joerg 
On Tue, Nov 15, 2011 at 12:45:33PM +0200, Ohad Ben-Cohen wrote:
 Hi Joerg,
 
 On Tue, Nov 15, 2011 at 12:39 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  It doesn't apply cleanly, can you please rebase the code, collect the
  Acked-by's and resend?
 
 Sure. I planned on resending after the hardware pgsize patches are
 merged, but please tell me if you prefer it the other way around.

It is almost merged :) I just had to struggle with some compile errors
from upstream in my tests. But I think I will finish testing today and
push everything out.

Thanks,

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-10-11 Thread Roedel, Joerg 
Hi Ohad,

On Mon, Oct 10, 2011 at 01:02:46PM -0400, Ohad Ben-Cohen wrote:
 On Mon, Oct 10, 2011 at 5:36 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  Yes, somthing like that. Probably the iommu_ops-unmap function should
  be turned into a unmap_page function call which only takes an iova and
  no size parameter. The iommu-driver unmaps the page pointing to that
  iova and returns the size of the page unmapped. This still allows the
  simple implementation for the unmap-call.
 
 Yes, exactly. It will take some time to migrate all drivers (today we
 have 4 drivers, each of which is implementing a slightly different
 -unmap() semantics), but at least let's not accept any new driver
 that doesn't adhere to this, otherwise it's going to be even harder
 for the API to evolve.

Agreed. We should change the existing drivers one-by-one.

  We don't need to really enforce the calls to be symetric. But we can
  define that we only give the guarantee about what will be unmapped
  when the calls are symetric.
 
 Sounds good to me. I'll add this to the kernel doc patch (which I'll
 submit after this patch set materializes), and when/if we move to
 symmetric only, we will update it.

Great. So if no one disagrees with this direction (this means all the
other people on Cc :) ) it is the way to.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-10-11 Thread Roedel, Joerg 
On Mon, Oct 10, 2011 at 06:01:06PM -0400, Ohad Ben-Cohen wrote:
  On Mon, Oct 10, 2011 at 5:36 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  The master branch is best to base your patches on for generic work.
 
 It looks like the master branch is missing something like this:
 
 From acb316aa4bcaf383e8cb1580e30c8635e0a34369 Mon Sep 17 00:00:00 2001
 From: Ohad Ben-Cohen o...@wizery.com
 Date: Mon, 10 Oct 2011 23:55:51 +0200
 Subject: [PATCH] iommu/core: fix build issue
 
 Fix this:
 
 drivers/iommu/iommu.c: In function 'iommu_commit':
 drivers/iommu/iommu.c:291: error: 'iommu_ops' undeclared (first use in
 this function)
 
 Signed-off-by: Ohad Ben-Cohen o...@wizery.com
 ---
  drivers/iommu/iommu.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
 index 909b0d2..a5131f1 100644
 --- a/drivers/iommu/iommu.c
 +++ b/drivers/iommu/iommu.c
 @@ -288,7 +288,7 @@ EXPORT_SYMBOL_GPL(iommu_unmap);
 
  void iommu_commit(struct iommu_domain *domain)
  {
 - if (iommu_ops-commit)
 - iommu_ops-commit(domain);
 + if (domain-ops-commit)
 + domain-ops-commit(domain);
  }
  EXPORT_SYMBOL_GPL(iommu_commit);
 -- 
 1.7.4.1

Whoops. This iommu_commit branch wasn't meant to be in master already.
I'll try to fix that.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-10-11 Thread Roedel, Joerg 
On Mon, Oct 10, 2011 at 06:49:32PM -0400, Ohad Ben-Cohen wrote:
 -int iommu_unmap(struct iommu_domain *domain, unsigned long iova, int 
 gfp_order)
 +int iommu_unmap(struct iommu_domain *domain, unsigned long iova, int size)
  {
 -   size_t size;
 +   int order, unmapped_size, unmapped_order, total_unmapped = 0;
 
 if (unlikely(domain-ops-unmap == NULL))
 return -ENODEV;
 
 -   size = PAGE_SIZE  gfp_order;
 +   /*
 +* The virtual address, as well as the size of the mapping, must be
 +* aligned (at least) to the size of the smallest page supported
 +* by the hardware
 +*/
 +   if (!IS_ALIGNED(iova | size, domain-ops-min_pagesz)) {
 +   pr_err(unaligned: iova 0x%lx size 0x%x min_pagesz 0x%x\n,
 +   iova, size, domain-ops-min_pagesz);
 +   return -EINVAL;
 +   }
 +
 +   pr_debug(unmap this: iova 0x%lx size 0x%x\n, iova, size);
 +
 +   while (size  total_unmapped) {
 +   order = get_order(size - total_unmapped);

Hmm, this actually unmaps more than requested, even in the symetric
case. Imgine the user wants to unmap a 12kb region, then order
will be 4 and this code will tell the iommu-driver to unmap 16kb.

You need to make sure that you don;t pass larger regions then requested
to the low-level driver. Some logic like in the iommu_map function
should do it.

 +
 +   unmapped_order = domain-ops-unmap(domain, iova, order);
 +   if (unmapped_order  0)
 +   break;
 +
 +   pr_debug(unmapped: iova 0x%lx order %d\n, iova,
 +   unmapped_order);
 +
 +   unmapped_size = 0x1000UL  unmapped_order;

Please use PAGE_SIZE instead of 0x1000UL.

 
 -   BUG_ON(!IS_ALIGNED(iova, size));
 +   iova += unmapped_size;
 +   total_unmapped += unmapped_size;
 +   }
 
 -   return domain-ops-unmap(domain, iova, gfp_order);
 +   return total_unmapped;
  }
  EXPORT_SYMBOL_GPL(iommu_unmap);
 
 diff --git a/drivers/iommu/omap-iovmm.c b/drivers/iommu/omap-iovmm.c
 index e8fdb88..4a8f761 100644
 --- a/drivers/iommu/omap-iovmm.c
 +++ b/drivers/iommu/omap-iovmm.c
 @@ -409,7 +409,6 @@ static int map_iovm_area(struct iommu_domain
 *domain, struct iovm_struct *new,
 unsigned int i, j;
 struct scatterlist *sg;
 u32 da = new-da_start;
 -   int order;
 
 if (!domain || !sgt)
 return -EINVAL;
 @@ -428,12 +427,10 @@ static int map_iovm_area(struct iommu_domain
 *domain, struct iovm_struct *new,
 if (bytes_to_iopgsz(bytes)  0)
 goto err_out;
 
 -   order = get_order(bytes);
 -
 pr_debug(%s: [%d] %08x %08x(%x)\n, __func__,
  i, da, pa, bytes);
 
 -   err = iommu_map(domain, da, pa, order, flags);
 +   err = iommu_map(domain, da, pa, bytes, flags);
 if (err)
 goto err_out;
 
 @@ -448,10 +445,9 @@ err_out:
 size_t bytes;
 
 bytes = sg-length + sg-offset;
 -   order = get_order(bytes);
 
 /* ignore failures.. we're already handling one */
 -   iommu_unmap(domain, da, order);
 +   iommu_unmap(domain, da, bytes);
 
 da += bytes;
 }
 @@ -474,13 +470,11 @@ static void unmap_iovm_area(struct iommu_domain
 *domain, struct omap_iommu *obj,
 start = area-da_start;
 for_each_sg(sgt-sgl, sg, sgt-nents, i) {
 size_t bytes;
 -   int order;
 
 bytes = sg-length + sg-offset;
 -   order = get_order(bytes);
 
 -   err = iommu_unmap(domain, start, order);
 -   if (err  0)
 +   err = iommu_unmap(domain, start, bytes);
 +   if (err  bytes)
 break;
 
 dev_dbg(obj-dev, %s: unmap %08x(%x) %08x\n,
 diff --git a/include/linux/iommu.h b/include/linux/iommu.h
 index 710291f..a10a86c 100644
 --- a/include/linux/iommu.h
 +++ b/include/linux/iommu.h
 @@ -48,6 +48,22 @@ struct iommu_domain {
 
  #ifdef CONFIG_IOMMU_API
 
 +/**
 + * struct iommu_ops - iommu ops and capabilities
 + * @domain_init: init iommu domain
 + * @domain_destroy: destroy iommu domain
 + * @attach_dev: attach device to an iommu domain
 + * @detach_dev: detach device from an iommu domain
 + * @map: map a physically contiguous memory region to an iommu domain
 + * @unmap: unmap a physically contiguous memory region from an iommu domain
 + * @iova_to_phys: translate iova to physical address
 + * @domain_has_cap: domain capabilities query
 + * @commit: commit iommu domain
 + * @pgsize_bitmap: bitmap of supported page sizes
 + * @min_pagesz: smallest page size supported. note: this member is private
 + * to the IOMMU core, and

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-10-10 Thread Roedel, Joerg 
Hi Ohad,

sorry, I was on vacation last week and had no time to look into this.

On Sun, Oct 02, 2011 at 11:58:12AM -0400, Ohad Ben-Cohen wrote:
  drivers/iommu/iommu.c  |  138 ---
  drivers/iommu/omap-iovmm.c |   12 +---
  include/linux/iommu.h  |6 +-
  virt/kvm/iommu.c   |4 +-
  4 files changed, 137 insertions(+), 23 deletions(-)
 
 diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
 index a7b0862..f23563f 100644
 --- a/drivers/iommu/iommu.c
 +++ b/drivers/iommu/iommu.c
 @@ -16,6 +16,8 @@
   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
   */
 
 +#define pr_fmt(fmt)%s:  fmt, __func__
 +
  #include linux/kernel.h
  #include linux/bug.h
  #include linux/types.h
 @@ -23,15 +25,54 @@
  #include linux/slab.h
  #include linux/errno.h
  #include linux/iommu.h
 +#include linux/bitmap.h

Is this still required?

 
  static struct iommu_ops *iommu_ops;
 
 +/* bitmap of supported page sizes */
 +static unsigned long iommu_pgsize_bitmap;
 +
 +/* size of the smallest supported page (in bytes) */
 +static unsigned int iommu_min_pagesz;
 +
 +/**
 + * register_iommu() - register an IOMMU hardware
 + * @ops: iommu handlers
 + * @pgsize_bitmap: bitmap of page sizes supported by the hardware
 + *
 + * Note: this is a temporary function, which will be removed once
 + * all IOMMU drivers are converted. The only reason it exists is to
 + * allow splitting the pgsizes changes to several patches in order to ease
 + * the review.
 + */
 +void register_iommu_pgsize(struct iommu_ops *ops, unsigned long 
 pgsize_bitmap)
 +{
 +   if (iommu_ops || iommu_pgsize_bitmap || !pgsize_bitmap)
 +   BUG();
 +
 +   iommu_ops = ops;
 +   iommu_pgsize_bitmap = pgsize_bitmap;
 +
 +   /* find out the minimum page size only once */
 +   iommu_min_pagesz = 1  __ffs(pgsize_bitmap);
 +}

Hmm, I thought a little bit about that and came to the conculusion it
might be best to just keep the page-sizes as a part of the iommu_ops
structure. So there is no need to extend the register_iommu interface.

Also, the bus_set_iommu interface is now in the -next branch. Would be
good if you rebase the patches to that interface.

You can find the current iommu tree with these changes at

git://git.8bytes.org/scm/iommu.git

 @@ -115,26 +156,103 @@ 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, int gfp_order, int prot)
 + phys_addr_t paddr, size_t size, int prot)
  {
 -   size_t size;
 +   int ret = 0;
 +
 +   /*
 +* both the virtual address and the physical one, as well as
 +* the size of the mapping, must be aligned (at least) to the
 +* size of the smallest page supported by the hardware
 +*/
 +   if (!IS_ALIGNED(iova | paddr | size, iommu_min_pagesz)) {
 +   pr_err(unaligned: iova 0x%lx pa 0x%lx size 0x%lx min_pagesz 
 +   0x%x\n, iova, (unsigned long)paddr,
 +   (unsigned long)size, iommu_min_pagesz);
 +   return -EINVAL;
 +   }
 
 -   size = 0x1000UL  gfp_order;
 +   pr_debug(map: iova 0x%lx pa 0x%lx size 0x%lx\n, iova,
 +   (unsigned long)paddr, (unsigned long)size);
 
 -   BUG_ON(!IS_ALIGNED(iova | paddr, size));
 +   while (size) {
 +   unsigned long pgsize, addr_merge = iova | paddr;
 +   unsigned int pgsize_idx;
 
 -   return iommu_ops-map(domain, iova, paddr, gfp_order, prot);
 +   /* Max page size that still fits into 'size' */
 +   pgsize_idx = __fls(size);
 +
 +   /* need to consider alignment requirements ? */
 +   if (likely(addr_merge)) {
 +   /* Max page size allowed by both iova and paddr */
 +   unsigned int align_pgsize_idx = __ffs(addr_merge);
 +
 +   pgsize_idx = min(pgsize_idx, align_pgsize_idx);
 +   }
 +
 +   /* build a mask of acceptable page sizes */
 +   pgsize = (1UL  (pgsize_idx + 1)) - 1;
 +
 +   /* throw away page sizes not supported by the hardware */
 +   pgsize = iommu_pgsize_bitmap;

I think we need some care here and check pgsize for 0. A BUG_ON should
do.

 +
 +   /* pick the biggest page */
 +   pgsize_idx = __fls(pgsize);
 +   pgsize = 1UL  pgsize_idx;
 +
 +   /* convert index to page order */
 +   pgsize_idx -= PAGE_SHIFT;
 +
 +   pr_debug(mapping: iova 0x%lx pa 0x%lx order %u\n, iova,
 +   (unsigned long)paddr, pgsize_idx);
 +
 +   ret = iommu_ops-map(domain, iova, paddr, pgsize_idx, prot);
 +   if (ret)
 +   break;

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-10-10 Thread Roedel, Joerg 
Hi Ohad,

On Mon, Oct 10, 2011 at 09:59:22AM -0400, Ohad Ben-Cohen wrote:
  Also, the bus_set_iommu interface is now in the -next branch. Would be
  good if you rebase the patches to that interface.
 
 Sure. It's a little tricky though: which branch do I base this on ?
 Are you ok with me basing this on your 'next' branch ? My current
 stack depends at least on three branches of yours, so that would be
 helpful for me (and less merging conflicts for you I guess :).

The master branch is best to base your patches on for generic work. For
more specific things like omap-only changes you can use the topic
branches.

  I think we need some care here and check pgsize for 0. A BUG_ON should
  do.
 
 I can add it if you prefer, but I don't think it can really happen:
 basically, it means that we chose a too small and unsupported page
 bit, which can't happen as long as we check for IS_ALIGNED(iova |
 paddr | size, iommu_min_pagesz) in the beginning of iommu_map.

It can happen when there is a bug somewhere :) So a BUG_ON will yell
then and makes debugging easier. An alternative is to use a WARN_ON and
let the map-call fail in this case.

 Ok, let's discuss the semantics of -unmap().
 
 There isn't a clear documentation of that API (we should probably add
 some kernel docs after we nail it down now), but judging from the
 existing users (mainly kvm) and drivers, it seems that iommu_map() and
 iommu_unmap() aren't symmetric: users rely on unmap() to return the
 actual size that was unmapped. IOMMU drivers, in turn, should check
 which page is mapped on 'iova', unmap it, and return its size.

Right, currently the map/unmap calls are not symetric. But I think they
should be to get a clean semantic. Without this requirement and multiple
page-sizes in use the iommu-code may has to unmap more address space then
requested. The user doesn't know what will be unmapped so it has to make
sure that no DMA is happening while unmap runs.

When we require the calls to be symetric we can give a guarantee that
only the requested region is unmapped and allow DMA to the untouched
part of the address-space while unmap() is running.

So when the call-places to not follow this restriction we should convert
them mid-term.

 This way iommu_unmap() becomes very simple: it just iterates through
 the region, relying on iommu_ops-unmap() to return the sizes that
 were actually unmapped (very similar to how amd's iommu_unmap_page
 works today). This also means that iommu_ops-unmap() doesn't really
 need a size/order argument and we can remove it (after all drivers
 fully migrate..).

Yes, somthing like that. Probably the iommu_ops-unmap function should
be turned into a unmap_page function call which only takes an iova and
no size parameter. The iommu-driver unmaps the page pointing to that
iova and returns the size of the page unmapped. This still allows the
simple implementation for the unmap-call.

This change is no requirement for this patch-set, but if we agree on it
this patch-set should keep that direction in mind.

 The other approach which you suggest means symmetric iommu_map() and
 iommu_unmap(). It means adding a 'paddr' parameter to iommu_unmap(),
 which is easy, but maybe more concerning is the limitation that it
 incurs: users will now have to call iommu_unmap() exactly as they
 called iommu_map() beforehand. Note sure how well this will fly with
 the existing users (kvm ?) and whether we really want to enforce this
 (it doesn't mean drivers need to deal with page-size complexity. they
 are required to unmap a single page at a time, and iommu_unmap() will
 do the work for them).

It will work with KVM, that is no problem. We don't need to really
enforce the calls to be symetric. But we can define that we only give
the guarantee about what will be unmapped when the calls are symetric.
For example:

iommu_map(  0, 0x10);
iommu_unmap(0, 0x10); /* Guarantee that it will only unmap
 the range 0-0x10 */

whereas:

iommu_map(  0, 0x10);
iommu_unmap(0,   0x1000); /* Guarantees that 0-0x1000 is
 unmapped, but other undefined parts
 of the address space may be
 unmapped too, up to the whole
 address space */

The alternative is that we implement page-splitting in the iommu_unmap
function. But that introduces complexity I am not sure we really need.
KVM for example just unmaps the whole address-space on destruction. For
the generic dma_ops this is also not required because the dma_map*
functions already have the requirement to be symetric.

 
 Another discussion:
 
 I think we better change iommu_ops-map() to directly take a 'size'
 (in bytes) instead of an 'order' (of pages). Most (all?) drivers just
 immediately do 'size = 0x1000UL  gfp_order', so this whole size -
 order - size back and forth seems redundant.

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-09-27 Thread Roedel, Joerg 
On Fri, Sep 16, 2011 at 01:51:41PM -0400, Ohad Ben-Cohen wrote:
  drivers/iommu/iommu.c  |  158 
 +---
  drivers/iommu/omap-iovmm.c |   12 +---
  include/linux/iommu.h  |6 +-
  virt/kvm/iommu.c   |4 +-
  4 files changed, 157 insertions(+), 23 deletions(-)
 
 diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
 index c68ff29..7c01c8c 100644
 --- a/drivers/iommu/iommu.c
 +++ b/drivers/iommu/iommu.c
 @@ -16,6 +16,8 @@
   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
   */
 
 +#define pr_fmt(fmt)%s:  fmt, __func__
 +
  #include linux/kernel.h
  #include linux/bug.h
  #include linux/types.h
 @@ -23,15 +25,73 @@
  #include linux/slab.h
  #include linux/errno.h
  #include linux/iommu.h
 +#include linux/bitmap.h
 
  static struct iommu_ops *iommu_ops;
 
 +/* bitmap of supported page sizes */
 +static unsigned long *iommu_pgsize_bitmap;
 +
 +/* number of bits used to represent the supported pages */
 +static unsigned int iommu_nr_page_bits;
 +
 +/* size of the smallest supported page (in bytes) */
 +static unsigned int iommu_min_pagesz;
 +
 +/* bit number of the smallest supported page */
 +static unsigned int iommu_min_page_idx;
 +
 +/**
 + * register_iommu() - register an IOMMU hardware
 + * @ops: iommu handlers
 + * @pgsize_bitmap: bitmap of page sizes supported by the hardware
 + * @nr_page_bits: size of @pgsize_bitmap (in bits)
 + *
 + * Note: this is a temporary function, which will be removed once
 + * all IOMMU drivers are converted. The only reason it exists is to
 + * allow splitting the pgsizes changes to several patches in order to ease
 + * the review.
 + */
 +void register_iommu_pgsize(struct iommu_ops *ops, unsigned long 
 *pgsize_bitmap,
 +   unsigned int nr_page_bits)

I think a plain unsigned long value is sufficient to encode the
supported page-sizes. No need to use a full bitmap.

  int iommu_map(struct iommu_domain *domain, unsigned long iova,
 - phys_addr_t paddr, int gfp_order, int prot)
 + phys_addr_t paddr, size_t size, int prot)
  {
 -   size_t size;
 +   int ret = 0;
 +
 +   /*
 +* both the virtual address and the physical one, as well as
 +* the size of the mapping, must be aligned (at least) to the
 +* size of the smallest page supported by the hardware
 +*/
 +   if (!IS_ALIGNED(iova | paddr | size, iommu_min_pagesz)) {
 +   pr_err(unaligned: iova 0x%lx pa 0x%lx size 0x%lx min_pagesz 
 +   0x%x\n, iova, (unsigned long)paddr,
 +   (unsigned long)size, iommu_min_pagesz);
 +   return -EINVAL;
 +   }
 +
 +   pr_debug(map: iova 0x%lx pa 0x%lx size 0x%lx\n, iova,
 +   (unsigned long)paddr, (unsigned long)size);
 +
 +   while (size) {
 +   unsigned long pgsize = iommu_min_pagesz;
 +   unsigned long idx = iommu_min_page_idx;
 +   unsigned long addr_merge = iova | paddr;
 +   int order;
 +
 +   /* find the max page size with which iova, paddr are aligned 
 */
 +   for (;;) {
 +   unsigned long try_pgsize;
 +
 +   idx = find_next_bit(iommu_pgsize_bitmap,
 +   iommu_nr_page_bits, idx + 1);
 +
 +   /* no more pages to check ? */
 +   if (idx = iommu_nr_page_bits)
 +   break;
 +
 +   try_pgsize = 1  idx;
 
 -   size = 0x1000UL  gfp_order;
 +   /* page too big ? addresses not aligned ? */
 +   if (size  try_pgsize ||
 +   !IS_ALIGNED(addr_merge, try_pgsize))
 +   break;
 
 -   BUG_ON(!IS_ALIGNED(iova | paddr, size));
 +   pgsize = try_pgsize;
 +   }

With an unsigned long you can use plain and fast bit_ops instead of the
full bitmap functions. 

 
 -   return iommu_ops-map(domain, iova, paddr, gfp_order, prot);
 +   order = get_order(pgsize);
 +
 +   pr_debug(mapping: iova 0x%lx pa 0x%lx order %d\n, iova,
 +   (unsigned long)paddr, order);
 +
 +   ret = iommu_ops-map(domain, iova, paddr, order, prot);
 +   if (ret)
 +   break;
 +
 +   size -= pgsize;
 +   iova += pgsize;
 +   paddr += pgsize;
 +   }
 +
 +   return ret;
  }
  EXPORT_SYMBOL_GPL(iommu_map);
 

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-09-27 Thread Roedel, Joerg 
On Tue, Sep 27, 2011 at 08:26:29AM -0400, Ohad Ben-Cohen wrote:
  With an unsigned long you can use plain and fast bit_ops instead of the
  full bitmap functions.
 
 Not sure I follow; the only bit operation I'm using while mapping is
 find_next_bit() (which is a bitops.h method).
 
 What other faster variant are you referring to ?

You pass a pointer to an unsigned long for the page-size bitmap. This
allows to use an array of unsigned long. But a single unsigned long is
sufficient and you can use functions like ffs() and fls() together with
shifting. These functions often translate to a single intruction in the
binary. The find_next_bit function has much more overhead because it
needs to handle the array-of-ulong case.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] iommu/core: split mapping to page sizes as supported by the hardware

2011-09-27 Thread Roedel, Joerg 
On Tue, Sep 27, 2011 at 09:28:37AM -0400, Ohad Ben-Cohen wrote:
 So you're suggesting to re-implement find_next_bit() using ffs()/fls()
 and shifting ?

No. I suggest a simpler and shorter algorithm using the bit helpers.
Something like that:

min_idx = __ffs(iommu_page_sizes);

while (size) {
/* Max alignment allowed by current physical address */
phys_idx = __ffs(phys);

/* Max alignment allowed by current size */
size_idx = __fls(size);

/* special case: iova == 0 */
if (likely(phys))
idx = min(phys_idx, size_idx);
else
idx = size_idx;

BUG_ON(idx  min_idx);

psize = 1UL  idx;

/* search next smaller page-size supported */
while (psize  !(iommu_page_sizes  psize))
psize = 1;

BUG_ON(psize == 0);

iommu_ops-map(domain, iova, phys, get_order(psize), prot);

iova += psize;
phys += psize;
size -= psize;
}

It is only C-style pseudo-code, of course. These __ffs and __fls lines
all translate to a single instruction later. The find_next_bit()
function has a lot more overhead because it needs to take account of
real bitmaps (arrays of ulong). But this complexity is not required
here.

And yes, overhead is important when we implement the generic dma-ops
on-top of the iommu-api because this will make the iommu_map function a
fast-path. So we really care about overhead here.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iommu/omap: Fix build error with !IOMMU_SUPPORT

2011-09-23 Thread Roedel, Joerg 
On Thu, Sep 22, 2011 at 09:48:27AM -0400, Felipe Contreras wrote:
 On Tue, Sep 20, 2011 at 1:54 PM, Laurent Pinchart
 laurent.pinch...@ideasonboard.com wrote:
  On Tuesday 20 September 2011 12:01:46 Roedel, Joerg wrote:
  On Sat, Sep 17, 2011 at 08:02:22PM -0400, Laurent Pinchart wrote:
   On Wednesday 14 September 2011 16:07:39 Joerg Roedel wrote:
Without this patch it is possible to select the VIDEO_OMAP3
driver which then selects OMAP_IOVMM. But the omap iommu
driver is not compiled without IOMMU_SUPPORT enabled. Fix
that by forcing OMAP_IOMMU and OMAP_IOVMM are enabled before
VIDEO_OMAP3 can be selected.
  
   What about making VIDEO_OMAP3 select IOMMU_SUPPORT instead then ? Your
   patch would make the OMAP3 ISP driver disappear from the menu until
   IOMMU_SUPPORT gets turned on, which can confuse users.
 
  Using 'depends on' rather then 'selects' is common standard in Kconfig.
  You can't select PCI drivers without selecting PCI first, for example.
 
  You wouldn't expect a PCI driver to work without PCI support. My concern is
  that most OMAP3 ISP users won't know that IOMMU supports is required. Feel
  free to ignore it though :-)
 
 I agree with Laurent. As a user, why should I care about IOMMU? I just
 want this thing.

Because enabling IOMMU can have other side-effects on the system and as
a user you want to be aware that it is enabled. A user not wanting the
iommu driver can just disable it then without the need to de-select all
drivers depending on it first.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iommu/omap: Fix build error with !IOMMU_SUPPORT

2011-09-20 Thread Roedel, Joerg 
Hi Laurent,

On Sat, Sep 17, 2011 at 08:02:22PM -0400, Laurent Pinchart wrote:
 On Wednesday 14 September 2011 16:07:39 Joerg Roedel wrote:
  Without this patch it is possible to select the VIDEO_OMAP3
  driver which then selects OMAP_IOVMM. But the omap iommu
  driver is not compiled without IOMMU_SUPPORT enabled. Fix
  that by forcing OMAP_IOMMU and OMAP_IOVMM are enabled before
  VIDEO_OMAP3 can be selected.
 
 What about making VIDEO_OMAP3 select IOMMU_SUPPORT instead then ? Your patch 
 would make the OMAP3 ISP driver disappear from the menu until IOMMU_SUPPORT 
 gets turned on, which can confuse users.

Using 'depends on' rather then 'selects' is common standard in Kconfig.
You can't select PCI drivers without selecting PCI first, for example.
Further selecting whole drivers implicitly isn't a good idea. This can
grow out of control very quickly.

Regards,

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iommu/omap: Fix build error with !IOMMU_SUPPORT

2011-09-15 Thread Roedel, Joerg 
On Wed, Sep 14, 2011 at 02:57:13PM -0400, Ohad Ben-Cohen wrote:
 On Wed, Sep 14, 2011 at 5:07 PM, Joerg Roedel joerg.roe...@amd.com wrote:
  Without this patch it is possible to select the VIDEO_OMAP3
  driver which then selects OMAP_IOVMM. But the omap iommu
  driver is not compiled without IOMMU_SUPPORT enabled. Fix
  that by forcing OMAP_IOMMU and OMAP_IOVMM are enabled before
  VIDEO_OMAP3 can be selected.
 
 I'm ok with this but wouldn't it be simpler if we drop IOMMU_SUPPORT
 altogether (and just use a plain menu instead) ?
 
 Users will then be presented with only a single iommu-related
 question: whether they want their iommu driver built or not.

This doesn't work on platforms that may have more than one possible
iommu driver, like x86 for example.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm: omap: Fix build error in ispccdc.c

2011-09-14 Thread Roedel, Joerg 
Ping?

On Tue, Sep 06, 2011 at 10:02:15AM -0400, Joerg Roedel wrote:
 The following build error occurs with 3.1-rc5:
 
   CC  drivers/media/video/omap3isp/ispccdc.o
 /home/data/repos/linux.trees.git/drivers/media/video/omap3isp/ispccdc.c: In 
 function 'ccdc_lsc_config':
 /home/data/repos/linux.trees.git/drivers/media/video/omap3isp/ispccdc.c:427:2:
  error: implicit declaration of function 'kzalloc' 
 [-Werror=implicit-function-declaration]
 /home/data/repos/linux.trees.git/drivers/media/video/omap3isp/ispccdc.c:427:6:
  warning: assignment makes pointer from integer without a cast [enabled by 
 default]
 
 This patch adds the missing 'linux/slab.h' include to fix the problem.
 
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: linux-me...@vger.kernel.org
 Cc: linux-omap@vger.kernel.org
 Cc: linux-arm-ker...@lists.infradead.org
 Cc: linux-ker...@vger.kernel.org
 Signed-off-by: Joerg Roedel joerg.roe...@amd.com
 ---
  drivers/media/video/omap3isp/ispccdc.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/media/video/omap3isp/ispccdc.c 
 b/drivers/media/video/omap3isp/ispccdc.c
 index 9d3459d..80796eb 100644
 --- a/drivers/media/video/omap3isp/ispccdc.c
 +++ b/drivers/media/video/omap3isp/ispccdc.c
 @@ -31,6 +31,7 @@
  #include linux/dma-mapping.h
  #include linux/mm.h
  #include linux/sched.h
 +#include linux/slab.h
  #include media/v4l2-event.h
  
  #include isp.h
 -- 
 1.7.4.1

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] iommu/omap: migrate to the generic fault report mechanism

2011-09-14 Thread Roedel, Joerg 
On Tue, Sep 13, 2011 at 03:26:29PM -0400, Ohad Ben-Cohen wrote:
 Start using the generic fault report mechanism, as provided by
 the IOMMU core, and remove its now-redundant omap_iommu_set_isr API.
 
 Currently we're only interested in letting upper layers know about the
 fault, so in case the faulting device is a remote processor, they could
 restart it.
 
 Dynamic PTE/TLB loading is not supported.
 
 Signed-off-by: Ohad Ben-Cohen o...@wizery.com
 ---
  arch/arm/plat-omap/include/plat/iommu.h |3 +--
  drivers/iommu/omap-iommu.c  |   31 
 +++
  2 files changed, 4 insertions(+), 30 deletions(-)

Applied both to new branch iommu/fault-reporting and merged into my next
branch. Thanks Ohad.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] iommu/core: add fault reporting mechanism

2011-09-13 Thread Roedel, Joerg 
On Mon, Sep 12, 2011 at 12:21:13PM -0400, Ohad Ben-Cohen wrote:
 On Mon, Sep 12, 2011 at 7:02 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  I still don't get the need for this. It would make sense to encode
  different types of faults, like page-faults or interrupt-faults.
 
 Right.
 
  When I read the comment above it sounds more like you want to encode
  different error-levels, like recoverable and unrecoverable error.
  The exact meaning of these values need to be clarified.
 
 Well, we currently only need to say something bad has happened.
 
 We don't need at this point to tell whether it's a hardware bug,
 inconsistent data, missing page-table entries or whatnot, because we
 don't expect the user (or the iommu core itself) to do anything about
 it. Not that it's not possible though: a valid response one day would
 be to fix the page-table or add a missing TLB (depending on the mode
 the hardware is configured to) but this is not (yet?) implemented. So
 a general unrecoverable error is enough at this point, but it's
 certainly makes sense to allow drivers to provide additional types of
 errors/faults - once they are implemented.

But besides real faults all this can be handled in the iommu-driver
itself, right? So there is no need to communicate other errors than
page-faults up to the driver.

For now I think it is the best to remove this IOMMU_ERROR thing. It is
inherent to the function call already. When a real use-case comes up we
can easily add it later.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] iommu/core: split mapping to page sizes as supported by the hardware

2011-09-13 Thread Roedel, Joerg 
On Wed, Sep 07, 2011 at 02:53:24PM -0400, Ohad Ben-Cohen wrote:

  drivers/iommu/amd_iommu.c   |   20 ++-
  drivers/iommu/intel-iommu.c |   20 ++-
  drivers/iommu/iommu.c   |  129 
 +++
  drivers/iommu/msm_iommu.c   |8 ++-
  drivers/iommu/omap-iommu.c  |6 ++-
  drivers/iommu/omap-iovmm.c  |   12 +---
  include/linux/iommu.h   |7 +-
  virt/kvm/iommu.c|4 +-
  8 files changed, 176 insertions(+), 30 deletions(-)

Please split this patch into the core-change and patches for the
individual iommu-drivers and post this as a seperate patch-set.

 
 diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
 index a14f8dc..5cdfa91 100644
 --- a/drivers/iommu/amd_iommu.c
 +++ b/drivers/iommu/amd_iommu.c
 @@ -2488,12 +2488,30 @@ static unsigned device_dma_ops_init(void)
  }
 
  /*
 + * This bitmap is used to advertise the page sizes our hardware support
 + * to the IOMMU core, which will then use this information to split
 + * physically contiguous memory regions it is mapping into page sizes
 + * that we support.
 + *
 + * Traditionally the IOMMU core just handed us the mappings directly,
 + * after making sure the size is an order of a 4KB page and that the
 + * mapping has natural alignment.
 + *
 + * To retain this behavior, we currently advertise that we support
 + * all page sizes that are an order of 4KB.
 + *
 + * If at some point we'd like to utilize the IOMMU core's new behavior,
 + * we could change this to advertise the real page sizes we support.
 + */
 +static unsigned long amd_iommu_pgsizes = ~0xFFFUL;
 +
 +/*
   * The function which clues the AMD IOMMU driver into dma_ops.
   */
 
  void __init amd_iommu_init_api(void)
  {
 -   register_iommu(amd_iommu_ops);
 +   register_iommu(amd_iommu_ops, amd_iommu_pgsizes, BITS_PER_LONG);
  }
 
  int __init amd_iommu_init_dma_ops(void)
 diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
 index c621c98..a8c91a6 100644
 --- a/drivers/iommu/intel-iommu.c
 +++ b/drivers/iommu/intel-iommu.c
 @@ -3426,6 +3426,24 @@ static struct notifier_block device_nb = {
 .notifier_call = device_notifier,
  };
 
 +/*
 + * This bitmap is used to advertise the page sizes our hardware support
 + * to the IOMMU core, which will then use this information to split
 + * physically contiguous memory regions it is mapping into page sizes
 + * that we support.
 + *
 + * Traditionally the IOMMU core just handed us the mappings directly,
 + * after making sure the size is an order of a 4KB page and that the
 + * mapping has natural alignment.
 + *
 + * To retain this behavior, we currently advertise that we support
 + * all page sizes that are an order of 4KB.
 + *
 + * If at some point we'd like to utilize the IOMMU core's new behavior,
 + * we could change this to advertise the real page sizes we support.
 + */
 +static unsigned long intel_iommu_pgsizes = ~0xFFFUL;

Intel IOMMU does not support arbitrary page-sizes, afaik.

 +
  int __init intel_iommu_init(void)
  {
 int ret = 0;
 @@ -3486,7 +3504,7 @@ int __init intel_iommu_init(void)
 
 init_iommu_pm_ops();
 
 -   register_iommu(intel_iommu_ops);
 +   register_iommu(intel_iommu_ops, intel_iommu_pgsizes, BITS_PER_LONG);
 
 bus_register_notifier(pci_bus_type, device_nb);
 
 diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
 index c68ff29..e07ea03 100644
 --- a/drivers/iommu/iommu.c
 +++ b/drivers/iommu/iommu.c
 @@ -16,6 +16,8 @@
   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
   */
 
 +#define pr_fmt(fmt)%s:  fmt, __func__
 +
  #include linux/kernel.h
  #include linux/bug.h
  #include linux/types.h
 @@ -23,15 +25,41 @@
  #include linux/slab.h
  #include linux/errno.h
  #include linux/iommu.h
 +#include linux/bitmap.h
 
  static struct iommu_ops *iommu_ops;
 
 -void register_iommu(struct iommu_ops *ops)
 +/* bitmap of supported page sizes */
 +static unsigned long *iommu_pgsize_bitmap;
 +
 +/* number of bits used to represent the supported pages */
 +static unsigned int iommu_nr_page_bits;
 +
 +/* size of the smallest supported page (in bytes) */
 +static unsigned int iommu_min_pagesz;
 +
 +/* bit number of the smallest supported page */
 +static unsigned int iommu_min_page_idx;
 +
 +/**
 + * register_iommu() - register an IOMMU hardware
 + * @ops: iommu handlers
 + * @pgsize_bitmap: bitmap of page sizes supported by the hardware
 + * @nr_page_bits: size of @pgsize_bitmap (in bits)
 + */
 +void register_iommu(struct iommu_ops *ops, unsigned long *pgsize_bitmap,
 +   unsigned int nr_page_bits)
  {
 -   if (iommu_ops)
 +   if (iommu_ops || iommu_pgsize_bitmap || !nr_page_bits)
 BUG();
 
 iommu_ops = ops;
 +   iommu_pgsize_bitmap = pgsize_bitmap;
 +   iommu_nr_page_bits = nr_page_bits;
 +
 +   /* find the minimum page size and its index only once */
 +

Re: [PATCH 3/3] iommu/core: split mapping to page sizes as supported by the hardware

2011-09-13 Thread Roedel, Joerg 
On Tue, Sep 13, 2011 at 06:34:23AM -0400, Ohad Ben-Cohen wrote:
 Hi Joerg,
 
 On Tue, Sep 13, 2011 at 1:10 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  Please split this patch into the core-change and patches for the
  individual iommu-drivers and post this as a seperate patch-set.
 
 But we'll be breaking bisectibility this way, no?

Not necessarily. You could implement this side-by-side with the old code
until all drivers are converted and remove the old code then. This keeps
bisectability.

  Intel IOMMU does not support arbitrary page-sizes, afaik.
 
 It does; besides the usual 4K it has super page sizes support of
 2MB, 1GB, 512GB and 1TB.

But the value ~0xfffUL indicates support for 4k, 8k, 16k .. 2^63, no?

 
  +       pr_debug(map: iova 0x%lx pa 0x%lx size 0x%lx\n, iova,
  +                                       (unsigned long)paddr, size);
 
  Please keep the debug-code in a seperate patch in your dev-tree. No need
  for it to be merged upstream.
 
 It's actually useful sometimes to have those around - it's off by
 default, and can be enabled only when needed (CONFIG_DYNAMIC_DEBUG).
 
 But I don't mind removing them.

Ah right, it is just debug, so I am fine keeping it.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] iommu/core: add fault reporting mechanism

2011-09-12 Thread Roedel, Joerg 
On Wed, Sep 07, 2011 at 02:53:22PM -0400, Ohad Ben-Cohen wrote:
  struct device;
 +struct iommu_domain;
 +
 +/**
 + * enum iommu_fault_types - iommu fault types
 + *
 + * @IOMMU_ERROR: Unrecoverable error
 + *
 + * Currently we only support a generic error fault type.
 + * Future users, which will require more informative fault types, will add
 + * them as needed.
 + */
 +enum iommu_fault_types {
 + IOMMU_ERROR,
 +};

I still don't get the need for this. It would make sense to encode
different types of faults, like page-faults or interrupt-faults. That is
what I read from the name of the enum.
When I read the comment above it sounds more like you want to encode
different error-levels, like recoverable and unrecoverable error.
The exact meaning of these values need to be clarified.

 +/**
 + * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
 + * @domain: the iommu domain where the fault has happened
 + * @dev: the device where the fault has happened
 + * @iova: the faulting address
 + * @flags: mmu fault flags (e.g. IOMMU_FAULT_READ/IOMMU_FAULT_WRITE/...)
 + * @event: the mmu fault type

Please place 'event' before iova when you keep it, and not at the end.
Then you have 'where' and 'what' of the fault first before the details
(iova, flags).

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/7] iommu: fixes extensions

2011-09-06 Thread Roedel, Joerg 
On Fri, Sep 02, 2011 at 01:32:29PM -0400, Ohad Ben-Cohen wrote:
 5 first patches are relatively small iommu fixes/cleanups.
 
 2 last patches are proposals for core iommu extensions:
 - Add fault report mechanism, needed for recovery of remote processors
   trying to access unmapped addresses.
 - Add splitting of memory regions to pages in the iommu core itself
   (according to hardware capabilities as advertised by the iommu drivers).
   This is needed to prevent duplication of this logic by
   the iommu users/drivers themselves.
 
 The patches are based on Joerg's arm/omap branch.
 
 Tested with OMAP3 (omap3isp) + OMAP4 (rpmsg/remoteproc).
 
 Laurent Pinchart (1):
   iommu/omap-iovmm: support non page-aligned buffers in iommu_vmap
 
 Ohad Ben-Cohen (6):
   iommu/omap: cleanup: remove a redundant 'return' statement
   iommu/core: use the existing IS_ALIGNED macro
   iommu/omap: -unmap() should return order of unmapped page
   iommu/msm: -unmap() should return order of unmapped page
   iommu/core: add fault reporting
   iommu/core: split mapping to page sizes as supported by the hardware

Ok, I applied 1-5 to their respective branches. Patch 6 needs some more
discussion to make sure the interface is generally usable. Patch 7 seems
to be a starting point for now. This definitly requires conversion of
the other IOMMU drivers too.
Please make individual patch-sets from patch 6 and 7 respectivly.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 6/7] iommu/core: add fault reporting

2011-09-05 Thread Roedel, Joerg 
On Fri, Sep 02, 2011 at 01:32:35PM -0400, Ohad Ben-Cohen wrote:
 -struct iommu_domain *iommu_domain_alloc(void)
 +/**
 + * iommu_domain_alloc() - allocate and initialize a new iommu domain
 + * @handler: an optional pointer to a fault handler, or NULL if not needed
 + *
 + * Returns the new domain, or NULL on error.
 + */
 +struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler)

Please add a seperate function for setting the fault-handler. It is
optional, so no need to be a value of the alloc-function.

 +/**
 + * enum iommu_fault_types - iommu fault types
 + *
 + * @IOMMU_ERROR: Unrecoverable error
 + * @IOMMU_TLBMISS: TLB miss while the page table walker is disabled
 + * @IOMMU_NOPTE: TLB miss and no PTE for the requested address
 + */
 +enum iommu_fault_types {
 + IOMMU_ERROR,
 + IOMMU_TLBMISS,
 + IOMMU_NOPTE,
 +};

Can you elaborate a bit on what the user of the api will do different
between IOMMU_TLBMISS and IOMMU_NOPTE?
My feeling is that those differences should be handled internally in the
IOMMU driver, but probably I miss a use-case.
Also, we need some flags to distinguish between the type of the fault
(read, write, ...).

Joerg


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iommu: omap_iovmm: support non page-aligned buffers in iommu_vmap

2011-09-01 Thread Roedel, Joerg 
On Wed, Aug 31, 2011 at 12:56:26PM -0400, Laurent Pinchart wrote:
 True, but if we implement address rounding transparently in the IOMMU layer 
 Ohad's concern can be valid, depending on whether the device is trusted. If 
 we 
 decide to push address rounding to the drivers that decision can be made on a 
 per-device basis. However, drivers are usually not aware of what granularity 
 the IOMMU works on, so that wouldn't be straightforward and clean.

Drivers usually just use the DMA-API, so they can not assume at all that
any protection happens. The problem also can't be really solved without
changing/breaking the DMA-API. It is defined to work on byte granularity
while the hardware only works with pages.

The only way to work around this it to implement the driver in a way so
that they only map page-aligned buffers where the size is also a
multiple of a page-size.

In this case you can assume the page-size you are working on because you
already assume in the driver that an iommu is in use.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iommu: omap_iovmm: support non page-aligned buffers in iommu_vmap

2011-09-01 Thread Roedel, Joerg 
On Thu, Sep 01, 2011 at 09:31:13AM -0400, Laurent Pinchart wrote:
 Hi Ohad,
 
 On Thursday 01 September 2011 13:47:26 Ohad Ben-Cohen wrote:
  On Wed, Aug 31, 2011 at 1:52 PM, Ohad Ben-Cohen o...@wizery.com wrote:
   From: Laurent Pinchart laurent.pinch...@ideasonboard.com
   
   omap_iovmm requires page-aligned buffers, and that sometimes causes
   omap3isp failures (i.e. whenever the buffer passed from userspace is not
   page-aligned).
   
   Remove this limitation by rounding the address of the first page entry
   down, and adding the offset back to the device address.
  
  Seems like the unmap paths were skipped (need to adjust the sizes in
  the unmap path too).
  
  Laurent, if it looks good to you, I'll just squash it to the original
  patch and repost:
 
 Do you have a tree where the current code base can be found ?

Please base your upstream-patches against

git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git arm/omap

Thanks,

Joerg


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iommu: omap_iovmm: support non page-aligned buffers in iommu_vmap

2011-08-31 Thread Roedel, Joerg 
On Wed, Aug 31, 2011 at 06:52:08AM -0400, Ohad Ben-Cohen wrote:
 On Mon, Aug 29, 2011 at 10:36 PM, Ohad Ben-Cohen o...@wizery.com wrote:
  From: Laurent Pinchart laurent.pinch...@ideasonboard.com
 
  omap_iovmm requires page-aligned buffers, and that sometimes causes
  omap3isp failures (i.e. whenever the buffer passed from userspace is not
  page-aligned).
 
  Remove this limitation by rounding the address of the first page entry
  down, and adding the offset back to the device address.
 
 I'm having second thoughts about this.
 
 Obviously it works for omap3isp and its users because the buffer gets
 mapped and everyone is happy.
 
 But I'm not sure this is a valid IOMMU interface that the kernel
 should have, because effectively we're now mapping physical memory
 which nobody asked us to, and which might contain sensitive stuff we
 don't want to give the device (e.g. a remote processor which might be
 running rogue code) access to.
 
 Thoughts ?

Do you mean the parts of the pages you map to the device that are not in
the requested range (basically everything before offset and all after
size)?
This issue exists in other iommu drivers as well. It is inherent to how
the dma-api is defined and how the iommu hardware works.
The dma-api can work on byte granularity while the hardware usually only
works on page granularity.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] omap: iommu migration, relocation and cleanups

2011-08-26 Thread Roedel, Joerg 
On Wed, Aug 24, 2011 at 01:08:07PM -0400, Ohad Ben-Cohen wrote:
 Ohad Ben-Cohen (7):
   omap: iommu: migrate to the generic IOMMU API
   omap: iommu/iovmm: move to dedicated iommu folder
   omap: iommu: stop exporting local functions
   omap: iommu: PREFETCH_IOTLB cleanup
   omap: iovmm: remove unused functionality
   omap: iommu: remove unused exported API
   omap: iommu: omapify 'struct iommu' and exposed API

Applied all to arm/omap, thanks Ohad. I also put a commit on-top which
finished the mutex-spin_lock conversion. Looks like one call-place was
missed. See attached patch.

commit 4234541f1a64d9dc6d489cf8f614dc01c62360f6
Author: Joerg Roedel joerg.roe...@amd.com
Date:   Fri Aug 26 13:20:06 2011 +0200

omap: iommu: Fix up mutex-spin_lock conversion of iommu_lock

The omap_iommu_set_isr() was still using the mutex functions
but the iommu_lock was converted to a spin_lock. Fix that
up.

Signed-off-by: Joerg Roedel joerg.roe...@amd.com

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index dad45ab..90744af 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -918,14 +918,14 @@ int omap_iommu_set_isr(const char *name,
return -ENODEV;
 
obj = to_iommu(dev);
-   mutex_lock(obj-iommu_lock);
+   spin_lock(obj-iommu_lock);
if (obj-refcount != 0) {
-   mutex_unlock(obj-iommu_lock);
+   spin_unlock(obj-iommu_lock);
return -EBUSY;
}
obj-isr = isr;
obj-isr_priv = isr_priv;
-   mutex_unlock(obj-iommu_lock);
+   spin_unlock(obj-iommu_lock);
 
return 0;
 }

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] omap: iommu: migrate to the generic IOMMU API

2011-08-24 Thread Roedel, Joerg 
On Wed, Aug 24, 2011 at 08:46:13AM -0400, Ohad Ben-Cohen wrote:
 On Tue, Aug 23, 2011 at 5:59 PM, Ohad Ben-Cohen o...@wizery.com wrote:
  On Tue, Aug 23, 2011 at 5:07 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  Can this be easily converted to a spin_lock?
 
  Sure, thanks for reviewing.
 
 Taking a second look, I don't think it's necessary - the mutex isn't
 used to protect the page table. The page table is protected by a spin
 lock, so map/unmap operations can be called from an atomic context.
 
 The mutex is only part of the attach/deattach operations, which are
 already used today in process context, so I guess it's safe.

Yes, it should be safe in your context. But the iommu-api is generic and
I would prefer that all functions it provides can be called from any
context.
Or is the time required for attaching/detaching too long so that it
makes sense to put secondary threads to sleep?

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] omap: iommu: migrate to the generic IOMMU API

2011-08-23 Thread Roedel, Joerg 
On Wed, Aug 17, 2011 at 07:10:02PM -0400, Ohad Ben-Cohen wrote:
 +/**
 + * omap_iommu_attach() - attach iommu device to an iommu domain
 + * @dev:   target omap iommu device
 + * @iopgd: page table
   **/
 -struct iommu *iommu_get(const char *name)
 +static struct iommu *omap_iommu_attach(struct device *dev, u32 *iopgd)
  {
 int err = -ENOMEM;
 -   struct device *dev;
 -   struct iommu *obj;
 -
 -   dev = driver_find_device(omap_iommu_driver.driver, NULL, (void 
 *)name,
 -device_match_by_alias);
 -   if (!dev)
 -   return ERR_PTR(-ENODEV);
 -
 -   obj = to_iommu(dev);
 +   struct iommu *obj = to_iommu(dev);
 
 mutex_lock(obj-iommu_lock);

I don't think using a mutex here is a good idea. This prevents this
function to be called from atomic context. The iommu_map and _unmap
functions need to allowed in atomic-context and I don't want different
context constraints within the iommu-api. Can this be easily converted
to a spin_lock?

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] omap: iommu migration, relocation and cleanups

2011-08-23 Thread Roedel, Joerg 
On Wed, Aug 17, 2011 at 07:10:01PM -0400, Ohad Ben-Cohen wrote:
 1. Migrate OMAP's iommu driver to the generic IOMMU API, and move it
 to the dedicated iommu folder.
 
 2. Fix omap3isp appropriately so it doesn't break.
 
 3. Adapt OMAP's iovmm appropriately as well, because omap3isp still relies
 on it. The plan is eventually to remove iovmm and replace it with the
 upcoming IOMMU-based generic DMA-API. Move iovmm to the iommu folder too;
 it belongs there more than it belongs in the OMAP folders.
 
 4. Start cleaning up OMAP's iommu components towards the end goal, where
 1) omap-iommu no longer exports public API (and instead provides its
entire functionality via the generic IOMMU API).
 2) omap-iovmm is removed.

Besides the locking issue these patches look good to me. Please repost
when this is solved and I will put it in my tree then.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] x86/ia64: intel-iommu: move to drivers/iommu/

2011-06-17 Thread Roedel, Joerg 
On Fri, Jun 10, 2011 at 05:55:47PM -0400, Ohad Ben-Cohen wrote:
 This should ease finding similarities with different platforms,
 with the intention of solving problems once in a generic framework
 which everyone can use.
 
 Note: to move intel-iommu.c, the declaration of 
 pci_find_upstream_pcie_bridge()
 has to move from drivers/pci/pci.h to include/linux/pci.h. This is handled
 in this patch, too.

David, does this look good to you? Can you send an ACK if so?

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] drivers/iommu/ relocations

2011-06-14 Thread Roedel, Joerg 
On Fri, Jun 10, 2011 at 05:55:43PM -0400, Ohad Ben-Cohen wrote:
 Ohad Ben-Cohen (4):
   drivers: iommu: move to a dedicated folder
   msm: iommu: move to drivers/iommu/
   x86: amd_iommu: move to drivers/iommu/
   x86/ia64: intel-iommu: move to drivers/iommu/

Btw, what tree do these patches apply against? I tried to apply them to
v3.0-rc3 and got conflicts. Can you rebase them against -rc3?

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] drivers/iommu/ relocations

2011-06-14 Thread Roedel, Joerg 
On Tue, Jun 14, 2011 at 06:44:56AM -0400, Ohad Ben-Cohen wrote:
 On Tue, Jun 14, 2011 at 1:29 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  Btw, what tree do these patches apply against?
 
 v3.0-rc2
 
  I tried to apply them to v3.0-rc3 and got conflicts.
 
 What kind of conflicts ? I just tried rc3 and it applied cleanly.
 
 Just in case, I'm attaching the patches too.
 
 Tell me if you still see any issue, thanks !

The patches from your archive applied fine. Probably the ones I got were
screwed up by exchange :/

I go forward an test them now.

Thanks,

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] drivers/iommu/ relocations

2011-06-08 Thread Roedel, Joerg 
(Cc'ing Ingo)

On Wed, Jun 08, 2011 at 04:34:18AM -0400, Ohad Ben-Cohen wrote:
 Create a dedicated iommu drivers folder, put the base iommu code there,
 and move the existing IOMMU API users as well (msm-iommu, amd_iommu and
 intel-iommu).
 
 Putting all iommu drivers together will ease finding similarities
 between different platforms, with the intention of solving problems once,
 in a generic framework, which everyone can use.
 
 OMAP's iommu will be moved too as soon as it's migrated.

Great, thanks. I'll apply the patches as soon as the relevant ACKs come
in. Looking at the MAINTAINERS file David Brown needs to ACK the MSM
patch and David Woodhouse the VT-d patch.

David B., David W., is this direction ok for both of you?

A more important question is how we handle the IOMMU tree. Currently the
situation is as follows:

* The AMD IOMMU changes go upstream through Ingo
* David Woohouse has his own tree which he sents directly to
  Linus
* Not sure about the ARM IOMMU code
* And to comlicate things further there is the upcoming ARM
  integration tree which may contain code that depends on IOMMU
  changes

My suggestion is that the ARM tree pulls in the necessary changes from
the IOMMU tree and the IOMMU code goes upstream through Ingo or directly
to Linus (with some time in linux-next, of course). Thoughts?

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] x86: intel-iommu: move to drivers/iommu/

2011-06-08 Thread Roedel, Joerg 
On Wed, Jun 08, 2011 at 05:17:38AM -0400, David Woodhouse wrote:
 On Wed, 2011-06-08 at 11:34 +0300, Ohad Ben-Cohen wrote:
  --- a/drivers/pci/Makefile
  +++ b/drivers/pci/Makefile
  @@ -30,7 +30,7 @@ obj-$(CONFIG_PCI_MSI) += msi.o
   obj-$(CONFIG_HT_IRQ) += htirq.o
   
   # Build Intel IOMMU support
  -obj-$(CONFIG_DMAR) += dmar.o iova.o intel-iommu.o
  +obj-$(CONFIG_DMAR) += dmar.o iova.o
   
   obj-$(CONFIG_INTR_REMAP) += dmar.o intr_remapping.o
   
 
 At least iova.o wants to go with it. That's one of the parts that is a
 candidate for harmonisation across IOMMU implementations, either by
 removing it or by having others use it too. It's how we allocate virtual
 I/O address space.
 
 I suspect the interrupt remapping support may well want to move with it
 too. It's no more out-of-place in drivers/iommu than it is in
 drivers/pci. And then you can certainly move dmar.o too.

Interrupt remapping certainly makes sense too. I am not sure yet how to
generalize it because the AMD version of it is significantly different
from VT-d, but we'll see.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] drivers/iommu/ relocations

2011-06-08 Thread Roedel, Joerg 
On Wed, Jun 08, 2011 at 09:11:16AM -0400, Ohad Ben-Cohen wrote:
 On Wed, Jun 8, 2011 at 4:05 PM, Matthew Wilcox matt...@wil.cx wrote:
  You've missed at least parisc, ia64, alpha, sparc, powerpc and sh which
  have IOMMUs.
 
 None of these seem to call register_iommu.
 
  Not that they necessarily all need to be moved across in
  one patchset, but saying all iommu drivers is clearly false.
 
 I've moved only the existing IOMMU API users.

And I think that is good for now. The iommu-api needs to be extended to
cover all kinds of iommus (like the iommus available on other
architectures). It is definitly the plan to do that and have a common
dma_ops implementation for all of them. But lets start small by now and
move forward step by step :-)

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/6] iommu: generic api migration and grouping

2011-06-07 Thread Roedel, Joerg 
On Mon, Jun 06, 2011 at 04:09:33PM -0400, Ohad Ben-Cohen wrote:
 On Mon, Jun 6, 2011 at 10:20 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  Well, it certainly makes sense to have a single implementation for this.
  But I want to hide this complexity to the user of the IOMMU-API. The
  best choice is to put this into the layer between the IOMMU-API and the
  backend implementation.
 
 I agree.
 
 The IOMMU API should take physically contiguous regions from the user,
 split them up according to page-sizes (/alignment requirements)
 supported by the hardware, and then tell the underlying implementation
 what to map where.

Yup. Btw, is there any IOMMU hardware which supports non-natural
alignment? In this case we need to expose these requirements somehow.

Regards,

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/6] iommu: generic api migration and grouping

2011-06-07 Thread Roedel, Joerg 
On Tue, Jun 07, 2011 at 05:22:11AM -0400, Ohad Ben-Cohen wrote:
 On Tue, Jun 7, 2011 at 10:52 AM, Roedel, Joerg joerg.roe...@amd.com wrote:
  Yup. Btw, is there any IOMMU hardware which supports non-natural
  alignment? In this case we need to expose these requirements somehow.
 
 Not sure there are. Let's start with natural alignment, and extend it
 only if someone with additional requirements shows up.

Btw, mind to split out your changes which move the iommu-api into
drivers/iommu? I can merge them meanwhile into my iommu tree and start
working on a proposal for the generic large page-size support.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/6] iommu: generic api migration and grouping

2011-06-06 Thread Roedel, Joerg 
Hi,

On Thu, Jun 02, 2011 at 06:27:37PM -0400, Ohad Ben-Cohen wrote:
 First stab at iommu consolidation:
 
 - Migrate OMAP's iommu driver to the generic iommu API. With this in hand,
   users can now start using the generic iommu layer instead of calling
   omap-specific iommu API.
 
   New code that requires functionality missing from the generic iommu api,
   will add that functionality in the generic framework (e.g. adding framework
   awareness to multi page sizes, supported by the underlying hardware, will
   avoid the otherwise-inevitable code duplication when mapping a memory
   region).

The IOMMU-API already supports multiple page-sizes. See the
'order'-parameter of the map/unmap functions.

   Further generalizing of iovmm strongly depends on our broader plans for
   providing a generic virtual memory manager and allocation framework
   (which, as discussed, should be separated from a specific mapper).

The generic vmm for DMA is called DMA-API :) Any reason for not using
that (those reasons should be fixed)?

   iovmm has a mainline user: omap3isp, and therefore must be maintained,
   but new potential users will either have to generalize it, or come up
   with a different generic framework that will replace it.

Moving to the DMA-API should be considered here. If it is too much work
iovmm can stay for a while, but the goal should be to get rid of it and
only use the DMA-API.

 - Create a dedicated iommu drivers folder (and put the base iommu code there)

Very good idea.


Joerg

P.S.: Please also Cc the iommu-list (io...@lists.linux-foundation.org)
  in the future for IOMMU related patches.

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/6] iommu: generic api migration and grouping

2011-06-06 Thread Roedel, Joerg 
On Mon, Jun 06, 2011 at 11:15:30AM -0400, Ohad Ben-Cohen wrote:

 This is insufficient; users need somehow to tell what page sizes are
 supported by the underlying hardware (we can't assume host page-sizes,
 and we want to use bigger pages whenever possible, to relax the TLB
 pressure).
/
What does the IOMMU-API user need this info for? On the x86 IOMMUs these
details are handled transparently by the IOMMU driver.

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/6] iommu: generic api migration and grouping

2011-06-06 Thread Roedel, Joerg 
On Mon, Jun 06, 2011 at 12:36:13PM -0400, Ohad Ben-Cohen wrote:
 On Mon, Jun 6, 2011 at 6:35 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
  On Mon, Jun 06, 2011 at 11:15:30AM -0400, Ohad Ben-Cohen wrote:
 
  This is insufficient; users need somehow to tell what page sizes are
  supported by the underlying hardware (we can't assume host page-sizes,
  and we want to use bigger pages whenever possible, to relax the TLB
  pressure).
  /
  What does the IOMMU-API user need this info for? On the x86 IOMMUs these
  details are handled transparently by the IOMMU driver.
 
 That's one way to do that, but then it means duplicating this logic
 inside the different IOMMU implementations.
 
 Take the OMAP (and seemingly MSM too) example: we have 4KB, 64KB, 1MB
 and 16MB page-table entries. When we map a memory region, we need to
 break it up to a minimum number of pages (while validating
 sizes/alignments are sane). It's not complicated, but it can be nice
 if it'd be implemented only once.

Well, it certainly makes sense to have a single implementation for this.
But I want to hide this complexity to the user of the IOMMU-API. The
best choice is to put this into the layer between the IOMMU-API and the
backend implementation.

 In addition, unless we require 'va' and 'pa' to have the exact same
 alignment, we might run into specific page configuration that the
 IOMMU implementation cannot restore on -unmap, since unmap only takes
 'va' and 'order'. So we will either have to supply 'pa' too, or have
 the implementation remember the mapping in order to unmap it later.
 That begins to be a bit messy...

That interface is not put into stone. There were other complains about
the -unmap part recently, so there is certainly room for improvement
there.

Regards,

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html