Re: [PATCH v8 7/9] vfio/mdev: Add iommu related member in mdev_device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jason,

On 4/7/21 4:00 AM, Jason Gunthorpe wrote:
On Mon, Mar 25, 2019 at 09:30:34AM +0800, Lu Baolu wrote:
A parent device might create different types of mediated
devices. For example, a mediated device could be created
by the parent device with full isolation and protection
provided by the IOMMU. One usage case could be found on
Intel platforms where a mediated device is an assignable
subset of a PCI, the DMA requests on behalf of it are all
tagged with a PASID. Since IOMMU supports PASID-granular
translations (scalable mode in VT-d 3.0), this mediated
device could be individually protected and isolated by an
IOMMU.

This patch adds a new member in the struct mdev_device to
indicate that the mediated device represented by mdev could
be isolated and protected by attaching a domain to a device
represented by mdev->iommu_device. It also adds a helper to
add or set the iommu device.

* mdev_device->iommu_device
   - This, if set, indicates that the mediated device could
     be fully isolated and protected by IOMMU via attaching
     an iommu domain to this device. If empty, it indicates
     using vendor defined isolation, hence bypass IOMMU.

* mdev_set/get_iommu_device(dev, iommu_device)
   - Set or get the iommu device which represents this mdev
     in IOMMU's device scope. Drivers don't need to set the
     iommu device if it uses vendor defined isolation.

Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
Cc: Liu Yi L <yi.l.liu@xxxxxxxxx>
Suggested-by: Kevin Tian <kevin.tian@xxxxxxxxx>
Suggested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx>
---
  drivers/vfio/mdev/mdev_core.c    | 18 ++++++++++++++++++
  drivers/vfio/mdev/mdev_private.h |  1 +
  include/linux/mdev.h             | 14 ++++++++++++++
  3 files changed, 33 insertions(+)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b96fedc77ee5..1b6435529166 100644
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -390,6 +390,24 @@ int mdev_device_remove(struct device *dev, bool force_remove)
  	return 0;
  }
+int mdev_set_iommu_device(struct device *dev, struct device *iommu_device)
+{
+	struct mdev_device *mdev = to_mdev_device(dev);
+
+	mdev->iommu_device = iommu_device;
+
+	return 0;
+}
+EXPORT_SYMBOL(mdev_set_iommu_device);

I was looking at these functions when touching the mdev stuff and I
have some concerns.

1) Please don't merge dead code. It is a year later and there is still
    no in-tree user for any of this. This is not our process. Even
    worse it was exported so it looks like this dead code is supporting
    out of tree modules.

2) Why is this like this? Every struct device already has a connection
    to the iommu layer and every mdev has a struct device all its own.

    Why did we need to add special 'if (mdev)' stuff all over the
    place? This smells like the same abuse Thomas
    and I pointed out for the interrupt domains.

    After my next series the mdev drivers will have direct access to
    the vfio_device. So an alternative to using the struct device, or
    adding 'if mdev' is to add an API to the vfio_device world to
    inject what iommu configuration is needed from that direction
    instead of trying to discover it from a struct device.

Just want to make sure that I understand you correctly.

We should use the existing IOMMU in-kernel APIs to connect mdev with the
iommu subsystem, so that the upper lays don't need to use something
like (if dev_is_mdev) to handle mdev differently. Do I get you
correctly?


3) The vfio_bus_is_mdev() and related symbol_get() nonsense in
    drivers/vfio/vfio_iommu_type1.c has to go, for the same reasons
    it was not acceptable to do this for the interrupt side either.

Yes. Agreed. I will look into it.


4) It seems pretty clear to me this will be heavily impacted by the
    /dev/ioasid discussion. Please consider removing the dead code now.

Basically, please fix this before trying to get idxd mdev merged as
the first user.

Jason


Best regards,
baolu



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux