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

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

[Tomasz Figa]

To flush created mappings, current mapping code relies on the fact that
during unmap the driver zaps every IOVA being unmapped and that it is
enough to zap a single IOVA of page table to remove the entire page
table from IOMMU cache. Based on these assumptions the driver was made to
simply zap the first IOVA of the mapping being created. This is enough
to invalidate first page table, which could be shared with another
mapping (and thus could be already present in IOMMU cache), but
unfortunately it does not do anything about the last page table that
could be shared with other mappings as well.

Moreover, the flushing is performed before page table contents are
actually modified, so there is a race between the CPU updating the page
tables and hardware that could be possibly running at the same time and
triggering IOMMU look-ups, which could bring back the page tables back
to the cache.

To fix both issues, this patch makes the mapping code zap first and last
(if they are different) IOVAs of new mapping after the page table is
updated.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
 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 4015560..31004c0 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -551,6 +551,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)
 {
@@ -575,12 +584,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);
@@ -630,6 +633,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 */
-- 
2.2.0.rc0.207.ga3a616c

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

[Daniel Kurtz]

On Mon, Apr 20, 2015 at 7:43 PM, Tomasz Figa <tfiga@chromium.org> wrote:
> To flush created mappings, current mapping code relies on the fact that
> during unmap the driver zaps every IOVA being unmapped and that it is
> enough to zap a single IOVA of page table to remove the entire page
> table from IOMMU cache. Based on these assumptions the driver was made to
> simply zap the first IOVA of the mapping being created. This is enough
> to invalidate first page table, which could be shared with another
> mapping (and thus could be already present in IOMMU cache), but
> unfortunately it does not do anything about the last page table that
> could be shared with other mappings as well.
>
> Moreover, the flushing is performed before page table contents are
> actually modified, so there is a race between the CPU updating the page
> tables and hardware that could be possibly running at the same time and
> triggering IOMMU look-ups, which could bring back the page tables back
> to the cache.
>
> To fix both issues, this patch makes the mapping code zap first and last
> (if they are different) IOVAs of new mapping after the page table is
> updated.
>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>

You probably want to remove the "CHROMIUM: " label in the subject.
Other than that, this is still:
Reviewed-by: Daniel Kurtz <djkurtz@chromium.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 4015560..31004c0 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -551,6 +551,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)
>  {
> @@ -575,12 +584,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);
> @@ -630,6 +633,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 */
> --
> 2.2.0.rc0.207.ga3a616c
>

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

[Tomasz Figa]

On Thu, Apr 23, 2015 at 6:09 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Mon, Apr 20, 2015 at 7:43 PM, Tomasz Figa <tfiga@chromium.org> wrote:
>> To flush created mappings, current mapping code relies on the fact that
>> during unmap the driver zaps every IOVA being unmapped and that it is
>> enough to zap a single IOVA of page table to remove the entire page
>> table from IOMMU cache. Based on these assumptions the driver was made to
>> simply zap the first IOVA of the mapping being created. This is enough
>> to invalidate first page table, which could be shared with another
>> mapping (and thus could be already present in IOMMU cache), but
>> unfortunately it does not do anything about the last page table that
>> could be shared with other mappings as well.
>>
>> Moreover, the flushing is performed before page table contents are
>> actually modified, so there is a race between the CPU updating the page
>> tables and hardware that could be possibly running at the same time and
>> triggering IOMMU look-ups, which could bring back the page tables back
>> to the cache.
>>
>> To fix both issues, this patch makes the mapping code zap first and last
>> (if they are different) IOVAs of new mapping after the page table is
>> updated.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>
> You probably want to remove the "CHROMIUM: " label in the subject.

Sorry, I removed gerrit tags, but hurrying up too much, I missed the
label. I see, though, that Joerg has applied this patch already with
this fixed up. Thanks.

Best regards,
Tomasz