Re: [PATCH v4 6/8] vfio/mdev: Add iommu place holders in mdev_device

From: Lu Baolu
Date: Thu Nov 15 2018 - 20:23:39 EST

Hi,

On 11/16/18 5:31 AM, Kirti Wankhede wrote:


On 11/7/2018 7:18 AM, Lu Baolu wrote:
Hi Alex,

On 11/7/18 7:53 AM, Alex Williamson wrote:
On Mon, 5 Nov 2018 15:34:06 +0800
Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> 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 two new members in struct 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.

* iommu_domain
ÂÂ - This is a place holder for an iommu domain. A domain
ÂÂÂÂ could be store here for later use once it has been
ÂÂÂÂ attached to the iommu_device of this mdev.

Below helpers are added to set and get above iommu device
and iommu domain pointers.

* 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.

* mdev_set/get_iommu_domain(domain)
ÂÂ - A iommu domain which has been attached to the iommu
ÂÂÂÂ device in order to protect and isolate the mediated
ÂÂÂÂ device will be kept in the mdev data structure and
ÂÂÂÂ could be retrieved later.

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>
---
 drivers/vfio/mdev/mdev_core.c | 36 ++++++++++++++++++++++++++++++++
 drivers/vfio/mdev/mdev_private.h | 2 ++
 include/linux/mdev.h | 23 ++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/vfio/mdev/mdev_core.c
b/drivers/vfio/mdev/mdev_core.c
index 0212f0ee8aea..5119809225c5 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -390,6 +390,42 @@ 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);
+
+struct device *mdev_get_iommu_device(struct device *dev)
+{
+ÂÂÂ struct mdev_device *mdev = to_mdev_device(dev);
+
+ÂÂÂ return mdev->iommu_device;
+}
+EXPORT_SYMBOL(mdev_get_iommu_device);
+
+int mdev_set_iommu_domain(struct device *dev, void *domain)
+{
+ÂÂÂ struct mdev_device *mdev = to_mdev_device(dev);
+
+ÂÂÂ mdev->iommu_domain = domain;
+
+ÂÂÂ return 0;
+}
+EXPORT_SYMBOL(mdev_set_iommu_domain);
+
+void *mdev_get_iommu_domain(struct device *dev)
+{
+ÂÂÂ struct mdev_device *mdev = to_mdev_device(dev);
+
+ÂÂÂ return mdev->iommu_domain;
+}
+EXPORT_SYMBOL(mdev_get_iommu_domain);
+
 static int __init mdev_init(void)
 {
ÂÂÂÂÂ return mdev_bus_register();
diff --git a/drivers/vfio/mdev/mdev_private.h
b/drivers/vfio/mdev/mdev_private.h
index b5819b7d7ef7..c01518068e84 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -34,6 +34,8 @@ struct mdev_device {
ÂÂÂÂÂ struct list_head next;
ÂÂÂÂÂ struct kobject *type_kobj;
ÂÂÂÂÂ bool active;
+ÂÂÂ struct device *iommu_device;
+ÂÂÂ void *iommu_domain;
 };
  #define to_mdev_device(dev) container_of(dev, struct
mdev_device, dev)
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index b6e048e1045f..c46777d3e568 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -14,6 +14,29 @@
 #define MDEV_H
  struct mdev_device;
+struct iommu_domain;
+
+/*
+ * Called by the parent device driver to set the PCI device which
represents

s/PCI //

There is no requirement or expectation that the device is PCI.


Fair enough.

+ * this mdev in iommu protection scope. By default, the iommu device
is NULL,
+ * that indicates using vendor defined isolation.
+ *
+ * @dev: the mediated device that iommu will isolate.
+ * @iommu_device: a pci device which represents the iommu for @dev.
+ *
+ * Return 0 for success, otherwise negative error value.
+ */
+int mdev_set_iommu_device(struct device *dev, struct device
*iommu_device);
+
+struct device *mdev_get_iommu_device(struct device *dev);
+
+/*
+ * Called by vfio iommu modules to save the iommu domain after a
domain being
+ * attached to the mediated device.
+ */
+int mdev_set_iommu_domain(struct device *dev, void *domain);
+
+void *mdev_get_iommu_domain(struct device *dev);

I can't say I really understand the purpose of this, the cover letter
indicates this is a placeholder, should we add it separately when we
have a requirement for it?

Oh, I am sorry that I used a wrong word. It's not a placeholder for
something designed for future, but adding two members that will be used
in the following patches. Since they will be used in anther modules
(like vfio_iommu), we need function interfaces to get and set them.

mdev->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.

mdev->iommu_domain:
 - This is used to save the pointer of an iommu domain. Once
ÂÂÂ a domain has been attached to the iommu_device, it should
ÂÂÂ be stored here.


I don't see mdev->iommu_domain is used anywhere in this series of patch.
If this is not being used, then no need to save it. With that symbols
mdev_set/get_iommu_domain(domain) are not required.

Yes. We won't use mdev->iommu_domain in this patch series. It should be
used by mdev parent driver to retrieve the default pasid of the domain.
Something like:

domain = mdev_get_iommu_domain(dev)
pasid = iommu_domain_get_attr(domain, DOMAIN_ATTR_AUXD_ID);
reg_write(pasid_reg, pasid);

I am okay if we remove mdev_set/get_iommu_domain from this patch series
and add it later when the parent driver comes.


Please keep symbols mdev_set/get_iommu_device(dev, iommu_device) non-GPL
same as other exported symbols from mdev_core module.

Yes. It will be fixed in the next version.


Thanks,
Kirti


Best regards,
Lu Baolu