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