xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains
@ 2020-01-29 14:55 Oleksandr Tyshchenko
  2020-01-29 16:07 ` Volodymyr Babchuk
  0 siblings, 1 reply; 5+ messages in thread
From: Oleksandr Tyshchenko @ 2020-01-29 14:55 UTC (permalink / raw)
  To: xen-devel
  Cc: Oleksandr Tyshchenko, Yoshihiro Shimoda, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

For the IPMMU-VMSA we need to prevent the use cases where devices
which use the same micro-TLB are assigned to different Xen domains
(micro-TLB cannot be shared between multiple Xen domains, since it
points to the context bank to use for the page walk).

As each Xen domain uses individual context bank pointed by context_id,
we can potentially recognize that use case by comparing current and new
context_id for the already enabled micro-TLB and prevent different
context bank from being set.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

---

CC: Julien Grall <julien@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
CC: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

---
 xen/drivers/passthrough/arm/ipmmu-vmsa.c | 44 +++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index 9cfae7e..c21d2d7 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -257,6 +257,7 @@ static DEFINE_SPINLOCK(ipmmu_devices_lock);
 #define IMUCTR_TTSEL_MMU(n)    ((n) << 4)
 #define IMUCTR_TTSEL_PMB       (8 << 4)
 #define IMUCTR_TTSEL_MASK      (15 << 4)
+#define IMUCTR_TTSEL_SHIFT     4
 #define IMUCTR_FLUSH           (1 << 1)
 #define IMUCTR_MMUEN           (1 << 0)
 
@@ -434,19 +435,45 @@ static void ipmmu_tlb_invalidate(struct ipmmu_vmsa_domain *domain)
 }
 
 /* Enable MMU translation for the micro-TLB. */
-static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
-                              unsigned int utlb)
+static int ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
+                             unsigned int utlb)
 {
     struct ipmmu_vmsa_device *mmu = domain->mmu;
+    uint32_t data;
+
+    /*
+     * We need to prevent the use cases where devices which use the same
+     * micro-TLB are assigned to different Xen domains (micro-TLB cannot be
+     * shared between multiple Xen domains, since it points to the context bank
+     * to use for the page walk).
+     * As each Xen domain uses individual context bank pointed by context_id,
+     * we can potentially recognize that use case by comparing current and new
+     * context_id for already enabled micro-TLB and prevent different context
+     * bank from being set.
+     */
+    data = ipmmu_read(mmu, IMUCTR(utlb));
+    if ( data & IMUCTR_MMUEN )
+    {
+        unsigned int context_id;
+
+        context_id = (data & IMUCTR_TTSEL_MASK) >> IMUCTR_TTSEL_SHIFT;
+        if ( domain->context_id != context_id )
+        {
+            dev_err(mmu->dev, "Micro-TLB %u already assigned to IPMMU context %u\n",
+                    utlb, context_id);
+            return -EINVAL;
+        }
+    }
 
     /*
      * TODO: Reference-count the micro-TLB as several bus masters can be
-     * connected to the same micro-TLB. Prevent the use cases where
-     * the same micro-TLB could be shared between multiple Xen domains.
+     * connected to the same micro-TLB.
      */
     ipmmu_write(mmu, IMUASID(utlb), 0);
-    ipmmu_write(mmu, IMUCTR(utlb), ipmmu_read(mmu, IMUCTR(utlb)) |
+    ipmmu_write(mmu, IMUCTR(utlb), data |
                 IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_MMUEN);
+
+    return 0;
 }
 
 /* Disable MMU translation for the micro-TLB. */
@@ -671,7 +698,12 @@ static int ipmmu_attach_device(struct ipmmu_vmsa_domain *domain,
         dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
 
     for ( i = 0; i < fwspec->num_ids; ++i )
-        ipmmu_utlb_enable(domain, fwspec->ids[i]);
+    {
+        int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);
+
+        if ( ret )
+            return ret;
+    }
 
     return 0;
 }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains
  2020-01-29 14:55 [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains Oleksandr Tyshchenko
@ 2020-01-29 16:07 ` Volodymyr Babchuk
  2020-01-30 14:34   ` Oleksandr
  0 siblings, 1 reply; 5+ messages in thread
From: Volodymyr Babchuk @ 2020-01-29 16:07 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: Stefano Stabellini, Julien Grall, Yoshihiro Shimoda,
	Oleksandr Tyshchenko, xen-devel, Volodymyr Babchuk


Hi Oleksandr,

Oleksandr Tyshchenko writes:

[...]
> @@ -434,19 +435,45 @@ static void ipmmu_tlb_invalidate(struct ipmmu_vmsa_domain *domain)
>  }
>  
>  /* Enable MMU translation for the micro-TLB. */
> -static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
> -                              unsigned int utlb)
> +static int ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
> +                             unsigned int utlb)
>  {
>      struct ipmmu_vmsa_device *mmu = domain->mmu;
> +    uint32_t data;
Just nitpicking: I believe, that "imuctr" is better name than "data".

> +
> +    /*
> +     * We need to prevent the use cases where devices which use the same
> +     * micro-TLB are assigned to different Xen domains (micro-TLB cannot be
> +     * shared between multiple Xen domains, since it points to the context bank
> +     * to use for the page walk).
> +     * As each Xen domain uses individual context bank pointed by context_id,
> +     * we can potentially recognize that use case by comparing current and new
> +     * context_id for already enabled micro-TLB and prevent different context
> +     * bank from being set.
> +     */
> +    data = ipmmu_read(mmu, IMUCTR(utlb));
I can see that this code is not covered by spinlock. So, I believe,
there can be a race comdition, when this register is being read on two
CPUs simultaneously.

> +    if ( data & IMUCTR_MMUEN )
> +    {
> +        unsigned int context_id;
> +
> +        context_id = (data & IMUCTR_TTSEL_MASK) >> IMUCTR_TTSEL_SHIFT;
> +        if ( domain->context_id != context_id )
> +        {
> +            dev_err(mmu->dev, "Micro-TLB %u already assigned to IPMMU context %u\n",
> +                    utlb, context_id);
> +            return -EINVAL;
> +        }
> +    }
>  
>      /*
>       * TODO: Reference-count the micro-TLB as several bus masters can be
> -     * connected to the same micro-TLB. Prevent the use cases where
> -     * the same micro-TLB could be shared between multiple Xen domains.
> +     * connected to the same micro-TLB.
>       */
>      ipmmu_write(mmu, IMUASID(utlb), 0);
> -    ipmmu_write(mmu, IMUCTR(utlb), ipmmu_read(mmu, IMUCTR(utlb)) |
> +    ipmmu_write(mmu, IMUCTR(utlb), data |
>                  IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_MMUEN);
> +
> +    return 0;
>  }
>  
>  /* Disable MMU translation for the micro-TLB. */
> @@ -671,7 +698,12 @@ static int ipmmu_attach_device(struct ipmmu_vmsa_domain *domain,
>          dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
>  
>      for ( i = 0; i < fwspec->num_ids; ++i )
> -        ipmmu_utlb_enable(domain, fwspec->ids[i]);
> +    {
> +        int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);
> +
> +        if ( ret )
> +            return ret;
I can't see error path where ipmmu_utlb_disable() would be called for
already enable uTLBs. Is this normal?

> +    }
>  
>      return 0;
>  }


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains
  2020-01-29 16:07 ` Volodymyr Babchuk
@ 2020-01-30 14:34   ` Oleksandr
  2020-01-30 15:05     ` Volodymyr Babchuk
  0 siblings, 1 reply; 5+ messages in thread
From: Oleksandr @ 2020-01-30 14:34 UTC (permalink / raw)
  To: Volodymyr Babchuk
  Cc: xen-devel, Yoshihiro Shimoda, Stefano Stabellini, Julien Grall,
	Oleksandr Tyshchenko


> Hi Oleksandr,

Hi Volodymyr


>> @@ -434,19 +435,45 @@ static void ipmmu_tlb_invalidate(struct ipmmu_vmsa_domain *domain)
>>   }
>>   
>>   /* Enable MMU translation for the micro-TLB. */
>> -static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
>> -                              unsigned int utlb)
>> +static int ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
>> +                             unsigned int utlb)
>>   {
>>       struct ipmmu_vmsa_device *mmu = domain->mmu;
>> +    uint32_t data;
> Just nitpicking: I believe, that "imuctr" is better name than "data".

Agree, will rename


>
>> +
>> +    /*
>> +     * We need to prevent the use cases where devices which use the same
>> +     * micro-TLB are assigned to different Xen domains (micro-TLB cannot be
>> +     * shared between multiple Xen domains, since it points to the context bank
>> +     * to use for the page walk).
>> +     * As each Xen domain uses individual context bank pointed by context_id,
>> +     * we can potentially recognize that use case by comparing current and new
>> +     * context_id for already enabled micro-TLB and prevent different context
>> +     * bank from being set.
>> +     */
>> +    data = ipmmu_read(mmu, IMUCTR(utlb));
> I can see that this code is not covered by spinlock. So, I believe,
> there can be a race comdition, when this register is being read on two
> CPUs simultaneously.

I don't think, ipmmu_assign(deassign)_device callbacks take a spinlock, 
so the micro-TLB management routines inside
are protected.


>   
>   /* Disable MMU translation for the micro-TLB. */
> @@ -671,7 +698,12 @@ static int ipmmu_attach_device(struct ipmmu_vmsa_domain *domain,
>           dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
>   
>       for ( i = 0; i < fwspec->num_ids; ++i )
> -        ipmmu_utlb_enable(domain, fwspec->ids[i]);
> +    {
> +        int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);
> +
> +        if ( ret )
> +            return ret;
> I can't see error path where ipmmu_utlb_disable() would be called for
> already enable uTLBs. Is this normal?

Good question. Indeed, we need to restore previous state in case of error.


I will add the following:

diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index c21d2d7..411fc0f 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -702,7 +702,12 @@ static int ipmmu_attach_device(struct 
ipmmu_vmsa_domain *domain,
          int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);

          if ( ret )
+        {
+            while ( i-- )
+                ipmmu_utlb_disable(domain, fwspec->ids[i]);
+
              return ret;
+        }
      }

      return 0;


-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains
  2020-01-30 14:34   ` Oleksandr
@ 2020-01-30 15:05     ` Volodymyr Babchuk
  2020-01-30 15:20       ` Volodymyr Babchuk
  0 siblings, 1 reply; 5+ messages in thread
From: Volodymyr Babchuk @ 2020-01-30 15:05 UTC (permalink / raw)
  To: Oleksandr
  Cc: Stefano Stabellini, Julien Grall, Yoshihiro Shimoda,
	Oleksandr Tyshchenko, xen-devel, Volodymyr Babchuk


Hi Oleksandr,

Oleksandr writes:

[...]

>>> +
>>> +    /*
>>> +     * We need to prevent the use cases where devices which use the same
>>> +     * micro-TLB are assigned to different Xen domains (micro-TLB cannot be
>>> +     * shared between multiple Xen domains, since it points to the context bank
>>> +     * to use for the page walk).
>>> +     * As each Xen domain uses individual context bank pointed by context_id,
>>> +     * we can potentially recognize that use case by comparing current and new
>>> +     * context_id for already enabled micro-TLB and prevent different context
>>> +     * bank from being set.
>>> +     */
>>> +    data = ipmmu_read(mmu, IMUCTR(utlb));
>> I can see that this code is not covered by spinlock. So, I believe,
>> there can be a race comdition, when this register is being read on two
>> CPUs simultaneously.
>
> I don't think, ipmmu_assign(deassign)_device callbacks take a
> spinlock, so the micro-TLB management routines inside
> are protected.
Yeah, you are right. Somehow I missed this when checked the code yesterday.

>
>>     /* Disable MMU translation for the micro-TLB. */
>> @@ -671,7 +698,12 @@ static int ipmmu_attach_device(struct ipmmu_vmsa_domain *domain,
>>           dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
>>         for ( i = 0; i < fwspec->num_ids; ++i )
>> -        ipmmu_utlb_enable(domain, fwspec->ids[i]);
>> +    {
>> +        int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);
>> +
>> +        if ( ret )
>> +            return ret;
>> I can't see error path where ipmmu_utlb_disable() would be called for
>> already enable uTLBs. Is this normal?
>
> Good question. Indeed, we need to restore previous state in case of error.
>
>
> I will add the following:
>
> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> index c21d2d7..411fc0f 100644
> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> @@ -702,7 +702,12 @@ static int ipmmu_attach_device(struct
> ipmmu_vmsa_domain *domain,
>          int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);
>
>          if ( ret )
> +        {
> +            while ( i-- )
You will call
> +                ipmmu_utlb_disable(domain, fwspec->ids[i]);
for uTLB that caused the error. Likely, this uTLB right now is assigned
for another domain. So, you will disable active uTLB which belongs to
that domain. 

> +
>              return ret;
> +        }
>      }
>
>      return 0;


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains
  2020-01-30 15:05     ` Volodymyr Babchuk
@ 2020-01-30 15:20       ` Volodymyr Babchuk
  0 siblings, 0 replies; 5+ messages in thread
From: Volodymyr Babchuk @ 2020-01-30 15:20 UTC (permalink / raw)
  To: Volodymyr Babchuk
  Cc: Stefano Stabellini, Julien Grall, Yoshihiro Shimoda, Oleksandr,
	Oleksandr Tyshchenko, xen-devel



Volodymyr Babchuk writes:

Oleksandr,

[...]
>> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> index c21d2d7..411fc0f 100644
>> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> @@ -702,7 +702,12 @@ static int ipmmu_attach_device(struct
>> ipmmu_vmsa_domain *domain,
>>          int ret = ipmmu_utlb_enable(domain, fwspec->ids[i]);
>>
>>          if ( ret )
>> +        {
>> +            while ( i-- )
> You will call
>> +                ipmmu_utlb_disable(domain, fwspec->ids[i]);
> for uTLB that caused the error. Likely, this uTLB right now is assigned
> for another domain. So, you will disable active uTLB which belongs to
> that domain. 

Please disregard this. I realized that i-- will ensure that right index
will be used. Sorry for the noise.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-01-30 15:20 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-29 14:55 [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains Oleksandr Tyshchenko
2020-01-29 16:07 ` Volodymyr Babchuk
2020-01-30 14:34   ` Oleksandr
2020-01-30 15:05     ` Volodymyr Babchuk
2020-01-30 15:20       ` Volodymyr Babchuk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).