diff mbox series

iommu/arm: Don't allow the same micro-TLB to be shared between domains

Message ID 1580309714-21912-1-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series iommu/arm: Don't allow the same micro-TLB to be shared between domains | expand

Commit Message

Oleksandr Tyshchenko Jan. 29, 2020, 2:55 p.m. UTC 
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(-)

Comments

Volodymyr Babchuk Jan. 29, 2020, 4:07 p.m. UTC | #1 
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;
>  }
Oleksandr Tyshchenko Jan. 30, 2020, 2:34 p.m. UTC | #2 
> 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;
Volodymyr Babchuk Jan. 30, 2020, 3:05 p.m. UTC | #3 
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
Volodymyr Babchuk Jan. 30, 2020, 3:20 p.m. UTC | #4 
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.
diff mbox series

Patch

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;
 }