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

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

[Ohad Ben-Cohen]

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

  OMAP-specific api that is still exposed in the omap iommu driver can
  now be either moved to the generic iommu framework, or just removed (if not
  used).

  This api (and other omap-specific primitives like struct iommu) needs to
  be omapified (i.e. renamed to include an 'omap_' prefix). At this early
  point of this patch set this is too much churn though, so I'll do that
  in the following iteration, after (and if), the general direction is
  accepted.
  
- Migrate OMAP's iovmm (virtual memory manager) driver to the generic
  iommu API. With this in hand, iovmm no longer uses omap-specific api
  for mapping/unmapping operations. Nevertheless, iovmm is still coupled
  with omap's iommu even with this change: it assumes omap page sizes,
  and it uses omap's iommu objects to maintain its internal state.

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

  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.

- Migrate OMAP's iommu mainline user, omap3isp, to the generic API as well
  (so it doesn't break). As with iovmm, omap3isp still depends on
  omap's iommu, mainly because iovmm depends on it, but also for
  iommu context saving and restoring.

  It is definitely desirable to completely remove omap3isp's dependency
  on the omap-specific iommu layer, and that will be possible as the
  required functionality will be added to generic framework.

- Create a dedicated iommu drivers folder (and put the base iommu code there)
- Move OMAP's and MSM's iommu drivers to that drivers iommu folder

  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.

  I've only moved the omap and msm implementations for now, to demonstrate
  the idea (and support the ARM diet :), but if this is found desirable,
  we can bring in intel-iommu.c and amd_iommu.c as well.

Meta:

- This patch set is not bisectable; it was splitted (and ordered) this way
  to ease its review. Later iterations of this patch set will fix that
  (most likely by squashing the first three patches)
- Based on and tested with 3.0-rc1
- OMAP's iommu code was tested on both OMAP3 and OMAP4
- omap3isp code was tested with a sensor-less OMAP3 (memory-to-memory only)
  (thanks Laurent Pinchart for showing me the magic needed to test omap3isp :)
- MSM code was only compile tested

Ohad Ben-Cohen (6):
  omap: iommu: generic iommu api migration
  omap: iovmm: generic iommu api migration
  media: omap3isp: generic iommu api migration
  drivers: iommu: move to a dedicated folder
  omap: iommu/iovmm: move to dedicated iommu folder
  msm: iommu: move to dedicated iommu drivers folder

 arch/arm/mach-msm/Kconfig                          |   15 -
 arch/arm/mach-msm/Makefile                         |    2 +-
 arch/arm/plat-omap/Kconfig                         |   12 -
 arch/arm/plat-omap/Makefile                        |    2 -
 arch/arm/plat-omap/include/plat/iommu.h            |    3 +-
 arch/arm/plat-omap/{ => include/plat}/iopgtable.h  |   18 ++
 arch/arm/plat-omap/include/plat/iovmm.h            |   27 +-
 arch/x86/Kconfig                                   |    5 +-
 drivers/Kconfig                                    |    2 +
 drivers/Makefile                                   |    1 +
 drivers/base/Makefile                              |    1 -
 drivers/iommu/Kconfig                              |   32 +++
 drivers/iommu/Makefile                             |    5 +
 drivers/{base => iommu}/iommu.c                    |    0
 .../mach-msm/iommu.c => drivers/iommu/msm-iommu.c  |    0
 .../iommu/omap-iommu-debug.c                       |    2 +-
 .../iommu.c => drivers/iommu/omap-iommu.c          |  290 +++++++++++++++++---
 .../iovmm.c => drivers/iommu/omap-iovmm.c          |  113 +++++---
 drivers/media/video/Kconfig                        |    2 +-
 drivers/media/video/omap3isp/isp.c                 |   41 +++-
 drivers/media/video/omap3isp/isp.h                 |    3 +
 drivers/media/video/omap3isp/ispccdc.c             |   16 +-
 drivers/media/video/omap3isp/ispstat.c             |    6 +-
 drivers/media/video/omap3isp/ispvideo.c            |    4 +-
 24 files changed, 451 insertions(+), 151 deletions(-)
 rename arch/arm/plat-omap/{ => include/plat}/iopgtable.h (84%)
 create mode 100644 drivers/iommu/Kconfig
 create mode 100644 drivers/iommu/Makefile
 rename drivers/{base => iommu}/iommu.c (100%)
 rename arch/arm/mach-msm/iommu.c => drivers/iommu/msm-iommu.c (100%)
 rename arch/arm/plat-omap/iommu-debug.c => drivers/iommu/omap-iommu-debug.c (99%)
 rename arch/arm/plat-omap/iommu.c => drivers/iommu/omap-iommu.c (77%)
 rename arch/arm/plat-omap/iovmm.c => drivers/iommu/omap-iovmm.c (85%)

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

[Kyungmin Park]

Hi,

Good approach.
CC'ed the Samsung IOMMU developer. Marek.

BTW, Russell wants to use the DMA based IOMMU?

Please see the RFC patch, ARM: DMA-mapping & IOMMU integration
http://www.spinics.net/lists/linux-mm/msg19856.html

Thank you,
Kyungmin Park

On Fri, Jun 3, 2011 at 7:27 AM, Ohad Ben-Cohen <ohad@wizery.com> 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).
>
> ?OMAP-specific api that is still exposed in the omap iommu driver can
> ?now be either moved to the generic iommu framework, or just removed (if not
> ?used).
>
> ?This api (and other omap-specific primitives like struct iommu) needs to
> ?be omapified (i.e. renamed to include an 'omap_' prefix). At this early
> ?point of this patch set this is too much churn though, so I'll do that
> ?in the following iteration, after (and if), the general direction is
> ?accepted.
>
> - Migrate OMAP's iovmm (virtual memory manager) driver to the generic
> ?iommu API. With this in hand, iovmm no longer uses omap-specific api
> ?for mapping/unmapping operations. Nevertheless, iovmm is still coupled
> ?with omap's iommu even with this change: it assumes omap page sizes,
> ?and it uses omap's iommu objects to maintain its internal state.
>
> ?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).
>
> ?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.
>
> - Migrate OMAP's iommu mainline user, omap3isp, to the generic API as well
> ?(so it doesn't break). As with iovmm, omap3isp still depends on
> ?omap's iommu, mainly because iovmm depends on it, but also for
> ?iommu context saving and restoring.
>
> ?It is definitely desirable to completely remove omap3isp's dependency
> ?on the omap-specific iommu layer, and that will be possible as the
> ?required functionality will be added to generic framework.
>
> - Create a dedicated iommu drivers folder (and put the base iommu code there)
> - Move OMAP's and MSM's iommu drivers to that drivers iommu folder
>
> ?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.
>
> ?I've only moved the omap and msm implementations for now, to demonstrate
> ?the idea (and support the ARM diet :), but if this is found desirable,
> ?we can bring in intel-iommu.c and amd_iommu.c as well.
>
> Meta:
>
> - This patch set is not bisectable; it was splitted (and ordered) this way
> ?to ease its review. Later iterations of this patch set will fix that
> ?(most likely by squashing the first three patches)
> - Based on and tested with 3.0-rc1
> - OMAP's iommu code was tested on both OMAP3 and OMAP4
> - omap3isp code was tested with a sensor-less OMAP3 (memory-to-memory only)
> ?(thanks Laurent Pinchart for showing me the magic needed to test omap3isp :)
> - MSM code was only compile tested
>
> Ohad Ben-Cohen (6):
> ?omap: iommu: generic iommu api migration
> ?omap: iovmm: generic iommu api migration
> ?media: omap3isp: generic iommu api migration
> ?drivers: iommu: move to a dedicated folder
> ?omap: iommu/iovmm: move to dedicated iommu folder
> ?msm: iommu: move to dedicated iommu drivers folder
>
> ?arch/arm/mach-msm/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 15 -
> ?arch/arm/mach-msm/Makefile ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +-
> ?arch/arm/plat-omap/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? | ? 12 -
> ?arch/arm/plat-omap/Makefile ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 -
> ?arch/arm/plat-omap/include/plat/iommu.h ? ? ? ? ? ?| ? ?3 +-
> ?arch/arm/plat-omap/{ => include/plat}/iopgtable.h ?| ? 18 ++
> ?arch/arm/plat-omap/include/plat/iovmm.h ? ? ? ? ? ?| ? 27 +-
> ?arch/x86/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?5 +-
> ?drivers/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +
> ?drivers/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
> ?drivers/base/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 -
> ?drivers/iommu/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 32 +++
> ?drivers/iommu/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?5 +
> ?drivers/{base => iommu}/iommu.c ? ? ? ? ? ? ? ? ? ?| ? ?0
> ?.../mach-msm/iommu.c => drivers/iommu/msm-iommu.c ?| ? ?0
> ?.../iommu/omap-iommu-debug.c ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +-
> ?.../iommu.c => drivers/iommu/omap-iommu.c ? ? ? ? ?| ?290 +++++++++++++++++---
> ?.../iovmm.c => drivers/iommu/omap-iovmm.c ? ? ? ? ?| ?113 +++++---
> ?drivers/media/video/Kconfig ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
> ?drivers/media/video/omap3isp/isp.c ? ? ? ? ? ? ? ? | ? 41 +++-
> ?drivers/media/video/omap3isp/isp.h ? ? ? ? ? ? ? ? | ? ?3 +
> ?drivers/media/video/omap3isp/ispccdc.c ? ? ? ? ? ? | ? 16 +-
> ?drivers/media/video/omap3isp/ispstat.c ? ? ? ? ? ? | ? ?6 +-
> ?drivers/media/video/omap3isp/ispvideo.c ? ? ? ? ? ?| ? ?4 +-
> ?24 files changed, 451 insertions(+), 151 deletions(-)
> ?rename arch/arm/plat-omap/{ => include/plat}/iopgtable.h (84%)
> ?create mode 100644 drivers/iommu/Kconfig
> ?create mode 100644 drivers/iommu/Makefile
> ?rename drivers/{base => iommu}/iommu.c (100%)
> ?rename arch/arm/mach-msm/iommu.c => drivers/iommu/msm-iommu.c (100%)
> ?rename arch/arm/plat-omap/iommu-debug.c => drivers/iommu/omap-iommu-debug.c (99%)
> ?rename arch/arm/plat-omap/iommu.c => drivers/iommu/omap-iommu.c (77%)
> ?rename arch/arm/plat-omap/iovmm.c => drivers/iommu/omap-iovmm.c (85%)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

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

[Arnd Bergmann]

On Friday 03 June 2011, Ohad Ben-Cohen wrote:
> First stab at iommu consolidation:

Hi Ohad,

Great to see your progress here!
 
> - 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).
>
>   OMAP-specific api that is still exposed in the omap iommu driver can
>   now be either moved to the generic iommu framework, or just removed (if not
>   used).
> 
>   This api (and other omap-specific primitives like struct iommu) needs to
>   be omapified (i.e. renamed to include an 'omap_' prefix). At this early
>   point of this patch set this is too much churn though, so I'll do that
>   in the following iteration, after (and if), the general direction is
>   accepted.

Sounds all good.

> - Migrate OMAP's iovmm (virtual memory manager) driver to the generic
>   iommu API. With this in hand, iovmm no longer uses omap-specific api
>   for mapping/unmapping operations. Nevertheless, iovmm is still coupled
>   with omap's iommu even with this change: it assumes omap page sizes,
>   and it uses omap's iommu objects to maintain its internal state.
> 
>   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).
> 
>   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.

I think the future of iovmm is looking not so good. Marek Szyprowski
is working on a generic version of the dma-mapping API (dma_map_ops)
based on the iommu API. As far as I can tell, once we have that in
place, we you can migrate omap3isp from iovmm to dma-mapping and
remove iovmm.

Of course if there are things missing from the dma-mapping API
that are present in iovmm, we should know about them so that we can
extend the dma-mapping API accordingly.

> - Migrate OMAP's iommu mainline user, omap3isp, to the generic API as well
>   (so it doesn't break). As with iovmm, omap3isp still depends on
>   omap's iommu, mainly because iovmm depends on it, but also for
>   iommu context saving and restoring.
> 
>   It is definitely desirable to completely remove omap3isp's dependency
>   on the omap-specific iommu layer, and that will be possible as the
>   required functionality will be added to generic framework.

ok.

> - Create a dedicated iommu drivers folder (and put the base iommu code there)
>
> - Move OMAP's and MSM's iommu drivers to that drivers iommu folder
> 
>   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.
> 
>   I've only moved the omap and msm implementations for now, to demonstrate
>   the idea (and support the ARM diet :), but if this is found desirable,
>   we can bring in intel-iommu.c and amd_iommu.c as well.

Yes, very good idea.

	Arnd

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

[Ohad Ben-Cohen]

On Fri, Jun 3, 2011 at 1:27 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> First stab at iommu consolidation:

All but the first email in this submission were blocked by lakml's filters.

If anyone was looking for the patches, check them out in lkml's archives.

Here's the blocking notification:

> Your mail to 'linux-arm-kernel' with the subject
>
>   [RFC 6/6] msm: iommu: move to dedicated iommu drivers folder
>
> Is being held until the list moderator can review it for approval.
>
> The reason it is being held:
>
>   Message has a suspicious header
>
> Either the message will get posted to the list, or you will receive
> notification of the moderator's decision.  If you would like to cancel
> this posting, please visit the following URL:

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

[Ohad Ben-Cohen]

Hi Arnd,

On Fri, Jun 3, 2011 at 6:53 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> I think the future of iovmm is looking not so good. Marek Szyprowski
> is working on a generic version of the dma-mapping API (dma_map_ops)
> based on the iommu API.

Nice! I missed Marek's work somehow.

> As far as I can tell, once we have that in
> place, we you can migrate omap3isp from iovmm to dma-mapping and
> remove iovmm.

Sounds like a plan.

I'd still prefer us to take small steps here, and not gate the omap
iommu cleanups with Marek's generic dma_map_ops work though. Let's go
forward and migrate omap's iommu to the generic iommu API, so new code
will be able to use it (e.g. the long coming virtio-based IPC/AMP
framework).

We'll migrate iovmm/omap3isp just enough so they don't break, but once
the generic dma_map_ops work materializes, we'd be able to complete
the migration, remove iovmm, and decouple omap3isp from omap-specific
iommu APIs for good.

>> ? I've only moved the omap and msm implementations for now, to demonstrate
>> ? the idea (and support the ARM diet :), but if this is found desirable,
>> ? we can bring in intel-iommu.c and amd_iommu.c as well.
>
> Yes, very good idea.

Great!
(to move intel-iommu.c, we'll have to move the declaration of
pci_find_upstream_pcie_bridge() from drivers/pci/pci.h to
include/linux/pci.h, but that's probably not too bad).

Thanks,
Ohad.

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

[Ohad Ben-Cohen]

Hi Kyungmin,

On Fri, Jun 3, 2011 at 2:57 AM, Kyungmin Park <kmpark@infradead.org> wrote:
> Please see the RFC patch, ARM: DMA-mapping & IOMMU integration
> http://www.spinics.net/lists/linux-mm/msg19856.html

Marek's work somehow escaped me, thanks for the pointers !

Ohad.

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

[Arnd Bergmann]

On Sunday 05 June 2011, Ohad Ben-Cohen wrote:
> > As far as I can tell, once we have that in
> > place, we you can migrate omap3isp from iovmm to dma-mapping and
> > remove iovmm.
> 
> Sounds like a plan.
> 
> I'd still prefer us to take small steps here, and not gate the omap
> iommu cleanups with Marek's generic dma_map_ops work though. Let's go
> forward and migrate omap's iommu to the generic iommu API, so new code
> will be able to use it (e.g. the long coming virtio-based IPC/AMP
> framework).

Yes, of course. That's what I meant. Moving over omap to the IOMMU API
is required anyway, so there is no point delaying that.

> We'll migrate iovmm/omap3isp just enough so they don't break, but once
> the generic dma_map_ops work materializes, we'd be able to complete
> the migration, remove iovmm, and decouple omap3isp from omap-specific
> iommu APIs for good.

Ok, great!

	Arnd

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

[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 (iommu at 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

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

[Ohad Ben-Cohen]

Hi Joerg,

On Mon, Jun 6, 2011 at 1:09 PM, Roedel, Joerg <Joerg.Roedel@amd.com> wrote:
> The IOMMU-API already supports multiple page-sizes. See the
> 'order'-parameter of the map/unmap functions.

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

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

This is definitely something we will look into (dspbridge will need it
too, not only omap3isp).

Thanks,
Ohad.

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

[Ohad Ben-Cohen]

On Mon, Jun 6, 2011 at 12:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> I'd still prefer us to take small steps here, and not gate the omap
>> iommu cleanups with Marek's generic dma_map_ops work though. Let's go
>> forward and migrate omap's iommu to the generic iommu API, so new code
>> will be able to use it (e.g. the long coming virtio-based IPC/AMP
>> framework).
>
> Yes, of course. That's what I meant. Moving over omap to the IOMMU API
> is required anyway, so there is no point delaying that.

Ok, great !

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

[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

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

[Ohad Ben-Cohen]

On Mon, Jun 6, 2011 at 6:35 PM, Roedel, Joerg <Joerg.Roedel@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.

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...

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

[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.Roedel@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

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

[Ohad Ben-Cohen]

On Mon, Jun 6, 2011 at 10:20 PM, Roedel, Joerg <Joerg.Roedel@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.

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

Once the supported page sizes are exposed to the framework, the
current ->unmap API should probably be enough. 'va' + 'order' sounds
like all the information an implementation needs to unmap a page.

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

[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.Roedel@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

[RFC 2/6] omap: iovmm: generic iommu api migration

[Laurent Pinchart]

Hi Ohad,

Thanks for the patch.

On Friday 03 June 2011 00:27:39 Ohad Ben-Cohen wrote:
> Migrate omap's iovmm (virtual memory manager) to the generic iommu api.
> 
> This brings iovmm a step forward towards being completely non
> omap-specific (it's still assuming omap's iommu page sizes, and also
> maintaining state inside omap's internal iommu structure, but it no
> longer calls omap-specific iommu map/unmap api).
> 
> Further generalizing of iovmm (or complete removal) should take place
> together with broader plans of providing a generic virtual memory manager
> and allocation framework (de-coupled from specific mappers).
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

[snip]

> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 51ef43e..80bb2b6 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c

[snip]

> @@ -473,22 +475,22 @@ static int map_iovm_area(struct iommu *obj, struct
> iovm_struct *new, u32 pa;
>  		int pgsz;
>  		size_t bytes;
> -		struct iotlb_entry e;
> 
>  		pa = sg_phys(sg);
>  		bytes = sg_dma_len(sg);
> 
>  		flags &= ~IOVMF_PGSZ_MASK;
> +
>  		pgsz = bytes_to_iopgsz(bytes);
>  		if (pgsz < 0)
>  			goto err_out;
> -		flags |= pgsz;

pgsz isn't used anymore, you can remove it.

> +
> +		order = get_order(bytes);

Does iommu_map() handle offsets correctly, or does it expect pa to be aligned 
to an order (or other) boundary ? Same comment for iommu_unmap() in 
unmap_iovm_area().

>  		pr_debug("%s: [%d] %08x %08x(%x)\n", __func__,
>  			 i, da, pa, bytes);
> 
> -		iotlb_init_entry(&e, da, pa, flags);
> -		err = iopgtable_store_entry(obj, &e);
> +		err = iommu_map(domain, da, pa, order, flags);
>  		if (err)
>  			goto err_out;
> 
> @@ -502,9 +504,11 @@ err_out:
>  	for_each_sg(sgt->sgl, sg, i, j) {
>  		size_t bytes;
> 
> -		bytes = iopgtable_clear_entry(obj, da);
> +		bytes = sg_dma_len(sg);

As Russell pointed out, we should use sg->length instead of sg_dma_length(sg). 
sg_dma_length(sg) is only valid after the scatter list has been DMA-mapped, 
which doesn't happen in the iovmm driver. This applies to all sg_dma_len(sg) 
calls.

> +		order = get_order(bytes);
> 
> -		BUG_ON(!iopgsz_ok(bytes));
> +		/* ignore failures.. we're already handling one */
> +		iommu_unmap(domain, da, order);
> 
>  		da += bytes;
>  	}

-- 
Regards,

Laurent Pinchart

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

[Ohad Ben-Cohen]

On Tue, Jun 7, 2011 at 10:52 AM, Roedel, Joerg <Joerg.Roedel@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.

[RFC 1/6] omap: iommu: generic iommu api migration

[Laurent Pinchart]

Hi Ohad,

Thanks for the patch.

On Friday 03 June 2011 00:27:38 Ohad Ben-Cohen wrote:
> Migrate OMAP's iommu to the generic iommu api, so users can stay
> generic, and non-omap-specific code can be removed and eventually
> consolidated into a generic framework.
> 
> Tested on both OMAP3 and OMAP4.
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

[snip]

> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index 34fc31e..f06e99c 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c

[snip]

> +static int omap_iommu_domain_init(struct iommu_domain *domain)
> +{
> +	struct omap_iommu_domain *omap_domain;
> +
> +	omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
> +	if (!omap_domain) {
> +		pr_err("kzalloc failed\n");
> +		goto fail_nomem;
> +	}
> +
> +	omap_domain->pgtable = (u32 *)__get_free_pages(GFP_KERNEL,
> +					get_order(IOPGD_TABLE_SIZE));
> +	if (!omap_domain->pgtable) {
> +		pr_err("__get_free_pages failed\n");
> +		goto fail_nomem;
> +	}
> +
> +	BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));

Either __get_free_pages() guarantees that the allocated memory will be aligned 
on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or 
doesn't offer such guarantee, in which case the BUG_ON() will oops randomly. 
In both cases BUG_ON() should probably be avoided.

> +	memset(omap_domain->pgtable, 0, IOPGD_TABLE_SIZE);
> +	clean_dcache_area(omap_domain->pgtable, IOPGD_TABLE_SIZE);
> +	mutex_init(&omap_domain->lock);
> +
> +	domain->priv = omap_domain;
> +
> +	return 0;
> +
> +fail_nomem:
> +	kfree(omap_domain);
> +	return -ENOMEM;
> +}
> +
> +/* assume device was already detached */
> +static void omap_iommu_domain_destroy(struct iommu_domain *domain)
> +{
> +	struct omap_iommu_domain *omap_domain = domain->priv;
> +
> +	domain->priv = NULL;
> +
> +	kfree(omap_domain);

This leaks omap_domain->pgtable.

The free_pages() call in omap_iommu_remove() should be removed, as 
omap_iommu_probe() doesn't allocate the pages table anymore. You can also 
remove the the struct iommu::iopgd field.

> +}


> +
> +static phys_addr_t omap_iommu_iova_to_phys(struct iommu_domain *domain,
> +					  unsigned long da)
> +{
> +	struct omap_iommu_domain *omap_domain = domain->priv;
> +	struct iommu *oiommu = omap_domain->iommu_dev;
> +	struct device *dev = oiommu->dev;
> +	u32 *pgd, *pte;
> +	phys_addr_t ret = 0;
> +
> +	iopgtable_lookup_entry(oiommu, da, &pgd, &pte);
> +
> +	if (pte) {
> +		if (iopte_is_small(*pte))
> +			ret = omap_iommu_translate(*pte, da, IOPTE_MASK);
> +		else if (iopte_is_large(*pte))
> +			ret = omap_iommu_translate(*pte, da, IOLARGE_MASK);
> +		else
> +			dev_err(dev, "bogus pte 0x%x", *pte);
> +	} else {
> +		if (iopgd_is_section(*pgd))
> +			ret = omap_iommu_translate(*pgd, da, IOSECTION_MASK);
> +		else if (iopgd_is_super(*pgd))
> +			ret = omap_iommu_translate(*pgd, da, IOSUPER_MASK);
> +		else
> +			dev_err(dev, "bogus pgd 0x%x", *pgd);
> +	}
> +
> +	return ret;

You return 0 in the bogus pte/pgd cases. Is that intentional ?

> +}

-- 
Regards,

Laurent Pinchart

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

[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.Roedel@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

[RFC 2/6] omap: iovmm: generic iommu api migration

[Ohad Ben-Cohen]

Hi Laurent,

On Tue, Jun 7, 2011 at 12:05 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> pgsz isn't used anymore, you can remove it.

Ok.

>> + ? ? ? ? ? ? order = get_order(bytes);
>
> Does iommu_map() handle offsets correctly, or does it expect pa to be aligned
> to an order (or other) boundary ?

Right now we have a BUG_ON if pa is unaligned, but that can be changed
if needed (do we want it to handle offsets ?).

> As Russell pointed out, we should use sg->length instead of sg_dma_length(sg).
> sg_dma_length(sg) is only valid after the scatter list has been DMA-mapped,
> which doesn't happen in the iovmm driver. This applies to all sg_dma_len(sg)
> calls.

I'll make sure I don't introduce such calls, but it sounds like a
separate patch should take care of the existing ones; pls tell me if
you want me to send one.

Thanks,
Ohad.

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

[Ohad Ben-Cohen]

On Tue, Jun 7, 2011 at 12:58 PM, Roedel, Joerg <Joerg.Roedel@amd.com> wrote:
> 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.

Sure, that will be great. Thanks!

[RFC 1/6] omap: iommu: generic iommu api migration

[Ohad Ben-Cohen]

Hi Laurent,

On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> + ? ? BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));
>
> Either __get_free_pages() guarantees that the allocated memory will be aligned
> on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or
> doesn't offer such guarantee, in which case the BUG_ON() will oops randomly.

Curious, does it oops randomly today ?
(i just copied this from omap_iommu_probe, where it always existed).

It is a bit ugly though, and thinking on it again, 16KB is not that
big. We can just use kmalloc here, which does ensure the alignment
(or, better yet, kzalloc, and then ditch the memset).

> In both cases BUG_ON() should probably be avoided.

I disagree; we must check this so user data won't be harmed (hardware
requirement), and if a memory allocation API fails to meet its
requirements - that's really bad and user data is again at stake (much
more will break, not only the iommu driver).

> This leaks omap_domain->pgtable.
>
> The free_pages() call in omap_iommu_remove() should be removed, as
> omap_iommu_probe() doesn't allocate the pages table anymore.

thanks !

> You can also remove the the struct iommu::iopgd field.

No, I can't; it's used when the device is attached to an address space domain.

> You return 0 in the bogus pte/pgd cases. Is that intentional ?

Yes, that's probably the most (if any) reasonable value to return here
(all other iommu implementations are doing so too).

Thanks,
Ohad.

[RFC 2/6] omap: iovmm: generic iommu api migration

[Laurent Pinchart]

Hi Ohad,

On Tuesday 07 June 2011 12:28:53 Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 12:05 PM, Laurent Pinchart wrote:
> > pgsz isn't used anymore, you can remove it.
> 
> Ok.
> 
> >> +             order = get_order(bytes);
> > 
> > Does iommu_map() handle offsets correctly, or does it expect pa to be
> > aligned to an order (or other) boundary ?
> 
> Right now we have a BUG_ON if pa is unaligned, but that can be changed
> if needed (do we want it to handle offsets ?).

At least for the OMAP3 ISP we need to, as video buffers don't necessarily 
start on page boundaries.

> > As Russell pointed out, we should use sg->length instead of
> > sg_dma_length(sg). sg_dma_length(sg) is only valid after the scatter
> > list has been DMA-mapped, which doesn't happen in the iovmm driver. This
> > applies to all sg_dma_len(sg) calls.
> 
> I'll make sure I don't introduce such calls, but it sounds like a
> separate patch should take care of the existing ones; pls tell me if
> you want me to send one.

A separate patch is indeed needed, yes. As you're already working on iommu it 
might be simpler if you add it to your tree. Otherwise I can send it.

-- 
Regards,

Laurent Pinchart

[RFC 1/6] omap: iommu: generic iommu api migration

[Laurent Pinchart]

Hi Ohad,

On Tuesday 07 June 2011 13:19:05 Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart wrote:
> >> +     BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));
> > 
> > Either __get_free_pages() guarantees that the allocated memory will be
> > aligned on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is
> > unnecessary, or doesn't offer such guarantee, in which case the BUG_ON()
> > will oops randomly.
> 
> Curious, does it oops randomly today ?
> (i just copied this from omap_iommu_probe, where it always existed).

No that I know of :-)

> It is a bit ugly though, and thinking on it again, 16KB is not that
> big. We can just use kmalloc here, which does ensure the alignment
> (or, better yet, kzalloc, and then ditch the memset).
> 
> > In both cases BUG_ON() should probably be avoided.
> 
> I disagree; we must check this so user data won't be harmed (hardware
> requirement), and if a memory allocation API fails to meet its
> requirements - that's really bad and user data is again at stake (much
> more will break, not only the iommu driver).

My point is that if the allocator guarantees the alignment (not as a side 
effect of the implementation, but per its API) there's no need to check it 
again. As the alignement is required, we need an allocator that guarantees it 
anyway.

> > This leaks omap_domain->pgtable.
> > 
> > The free_pages() call in omap_iommu_remove() should be removed, as
> > omap_iommu_probe() doesn't allocate the pages table anymore.
> 
> thanks !
> 
> > You can also remove the the struct iommu::iopgd field.
> 
> No, I can't; it's used when the device is attached to an address space
> domain.

Right, my bad.

> > You return 0 in the bogus pte/pgd cases. Is that intentional ?
> 
> Yes, that's probably the most (if any) reasonable value to return here
> (all other iommu implementations are doing so too).

-- 
Regards,

Laurent Pinchart

[RFC 1/6] omap: iommu: generic iommu api migration

[Ohad Ben-Cohen]

On Tue, Jun 7, 2011 at 2:40 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> My point is that if the allocator guarantees the alignment (not as a side
> effect of the implementation, but per its API) there's no need to check it
> again. As the alignement is required, we need an allocator that guarantees it
> anyway.

I understand, but I'd still prefer to have an explicit check that the
hardware alignment requirement is met.

There's no cost in doing that (it's a cold path), and even if it would
only fail once and with an extremely broken kernel - it's worth it.
Will save huge amount of debugging pain (think of the poor guy that
will have to debug this...).

Thanks,
Ohad.

[RFC 2/6] omap: iovmm: generic iommu api migration

[Ohad Ben-Cohen]

Hi Laurent,

On Tue, Jun 7, 2011 at 2:26 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> Right now we have a BUG_ON if pa is unaligned, but that can be changed
>> if needed (do we want it to handle offsets ?).
>
> At least for the OMAP3 ISP we need to, as video buffers don't necessarily
> start on page boundaries.

Where do you take care of those potential offsets today ? Or do you
simply ignore the offsets and map the entire page ?

Seems like omap's iommu (mostly) rejects unaligned pa addresses, see:

4abb761749abfb4ec403e4054f9dae2ee604e54f "omap iommu: Reject unaligned
addresses at setting page table entry"

(this doesn't seem to cover 4KB entries though, only large pages,
sections and super sections)

> A separate patch is indeed needed, yes. As you're already working on iommu it
> might be simpler if you add it to your tree.

Sure, i'll send it.

Thanks,
Ohad.

[RFC 2/6] omap: iovmm: generic iommu api migration

[Laurent Pinchart]

Hi Ohad,

On Tuesday 07 June 2011 15:46:26 Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 2:26 PM, Laurent Pinchart wrote:
> >> Right now we have a BUG_ON if pa is unaligned, but that can be changed
> >> if needed (do we want it to handle offsets ?).
> > 
> > At least for the OMAP3 ISP we need to, as video buffers don't necessarily
> > start on page boundaries.
> 
> Where do you take care of those potential offsets today ? Or do you
> simply ignore the offsets and map the entire page ?

Here http://marc.info/?l=linux-omap&m=130693502326513&w=2 :-)

> Seems like omap's iommu (mostly) rejects unaligned pa addresses, see:
> 
> 4abb761749abfb4ec403e4054f9dae2ee604e54f "omap iommu: Reject unaligned
> addresses at setting page table entry"
> 
> (this doesn't seem to cover 4KB entries though, only large pages,
> sections and super sections)
> 
> > A separate patch is indeed needed, yes. As you're already working on
> > iommu it might be simpler if you add it to your tree.
> 
> Sure, i'll send it.

-- 
Regards,

Laurent Pinchart

[RFC 2/6] omap: iovmm: generic iommu api migration

[Ohad Ben-Cohen]

On Wed, Jun 8, 2011 at 1:46 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 07 June 2011 15:46:26 Ohad Ben-Cohen wrote:
>> Where do you take care of those potential offsets today ? Or do you
>> simply ignore the offsets and map the entire page ?
>
> Here http://marc.info/?l=linux-omap&m=130693502326513&w=2 :-)

:)

Ok so it seems iovmm will take care of that for now ?

Let's get the basics working with the IOMMU API and then revise this
when we switch from iovmm to the generic dma.

Thanks,
Ohad.