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

[Tian, Kevin]

> 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.

> 
> 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?

>                  case CACHE_TAG_DEVTLB:
>                          info = dev_iommu_priv_get(tag->dev);
>                          sid = PCI_DEVID(info->bus, info->devfn);
> 
> > +		case CACHE_TAG_DEVTLB:
> > +			info = dev_iommu_priv_get(tag->dev);
> > +			sid = PCI_DEVID(info->bus, info->devfn);
> > +
> > +			if (tag->pasid == IOMMU_NO_PASID)
> > +				qi_flush_dev_iotlb(iommu, sid, info->pfsid,
> > +						   info->ats_qdep, addr,
> mask);
> > +			else
> > +				qi_flush_dev_iotlb_pasid(iommu, sid, info-
> >pfsid,
> > +							 tag->pasid, info-
> >ats_qdep,
> > +							 addr, mask);
> > +
> > +			quirk_extra_dev_tlb_flush(info, addr, mask, tag-
> >pasid, info->ats_qdep);
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&domain->cache_lock, flags);
> 
> Best regards,
> baolu