------------------ Original ------------------ From: "Jason Gunthorpe" <jgg@nvidia.com>; Date: Fri, Sep 9, 2022 00:41 AM 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"<iommu@lists.linux.dev>;"linux-pci"<linux-pci@vger.kernel.org>;"linux-kernel"<linux-kernel@vger.kernel.org>; Subject: Re: [PATCH v13 09/13] iommu/sva: Refactoring iommu_sva_bind/unbind_device()
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