Re: [PATCH 3/4] iommu/vt-d: Support PASID DMA for in-kernel usage

[Lu Baolu]

On 12/9/21 3:16 AM, Jacob Pan wrote:
> Hi Jason,
>
> On Wed, 8 Dec 2021 09:22:55 -0400, Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> > On Tue, Dec 07, 2021 at 05:47:13AM -0800, Jacob Pan wrote:
> > > Between DMA requests with and without PASID (legacy), DMA mapping APIs
> > > are used indiscriminately on a device. Therefore, we should always match
> > > the addressing mode of the legacy DMA when enabling kernel PASID.
> > >
> > > This patch adds support for VT-d driver where the kernel PASID is
> > > programmed to match RIDPASID. i.e. if the device is in pass-through, the
> > > kernel PASID is also in pass-through; if the device is in IOVA mode, the
> > > kernel PASID will also be using the same IOVA space.
> > >
> > > There is additional handling for IOTLB and device TLB flush w.r.t. the
> > > kernel PASID. On VT-d, PASID-selective IOTLB flush is also on a
> > > per-domain basis; whereas device TLB flush is per device. Note that
> > > IOTLBs are used even when devices are in pass-through mode. ATS is
> > > enabled device-wide, but the device drivers can choose to manage ATS at
> > > per PASID level whenever control is available.
> > >
> > > Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > >   drivers/iommu/intel/iommu.c | 105 +++++++++++++++++++++++++++++++++++-
> > >   drivers/iommu/intel/pasid.c |   7 +++
> > >   include/linux/intel-iommu.h |   3 +-
> > >   3 files changed, 113 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > index 60253bc436bb..a2ef6b9e4bfc 100644
> > > +++ b/drivers/iommu/intel/iommu.c
> > > @@ -1743,7 +1743,14 @@ static void domain_flush_piotlb(struct
> > > intel_iommu *iommu, if (domain->default_pasid)
> > >   		qi_flush_piotlb(iommu, did, domain->default_pasid,
> > >   				addr, npages, ih);
> > > -
> > > +	if (domain->kernel_pasid && !domain_type_is_si(domain)) {
> > > +		/*
> > > +		 * REVISIT: we only do PASID IOTLB inval for FL, we
> > > could have SL
> > > +		 * for PASID in the future such as vIOMMU PT. this
> > > doesn't get hit.
> > > +		 */
> > > +		qi_flush_piotlb(iommu, did, domain->kernel_pasid,
> > > +				addr, npages, ih);
> > > +	}
> > >   	if (!list_empty(&domain->devices))
> > >   		qi_flush_piotlb(iommu, did, PASID_RID2PASID, addr,
> > > npages, ih); }
> > > @@ -5695,6 +5702,100 @@ static void intel_iommu_iotlb_sync_map(struct
> > > iommu_domain *domain, }
> > >   }
> > >   
> > > +static int intel_enable_pasid_dma(struct device *dev, u32 pasid)
> > > +{
> >
> > This seems like completely the wrong kind of op.
> >
> > At the level of the iommu driver things should be iommu_domain centric
> >
> > The op should be
> >
> > int attach_dev_pasid(struct iommu_domain *domain, struct device *dev,
> > ioasid_t pasid)
> >
> > Where 'dev' purpose is to provide the RID
> >
> > The iommu_domain passed in should be the 'default domain' ie the table
> > used for on-demand mapping, or the passthrough page table.
> Makes sense. DMA API is device centric, iommu API is domain centric. It
> should be the common IOMMU code to get the default domain then pass to
> vendor drivers. Then we can enforce default domain behavior across all
> vendor drivers.
> i.e. 	
> 	dom = iommu_get_dma_domain(dev);
> 	attach_dev_pasid(dom, dev, pasid);
>
> > > +	struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
> > > +	struct device_domain_info *info;
> >
> > I don't even want to know why an iommu driver is tracking its own
> > per-device state. That seems like completely wrong layering.
> This is for IOTLB and deTLB flush. IOTLB is flushed at per domain level,
> devTLB is per device.
>
> For multi-device groups, this is a need to track how many devices are using
> the kernel DMA PASID.
>
> Are you suggesting we add the tracking info in the generic layer? i.e.
> iommu_group.
>
> We could also have a generic device domain info to replace what is in VT-d
> and FSL IOMMU driver, etc.

The store place of per-device iommu driver private data has already been
standardized. The iommu core provides below interfaces for this purpose:

void dev_iommu_priv_set(struct device *dev, void *priv);
void *dev_iommu_priv_get(struct device *dev);

If we have anything generic among different vendor iommu drivers,
perhaps we could move them into dev->iommu.

Best regards,
baolu