Re: [RFC PATCH v6 08/11] iommufd: Add vIOMMU provider support

[Aneesh Kumar K . V]

Jason Gunthorpe <jgg@xxxxxxxxxx> writes:

> On Tue, Sep 29, 2026 at 06:15:39PM +0530, Aneesh Kumar K.V wrote:
>> Jason Gunthorpe <jgg@xxxxxxxxxx> writes:
>> 
>> > On Tue, Sep 29, 2026 at 11:44:28AM +0530, Aneesh Kumar K.V wrote:
>> >
>> >> tsm_viommu_get_ops() runs with pci_tsm_rwsem held for read and takes a
>> >> temporary reference on the backend module (the CCA module). This keeps
>> >> the selected ops callable until vIOMMU initialization completes. iommufd
>> >> then drops the reference. This does not pin a particular TSM
>> >> registration or prevent tsm_unregister().
>> >
>> > That seems over complicated. Maybe we can't get to the sane locking I
>> > suggested earlier where TSM module is stable while a driver is bound,
>> > but we absolutely must have sane locking where we can "pin" the tsm
>> > for a pdev and it cannot be unregistered for long periods of time,
>> > such as while a viommu/vdev exists.
>> >
>> > That period should start right before getting the ops and continue to
>> > until the viommu is destroyed.
>> >
>> > No hot unplug of tsm modules while things are active.
>> >
>> 
>> 
>> That is essentially how it works. I decided to take the module reference
>> here and the tsm_dev reference in viommu_init() to keep the rest of
>> viommu_alloc() cleaner. That is, we have:
>> 
>> 	struct module *owner = NULL;
>> 
>> 	ops = tsm_viommu_get_ops(idev->dev, cmd->type, &owner);
>> 
>> 	if (!ops) {
>> 		ops = iommu_dev->ops->get_viommu_ops(idev->dev, cmd->type);
>>                 
>> 	rc = ops->viommu_init(viommu, idev->dev,
>> 	if (rc)
>> 		goto out_put_hwpt;
>> 
>> out_put_idev:
>> 	module_put(owner);
>
> if we have a get_ops we need a put_ops()..
>
>> The tsm_dev and module details are needed only by tsm_viommu, not by a
>> generic SMMU driver. For viommu_init() to take ownership of the resources
>> acquired by get_ops(), I would either need to add a viommu_info argument
>> to viommu_init(), affecting all IOMMU driver implementations, or make the
>> error handling conditional and awkward.
>
> Just don't, iommufd can hold the tsm ops if it knows it created the
> viommu through tsm.
>
>
>> >> CCA vIOMMU initialization takes a tsm_dev reference, keeping the TSM
>> >> object and its PCI/TSM resources alive until the vIOMMU is
>> >> destroyed.
>> >
>> > iommufd should do this, so long as the viommu object exists the tsm
>> > for it exists. It should not be inside tsm drivers.
>> >
>> 
>> That would expose more TSM details to iommufd. The reference must also
>> be acquired under pci_tsm_rwsem.
>
> That seems like overkill.
>
>> /* Pin the current TSM and revalidate the selected vIOMMU operations. */
>> struct tsm_dev * pci_tsm_viommu_get_tsm_dev(struct pci_dev *pdev,
>> 		enum iommu_viommu_type type,
>> 		const struct iommufd_viommu_ops *expected_ops)
>> {
>> 	const struct pci_tsm_ops *ops;
>> 	const struct iommufd_viommu_ops *viommu_ops;
>> 	struct tsm_dev *tsm_dev;
>> 	int ret = -ENODEV;
>> 
>> 	{
>> 		guard(rwsem_read)(&pci_tsm_rwsem);
>> 		if (!pdev->tsm)
>> 			return ERR_PTR(-ENODEV);
>> 		if (pdev->tsm->tsm_dev->unregistering)
>> 			return ERR_PTR(-ENODEV);
>> 		ops = to_pci_tsm_ops(pdev->tsm);
>> 		if (!ops->viommu_get_ops)
>> 			return ERR_PTR(-ENODEV);
>> 		tsm_dev = pdev->tsm->tsm_dev;
>> 		/* Unlike get_device(), this also keeps PCI/TSM resources active. */
>> 		if (!tsm_try_get(tsm_dev))
>> 			return ERR_PTR(-ENODEV);
>
> This is way too complicated for what should be a very simple scheme :(
>
> rcu_read_lock()
> tsm = rcu_derference(pdev->tsm);
> if (!tsm)
>    return NULL;
> /* Prevent the module from unloading which must be the only way to
>    trigger unregister */
> if (!try_module_get(tsm->ops->module))
>    return NULL;
> rcu_read_unlock()
>
> And this should be exposed to iommufd..
>
> viommu_create:
>  tsm = tsm_get_device(pdev)
>  tsm_get_viommu_ops(tsm,...)
>  [..]
>
> viommu_destroy:
>   tsm_put_device(tsm)
>
> No lock, no registering FSM, just RCU free the pdev->tsm memory and do
> that module unload rcu synchronize during module unload. No hand of of
> the lifecylce to other layers.
>
> Hold the module get in iommufd inside the iommufd viommu object.
>
> It is simple and easy to understand.
>


Switching pdev->tsm to an RCU-protected pointer requires broader changes
to the existing TSM code. To move this series forward, I will continue
protecting it with pci_tsm_rwsem in the next revision, as that requires
fewer changes. We can revisit an RCU conversion later if needed?

With this approach, viommu_alloc() will do:
	struct tsm_dev *tsm_dev = NULL;


	tsm_dev = tsm_get_device(idev->dev);
	ops = tsm_dev ? tsm_viommu_get_ops(tsm_dev, idev->dev, cmd->type) : NULL;
	if (!ops) {
                tsm_dev = NULL;
		ops = iommu_dev->ops->get_viommu_ops(idev->dev, cmd->type);
        }

	viommu = (struct iommufd_viommu *)_iommufd_object_alloc_ucmd(
	viommu->tsm_dev = tsm_dev;
	tsm_dev = NULL;

	rc = ops->viommu_init(viommu, idev->dev,....)

....
	if (tsm_dev)
		tsm_put_device(tsm_dev);


void iommufd_viommu_destroy(struct iommufd_object *obj)
{
..
	if (viommu->tsm_dev)
		tsm_put_device(viommu->tsm_dev);
...
}

struct tsm_dev *pci_tsm_get_device(struct pci_dev *pdev)
{
	const struct pci_tsm_ops *ops;
	struct tsm_dev *tsm_dev;

	guard(rwsem_read)(&pci_tsm_rwsem);
	if (!pdev->tsm)
		return NULL;

	tsm_dev = pdev->tsm->tsm_dev;

	ops = tsm_dev->pci_ops;
	if (!try_module_get(ops->owner)) // arm-cca-host
		return ERR_PTR(-ENODEV);
	if (!tsm_try_get(tsm_dev)) {
		module_put(ops->owner);
		return ERR_PTR(-ENODEV);
	}
	return tsm_dev;
}

>
>> 		viommu_ops = ops->viommu_get_ops(&pdev->dev, type);
>> 		if (viommu_ops == expected_ops)
>> 			return tsm_dev;
>> 		if (IS_ERR(viommu_ops))
>> 			ret = PTR_ERR(viommu_ops);
>> 	}
>
>
> This should just be try_module_get and a touch of RCU.
>
>> > ideally the module refcount handles this and the only way to trigger a
>> > tsm remove is through module unload, with no sysfs path?
>> 
>> I agree. The existing code takes extra care to allow tsm_unregister().
>> However, if unloading arm-cca-host.ko is the only way to trigger
>> unregistration, as it currently is, we can avoid this complexity.
>
> I've been badly traumitized by hot unplug races, bugs and deadlock,
> let's not introduce anything like this here, there is no need.
>

Are you suggesting setting suppress_bind_attrs = true for the
arm-cca-host driver?

The driver model otherwise allows the driver to be unbound. For example,
what happens when arm-rmi device is unbound from the arm-cca-host
driver? We need to support tsm_unregister() in that case. I am also not
sure whether there are other paths that can call tsm_unregister().
Currently, we register the cleanup callback via

	tsm_dev = tsm_register(&sdev->dev, &cca_link_pci_ops);

	ret = devm_add_action_or_reset(&sdev->dev, cca_link_tsm_remove, tsm_dev);
	if (ret)

-aneesh