Re: [Xen-devel] [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains

[Oleksandr <olekstysh@xxxxxxxxx>]

> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel