Re: [PATCH v13 09/13] iommu/sva: Refactoring iommu_sva_bind/unbind_device()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: Baolu Lu <baolu.lu@linux.intel.com>,
	Joerg Roedel <joro@8bytes.org>,
	Christoph Hellwig <hch@infradead.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Fenghua Yu <fenghua.yu@intel.com>, Vinod Koul <vkoul@kernel.org>,
	Eric Auger <eric.auger@redhat.com>, Liu Yi L <yi.l.liu@intel.com>,
	Jacob jun Pan <jacob.jun.pan@intel.com>,
	Zhangfei Gao <zhangfei.gao@linaro.org>,
	Zhu Tony <tony.zhu@intel.com>,
	iommu@lists.linux.dev, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v13 09/13] iommu/sva: Refactoring iommu_sva_bind/unbind_device()
Date: Thu, 8 Sep 2022 13:41:43 -0300	[thread overview]
Message-ID: <YxobR/brG9VlxPvX@nvidia.com> (raw)
In-Reply-To: <YxoXfCQcD3yC5ppn@myrica>

On Thu, Sep 08, 2022 at 05:25:32PM +0100, Jean-Philippe Brucker wrote:
> On Wed, Sep 07, 2022 at 02:33:11PM -0300, Jason Gunthorpe wrote:
> > On Wed, Sep 07, 2022 at 10:54:54AM +0100, Jean-Philippe Brucker wrote:
> > 
> > > Is iommu_domain still going to represent both a device context (whole
> > > PASID table) and individual address spaces, or are you planning to move
> > > away from that?  What happens when a driver does:
> > > 
> > >   d1 = iommu_domain_alloc()
> > >   iommu_attach_device(d1)
> > >   d2 = iommu_sva_bind_device()
> > >   iommu_detach_device(d1)
> > > 
> > > Does detach
> > > (a) only disable the non-PASID address space?
> > > (b) disable everything?
> > > (c) fail because the driver didn't unbind first?
> > 
> > I think it must be (a), considering how everything is defined and the
> > needs for vIOMMU emulation.
> 
> Yes (a) is probably better. The SMMU driver currently implements (c) to
> ensure that you can't switch device driver without unbinding everything
> first, and we should keep that check somewhere

Yes, the owner stuff is a logical place to put that, when ownership
is all released the PASID table of the group must be empty. Lu?

> > Otherwise everything blows up if you attach an iommu_domain to two
> > RIDs - the API demands that every RID gets its own PASID mapping, even
> > if the RID shares iommu_domains. We do not have an API to share PASID
> > tables.
> 
> Well, we still do since SMMU implements it. Changing the API is fine, but
> someone will need to rework the SMMU driver to align with the new meaning
> of iommu_domain. I can take a stab if no one volunteers but probably not
> before next year.

I think given the current progression it is OK for this series to
leave the SMMU driver with a bit of a different view of how the API
should work. The main target here is the in-kernel drivers and they
won't trigger these cases.

As we extend things into userspace PASID support we may want to block
SMMU PASID support through iommfd/vfio until it is updated.

I've been thinking on and off about this - we likely need to have a
conformance test suite as part of iommufd that is specifically for
checking that the iommu driver meets the API. So it would do things
like exercise scenario (a), attaching domains to all sorts of
combinations, and so on.

We know this from other driver subsystems that drivers can implement
the uAPI incorrectly and it causes a lot of pain for userspace.

Jason