[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Oleksandr <olekstysh@xxxxxxxxx>
  • From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Date: Thu, 30 Jan 2020 15:05:43 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Aw3Ijzentq43ukfviPkxnvC6jAs9ateRdmCpBvgXcVY=; b=PJpeZQBb5Mi/JqYL/aeX7DmTgMQOSF/dsMdEs2KVgAqf60/URmLkZ8ikL5/QR/U5f9uFCi6OfESunCVLXVNfJzZ7i/bFd2gx61pPTjzYoN7DOw/gaDAP1oerBzMERkr/wGE2d7Bta6L58M5GQpB9SA1JJILIBvyz4TAf+719/8wtRqtRvHCvZzhdfQCazm3xSAjt2OS/X2fVyUK5B9QxnAuTYlhNBQtwnpK3Qq1nM/X10VVeAkmer5ycU3zm8SK8bRGfDiddRocmpEJuletQYmcErPe6fq4Ff7/0deUz5pvsCulgWeGpHqYrLpnArTx8E9LLqoN2mmgWqy8qismSwg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kANK8Eh++UfJsA2sa6NWmfAzTzQEyYnz89U33W3C6a3DWNz5qcoFY7DNd+llFYeBNxHr2eg1N1QwUPlngpyKZRgmVKKIPakcNVnAseEekD/eXeFxBhFtIN2hnEDgWpnSrW9V/cisVUkcwziknh2tyiqylCss0pCgy/UpVhDSs/SPUT5q1B6PmqEjz6i9kJeUhrICxzzTaglQhl7X7sKZ8hMqUg1/i5Q8MnMZQ6nZyVYDwI2BKllmXoPRbkTAKa64zBORf4mxwxLKrqFLlqGkFAitBzDCPZT19MmxUDD1WyIuTRje2nw7ruc29g0bwvUsXyaDf4FB1C9awKBJM9lu9g==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Volodymyr_Babchuk@xxxxxxxx;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 30 Jan 2020 15:05:48 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHV1rQn7uvWmtBMUUCKekbfSzRsQagBzvuAgAF4XICAAAi8gA==
  • Thread-topic: [PATCH] iommu/arm: Don't allow the same micro-TLB to be shared between domains

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.