Re: [PATCH v3 5/7] iommu/vt-d: Fix devTLB flush for vSVA

[Jacob Pan]

On Thu, 2 Jul 2020 10:39:26 +0200
Auger Eric <eric.auger@xxxxxxxxxx> wrote:

> Hi Jacob, Yi,
> On 7/1/20 5:33 PM, Jacob Pan wrote:
> > From: Liu Yi L <yi.l.liu@xxxxxxxxx>
> > 
> > For guest SVA usage, in order to optimize for less VMEXIT, guest
> > request of IOTLB flush also includes device TLB.
> > 
> > On the host side, IOMMU driver performs IOTLB and implicit devTLB
> > invalidation. When PASID-selective granularity is requested by the
> > guest we need to derive the equivalent address range for devTLB
> > instead of using the address information in the UAPI data. The
> > reason for that is, unlike IOTLB flush, devTLB flush does not
> > support PASID-selective granularity. This is to say, we need to set
> > the following in the PASID based devTLB invalidation descriptor:
> > - entire 64 bit range in address ~(0x1 << 63)
> > - S bit = 1 (VT-d CH 6.5.2.6).
> > 
> > Without this fix, device TLB flush range is not set properly for
> > PASID selective granularity. This patch also merged devTLB flush
> > code for both implicit and explicit cases.
> > 
> > Fixes: 6ee1b77ba3ac ("iommu/vt-d: Add svm/sva invalidate function")
> > Acked-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/iommu/intel/iommu.c | 28 ++++++++++++++++++----------
> >  1 file changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel/iommu.c
> > b/drivers/iommu/intel/iommu.c index 96340da57075..6a0c62c7395c
> > 100644 --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -5408,7 +5408,7 @@ intel_iommu_sva_invalidate(struct
> > iommu_domain *domain, struct device *dev, sid = PCI_DEVID(bus,
> > devfn); 
> >  	/* Size is only valid in address selective invalidation */
> > -	if (inv_info->granularity != IOMMU_INV_GRANU_PASID)
> > +	if (inv_info->granularity == IOMMU_INV_GRANU_ADDR)
> >  		size =
> > to_vtd_size(inv_info->addr_info.granule_size,
> > inv_info->addr_info.nb_granules); 
> > @@ -5417,6 +5417,7 @@ intel_iommu_sva_invalidate(struct
> > iommu_domain *domain, struct device *dev, IOMMU_CACHE_INV_TYPE_NR) {
> >  		int granu = 0;
> >  		u64 pasid = 0;
> > +		u64 addr = 0;
> >  
> >  		granu = to_vtd_granularity(cache_type,
> > inv_info->granularity); if (granu == -EINVAL) {
> > @@ -5456,24 +5457,31 @@ intel_iommu_sva_invalidate(struct
> > iommu_domain *domain, struct device *dev, (granu ==
> > QI_GRAN_NONG_PASID) ? -1 : 1 << size, inv_info->addr_info.flags &
> > IOMMU_INV_ADDR_FLAGS_LEAF); 
> > +			if (!info->ats_enabled)
> > +				break;
> >  			/*
> >  			 * Always flush device IOTLB if ATS is
> > enabled. vIOMMU
> >  			 * in the guest may assume IOTLB flush is
> > inclusive,
> >  			 * which is more efficient.
> >  			 */
> > -			if (info->ats_enabled)
> > -				qi_flush_dev_iotlb_pasid(iommu,
> > sid,
> > -						info->pfsid, pasid,
> > -						info->ats_qdep,
> > -
> > inv_info->addr_info.addr,
> > -						size);
> > -			break;
> > +			fallthrough;
> >  		case IOMMU_CACHE_INV_TYPE_DEV_IOTLB:
> > +			/*
> > +			 * There is no PASID selective flush for
> > device TLB, so
> > +			 * the equivalent of that is we set the
> > size to be the
> > +			 * entire range of 64 bit. User only
> > provides PASID info
> > +			 * without address info. So we set addr to
> > 0.  
> The "PASID selective flush for device TLB" terminology above sounds a
> bit confusing to me. I would rather say Intel device TLB has no
> support for OMMU_INV_GRANU_PASID granularity but only supports
> IOMMU_INV_GRANU_ADDR. Indeed 6.5.2.6 title is "PASID-based-Device-TLB
> Invalidate Descriptor"
Sounds good, will rewrite comment accordingly.

> > +			 */
> > +			if (inv_info->granularity ==
> > IOMMU_INV_GRANU_PASID) {
> > +				size = 64 - VTD_PAGE_SHIFT;
> > +				addr = 0;  
> I have my answer for previous patch review question. In that case the
> addr is not formatted with the least significant 0 matching the
> size_order.
> 
> > +			} else if (inv_info->granularity ==
> > IOMMU_INV_GRANU_ADDR)
> > +				addr = inv_info->addr_info.addr;
> > +
> >  			if (info->ats_enabled)
> >  				qi_flush_dev_iotlb_pasid(iommu,
> > sid, info->pfsid, pasid,
> > -						info->ats_qdep,
> > -
> > inv_info->addr_info.addr,
> > +						info->ats_qdep,
> > addr, size);
> >  			else
> >  				pr_warn_ratelimited("Passdown
> > device IOTLB flush w/o ATS!\n"); 
> 
> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
> 
> Thanks
> 
> Eric
> 

[Jacob Pan]