Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi all,

On Friday 14 March 2014 20:49:56 Suman Anna wrote:
> On 03/14/2014 12:51 PM, Santosh Shilimkar wrote:
> > On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
> >> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
> >>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
> >>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> >>>>> The page table entries must be cleaned from the cache before being
> >>>>> accessed by the IOMMU. Instead of implementing cache management
> >>>>> manually (and ignoring L2 cache), use clean_dcache_area() to make sure
> >>>>> the entries are visible to the device.
> >>>> 
> >>>> Thanks for fixing this, this has been long pending. There have been
> >>>> some previous attempts at this as well by Ramesh Gupta, with the last
> >>>> thread (a long time now) being
> >>>> http://marc.info/?t=134752035400001&r=1&w=2
> >>>> 
> >>>> Santosh,
> >>>> Can you please take a look at this patch and provide your comments?
> >>>> 
> >>>> regards
> >>>> Suman
> >>>> 
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/iommu/omap-iommu.c | 41 ++++++++++---------------------------
> >>>>>  1 file changed, 10 insertions(+), 31 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> >>>>> index a893eca..bb605c9 100644
> >>>>> --- a/drivers/iommu/omap-iommu.c
> >>>>> +++ b/drivers/iommu/omap-iommu.c
> >>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
> >>>>>    /*
> >>>>>     *	H/W pagetable operations
> >>>>>     */
> >>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
> >>>>> +static void flush_pgtable(void *addr, size_t size)
> >>>>>  {
> >>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
> >>>>> -	do {
> >>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
> >>>>> -		    : : "r" (first));
> >>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
> >>>>> -	} while (first <= last);
> >>>>> -}
> >>>>> -
> >>>>> -static void flush_iopte_range(u32 *first, u32 *last)
> >>>>> -{
> >>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
> >>>>> -	do {
> >>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
> >>>>> -		    : : "r" (first));
> >>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
> >>>>> -	} while (first <= last);
> >>>>> +	clean_dcache_area(addr, size);
> >>> 
> >>> I remember NAKing this approach in past and my stand remains same.
> >>> The cache APIs which you are trying to use here are not suppose
> >>> to be used outside.
> 
> So this particular change has a long history (have to dig through to educate
> myself).

Thank you for doing so, the result is pretty informative.

> I have not seen clean_dcache_area attempted before, and I wasn't sure myself
> it it can be used here. Ramesh and Fernando definitely did start out with
> dmac_flush_range and outer_flush_range which was definitely nacked [1].
> 
> >> Please note that the omap-iommu driver already uses clean_dcache_area().
> >> That's where I got the idea :-)
> > 
> > So that fall through the cracks then ;-)
> > 
> >>> I think the right way to fix this is to make use of streaming APIs.
> >>> If needed, IOMMU can have its own dma_ops for special case
> >>> handling if any.
> >> 
> >> I can replace clean_dcache_area() with dma_map_page() as done by the
> >> arm-smmu driver. If that's fine I'll modify this patch accordingly in
> >> v2.
> 
> Ramesh had also attempted to use dma_page_single() previously [2], and
> Russell has instead suggested to extend the cpu cache operations [3].
> Ramesh had worked based on this suggestion and the series reached v6 [4]
> (same as link I mentioned above) and this is the last attempt on this,
> but the thread went silent.

I'm not sure yet whether I totally agree with Russell here, not on the 
rationale of his opinion, but on what we're trying to achieve. Isn't the IOMMU 
just a device that can perform direct memory access to fetch page table 
entries ? It seems to me that what the driver needs is to make sure that 
changes to the page tables are visible to the device when performing direct 
memory access. That doesn't really differ from other DMA use cases, so why 
would it need an IOMMU-specific API ? Or does the OMAP IOMMU fetch page table 
entries using a special kind of DMA ?

> I am wondering if that is still valid and is the right way to go, as this
> seems to be a common problem. I do see dmac_flush_range being used for
> similar purposes in msm_iommu.c and exynos-iommu.c, so looks like they also
> fell through the cracks.
> 
> Laurent,
> Can you drop this patch out from v2 so that it is not clubbed with the small
> cleanup patches, and we can track this separately?

Sure, I'll do that.

> [1] http://marc.info/?l=linux-omap&m=129907009019355&w=2
> [2] http://marc.info/?t=130281804900005&r=1&w=2
> [3] http://marc.info/?l=linux-omap&m=131310179423214&w=2
> [4] http://marc.info/?t=134752035400001&r=1&w=2

-- 
Regards,

Laurent Pinchart