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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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