Re: [PATCH] CHROMIUM: iommu: rockchip: Make sure that page table state is coherent

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

On 23.03.15 10:38, Tomasz Figa wrote:
> Sorry, I had to dig my way out through my backlog.
> 
> On Tue, Mar 3, 2015 at 10:36 PM, Joerg Roedel <joro@8bytes.org> wrote:
>> On Mon, Feb 09, 2015 at 08:19:21PM +0900, Tomasz Figa wrote:
>>> Even though the code uses the dt_lock spin lock to serialize mapping
>>> operation from different threads, it does not protect from IOMMU
>>> accesses that might be already taking place and thus altering state
>>> of the IOTLB. This means that current mapping code which first zaps
>>> the page table and only then updates it with new mapping which is
>>> prone to mentioned race.
>>
>> Could you elabortate a bit on the race and why it is sufficient to zap
>> only the first and the last iova? From the description and the comments
>> in the patch this is not clear to me.
> 
> Let's start with why it's sufficient to zap only first and last iova.
> 
> While unmapping, the driver zaps all iovas belonging to the mapping,
> so the page tables not used by any mapping won't be cached. Now when
> the driver creates a mapping it might end up occupying several page
> tables. However, since the mapping area is virtually contiguous, only
> the first and last page table can be shared with different mappings.
> This means that only first and last iovas can be already cached. In
> fact, we could detect if first and last page tables are shared and do
> not zap at all, but this wouldn't really optimize too much. Why
> invalidating one iova is enough to invalidate the whole page table is
> unclear to me as well, but it seems to be the correct way on this
> hardware.

Hi,
It seems to me that actually each mapping needs exactly one page.
Since (as the inline doc in rk_iommu_map states) the pgsize_bitmap
makes sure that iova mappings fits exactly into one page table
since the mapping size is maximum 4M.

This actually means that if rk_dte_get_page_table does not allocate a
new page table but returns one that is already partially used from previous
mappings then two page tables might be required, but I think the iova
allocation somehow make sure that this will not be the case.

If it was the case then the code would be buggy because it means
that the loop in rk_iommu_map_iova will write behind the page table
given in rk_dte_get_page_table (which we didn't allocate)

So I it seems to me that calling 'rk_iommu_zap_iova(rk_domain, iova, SPAGE_SIZE);'
as done before this patch should be used, but be moved from
rk_dte_get_page_table to where rk_iommu_zap_iova_first_last is now

Thanks,
Dafna

> 
> As for the race, it's also kind of explained by the above. The already
> running hardware can trigger page table look-ups in the IOMMU and so
> caching of the page table between our zapping and updating its
> contents. With this patch zapping is performed after updating the page
> table so the race is gone.
> 
> Best regards,
> Tomasz
> 
>  From mboxrd@z Thu Jan  1 00:00:00 1970
> Return-Path: <linux-kernel-owner@vger.kernel.org>
> Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
> 	id S1753210AbbCWM3R (ORCPT <rfc822;w@1wt.eu>);
> 	Mon, 23 Mar 2015 08:29:17 -0400
> Received: from 8bytes.org ([81.169.241.247]:33957 "EHLO theia.8bytes.org"
> 	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
> 	id S1752552AbbCWM3M (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
> 	Mon, 23 Mar 2015 08:29:12 -0400
> Date: Mon, 23 Mar 2015 13:29:10 +0100
> From: Joerg Roedel <joro@8bytes.org>
> To: Tomasz Figa <tfiga@chromium.org>
> Cc: iommu@lists.linux-foundation.org,
>          "linux-arm-kernel@lists.infradead.org"
> 	<linux-arm-kernel@lists.infradead.org>,
>          "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
>          "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
>          Heiko Stuebner <heiko@sntech.de>, Daniel Kurtz <djkurtz@chromium.org>
> Subject: Re: [PATCH] CHROMIUM: iommu: rockchip: Make sure that page table
>   state is coherent
> Message-ID: <20150323122910.GO4441@8bytes.org>
> References: <1423480761-33453-1-git-send-email-tfiga@chromium.org>
>   <20150303133659.GD10502@8bytes.org>
>   <CAAFQd5Abk6X7AVTFaNuUSiShn31pzwwTE3VjfLnE4kyziAjy2A@mail.gmail.com>
> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> In-Reply-To: <CAAFQd5Abk6X7AVTFaNuUSiShn31pzwwTE3VjfLnE4kyziAjy2A@mail.gmail.com>
> User-Agent: Mutt/1.5.21 (2010-09-15)
> Sender: linux-kernel-owner@vger.kernel.org
> List-ID: <linux-kernel.vger.kernel.org>
> X-Mailing-List: linux-kernel@vger.kernel.org
> 
> Hi Tomasz,
> 
> On Mon, Mar 23, 2015 at 05:38:45PM +0900, Tomasz Figa wrote:
>> While unmapping, the driver zaps all iovas belonging to the mapping,
>> so the page tables not used by any mapping won't be cached. Now when
>> the driver creates a mapping it might end up occupying several page
>> tables. However, since the mapping area is virtually contiguous, only
>> the first and last page table can be shared with different mappings.
>> This means that only first and last iovas can be already cached. In
>> fact, we could detect if first and last page tables are shared and do
>> not zap at all, but this wouldn't really optimize too much. Why
>> invalidating one iova is enough to invalidate the whole page table is
>> unclear to me as well, but it seems to be the correct way on this
>> hardware.
>>
>> As for the race, it's also kind of explained by the above. The already
>> running hardware can trigger page table look-ups in the IOMMU and so
>> caching of the page table between our zapping and updating its
>> contents. With this patch zapping is performed after updating the page
>> table so the race is gone.
> 
> Okay, this makes sense. Can you add this information to the patch
> changelog and resend please?
> 
> Thanks,
> 
> 	Joerg
> 
> 
>  From mboxrd@z Thu Jan  1 00:00:00 1970
> From: Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Subject: [PATCH] CHROMIUM: iommu: rockchip: Make sure that page table state is
> 	coherent
> Date: Mon,  9 Feb 2015 20:19:21 +0900
> Message-ID: <1423480761-33453-1-git-send-email-tfiga@chromium.org>
> Mime-Version: 1.0
> Content-Type: text/plain; charset="us-ascii"
> Content-Transfer-Encoding: 7bit
> Return-path: <iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>
> List-Unsubscribe: <https://lists.linuxfoundation.org/mailman/options/iommu>,
> 	<mailto:iommu-request-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org?subject=unsubscribe>
> List-Archive: <http://lists.linuxfoundation.org/pipermail/iommu/>
> List-Post: <mailto:iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>
> List-Help: <mailto:iommu-request-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org?subject=help>
> List-Subscribe: <https://lists.linuxfoundation.org/mailman/listinfo/iommu>,
> 	<mailto:iommu-request-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org?subject=subscribe>
> Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> List-Id: iommu@lists.linux-foundation.org
> 
> Even though the code uses the dt_lock spin lock to serialize mapping
> operation from different threads, it does not protect from IOMMU
> accesses that might be already taking place and thus altering state
> of the IOTLB. This means that current mapping code which first zaps
> the page table and only then updates it with new mapping which is
> prone to mentioned race.
> 
> In addition, current code assumes that mappings are always > 4 MiB
> (which translates to 1024 PTEs) and so they would always occupy
> entire page tables. This is not true for mappings created by V4L2
> Videobuf2 DMA contig allocator.
> 
> This patch changes the mapping code to always zap the page table
> after it is updated, which avoids the aforementioned race and also
> zap the last page of the mapping to make sure that stale data is
> not cached from an already existing mapping.
> 
> Signed-off-by: Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Reviewed-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>   drivers/iommu/rockchip-iommu.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 6a8b1ec..b06fe76 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -544,6 +544,15 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain,
>   	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
>   }
>   
> +static void rk_iommu_zap_iova_first_last(struct rk_iommu_domain *rk_domain,
> +					 dma_addr_t iova, size_t size)
> +{
> +	rk_iommu_zap_iova(rk_domain, iova, SPAGE_SIZE);
> +	if (size > SPAGE_SIZE)
> +		rk_iommu_zap_iova(rk_domain, iova + size - SPAGE_SIZE,
> +					SPAGE_SIZE);
> +}
> +
>   static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
>   				  dma_addr_t iova)
>   {
> @@ -568,12 +577,6 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
>   	rk_table_flush(page_table, NUM_PT_ENTRIES);
>   	rk_table_flush(dte_addr, 1);
>   
> -	/*
> -	 * Zap the first iova of newly allocated page table so iommu evicts
> -	 * old cached value of new dte from the iotlb.
> -	 */
> -	rk_iommu_zap_iova(rk_domain, iova, SPAGE_SIZE);
> -
>   done:
>   	pt_phys = rk_dte_pt_address(dte);
>   	return (u32 *)phys_to_virt(pt_phys);
> @@ -623,6 +626,14 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr,
>   
>   	rk_table_flush(pte_addr, pte_count);
>   
> +	/*
> +	 * Zap the first and last iova to evict from iotlb any previously
> +	 * mapped cachelines holding stale values for its dte and pte.
> +	 * We only zap the first and last iova, since only they could have
> +	 * dte or pte shared with an existing mapping.
> +	 */
> +	rk_iommu_zap_iova_first_last(rk_domain, iova, size);
> +
>   	return 0;
>   unwind:
>   	/* Unmap the range of iovas that we just mapped */
>