Re: [PATCH v6 03/12] iommu: Add attach/detach_dev_pasid domain ops - Jean-Philippe Brucker

From: Jean-Philippe Brucker <jean-philippe@linaro.org>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Baolu Lu <baolu.lu@linux.intel.com>,
	Jason Gunthorpe <jgg@nvidia.com>, Joerg Roedel <joro@8bytes.org>,
	Christoph Hellwig <hch@infradead.org>,
	"Raj, Ashok" <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	"Jiang, Dave" <dave.jiang@intel.com>,
	Vinod Koul <vkoul@kernel.org>, Eric Auger <eric.auger@redhat.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Pan, Jacob jun" <jacob.jun.pan@intel.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 03/12] iommu: Add attach/detach_dev_pasid domain ops
Date: Wed, 11 May 2022 08:54:39 +0100	[thread overview]
Message-ID: <Yntrv+nq2t/IeVo2@myrica> (raw)
In-Reply-To: <BN9PR11MB5276100AC9BBB7DE5CB800CF8CC89@BN9PR11MB5276.namprd11.prod.outlook.com>

On Wed, May 11, 2022 at 04:09:14AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Wednesday, May 11, 2022 10:32 AM
> > 
> > On 2022/5/10 22:02, Jason Gunthorpe wrote:
> > > On Tue, May 10, 2022 at 02:17:29PM +0800, Lu Baolu wrote:
> > >
> > >> This adds a pair of common domain ops for this purpose and adds
> > helpers
> > >> to attach/detach a domain to/from a {device, PASID}.
> > >
> > > I wonder if this should not have a detach op - after discussing with
> > > Robin we can see that detach_dev is not used in updated
> > > drivers. Instead attach_dev acts as 'set_domain'
> > >
> > > So, it would be more symmetrical if attaching a blocking_domain to the
> > > PASID was the way to 'detach'.
> > >
> > > This could be made straightforward by following the sketch I showed to
> > > have a static, global blocing_domain and providing a pointer to it in
> > > struct iommu_ops
> > >
> > > Then 'detach pasid' is:
> > >
> > > iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev,
> > pasid);
> > >
> > > And we move away from the notion of 'detach' and in the direction that
> > > everything continuously has a domain set. PASID would logically
> > > default to blocking_domain, though we wouldn't track this anywhere.
> > 
> > I am not sure whether we still need to keep the blocking domain concept
> > when we are entering the new PASID world. Please allow me to wait and
> > listen to more opinions.
> > 
> 
> I'm with Jason on this direction. In concept after a PASID is detached it's
> essentially blocked. Implementation-wise it doesn't prevent the iommu
> driver from marking the PASID entry as non-present as doing in this
> series instead of actually pointing to the empty page table of the block
> domain. But api-wise it does make the entire semantics more consistent.

This is all internal to IOMMU so I don't think we should be concerned
about API consistency. I prefer a straighforward detach() operation
because that way IOMMU drivers don't have to keep track of which domain is
attached to which PASID. That code can be factored into the IOMMU core.

In addition to clearing contexts, detach() also needs to invalidate TLBs,
and for that the SMMU driver needs to know the old ASID (!= PASID) that
was used by the context descriptor. We can certainly work around a missing
detach() to implement this, but it will be convoluted.

Thanks,
Jean