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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/3/4 17:05, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Monday, March 4, 2024 4:24 PM
>>
>> On 2024/3/4 15:48, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.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