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
