Re: [PATCH v3 02/12] iommu/vt-d: Add cache tag invalidation helpers

[Baolu Lu]

On 4/23/24 4:42 PM, Tian, Kevin wrote:
> > From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Monday, April 22, 2024 1:30 PM
> >
> > On 4/16/24 4:06 PM, Lu Baolu wrote:
> > > +		case CACHE_TAG_NESTING_DEVTLB:
> > > +			/*
> > > +			 * Address translation cache in device side caches the
> > > +			 * result of nested translation. There is no easy way
> > > +			 * to identify the exact set of nested translations
> > > +			 * affected by a change in S2. So just flush the entire
> > > +			 * device cache.
> > > +			 */
> > > +			addr = 0;
> > > +			mask = MAX_AGAW_PFN_WIDTH;
> > > +			fallthrough;
> > I realized that the logic above is not right. Setting both @addr and
> > @mask to 0 doesn't means flush all caches on the device. I will change
> > it like below:
> I didn't get. Above code doesn't set @mask to 0.

Oh!? I have no idea why I read that as "mask = 0" now. Perhaps my brain
was on vacation earlier. :-)

> > diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
> > index e8418cdd8331..18debb82272a 100644
> > --- a/drivers/iommu/intel/cache.c
> > +++ b/drivers/iommu/intel/cache.c
> > @@ -302,9 +302,14 @@ void cache_tag_flush_range(struct dmar_domain
> > *domain, unsigned long start,
> >                            * affected by a change in S2. So just flush
> > the entire
> >                            * device cache.
> >                            */
> > -                       addr = 0;
> > -                       mask = MAX_AGAW_PFN_WIDTH;
> > -                       fallthrough;
> > +                       info = dev_iommu_priv_get(tag->dev);
> > +                       sid = PCI_DEVID(info->bus, info->devfn);
> > +
> > +                       qi_flush_dev_iotlb(iommu, sid, info->pfsid,
> > info->ats_qdep,
> > +                                          0, MAX_AGAW_PFN_WIDTH);
> > +                       quirk_extra_dev_tlb_flush(info, 0,
> > MAX_AGAW_PFN_WIDTH,
> > +                                                 IOMMU_NO_PASID,
> > info->ats_qdep);
> > +                       break;
> and I didn't get this change. It goes backward by ignoring tag->pasid.
>
> what's the exact problem of the fallthrough logic in original code?

Sorry! Please ignore this.

Best regards,
baolu