RE: [RFC PATCH 03/10] iommu/vt-d: Allocate groups for mediated devices

["Liu, Yi L" <yi.l.liu@intel.com>]

Hi Jean,

> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> Sent: Tuesday, July 24, 2018 7:31 PM
> 
> Hi Baolu,
> 
> On 24/07/18 03:22, Lu Baolu wrote:
> > Hi,
> >
> > On 07/23/2018 12:44 PM, Liu, Yi L wrote:
> >>> From: Lu Baolu [mailto:baolu.lu@linux.intel.com]
> >>> Sent: Sunday, July 22, 2018 2:09 PM
> >>>
> >>> With the Intel IOMMU supporting PASID granularity isolation and protection, a
> >>> mediated device could be isolated and protected by an IOMMU unit. We need to
> >>> allocate a new group instead of a PCI group.
> >> Existing vfio mdev framework also allocates an iommu group for mediate device.
> >>
> >> mdev_probe()
> >>   |_ mdev_attach_iommu()
> >>        |_ iommu_group_alloc()
> >
> > When external components ask iommu to allocate a group for a device,
> > it will call pci_device_group in Intel IOMMU driver's @device_group
> > callback. In another word, current Intel IOMMU driver doesn't aware
> > the mediated device and treat all devices as PCI ones. This patch
> > extends the @device_group call back to make it aware of a mediated
> > device.
> 
> I agree that allocating two groups for an mdev seems strange, and in my

There will not be two groups for a mdev. Pls refer to Patch 08/10 of this
series. Baolu added iommu_ops check when doing group allocation in
mdev_attach_iommu().

[RFC PATCH 08/10] vfio/mdev: Set iommu ops for mdev bus

@@ -21,6 +21,13 @@ static int mdev_attach_iommu(struct mdev_device *mdev)
 	int ret;
 	struct iommu_group *group;
 
+	/*
+	 * If iommu_ops is set for bus, add_device() will allocate
+	 * a group and add the device in the group.
+	 */
+	if (iommu_present(mdev->dev.bus))
+		return 0;
+

> opinion we shouldn't export the notion of mdev to IOMMU drivers.

The key idea of this RFC is to tag iommu domain with PASID, if any mdev
is added to such a domain, it would get the PASID and config in its parent.
Thus the transactions from mdev can be isolated in iommu hardware.

Based on this idea, mdevs can be managed in a flexible manner. e.g.
if two mdevs are assigned to same VM, they can share PASID. This share
can be easily achieve by adding them to the same domain. If we default
allocate a PASID for each mdev, it may be a waste.

With vendor-specific iommu driver handle the mdev difference, it can
largely keep the fundamental iommu concepts in current software
implementation.

> Otherwise each driver will have to add its own "dev_is_mdev()" special
> cases, which will get messy in the long run. Besides, the macro is
> currently private, and to be exported it should be wrapped in
> symbol_get/put(mdev_bus_type).

Agreed. Should figure out a better manner.

> There is another way: as we're planning to add a generic pasid_alloc()
> function to the IOMMU API, the mdev module itself could allocate a
> default PASID for each mdev by calling pasid_alloc() on the mdev's
> parent, and then do map()/unmap() with that PASID. This way we don't

so far, map/unmap is per-domain operation. In this way, passing PASID makes
it be kind of per-device operation. This may affect too much of existing software
implementation.

> have to add IOMMU ops to the mdev bus, everything can still be done
> using the ops of the parent. And IOMMU drivers "only" have to implement
> PASID ops, which will be reused by drivers other than mdev.
> 
> The allocated PASID also needs to be installed into the parent device.
> If the mdev module knows the PASID, we can do that by adding
> set_pasid(mdev, pasid) and clear_pasid(mdev, pasid) operations to
> mdev_parent_ops.

Your idea is fascinating. Pls feel free let us know if we missed any from you. :)

> Thanks,
> Jean

Thanks,
Yi Liu