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