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

[Baolu Lu]

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