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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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