Re: IOMMU DMA-mapping API for arm64 ?

Re: IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

Hi Everyone,

I was going through some iommu code in arch/arm and of some other
archs code. I have some doubts on this for refactoring and may need
some suggestions from you guys.

1. So, looking at other arch code, looks like they have their
different way of implementation of iova management and buffer
allocation. So to refactor the iommu common code out from arch/arm/ to
lib/iommu-helper, do we need to take care across all arch  ?

2. Should the approach be like take the common code(between arm/arm64)
and move it into lib/iommu-helper.c ?

Could someone give an example of what sort of code(will be better if
this is little more specific) we are talking here to be taken out to
lib/iommu-helper.c ? Earlier I was thinking of iova management can be
taken out but then I saw it might not be suited across all archs.

I am ready to do this work, but need some guidance from the experts .



Thanks
Ritesh






On Thu, Feb 6, 2014 at 5:29 PM, Ritesh Harjani <ritesh.harjani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Feb 6, 2014 at 3:43 PM, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> On Thu, Feb 06, 2014 at 05:21:53AM +0000, Ritesh Harjani wrote:
>>> I also saw one mail discussion between you guys on what is the best place for
>>> adding iommu support in ARM64, but couldn't see any followed up patches for the
>>> same.
>>
>> The decision was to make take the iommu code from arch/arm and turn it
>> into a library similar to swiotlb which can be used by the arch code. It
>> is on our to-do list but no ETA yet. If you volunteer to help it would
>> be great ;)
>
> Sure Catalin, will take out the common code from arch/arm and put into
> lib/iommu-helper which is the best place as discussed in many mail
> chains.
> Let me see how far I can go. Will keep you posted.
>
>>
>> --
>> Catalin
>
> Thanks
> Ritesh

Re: IOMMU DMA-mapping API for arm64 ?

[Will Deacon]

On Thu, Feb 27, 2014 at 05:15:38AM +0000, Ritesh Harjani wrote:
> Hi Everyone,

Hi Ritesh,

> I was going through some iommu code in arch/arm and of some other
> archs code. I have some doubts on this for refactoring and may need
> some suggestions from you guys.
> 
> 1. So, looking at other arch code, looks like they have their
> different way of implementation of iova management and buffer
> allocation. So to refactor the iommu common code out from arch/arm/ to
> lib/iommu-helper, do we need to take care across all arch  ?

Whilst the code should compile for all architectures, I don't think you need
to go round porting them all to use it. That can happen in a piecemeal
fashion as they get ported over to the generic code. Initially, I see arm and
arm64 as the users.

> 2. Should the approach be like take the common code(between arm/arm64)
> and move it into lib/iommu-helper.c ?

Well, something along those lines. We should put some thought into what the
interfaces should look like, rather than blindly copying everything out as
it stands.

> Could someone give an example of what sort of code(will be better if
> this is little more specific) we are talking here to be taken out to
> lib/iommu-helper.c ? Earlier I was thinking of iova management can be
> taken out but then I saw it might not be suited across all archs.
> 
> I am ready to do this work, but need some guidance from the experts .

Catalin can clarify here, but I think we'd basically want a generic
equivalent to arm_iommu_create_mapping and its associated halpers (i.e. the
dma_map_ops). The dma_map_ops will have generic parts (e.g. the interfaces
to the iommu API) but also architecturally-specific parts (e.g. cache
flushing for non-coherent devices), so that will need some thought.

Once that's done, we can later look at hooking this into device-tree in
order to set the correct ops each device.

Will

Re: IOMMU DMA-mapping API for arm64 ?

[Arnd Bergmann]

On Thursday 27 February 2014, Ritesh Harjani wrote:
> Hi Everyone,
> 
> I was going through some iommu code in arch/arm and of some other
> archs code. I have some doubts on this for refactoring and may need
> some suggestions from you guys.
> 
> 1. So, looking at other arch code, looks like they have their
> different way of implementation of iova management and buffer
> allocation. So to refactor the iommu common code out from arch/arm/ to
> lib/iommu-helper, do we need to take care across all arch  ?

I'd say not initially. The code can easily live in a generic place
but not be shared by everyone. If it turns out that another architecture
has a better allocator, then we can always change the common code
later.

> 2. Should the approach be like take the common code(between arm/arm64)
> and move it into lib/iommu-helper.c ?
> 
> Could someone give an example of what sort of code(will be better if
> this is little more specific) we are talking here to be taken out to
> lib/iommu-helper.c ? Earlier I was thinking of iova management can be
> taken out but then I saw it might not be suited across all archs.
> 
> I am ready to do this work, but need some guidance from the experts .

I think we should start by splitting out the iommu_coherent_ops structure
and everything that depends on. Noncoherent DMA is harder to do in
an architecture independent way, since we don't have a common way
to manage the cache across architectures. It would also be good
to follow the example of include/linux/swiotlb.h regarding the
public interface, to keep that part common. This way, ARM can easily
implement the noncoherent ops on top.

I would leave the iova management as implementation details and
not make that visible to architectures in the header file.

	Arnd

Re: IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

Thanks Will and Arnd for clarifying this.

The arch specific stuffs i.e. cache maintainance operations was
something even I wanted to get clarity on.
Let me get back with a raw patch based on your suggestions.

Thanks
Ritesh




On Fri, Feb 28, 2014 at 12:36 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 27 February 2014, Ritesh Harjani wrote:
>> Hi Everyone,
>>
>> I was going through some iommu code in arch/arm and of some other
>> archs code. I have some doubts on this for refactoring and may need
>> some suggestions from you guys.
>>
>> 1. So, looking at other arch code, looks like they have their
>> different way of implementation of iova management and buffer
>> allocation. So to refactor the iommu common code out from arch/arm/ to
>> lib/iommu-helper, do we need to take care across all arch  ?
>
> I'd say not initially. The code can easily live in a generic place
> but not be shared by everyone. If it turns out that another architecture
> has a better allocator, then we can always change the common code
> later.
>
>> 2. Should the approach be like take the common code(between arm/arm64)
>> and move it into lib/iommu-helper.c ?
>>
>> Could someone give an example of what sort of code(will be better if
>> this is little more specific) we are talking here to be taken out to
>> lib/iommu-helper.c ? Earlier I was thinking of iova management can be
>> taken out but then I saw it might not be suited across all archs.
>>
>> I am ready to do this work, but need some guidance from the experts .
>
> I think we should start by splitting out the iommu_coherent_ops structure
> and everything that depends on. Noncoherent DMA is harder to do in
> an architecture independent way, since we don't have a common way
> to manage the cache across architectures. It would also be good
> to follow the example of include/linux/swiotlb.h regarding the
> public interface, to keep that part common. This way, ARM can easily
> implement the noncoherent ops on top.
>
> I would leave the iova management as implementation details and
> not make that visible to architectures in the header file.
>
>         Arnd

Re: IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

On Fri, Feb 28, 2014 at 12:22 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Thu, Feb 27, 2014 at 05:15:38AM +0000, Ritesh Harjani wrote:
>> Hi Everyone,
>
> Hi Ritesh,

Hi Will,

>
>> I was going through some iommu code in arch/arm and of some other
>> archs code. I have some doubts on this for refactoring and may need
>> some suggestions from you guys.
>>
>> 1. So, looking at other arch code, looks like they have their
>> different way of implementation of iova management and buffer
>> allocation. So to refactor the iommu common code out from arch/arm/ to
>> lib/iommu-helper, do we need to take care across all arch  ?
>
> Whilst the code should compile for all architectures, I don't think you need
> to go round porting them all to use it. That can happen in a piecemeal
> fashion as they get ported over to the generic code. Initially, I see arm and
> arm64 as the users.
Got it.

>
>> 2. Should the approach be like take the common code(between arm/arm64)
>> and move it into lib/iommu-helper.c ?
>
> Well, something along those lines. We should put some thought into what the
> interfaces should look like, rather than blindly copying everything out as
> it stands.
>
>> Could someone give an example of what sort of code(will be better if
>> this is little more specific) we are talking here to be taken out to
>> lib/iommu-helper.c ? Earlier I was thinking of iova management can be
>> taken out but then I saw it might not be suited across all archs.
>>
>> I am ready to do this work, but need some guidance from the experts .
>
> Catalin can clarify here, but I think we'd basically want a generic
> equivalent to arm_iommu_create_mapping and its associated halpers (i.e. the

Do you mean here that dma_iommu_mapping structure can be taken out to
incude/linux/iommu-helper.h ?
Since arm_iommu_create_mapping strongly uses this structure. But if
this is the case than it will be completely arm specific
implementation.

> dma_map_ops). The dma_map_ops will have generic parts (e.g. the interfaces
> to the iommu API) but also architecturally-specific parts (e.g. cache
> flushing for non-coherent devices), so that will need some thought.
>

Architecturally-specific i.e. cache flush operations are not just for
non-coherent devices. Irrespective of coherent/non-coherent dma_ops,
every time we allocate the buffer by __iommu_alloc_buffer, we call for
__dma_clear_buffer (which does cache_flushing operation).

Yes, but for non-coherent devices, during map_page/map_sg,  we do
cache maintenance/invalidate/clean.

> Once that's done, we can later look at hooking this into device-tree in
> order to set the correct ops each device.
>
> Will

Re: IOMMU DMA-mapping API for arm64 ?

[Marek Szyprowski]

Hello,

Just to note. I would like to merge some more patches to iommu support 
in dma-mapping framework soon, so before starting any work on creating 
common helper code please also check "[PATCH 0/2] arm: dma-mapping: add 
dynamic resize of IOVA bitmap" patches:
http://thread.gmane.org/gmane.linux.kernel.iommu/4222/

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

Thanks Marek for bringing this into notice. I did see them into mail
threads, but had forgotten about that.

Thanks
Ritesh



On Mon, Mar 3, 2014 at 3:34 PM, Marek Szyprowski
<m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hello,
>
> Just to note. I would like to merge some more patches to iommu support in
> dma-mapping framework soon, so before starting any work on creating common
> helper code please also check "[PATCH 0/2] arm: dma-mapping: add dynamic
> resize of IOVA bitmap" patches:
> http://thread.gmane.org/gmane.linux.kernel.iommu/4222/
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

Hi Guys,

Need again some help in deciding which all functions/Data structures
needs to be moved out from arch/arm/mm/dma-mapping to
lib/iommu-helper.c.

1. There is this dma_iommu_mapping structure defination in
arch/arm/include/asm/dma-iommu.h. In arm this structure is declared as
"mapping" which is part of dev_archdata structure, which is used for
IOVA management.
Now, does this make sense to move this structure out to
include/linux/iommu-helper.h ? And define a config like
CONFIG_IOMMU_USE_HELPER_MAPPING (default n) which will be used to,
have a variable("mapping") defined or not in dev_archdata of all
archs.

    a. With this IOVA management can be completely moved out
lib/iommu-helper.c. And as Arnd said(below) this will be
implementation specific and will not be visible to architectures in
the header file.
    b. buffer allocation can be either ways moved out to
lib/iommu-helper.c. Except the atomic allocations (as of 1st step
atleast) which is part of arch/arm/mm/dma-mapping.c

Taking this above step, following are functions can be moved out:

extern void __iommu_detach_device(struct device *dev);
extern int __iommu_attach_device(struct device *dev, struct
dma_iommu_mapping *mapping);
extern void __iommu_release_mapping(struct dma_iommu_mapping *mapping);
extern struct dma_iommu_mapping *__iommu_init_mapping(struct bus_type
*bus, dma_addr_t base, size_t size);

__iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
dma_addr_t iommu_coherent_map_page(struct device *dev, struct page
*page, unsigned long offset, size_t size, enum dma_data_direction dir,
struct dma_attrs *attrs)
void iommu_coherent_unmap_page(struct device *dev, dma_addr_t handle,
size_t size, enum dma_data_direction dir, struct dma_attrs *attrs)
int iommu_coherent_map_sg(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction dir, struct dma_attrs *attrs)

and there are some more...

Current users of this refactored code to lib/iommu-helper.c will be
arm and arm64.


2. Now, if we don't bring this structure outside, there will be little
code which can be moved out. Following functions are what I can think
of:

struct page **iommu_alloc_buffer(struct device *dev, size_t size,
gfp_t gfp, struct dma_attrs *attrs);
int iommu_free_buffer(struct device *dev, struct page **pages, size_t
size, struct dma_attrs *attrs);
int iommu_map_pages(struct iommu_domain *domain, struct page **pages,
dma_addr_t iova, size_t size);
int iommu_dma_direction_to_prot(enum dma_data_direction dir);
void *iommu_alloc_remap(struct page **pages, size_t size, unsigned
long flags, pgprot_t prot, const void *caller);
int iommu_mmap_pages_prot(struct vm_area_struct *vma, struct page
**pages, pgprot_t prot);
and also say iommu_map_sg (excluing alloc_iova).



This might sound like too much to ask, but because of many arch's
dev_archdata will be affected, its a bit confusing to take this
decision.
I felt its better to ask before and have a theoretical design ready,
instead of cribbing my head to redo the design after the incorrect
changes.


Please let me know your thought on this.



On Fri, Feb 28, 2014 at 12:36 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 27 February 2014, Ritesh Harjani wrote:
>> Hi Everyone,
>>
>> I was going through some iommu code in arch/arm and of some other
>> archs code. I have some doubts on this for refactoring and may need
>> some suggestions from you guys.
>>
>> 1. So, looking at other arch code, looks like they have their
>> different way of implementation of iova management and buffer
>> allocation. So to refactor the iommu common code out from arch/arm/ to
>> lib/iommu-helper, do we need to take care across all arch  ?
>
> I'd say not initially. The code can easily live in a generic place
> but not be shared by everyone. If it turns out that another architecture
> has a better allocator, then we can always change the common code
> later.
>
>> 2. Should the approach be like take the common code(between arm/arm64)
>> and move it into lib/iommu-helper.c ?
>>
>> Could someone give an example of what sort of code(will be better if
>> this is little more specific) we are talking here to be taken out to
>> lib/iommu-helper.c ? Earlier I was thinking of iova management can be
>> taken out but then I saw it might not be suited across all archs.
>>
>> I am ready to do this work, but need some guidance from the experts .
>
> I think we should start by splitting out the iommu_coherent_ops structure
> and everything that depends on. Noncoherent DMA is harder to do in
> an architecture independent way, since we don't have a common way
> to manage the cache across architectures. It would also be good
> to follow the example of include/linux/swiotlb.h regarding the
> public interface, to keep that part common. This way, ARM can easily
> implement the noncoherent ops on top.

I think we can follow step 1 above, and move most of code to
lib/iommu-helper.h. Currently anyways only arm and arm64 will be using
this code.
Later based on the need of different archs, this code can be modified ??


>
> I would leave the iova management as implementation details and
> not make that visible to architectures in the header file.
>
>         Arnd


Thanks
Ritesh

Re: IOMMU DMA-mapping API for arm64 ?

[Will Deacon]

On Sat, Apr 26, 2014 at 03:28:37PM +0100, Ritesh Harjani wrote:
> Hi Guys,
> 
> Need again some help in deciding which all functions/Data structures
> needs to be moved out from arch/arm/mm/dma-mapping to
> lib/iommu-helper.c.
> 
> 1. There is this dma_iommu_mapping structure defination in
> arch/arm/include/asm/dma-iommu.h. In arm this structure is declared as
> "mapping" which is part of dev_archdata structure, which is used for
> IOVA management.
> Now, does this make sense to move this structure out to
> include/linux/iommu-helper.h ? And define a config like
> CONFIG_IOMMU_USE_HELPER_MAPPING (default n) which will be used to,
> have a variable("mapping") defined or not in dev_archdata of all
> archs.

In that case, wouldn't it make more sense to put the DMA mapping information
in the struct device itself, rather than the dev_archdata? The CMA data, for
example, is stored directly there.

Will

Re: IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

On Tue, Apr 29, 2014 at 6:22 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Sat, Apr 26, 2014 at 03:28:37PM +0100, Ritesh Harjani wrote:
>> Hi Guys,
>>
>> Need again some help in deciding which all functions/Data structures
>> needs to be moved out from arch/arm/mm/dma-mapping to
>> lib/iommu-helper.c.
>>
>> 1. There is this dma_iommu_mapping structure defination in
>> arch/arm/include/asm/dma-iommu.h. In arm this structure is declared as
>> "mapping" which is part of dev_archdata structure, which is used for
>> IOVA management.
>> Now, does this make sense to move this structure out to
>> include/linux/iommu-helper.h ? And define a config like
>> CONFIG_IOMMU_USE_HELPER_MAPPING (default n) which will be used to,
>> have a variable("mapping") defined or not in dev_archdata of all
>> archs.
>
> In that case, wouldn't it make more sense to put the DMA mapping information
> in the struct device itself, rather than the dev_archdata? The CMA data, for
> example, is stored directly there.

Yes, thanks for pointing out. So, we will have mapping variable put in
device structure.

Will,

So, you think option 1 should be carried out ? This way we can
decouple arch specific things in arch/arm/mm/dma-mapping and other
stuff like IOVA management and buffer management in lib/iommu-helper.c
(thats what lib/swiotlb does too). Only thing to note here is that, we
will have only ARM as the current user of this helper functions.

Once this refactoring is done, later we can modify the
lib/iommu-helper.c functions to make more sense for other archs as
well. My only concern here was that whether we can take out this
dma_iommu_mapping structure. Because currently all archs have their
own way of implementing IOVA management and buffer allocation, but as
in some mail chain it was pointed out that as when the need arises
other arch can modify iommu-helper to suit their needs too.


>
> Will

Marek/Catalin,

Could you guys also comment on this please. If we agree on this
(option 1), then I can take it from there and refactoring should be
fairly straight forward.

Thanks
Ritesh

Re: IOMMU DMA-mapping API for arm64 ?

[Will Deacon]

On Thu, Feb 06, 2014 at 05:05:45AM +0000, Ritesh Harjani wrote:
> Hi everyone,
> 
> I tried looking for IOMMU support in ARM64 but what I was able to see is
> only swiotlb is currently supported.
> 
> Based on my understanding for IOMMU support, we need DMA-MAPPING API to
> have IOMMU ops field, similar to what is present in arm32.  I could see
> the iommu field added in dev_archdata in below mentioned patch
> (arch/arm64/include/asm/device.h), but there is no ops field in
> arch/arm64/mm/dma-mapping.c ?

Correct, there needs to be a way to plumb dma-mapping into the IOMMU layer.

> I also saw one mail discussion between you guys on what is the best place
> for adding iommu support in ARM64, but couldn't see any followed up
> patches for the same.
> 
> Please tell us the current status/updates on the same. Your feedback will
> be greatly appreciated.

There are a few related topics here:

  (1) Moving a bunch of the boilerplate IOMMU code into a common place
      (iommu-helper.c or similar). This is because we don't want to blindly
      duplicate common code across architectures.

  (2) Standardising at least part of a system topology description in
      device-tree, so that we can deal with different combinations of
      {coherent,nocoherent} device {with,without} a series of IOMMUs.

Once both of those are sorted, the actual plumbing should be fairly
straightforward.

Will

IOMMU DMA-mapping API for arm64 ?

[Will Deacon]

On Thu, Feb 06, 2014 at 05:05:45AM +0000, Ritesh Harjani wrote:
> Hi everyone,
> 
> I tried looking for IOMMU support in ARM64 but what I was able to see is
> only swiotlb is currently supported.
> 
> Based on my understanding for IOMMU support, we need DMA-MAPPING API to
> have IOMMU ops field, similar to what is present in arm32.  I could see
> the iommu field added in dev_archdata in below mentioned patch
> (arch/arm64/include/asm/device.h), but there is no ops field in
> arch/arm64/mm/dma-mapping.c ?

Correct, there needs to be a way to plumb dma-mapping into the IOMMU layer.

> I also saw one mail discussion between you guys on what is the best place
> for adding iommu support in ARM64, but couldn't see any followed up
> patches for the same.
> 
> Please tell us the current status/updates on the same. Your feedback will
> be greatly appreciated.

There are a few related topics here:

  (1) Moving a bunch of the boilerplate IOMMU code into a common place
      (iommu-helper.c or similar). This is because we don't want to blindly
      duplicate common code across architectures.

  (2) Standardising at least part of a system topology description in
      device-tree, so that we can deal with different combinations of
      {coherent,nocoherent} device {with,without} a series of IOMMUs.

Once both of those are sorted, the actual plumbing should be fairly
straightforward.

Will

IOMMU DMA-mapping API for arm64 ?

[Ritesh Harjani]

[-- Attachment #1.1: Type: text/plain, Size: 1434 bytes --]

Hi everyone,

I tried looking for IOMMU support in ARM64 but what I was able to see is
only swiotlb is currently supported.

Based on my understanding for IOMMU support, we need DMA-MAPPING API to
have IOMMU ops field, similar to what is present in arm32.
I could see the iommu field added in dev_archdata in below mentioned patch
(arch/arm64/include/asm/device.h), but there is no ops field in
arch/arm64/mm/dma-mapping.c ?

I also saw one mail discussion between you guys on what is the best place
for adding iommu support in ARM64, but couldn't see any followed up patches
for the same.

Please tell us the current status/updates on the same. Your feedback will
be greatly appreciated.


commit 73150c983ac1f9b7653cfd3823b1ad4a44aad3bf
Author: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Date:   Mon Jun 10 19:34:42 2013 +0100

    arm64: device: add iommu pointer to device archdata

    When using an IOMMU for device mappings, it is necessary to keep a
    pointer between the device and the IOMMU to which it is attached in
    order to obtain the correct IOMMU when attaching the device to a domain.

    This patch adds an iommu pointer to the dev_archdata structure, in a
    similar manner to other architectures (ARM, PowerPC, x86, ...).

    Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
    Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>


Thanks
Ritesh

[-- Attachment #1.2: Type: text/html, Size: 1932 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]