Re: [PATCH RFCv1 07/14] iommufd: Add viommu set/unset_dev_id ops

From: Nicolin Chen <nicolinc@nvidia.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: <will@kernel.org>, <robin.murphy@arm.com>, <kevin.tian@intel.com>,
	<suravee.suthikulpanit@amd.com>, <joro@8bytes.org>,
	<linux-kernel@vger.kernel.org>, <iommu@lists.linux.dev>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-tegra@vger.kernel.org>, <yi.l.liu@intel.com>,
	<eric.auger@redhat.com>, <vasant.hegde@amd.com>,
	<jon.grimm@amd.com>, <santosh.shukla@amd.com>,
	<Dhaval.Giani@amd.com>, <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH RFCv1 07/14] iommufd: Add viommu set/unset_dev_id ops
Date: Tue, 14 May 2024 18:59:07 -0700	[thread overview]
Message-ID: <ZkQW6/OAQ8MzN6Go@nvidia.com> (raw)
In-Reply-To: <ZkOI8ztR1mUMJ8oe@nvidia.com>

On Tue, May 14, 2024 at 12:53:23PM -0300, Jason Gunthorpe wrote:
> On Sun, May 12, 2024 at 09:39:15PM -0700, Nicolin Chen wrote:
> > On Sun, May 12, 2024 at 11:46:54AM -0300, Jason Gunthorpe wrote:
> > > On Fri, Apr 12, 2024 at 08:47:04PM -0700, Nicolin Chen wrote:
> > > > Add a pair of ops to set and unet device's virtual ID that belongs to
> > > > a viommu object. They will be used, in the following patch, by iommufd
> > > > to support some HW-acceleration feature from the host level.
> > > > 
> > > > For instance, every device behind an ARM SMMU has a Stream ID. The ID
> > > > is used by ATC invalidation commands so SMMU HW can direct invalidation
> > > > requests to the corresponding PCI device where the ID belongs to. In a
> > > > virtualization use case, a passthroughed device in the VM will have a
> > > > virtuail Stream ID, used by the ATC invalidation commands in the guest
> > > > system. NVIDIA's CMDQV extension for SMMUv3 provides a v-interface to
> > > > execute the guest-level ATC invalidation commands directly, yet needs
> > > > the HW to be aware of its virtual Stream ID so it can replace with its
> > > > physical Stream ID.
> > > 
> > > I imagine using this as well for the ATC invalidation commands. It
> > > would be very easy and simplifying if the command fixup just extracted
> > > the vSID from the ATC invalidation and used an xarray to turn it into
> > > a pSID and then pushed the resulting command.
> > 
> > You mean the nested SMMU series right? Actually the set_dev_id
> > ioctl was a part of that until we wanted to try DEV_INVALIDATE.
> 
> Yes, there is nothing inherently wrong with DEV_INVALIDATE, we could
> continue to use that as the API and automatically pick up the VIOMMU
> instance from the nesting domain to process the ATS.
> 
> The VMM needs a reliable place to send the CMDQ data, on ARM/AMD this
> needs to be an always available global-to-the-viommu thing. Intel
> needs to associate the virtual invalidation with the correct nesting
> domain as well.
> 
> So I original thought it would nice and simple to have a
> VIOMMU_INVALIDATE as well.
> 
> But.. If we need a nesting domain that is indentity (ie the S2) then
> when the VIOMMU is created then we'd also create an identity nesting
> domain as well.

So, you want a proxy S1 domain for a device to attach, in case
of a stage-2 only setup, because an S2 domain will no longer has
a VMID, since it's shared among viommus. In the SMMU driver case,
an arm_smmu_domain won't have an smmu pointer, so a device can't
attach to an S2 domain but always an nested S1 domain, right?

> Functionally we could use that global nesting domain
> to deliver the DEV_INVALIDATE too.

If my narrative above is correct, the device is actually still
attached to S2 domain via a proxy nested S1 domain. What cache
do we need to invalidate except S2 mappings in this case?

> > So again, yes, it makes sense to me that we move viommu and the
> > set_dev_id to the nested series, and then drop DEV_INVALIDATE.
> 
> I would like to do this bit by bit. viommu is a big series on its own.
> 
> DEV_INVALIDATE is fine, it just can't do ATS invalidation.

I am not very sure about AMD. Intel doesn't need DEV_INVALIDATE
at this moment. SMMU only uses DEV_INVALIDATE for ATC and CD
invalidations, which can be both shifted to VIOMMU_INVALIDATE.

Same question: any other case can we use the DEV_INVALIDATE for?

> We can add ATS invalidation after either as an enhancement as part of
> adding the VIOMMU either as DEV_INVALIDATE or VIOMMU_INVALIDATE (or
> both)

Yea, maybe step by step like this:

Part-1 VIOMMU_ALLOC and VIOMMU_ATTACH
Part-2 VIOMMU_SET/UNSET_VDEV_ID
Part-3 VIOMMU_INVALIDATE
Part-4 VQUEUE_ALLOC
...

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel