Re: [PATCH v1 04/10] iommufd/viommu: Allow drivers to control vdev_id lifecycle

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Sep 05, 2024 at 03:01:19PM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 27, 2024 at 10:02:06AM -0700, Nicolin Chen wrote:
> > The iommufd core provides a lookup helper for an IOMMU driver to find a
> > device pointer by device's per-viommu virtual ID. Yet a driver may need
> > an inverted lookup to find a device's per-viommu virtual ID by a device
> > pointer, e.g. when reporting virtual IRQs/events back to the user space.
> > In this case, it'd be unsafe for iommufd core to do an inverted lookup,
> > as the driver can't track the lifecycle of a viommu object or a vdev_id
> > object.
> > 
> > Meanwhile, some HW can even support virtual device ID lookup by its HW-
> > accelerated virtualization feature. E.g. Tegra241 CMDQV HW supports to
> > execute vanilla guest-issued SMMU commands containing virtual Stream ID
> > but requires software to configure a link between virtual Stream ID and
> > physical Stream ID via HW registers. So not only the iommufd core needs
> > a vdev_id lookup table, drivers will want one too.
> > 
> > Given the two justifications above, it's the best practice to provide a
> > a pair of set_vdev_id/unset_vdev_id ops in the viommu ops, so a driver
> > can implement them to control a vdev_id's lifecycle, and configure the
> > HW properly if required.
> 
> I think the lifecycle rules should be much simpler.
> 
> If a nested domain is attached to a STE/RID/device then the vIOMMU
> affiliated with that nested domain is pinned while the STE is in place
> 
> So the driver only need to provide locking around attach changing the
> STE's vIOMMU vs async operations translating from a STE to a
> vIOMMU. This can be a simple driver lock of some kind, ie a rwlock
> across the STE table.
> 
> Generally that is how all the async events should work, go from the
> STE to the VIOMMU to a iommufd callback to the iommufd event
> queue. iommufd will translate the struct device from the driver to an
> idev_id (or maybe even a vdevid) the same basic way the PRI code works

I am trying to draft something following this, and here is what
it would look like:

------------------------draft---------------------------
struct arm_smmu_master {
	....
+	struct rw_semaphore lock;
+	struct arm_vsmmu *vsmmu;
	....
};

->attach_dev() {
	down_write(&master->lock);
	if (domain->type == IOMMU_DOMAIN_NESTED)
		master->vsmmu = to_smmu_domain(domain)->vsmmu;
	else
		master->vsmmu = NULL;
	up_write(&master->lock);
}

isr() {
	down_read(&master->lock);
	if (master->vsmmu) {
		xa_lock(&master->vsmmu->core.vdevs);
		vdev = iommufd_viommu_dev_to_vdev(&master->vsmmu->core,
						  master->dev);
		if (vdev) {
			struct iommu_virq_arm_smmuv3 virq_data = evt;

			virq_data.evt[0] &= ~EVTQ_0_SID;
			virq_data.evt[0] |= FIELD_PREP(EVTQ_0_SID, vdev->id);
			return iommufd_viommu_report_irq(
					vdev->viommu,
					IOMMU_VIRQ_TYPE_ARM_SMMUV3, &virq_data,
					sizeof(virq_data));
		} else {
			rc = -ENOENT;
		}
		xa_unlock(&master->vsmmu->core.vdevs);
	}
	up_read(&master->lock);
}
--------------------------------------------------------

[Comparison]      | This v1                 | Draft
1. Adds to master | A lock and vdev ptr     | A lock and viommu ptr
2. Set/unset ptr  | In ->vdevice_alloc/free | In all ->attach_dev
3. Do dev_to_vdev | master->vdev->id        | attach_handle?

Both solutions needs a driver-level lock and an extra pointer in
the master structure. And both ISR routines require that driver-
level lock to avoid race against attach_dev v.s. vdev alloc/free.
Overall, taking step.3 into consideration, it seems that letting
master lock&hold the vdev pointer (i.e. this v1) is simpler?

As for the implementation of iommufd_viommu_dev_to_vdev(), I read
the attach_handle part in the PRI code, yet I don't see the lock
that protects the handle returned by iommu_attach_handle_get() in
iommu_report_device_fault/find_fault_handler().

And I see the kdoc of iommu_attach_handle_get() mentioning:
  "Callers are required to synchronize the call of
   iommu_attach_handle_get() with domain attachment
   and detachment. The attach handle can only be used
   during its life cycle."
But the caller iommu_report_device_fault() is an async event that
cannot guarantee the lifecycle. Would you please shed some light?

Thanks
Nicolin