[4/5] iommu/rockchip: add ARM64 cache flush operation for iommu
diff mbox

Message ID 1463967439-13354-5-git-send-email-zhengsq@rock-chips.com
State New
Headers show

Commit Message

Shunqian Zheng May 23, 2016, 1:37 a.m. UTC
From: Simon <xxm@rock-chips.com>

Signed-off-by: Simon <xxm@rock-chips.com>
---
 drivers/iommu/rockchip-iommu.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Robin Murphy May 23, 2016, 10:44 a.m. UTC | #1
On 23/05/16 02:37, Shunqian Zheng wrote:
> From: Simon <xxm@rock-chips.com>
>
> Signed-off-by: Simon <xxm@rock-chips.com>
> ---
>   drivers/iommu/rockchip-iommu.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 043d18c..1741b65 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -95,12 +95,16 @@ struct rk_iommu {
>
>   static inline void rk_table_flush(u32 *va, unsigned int count)
>   {
> +#if defined(CONFIG_ARM)
>   	phys_addr_t pa_start = virt_to_phys(va);
>   	phys_addr_t pa_end = virt_to_phys(va + count);
>   	size_t size = pa_end - pa_start;
>
>   	__cpuc_flush_dcache_area(va, size);
>   	outer_flush_range(pa_start, pa_end);
> +#elif defined(CONFIG_ARM64)
> +	__dma_flush_range(va, va + count);
> +#endif

Ugh, please don't use arch-private cache maintenance functions directly 
from a driver. Allocating/mapping page tables to be read by the IOMMU is 
still DMA, so using the DMA APIs is the correct way to manage them, 
*especially* if it needs to work across multiple architectures.

Robin.

>   }
>
>   static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)
>
Catalin Marinas May 23, 2016, 1:35 p.m. UTC | #2
On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
> On 23/05/16 02:37, Shunqian Zheng wrote:
> >From: Simon <xxm@rock-chips.com>
> >
> >Signed-off-by: Simon <xxm@rock-chips.com>
> >---
> >  drivers/iommu/rockchip-iommu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> >index 043d18c..1741b65 100644
> >--- a/drivers/iommu/rockchip-iommu.c
> >+++ b/drivers/iommu/rockchip-iommu.c
> >@@ -95,12 +95,16 @@ struct rk_iommu {
> >
> >  static inline void rk_table_flush(u32 *va, unsigned int count)
> >  {
> >+#if defined(CONFIG_ARM)
> >  	phys_addr_t pa_start = virt_to_phys(va);
> >  	phys_addr_t pa_end = virt_to_phys(va + count);
> >  	size_t size = pa_end - pa_start;
> >
> >  	__cpuc_flush_dcache_area(va, size);
> >  	outer_flush_range(pa_start, pa_end);
> >+#elif defined(CONFIG_ARM64)
> >+	__dma_flush_range(va, va + count);
> >+#endif
> 
> Ugh, please don't use arch-private cache maintenance functions directly from
> a driver. Allocating/mapping page tables to be read by the IOMMU is still
> DMA, so using the DMA APIs is the correct way to manage them, *especially*
> if it needs to work across multiple architectures.

I fully agree, these functions should not be used in drivers.
Shunqian Zheng May 24, 2016, 2:31 a.m. UTC | #3
Catalin, Robin,

On 2016年05月23日 21:35, Catalin Marinas wrote:
> On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
>> On 23/05/16 02:37, Shunqian Zheng wrote:
>>> From: Simon <xxm@rock-chips.com>
>>>
>>> Signed-off-by: Simon <xxm@rock-chips.com>
>>> ---
>>>   drivers/iommu/rockchip-iommu.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>>> index 043d18c..1741b65 100644
>>> --- a/drivers/iommu/rockchip-iommu.c
>>> +++ b/drivers/iommu/rockchip-iommu.c
>>> @@ -95,12 +95,16 @@ struct rk_iommu {
>>>
>>>   static inline void rk_table_flush(u32 *va, unsigned int count)
>>>   {
>>> +#if defined(CONFIG_ARM)
>>>   	phys_addr_t pa_start = virt_to_phys(va);
>>>   	phys_addr_t pa_end = virt_to_phys(va + count);
>>>   	size_t size = pa_end - pa_start;
>>>
>>>   	__cpuc_flush_dcache_area(va, size);
>>>   	outer_flush_range(pa_start, pa_end);
>>> +#elif defined(CONFIG_ARM64)
>>> +	__dma_flush_range(va, va + count);
>>> +#endif
>> Ugh, please don't use arch-private cache maintenance functions directly from
>> a driver. Allocating/mapping page tables to be read by the IOMMU is still
>> DMA, so using the DMA APIs is the correct way to manage them, *especially*
>> if it needs to work across multiple architectures.
It's easier for us if changing  the __dma_flush_range() to 
__flush_dcache_area() is acceptable here?

Thank you,
- shunqian
> I fully agree, these functions should not be used in drivers.
Catalin Marinas May 24, 2016, 9:59 a.m. UTC | #4
On Tue, May 24, 2016 at 10:31:17AM +0800, Shunqian Zheng wrote:
> On 2016年05月23日 21:35, Catalin Marinas wrote:
> >On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
> >>On 23/05/16 02:37, Shunqian Zheng wrote:
> >>>From: Simon <xxm@rock-chips.com>
> >>>
> >>>Signed-off-by: Simon <xxm@rock-chips.com>
> >>>---
> >>>  drivers/iommu/rockchip-iommu.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>>diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> >>>index 043d18c..1741b65 100644
> >>>--- a/drivers/iommu/rockchip-iommu.c
> >>>+++ b/drivers/iommu/rockchip-iommu.c
> >>>@@ -95,12 +95,16 @@ struct rk_iommu {
> >>>
> >>>  static inline void rk_table_flush(u32 *va, unsigned int count)
> >>>  {
> >>>+#if defined(CONFIG_ARM)
> >>>  	phys_addr_t pa_start = virt_to_phys(va);
> >>>  	phys_addr_t pa_end = virt_to_phys(va + count);
> >>>  	size_t size = pa_end - pa_start;
> >>>
> >>>  	__cpuc_flush_dcache_area(va, size);
> >>>  	outer_flush_range(pa_start, pa_end);
> >>>+#elif defined(CONFIG_ARM64)
> >>>+	__dma_flush_range(va, va + count);
> >>>+#endif
> >>Ugh, please don't use arch-private cache maintenance functions directly from
> >>a driver. Allocating/mapping page tables to be read by the IOMMU is still
> >>DMA, so using the DMA APIs is the correct way to manage them, *especially*
> >>if it needs to work across multiple architectures.
> 
> It's easier for us if changing  the __dma_flush_range() to
> __flush_dcache_area() is acceptable here?

It's not really acceptable for arm64, nor for arm32. Please fix this
driver in a similar way to commit e3c971960fd4 ("iommu/tegra-smmu:
Convert to use DMA API").

The only place where we allowed __flush_dcache_area() is in the GICv3
driver and that's because it hasn't been wired as a platform device
(yet).

Patch
diff mbox

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 043d18c..1741b65 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -95,12 +95,16 @@  struct rk_iommu {
 
 static inline void rk_table_flush(u32 *va, unsigned int count)
 {
+#if defined(CONFIG_ARM)
 	phys_addr_t pa_start = virt_to_phys(va);
 	phys_addr_t pa_end = virt_to_phys(va + count);
 	size_t size = pa_end - pa_start;
 
 	__cpuc_flush_dcache_area(va, size);
 	outer_flush_range(pa_start, pa_end);
+#elif defined(CONFIG_ARM64)
+	__dma_flush_range(va, va + count);
+#endif
 }
 
 static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)