linux-kernel - Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 2 Jul 2024 14:25:05 +0800
From: Yi Liu <yi.l.liu@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, "Tian, Kevin" <kevin.tian@...el.com>,
	Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy
	<robin.murphy@....com>, Jason Gunthorpe <jgg@...pe.ca>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context
 change

On 2024/7/2 12:51, Baolu Lu wrote:
> On 2024/7/2 10:43, Baolu Lu wrote:
>> On 7/2/24 9:47 AM, Baolu Lu wrote:
>>> On 7/2/24 9:11 AM, Tian, Kevin wrote:
>>>>> From: Lu Baolu<baolu.lu@...ux.intel.com>
>>>>> Sent: Monday, July 1, 2024 7:23 PM
>>>>> +
>>>>> +    /*
>>>>> +     * For scalable mode:
>>>>> +     * - Domain-selective PASID-cache invalidation to affected domains
>>>>> +     * - Domain-selective IOTLB invalidation to affected domains
>>>>> +     * - Global Device-TLB invalidation to affected functions
>>>>> +     */
>>>>> +    if (flush_domains) {
>>>>> +        /*
>>>>> +         * If the IOMMU is running in scalable mode and there might
>>>>> +         * be potential PASID translations, the caller should hold
>>>>> +         * the lock to ensure that context changes and cache flushes
>>>>> +         * are atomic.
>>>>> +         */
>>>>> +        assert_spin_locked(&iommu->lock);
>>>>> +        for (i = 0; i < info->pasid_table->max_pasid; i++) {
>>>>> +            pte = intel_pasid_get_entry(info->dev, i);
>>>>> +            if (!pte || !pasid_pte_is_present(pte))
>>>>> +                continue;
>>>>> +
>>>>> +            did = pasid_get_domain_id(pte);
>>>>> +            qi_flush_pasid_cache(iommu, did,
>>>>> QI_PC_ALL_PASIDS, 0);
>>>>> +            iommu->flush.flush_iotlb(iommu, did, 0, 0,
>>>>> DMA_TLB_DSI_FLUSH);
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    __context_flush_dev_iotlb(info);
>>>>> +}
>>>> this only invalidates devtlb w/o PASID. We miss a pasid devtlb 
>>>> invalidation
>>>> with global bit set.
>>>
>>> I am not sure about this. The spec says "Global Device-TLB invalidation
>>> to affected functions", I am not sure whether this implies any PASID-
>>> based-Device-TLB invalidation.
>>
>> I just revisited the spec, Device-TLB invalidation only covers caches
>> for requests-without-PASID. If pasid translation is affected while
>> updating the context entry, we should also take care of the caches for
>> requests-with-pasid.
>>

hmmm. "Table 25. Guidance to Software for Invalidations" only mentions
global devTLB invalidation. 10.3.8 PASID and Global Invalidate of PCIe 6.2
spec has below definition.

For Invalidation Requests that do not have a PASID, the ATC shall:
• Invalidate ATC entries within the indicate memory range that were 
requested without a PASID value.
• Invalidate ATC entries at all addresses that were requested with any 
PASID value.

AFAIK. A devTLB invalidate descriptor submitted to VT-d should be converted
to be a PCIe ATC invalidation request without PASID prefix. So it may be
subjected to the above definition. If so, no need to have a PASID-based 
devTLB invalidation descriptor.

>> I will add below line to address this.
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 9a7b5668c723..91db0876682e 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -932,6 +932,7 @@ void intel_context_flush_present(struct 
>> device_domain_info *info,
>>                          did = pasid_get_domain_id(pte);
>>                          qi_flush_pasid_cache(iommu, did, 
>> QI_PC_ALL_PASIDS, 0);
>>                          iommu->flush.flush_iotlb(iommu, did, 0, 0, 
>> DMA_TLB_DSI_FLUSH);
>> +                       pasid_cache_invalidation_with_pasid(iommu, did, i);
> 
> Should be
> 
>      devtlb_invalidation_with_pasid(iommu, info->dev, i);
> 
> Sorry for the typo.
> 
> Best regards,
> baolu
> 

-- 
Regards,
Yi Liu