RE: [PATCH v2 03/16] iommu: introduce iommu invalidate API function

[Liu, Yi L]

> On Tue, 10 Oct 2017 15:35:42 +0200
> Joerg Roedel <joro@xxxxxxxxxx> wrote:
> 
> > On Thu, Oct 05, 2017 at 04:03:31PM -0700, Jacob Pan wrote:
> > > +int iommu_invalidate(struct iommu_domain *domain,
> > > +		struct device *dev, struct tlb_invalidate_info
> > > *inv_info)
> >
> > This name is way too generic, it should at least be called
> > iommu_svm_invalidate() or something like that. With the name above it
> > is easily confused with the other TLB invalidation functions of the
> > IOMMU-API.
> >
> Good point. I was calling it iommu_passdown_invalidate() originally.
> The invalidation request comes from guest or user space instead of in-kernel unmap
> kind of calls.

[Liu, Yi L] I agree that iommu_invalidate() is too generic. Additionally, also better to avoid
making it svm specific.

The reason we introduce this API is in vSVM case is that guest owns the first level page
table(vtd). If we use similar mechanism for vIOVA, then we also need to passdown guest's
vIOVA tlb flush.

Since it is to expose an API for iommu tlb flushes requests from userspace/guest which is out
of iommu. How about naming it as iommu_tlb_external_invalidate()?

> > > +enum iommu_inv_granularity {
> > > +	IOMMU_INV_GRANU_GLOBAL,		/* all TLBs
> > > invalidated */
> >
> > Is that needed? We certainly don't want to give userspace/guests that
> > fine-grained control about IOMMU cache invalidations.
> >
> > In the end a guest issues flush-global command does not translate to a
> > flush-global on the host, but to separate flushes for the domains the
> > guest uses.
> >
> Right, guest should not go beyond its own domain.

[Liu, Yi L] So far, for virtualization, we would not allow any guest to flush all the
physical iommu tlb. Hypervisor will limit it even if guest issues invalidate with global
granularity. Jacob just wants to show all the possible granularity. Actually, global gran
can be a big hammer to clear cache, maybe there is usage somehow. For now, I think
we may just remove it.	

Thanks,
Yi L

> > > +	IOMMU_INV_GRANU_DOMAIN,		/* all TLBs
> > > associated with a domain */
> > > +	IOMMU_INV_GRANU_DEVICE,		/* caching
> > > structure associated with a
> > > +					 * device ID
> >
> > What is the difference between a DOMAIN and a DEVICE flush?
> >
> Those are based on vt-d context cache flush granularity, domain selective flushes all
> context caches associated with a domain ID.
> Device selective flush flushes context caches of a source ID.
> But like you pointed out below, since context cache flush will come in as unbind call,
> there is no need to do passdown invalidate. I can remove that.
> 
> Here I am trying to use all generic definitions, which is a superset of all vendor
> models. I am likely missing out some non-vt-d cases.
> 
> > > +	IOMMU_INV_GRANU_DOMAN_PAGE,	/* address range with a
> > > domain */
> > > +	IOMMU_INV_GRANU_ALL_PASID,	/* cache of a given
> > > PASID */
> > > +	IOMMU_INV_GRANU_PASID_SEL,	/* only invalidate
> > > specified PASID */ +
> > > +	IOMMU_INV_GRANU_NG_ALL_PASID,	/* non-global within
> > > all PASIDs */
> > > +	IOMMU_INV_GRANU_NG_PASID,	/* non-global within a
> > > PASIDs */
> > > +	IOMMU_INV_GRANU_PAGE_PASID,	/* page-selective
> > > within a PASID */
> > > +	IOMMU_INV_NR_GRANU,
> > > +};
> > > +
> > > +enum iommu_inv_type {
> > > +	IOMMU_INV_TYPE_DTLB,	/* device IOTLB */
> > > +	IOMMU_INV_TYPE_TLB,	/* IOMMU paging structure cache
> > > */
> > > +	IOMMU_INV_TYPE_PASID,	/* PASID cache */
> > > +	IOMMU_INV_TYPE_CONTEXT,	/* device context entry
> > > cache */
> >
> > Is that really needed? When the guest updates it context-entry
> > equivalent it translates to bind_pasid_table/unbind_pasid_table calls,
> > no?
> >
> Right no need to passdown context cache invalidation for VT-d. I just wasn't sure it is
> the same for all models. Again, trying to have a superset of generic fields.
> 
> Thanks!
> 
> Jacob