Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()

[Yi Liu]

On 2022/3/29 16:42, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Tuesday, March 29, 2022 1:38 PM
> >
> > Some of the interfaces in the IOMMU core require that only a single
> > kernel device driver controls the device in the IOMMU group. The
> > existing method is to check the device count in the IOMMU group in
> > the interfaces. This is unreliable because any device added to the
> > IOMMU group later breaks this assumption without notifying the driver
> > using the interface. This adds iommu_group_singleton_lockdown() that
> > checks the requirement and locks down the IOMMU group with only single
> > device driver bound.
> >
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/iommu.c | 30 ++++++++++++++++++------------
> >   1 file changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 0c42ece25854..9fb8a5b4491e 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -48,6 +48,7 @@ struct iommu_group {
> >   	struct list_head entry;
> >   	unsigned int owner_cnt;
> >   	void *owner;
> > +	bool singleton_lockdown;
> >   };
> >
> >   struct group_device {
> > @@ -968,15 +969,16 @@ void iommu_group_remove_device(struct device
> > *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(iommu_group_remove_device);
> >
> > -static int iommu_group_device_count(struct iommu_group *group)
> > +/* Callers should hold the group->mutex. */
> > +static bool iommu_group_singleton_lockdown(struct iommu_group *group)
> >   {
> > -	struct group_device *entry;
> > -	int ret = 0;
> > -
> > -	list_for_each_entry(entry, &group->devices, list)
> > -		ret++;
> > +	if (group->owner_cnt != 1 ||
> > +	    group->domain != group->default_domain ||
> > +	    group->owner)
> > +		return false;
>
> Curious why there will be a case where group uses default_domain
> while still having a owner? I have the impression that owner is used
> for userspace DMA where a different domain is used.
>
> > +	group->singleton_lockdown = true;
> >
> > -	return ret;
> > +	return true;
> >   }
>
> btw I'm not sure whether this is what SVA requires. IIRC the problem with
> SVA is because PASID TLP prefix is not counted in PCI packet routing thus
> a DMA target address with PASID might be treated as P2P if the address
> falls into the MMIO BAR of other devices in the group. This is why the
> original code needs to strictly apply SVA in a group containing a single
> device, instead of a group attached by a single driver, unless we want to
> reserve those MMIO ranges in CPU VA space.
>
> Jean can correct me if my memory is wrong.

I think so. I remember the major gap is PASID info is not used in the PCI's 
address based TLP routing mechanism. So P2P may happen if the address
happens to be device MMIO. Per the commit message from Jean. Even for 
singular groups, it's not an easy thing. So non-sigleton groups are not
considered so far.

"
IOMMU groups with more than one device aren't supported for SVA at the
moment. There may be P2P traffic between devices within a group, which
cannot be seen by an IOMMU (note that supporting PASID doesn't add any
form of isolation with regard to P2P). Supporting groups would require
calling bind() for all bound processes every time a device is added to a
group, to perform sanity checks (e.g. ensure that new devices support
PASIDs at least as big as those already allocated in the group). It also
means making sure that reserved ranges (IOMMU_RESV_*) of all devices are
carved out of processes. This is already tricky with single devices, but
becomes very difficult with groups. Since SVA-capable devices are expected
to be cleanly isolated, and since we don't have any way to test groups or
hot-plug, we only allow singular groups for now.
"

https://lore.kernel.org/kvm/20180511190641.23008-3-jean-philippe.brucker@xxxxxxx/

--
Regards,
Yi Liu