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

[Yi Liu]

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@xxxxxxxxxxxxxxx>
> > > > > 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