[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <2d2b605f-3b95-4764-b7c3-005824709078@linux.intel.com>
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
Powered by blists - more mailing lists