linux-kernel - Re: [PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in probe path

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] [day] [month] [year] [list]

Date: Mon, 4 Mar 2024 19:13:27 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: "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>, "Badger, Eric" <ebadger@...estorage.com>
Cc: baolu.lu@...ux.intel.com, "iommu@...ts.linux.dev"
 <iommu@...ts.linux.dev>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in
 probe path

On 2024/3/4 17:05, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@...ux.intel.com>
>> Sent: Monday, March 4, 2024 4:24 PM
>>
>> On 2024/3/4 15:48, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>>>> Sent: Thursday, February 29, 2024 5:48 PM
>>>>
>>>> +	/*
>>>> +	 * For kdump case, at this point, the device is supposed to finish
>>>> +	 * reset at its driver probe stage, so no in-flight DMA will exist,
>>>> +	 * and we don't need to worry anymore hereafter.
>>>> +	 */
>>>> +	if (context_copied(iommu, bus, devfn)) {
>>>> +		context_clear_entry(context);
>>>> +		if (!ecap_coherent(iommu->ecap))
>>>> +			clflush_cache_range(context, sizeof(*context));
>>>> +		sm_context_flush_caches(dev);
>>>> +		clear_context_copied(iommu, bus, devfn);
>>>> +	}
>>>
>>> it's unclear to me why this doesn't need refer to old did as done in the
>>> existing code. If scalable mode makes any difference could you extend
>>> the comment to explain so people can avoid similar confusion when
>>> comparing the different paths between legacy and sm?
>>
>> The previous code gets the domain ID from the copied context entry:
>>
>> u16 did_old = context_domain_id(context);
>>
>> This makes no sense for scalable mode, as the domain ID has been moved
>> to the PASID entry in scalable mode. As the result, did_old always gets
>> 0.
> 
> The point is whether the driver requires to invalidate cache for old did
> which is orthogonal to using legacy or sm. If yes, then we should fix the
> code to find the right did instead of ignoring it. If no then the legacy path
> should be cleared too to avoid unnecessary burden.
> 
>>
>>> anyway it's kind of a semantics change probably worth a separate patch
>>> to special case sm for bisect and then doing cleanup...
>>
>> This change doesn't impact anything as the hardware will skip domain id
>> field in the Context-cache Invalidate Descriptor in scalable mode.
>>
> 
> no semantics change but if old code has bug we should fix it instead
> of carrying the behavior. 😊

The driver is required to invalidate the cache for the old did.

The previous code invalidated the cache twice, once in
intel_pasid_tear_down_entry() and another time in
domain_context_clear().

The new code attempts to eliminate this duplication by invalidating the
cache for the did during blocking domain attachment and skipping it in
sm-context-entry teardown.

Best regards,
baolu