> From: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, September 29, 2021 12:56 PM
>
> >
> > Unlike vfio, iommufd adopts a device-centric design with all group
> > logistics hidden behind the fd. Binding a device to iommufd serves
> > as the contract to get security context established (and vice versa
> > for unbinding). One additional requirement in iommufd is to manage the
> > switch between multiple security contexts due to decoupled bind/attach:
> >
> > 1) Open a device in "/dev/vfio/devices" with user access blocked;
>
> Probably worth clarifying that (1) must happen for *all* devices in
> the group before (2) happens for any device in the group.
No. User access is naturally blocked for other devices as long as they
are not opened yet.
>
> > 2) Bind the device to an iommufd with an initial security context
> > (an empty iommu domain which blocks dma) established for its
> > group, with user access unblocked;
> >
> > 3) Attach the device to a user-specified ioasid (shared by all devices
> > attached to this ioasid). Before attaching, the device should be first
> > detached from the initial context;
>
> So, this step can implicitly but observably change the behaviour for
> other devices in the group as well. I don't love that kind of
> difficult to predict side effect, which is why I'm *still* not totally
> convinced by the device-centric model.
which side-effect is predicted here? The user anyway needs to be
aware of such group restriction regardless whether it uses group
or nongroup interface.
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 5ea3a007fd7c..bffd84e978fb 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -45,6 +45,8 @@ struct iommu_group {
> > struct iommu_domain *default_domain;
> > struct iommu_domain *domain;
> > struct list_head entry;
> > + unsigned long user_dma_owner_id;
>
> Using an opaque integer doesn't seem like a good idea. I think you
> probably want a pointer to a suitable struct dma_owner or the like
> (you could have one embedded in each iommufd struct, plus a global
> static kernel_default_owner).
>
For remaining comments you may want to look at the latest discussion
here:
https://lore.kernel.org/kvm/20210928140712.GL964074@xxxxxxxxxx/
It relies on driver core change to manage group ownership gracefully.
No BUG_ON() is triggered any more for driver binding. There a fd will
be passed in to mark the ownership.
Thanks
Kevin
