vfio/mdev: IOMMU aware mediated device
Subsystems (4)
v5 cover vfio/mdev: IOMMU aware mediated device
Hi,
The Mediate Device is a framework for fine-grained physical device sharing across the isolated domains. Currently the mdev framework is designed to be independent of the platform IOMMU support. As the result, the DMA isolation relies on the mdev parent device in a vendor specific way.
There are several cases where a mediated device could be protected and isolated by the platform IOMMU. For example, Intel vt-d rev3.0
[1] introduces a new translation mode called 'scalable mode', which enables PASID-granular translations. The vt-d scalable mode is the key ingredient for Scalable I/O Virtualization [2] [3] which allows sharing a device in minimal possible granularity (ADI - Assignable Device Interface).
A mediated device backed by an ADI could be protected and isolated by the IOMMU since 1) the parent device supports tagging an unique PASID to all DMA traffic out of the mediated device; and 2) the DMA translation unit (IOMMU) supports the PASID granular translation. We can apply IOMMU protection and isolation to this kind of devices just as what we are doing with an assignable PCI device.
In order to distinguish the IOMMU-capable mediated devices from those which still need to rely on parent devices, this patch set adds one new member 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.
Below helpers are added to set and get above iommu device in mdev core implementation.
* 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.
The mdev parent device driver could opt-in that the mdev could be fully isolated and protected by the IOMMU when the mdev is being created by invoking mdev_set_iommu_device() in its @create().
In the vfio_iommu_type1_attach_group(), a domain allocated through iommu_domain_alloc() will be attached to the mdev iommu device if an iommu device has been set. Otherwise, the dummy external domain will be used and all the DMA isolation and protection are routed to parent driver as the result.
On IOMMU side, a basic requirement is allowing to attach multiple domains to a PCI device if the device advertises the capability and the IOMMU hardware supports finer granularity translations than the normal PCI Source ID based translation.
As the result, a PCI device could work in two modes: normal mode and auxiliary mode. In the normal mode, a pci device could be isolated in the Source ID granularity; the pci device itself could be assigned to a user application by attaching a single domain to it. In the auxiliary mode, a pci device could be isolated in finer granularity, hence subsets of the device could be assigned to different user level application by attaching a different domain to each subset.
Below APIs are introduced in iommu generic layer for aux-domain purpose:
* iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
- Check whether both IOMMU and device support IOMMU aux domain feature. Below aux-domain specific interfaces are available only after this returns true.
* iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX)
- Enable/disable device specific aux-domain feature.
* iommu_aux_attach_device(domain, dev)
- Attaches @domain to @dev in the auxiliary mode. Multiple domains could be attached to a single device in the auxiliary mode with each domain representing an isolated address space for an assignable subset of the device.
* iommu_aux_detach_device(domain, dev)
- Detach @domain which has been attached to @dev in the auxiliary mode.
* iommu_aux_get_pasid(domain, dev)
- Return ID used for finer-granularity DMA translation. For the Intel Scalable IOV usage model, this will be a PASID. The device which supports Scalable IOV needs to write this ID to the device register so that DMA requests could be tagged with a right PASID prefix.
In order for the ease of discussion, sometimes we call "a domain in auxiliary mode' or simply 'an auxiliary domain' when a domain is attached to a device for finer granularity translations. But we need to keep in mind that this doesn't mean there is a differnt domain type. A same domain could be bound to a device for Source ID based translation, and bound to another device for finer granularity translation at the same time.
This patch series extends both IOMMU and vfio components to support mdev device passing through when it could be isolated and protected by the IOMMU units. The first part of this series (PATCH 1/08~5/08) adds the interfaces and implementation of the multiple domains per device. The second part (PATCH 6/08~8/08) adds the iommu device attribute to each mdev, determines isolation type according to the existence of an iommu device when attaching group in vfio type1 iommu module, and attaches the domain to iommu aware mediated devices.
References:
[1] https://software.intel.com/en-us/download/intel-virtualization-technology-for-directed-io-architecture-specification
[2] https://software.intel.com/en-us/download/intel-scalable-io-virtualization-technical-specification
[3] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf
Best regards,
Lu Baolu
Change log:
v4->v5:
- The iommu APIs have been updated with Joerg's proposal posted
here https://www.spinics.net/lists/iommu/msg31874.html.
- Some typos in commit message and comments have been fixed.
- PATCH 3/8 was split from 4/8 to ease code review.
- mdev->domain was removed and could bring back when there's a real consumer.
- Other code review comments I received during v4 review period except the EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL in PATCH 6/8.
- Rebase all patches to 5.0-rc1.
v3->v4:
- Use aux domain specific interfaces for domain attach and detach.
- Rebase all patches to 4.20-rc1.
v2->v3:
- Remove domain type enum and use a pointer on mdev_device instead.
- Add a generic interface for getting/setting per device iommu attributions. And use it for query aux domain capability, enable aux domain and disable aux domain purpose.
- Reuse iommu_domain_get_attr() to retrieve the id in a aux domain.
- We discussed the impact of the default domain implementation on reusing iommu_at(de)tach_device() interfaces. We agreed that reusing iommu_at(de)tach_device() interfaces is the right direction and we could tweak the code to remove the impact.
https://www.spinics.net/lists/kvm/msg175285.html
- Removed the RFC tag since no objections received.
- This patch has been submitted separately.
https://www.spinics.net/lists/kvm/msg173936.html
v1->v2:
- Rewrite the patches with the concept of auxiliary domains.
Lu Baolu (8):
iommu: Add APIs for multiple domains per device iommu/vt-d: Add per-device IOMMU feature ops entries iommu/vt-d: Move common code out of iommu_attch_device() iommu/vt-d: Aux-domain specific domain attach/detach iommu/vt-d: Return ID associated with an auxiliary domain vfio/mdev: Add iommu related member in mdev_device vfio/type1: Add domain at(de)taching group helpers vfio/type1: Handle different mdev isolation type
drivers/iommu/intel-iommu.c | 304 ++++++++++++++++++++++++++++--- drivers/iommu/iommu.c | 80 ++++++++ drivers/vfio/mdev/mdev_core.c | 18 ++ drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/vfio_iommu_type1.c | 132 ++++++++++++-- include/linux/intel-iommu.h | 11 ++ include/linux/iommu.h | 61 +++++++ include/linux/mdev.h | 14 ++ 8 files changed, 585 insertions(+), 36 deletions(-)
Signature · 1 line
2.17.1
PATCH v5 1/8 iommu: Add APIs for multiple domains per device
Sharing a physical PCI device in a finer-granularity way is becoming a consensus in the industry. IOMMU vendors are also engaging efforts to support such sharing as well as possible. Among the efforts, the capability of support finer-granularity DMA isolation is a common requirement due to the security consideration. With finer-granularity DMA isolation, all DMA requests out of or to a subset of a physical PCI device can be protected by the IOMMU. As a result, there is a request in software to attach multiple domains to a physical PCI device. One example of such use model is the Intel Scalable IOV [1] [2]. The Intel vt-d 3.0 spec [3] introduces the scalable mode which enables PASID granularity DMA isolation.
This adds the APIs to support multiple domains per device. In order to ease the discussions, we call it 'a domain in auxiliary mode' or simply 'auxiliary domain' when multiple domains are attached to a physical device.
The APIs include:
* iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
- Check whether both IOMMU and device support IOMMU aux domain feature. Below aux-domain specific interfaces are available only after this returns true.
* iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX)
- Enable/disable device specific aux-domain feature.
* iommu_aux_attach_device(domain, dev)
- Attaches @domain to @dev in the auxiliary mode. Multiple domains could be attached to a single device in the auxiliary mode with each domain representing an isolated address space for an assignable subset of the device.
* iommu_aux_detach_device(domain, dev)
- Detach @domain which has been attached to @dev in the auxiliary mode.
* iommu_aux_get_pasid(domain, dev)
- Return ID used for finer-granularity DMA translation. For the Intel Scalable IOV usage model, this will be a PASID. The device which supports Scalable IOV needs to write this ID to the device register so that DMA requests could be tagged with a right PASID prefix.
This has been updated with the latest proposal from Joerg posted here [5].
Many people involved in discussions of this design.
Kevin Tian <[email protected]> Liu Yi L <[email protected]> Ashok Raj <[email protected]> Sanjay Kumar <[email protected]> Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]> Alex Williamson <[email protected]> Jean-Philippe Brucker <[email protected]> Joerg Roedel <[email protected]>
and some discussions can be found here [4] [5].
[1] https://software.intel.com/en-us/download/intel-scalable-io-virtualization-technical-specification
[2] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf
[3] https://software.intel.com/en-us/download/intel-virtualization-technology-for-directed-io-architecture-specification
[4] https://lkml.org/lkml/2018/7/26/4
[5] https://www.spinics.net/lists/iommu/msg31874.html
--- drivers/iommu/iommu.c | 80 +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+)
drivers/iommu/iommu.c +80 -0
@@ -2033,3 +2033,83 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)return 0 ; }EXPORT_SYMBOL_GPL (iommu_fwspec_add_ids); + +/* +* Per device IOMMU features. +*/ +bool iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_has_feat) +return ops->dev_has_feat (dev, feat); + +return false; +} +EXPORT_SYMBOL_GPL (iommu_dev_has_feature); + +int iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_enable_feat) +return ops->dev_enable_feat (dev, feat); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (iommu_dev_enable_feature); + +int iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_disable_feat) +return ops->dev_disable_feat (dev, feat); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (iommu_dev_disable_feature); + +/* +* Aux-domain specific attach/detach. +* +* Only works if iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX) returns true. +* Also, as long as domains are attached to a device through this interface, +* any tries to call iommu_attach_device() should fail (iommu_detach_device() +* can't fail, so we fail on the tryint to re-attach). This should make us safe +* against a device being attached to a guest as a whole while there are still +* pasid users on it (aux and sva). +*/ +int iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev) +{ +int ret = -ENODEV ; + +if (domain->ops->aux_attach_dev) + ret = domain->ops->aux_attach_dev (domain, dev); + +if (!ret) +trace_attach_device_to_domain (dev); + +return ret; +} +EXPORT_SYMBOL_GPL (iommu_aux_attach_device); + +void iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev) +{ +if (domain->ops->aux_detach_dev) { + domain->ops->aux_detach_dev (domain, dev); +trace_detach_device_from_domain (dev); + } +} +EXPORT_SYMBOL_GPL (iommu_aux_detach_device); + +int iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev) +{ +int ret = -ENODEV ; + +if (domain->ops->aux_get_pasid) + ret = domain->ops->aux_get_pasid (domain, dev); + +return ret; +} +EXPORT_SYMBOL_GPL (iommu_aux_get_pasid);
include/linux/iommu.h +61 -0
91 lines not on this page · raw message
Signature · 1 line
2.17.1
Re: [PATCH v5 1/8] iommu: Add APIs for multiple domains per device
Quoted 68 lines · On Thu, 10 Jan 2019 11:00:20 +0800 Lu Baolu <[email protected]> wrote:
Sharing a physical PCI device in a finer-granularity way is becoming a consensus in the industry. IOMMU vendors are also engaging efforts to support such sharing as well as possible. Among the efforts, the capability of support finer-granularity DMA isolation is a common requirement due to the security consideration. With finer-granularity DMA isolation, all DMA requests out of or to a subset of a physical PCI device can be protected by the IOMMU. As a result, there is a request in software to attach multiple domains to a physical PCI device. One example of such use model is the Intel Scalable IOV [1] [2]. The Intel vt-d 3.0 spec [3] introduces the scalable mode which enables PASID granularity DMA isolation.
This adds the APIs to support multiple domains per device. In order to ease the discussions, we call it 'a domain in auxiliary mode' or simply 'auxiliary domain' when multiple domains are attached to a physical device.
The APIs include:
*
iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
- Check whether both IOMMU and device support IOMMU aux domain feature. Below aux-domain specific interfaces are available only after this returns true.* iommu_dev_enable/disable_feature(dev,
IOMMU_DEV_FEAT_AUX)
- Enable/disable device specific aux-domain feature.*
iommu_aux_attach_device(domain, dev)
- Attaches @domain to @dev in the auxiliary mode. Multiple domains could be attached to a single device in the auxiliary mode with each domain representing an isolated address space for an assignable subset of the device.*
iommu_aux_detach_device(domain, dev)
- Detach @domain which has been attached to @dev in the auxiliary mode.*
iommu_aux_get_pasid(domain, dev)
- Return ID used for finer-granularity DMA translation. For the Intel Scalable IOV usage model, this will be a PASID. The device which supports Scalable IOV needs to write this ID to the device register so that DMA requests could be tagged with a right PASID prefix.This has been updated with the latest proposal from Joerg posted here [5].
Many people involved in discussions of this design.
Kevin Tian <[email protected]>
Liu Yi L <[email protected]>
Ashok Raj <[email protected]>
Sanjay Kumar <[email protected]>
Jacob Pan <[email protected]> Alex Williamson <[email protected]> Jean-Philippe Brucker <[email protected]> Joerg Roedel <[email protected]>and some discussions can be found here [4] [5].
[1] https://software.intel.com/en-us/download/intel-scalable-io-virtualization-technical-specification
[2] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf
[3] https://software.intel.com/en-us/download/intel-virtualization-technology-for-directed-io-architecture-specification
[4] https://lkml.org/lkml/2018/7/26/4
[5] https://www.spinics.net/lists/iommu/msg31874.html
Quoted 8 lines
Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Kevin Tian <[email protected]>Cc Liu Yi L <[email protected]>Suggested-by Kevin Tian <[email protected]>Suggested-by Jean-Philippe Brucker <[email protected]>Suggested-by Joerg Roedel <[email protected]>Signed-off-by Lu Baolu <[email protected]>
One trivial comment inline.
Quoted 47 lines
--- drivers/iommu/iommu.c | 80 +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+)drivers/iommu/iommu.c +34
-0@@ -2033,3 +2033,83 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)return 0 ; }EXPORT_SYMBOL_GPL (iommu_fwspec_add_ids); + +/* +* Per device IOMMU features. +*/ +bool iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_has_feat) +return ops->dev_has_feat (dev, feat); + +return false; +} +EXPORT_SYMBOL_GPL (iommu_dev_has_feature); + +int iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_enable_feat) +return ops->dev_enable_feat (dev, feat); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (iommu_dev_enable_feature); + +int iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_disable_feat) +return ops->dev_disable_feat (dev, feat); + +return -ENODEV ;
drivers/iommu/iommu.c int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids) · line 2079
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+} +EXPORT_SYMBOL_GPL (iommu_dev_disable_feature); + +/* +* Aux-domain specific attach/detach. +* +* Only works if iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX) returns true. +* Also, as long as domains are attached to a device through this interface, +* any tries to call iommu_attach_device() should fail (iommu_detach_device() +* can't fail, so we fail on the tryint to re-attach). This should make us safe
when trying to re-attach. (perhaps?)
Quoted 136 lines
drivers/iommu/iommu.c +36
-0+* against a device being attached to a guest as a whole while there are still +* pasid users on it (aux and sva). +*/ +int iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev) +{ +int ret = -ENODEV ; + +if (domain->ops->aux_attach_dev) + ret = domain->ops->aux_attach_dev (domain, dev); + +if (!ret) +trace_attach_device_to_domain (dev); + +return ret; +} +EXPORT_SYMBOL_GPL (iommu_aux_attach_device); + +void iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev) +{ +if (domain->ops->aux_detach_dev) { + domain->ops->aux_detach_dev (domain, dev); +trace_detach_device_from_domain (dev); + } +} +EXPORT_SYMBOL_GPL (iommu_aux_detach_device); + +int iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev) +{ +int ret = -ENODEV ; + +if (domain->ops->aux_get_pasid) + ret = domain->ops->aux_get_pasid (domain, dev); + +return ret; +} +EXPORT_SYMBOL_GPL (iommu_aux_get_pasid);include/linux/iommu.h +61
-0@@ -156,6 +156,11 @@ struct iommu_resv_region {enum iommu_resv_type type; }; +/* Per device IOMMU features */ +enum iommu_dev_features { +IOMMU_DEV_FEAT_AUX ,/* Aux-domain feature */ +}; +#ifdef CONFIG_IOMMU_API /** @@ -183,6 +188,10 @@ struct iommu_resv_region {* @domain_window_enable: Configure and enable a particular window for a domain * @domain_window_disable: Disable a particular window for a domain * @of_xlate: add OF master IDs to iommu grouping +* @dev_has/enable/disable_feat: per device entries to check/enable/disable +* iommu specific features. +* @aux_attach/detach_dev: aux-domain specific attach/detach entries. +* @aux_get_pasid: get the pasid given an aux-domain * @pgsize_bitmap: bitmap of all possible supported page sizes */ struct iommu_ops {@@ -226,6 +235,16 @@ struct iommu_ops {int (*of_xlate)(struct device *dev,struct of_phandle_args *args);bool (*is_attach_deferred)(struct iommu_domain *domain,struct device *dev); +/* Per device IOMMU features */ +bool (*dev_has_feat)(struct device *dev,enum iommu_dev_features f); +int (*dev_enable_feat)(struct device *dev,enum iommu_dev_features f); +int (*dev_disable_feat)(struct device *dev,enum iommu_dev_features f); + +/* Aux-domain specific attach/detach entries */ +int (*aux_attach_dev)(struct iommu_domain *domain,struct device *dev); +void (*aux_detach_dev)(struct iommu_domain *domain,struct device *dev); +int (*aux_get_pasid)(struct iommu_domain *domain,struct device *dev); +unsigned long pgsize_bitmap; };@@ -412,6 +431,13 @@ static inline void dev_iommu_fwspec_set(struct device *dev,int iommu_probe_device (struct device *dev);void iommu_release_device (struct device *dev); +bool iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat); +int iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat); +int iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat); +int iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev); +void iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev); +int iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev); +#else /* CONFIG_IOMMU_API */ struct iommu_ops {};@@ -696,6 +722,41 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)return NULL ; } +static inline bool +iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat) +{ +return false; +} + +static inline int +iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat) +{ +return -ENODEV ; +} + +static inline int +iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat) +{ +return -ENODEV ; +} + +static inline int +iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev) +{ +return -ENODEV ; +} + +static inline void +iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev) +{ +} + +static inline int +iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev) +{ +return -ENODEV ; +} +#endif /* CONFIG_IOMMU_API */ #ifdef CONFIG_IOMMU_DEBUGFS
Re: [PATCH v5 1/8] iommu: Add APIs for multiple domains per device
Hi,
Quoted 80 lines · Jonathan Cameron
Quoted 68 lines · On Thu, 10 Jan 2019 11:00:20 +0800 Lu Baolu <[email protected]> wrote:
Sharing a physical PCI device in a finer-granularity way is becoming a consensus in the industry. IOMMU vendors are also engaging efforts to support such sharing as well as possible. Among the efforts, the capability of support finer-granularity DMA isolation is a common requirement due to the security consideration. With finer-granularity DMA isolation, all DMA requests out of or to a subset of a physical PCI device can be protected by the IOMMU. As a result, there is a request in software to attach multiple domains to a physical PCI device. One example of such use model is the Intel Scalable IOV [1] [2]. The Intel vt-d 3.0 spec [3] introduces the scalable mode which enables PASID granularity DMA isolation.
This adds the APIs to support multiple domains per device. In order to ease the discussions, we call it 'a domain in auxiliary mode' or simply 'auxiliary domain' when multiple domains are attached to a physical device.
The APIs include:
*
iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
- Check whether both IOMMU and device support IOMMU aux domain feature. Below aux-domain specific interfaces are available only after this returns true.* iommu_dev_enable/disable_feature(dev,
IOMMU_DEV_FEAT_AUX)
- Enable/disable device specific aux-domain feature.*
iommu_aux_attach_device(domain, dev)
- Attaches @domain to @dev in the auxiliary mode. Multiple domains could be attached to a single device in the auxiliary mode with each domain representing an isolated address space for an assignable subset of the device.*
iommu_aux_detach_device(domain, dev)
- Detach @domain which has been attached to @dev in the auxiliary mode.*
iommu_aux_get_pasid(domain, dev)
- Return ID used for finer-granularity DMA translation. For the Intel Scalable IOV usage model, this will be a PASID. The device which supports Scalable IOV needs to write this ID to the device register so that DMA requests could be tagged with a right PASID prefix.This has been updated with the latest proposal from Joerg posted here [5].
Many people involved in discussions of this design.
Kevin Tian <[email protected]>
Liu Yi L <[email protected]>
Ashok Raj <[email protected]>
Sanjay Kumar <[email protected]>
Jacob Pan <[email protected]> Alex Williamson <[email protected]> Jean-Philippe Brucker <[email protected]> Joerg Roedel <[email protected]>and some discussions can be found here [4] [5].
[1] https://software.intel.com/en-us/download/intel-scalable-io-virtualization-technical-specification
[2] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf
[3] https://software.intel.com/en-us/download/intel-virtualization-technology-for-directed-io-architecture-specification
[4] https://lkml.org/lkml/2018/7/26/4
[5] https://www.spinics.net/lists/iommu/msg31874.htmlQuoted 8 lines
Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Kevin Tian <[email protected]>Cc Liu Yi L <[email protected]>Suggested-by Kevin Tian <[email protected]>Suggested-by Jean-Philippe Brucker <[email protected]>Suggested-by Joerg Roedel <[email protected]>Signed-off-by Lu Baolu <[email protected]>
Quoted 1 line
One trivial comment inline.
Thank you!
Quoted 52 lines
Quoted 52 lines
--- drivers/iommu/iommu.c | 80 +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+)drivers/iommu/iommu.c +38
-0@@ -2033,3 +2033,83 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)return 0 ; }EXPORT_SYMBOL_GPL (iommu_fwspec_add_ids); + +/* +* Per device IOMMU features. +*/ +bool iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_has_feat) +return ops->dev_has_feat (dev, feat); + +return false; +} +EXPORT_SYMBOL_GPL (iommu_dev_has_feature); + +int iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_enable_feat) +return ops->dev_enable_feat (dev, feat); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (iommu_dev_enable_feature); + +int iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat) +{ +const struct iommu_ops *ops = dev->bus->iommu_ops; + +if (ops && ops->dev_disable_feat) +return ops->dev_disable_feat (dev, feat); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (iommu_dev_disable_feature); + +/*
Quoted 8 lines
drivers/iommu/iommu.c int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids) · line 2079
Quoted 6 lines
drivers/iommu/iommu.c +6
-0+* Aux-domain specific attach/detach. +* +* Only works if iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX) returns true. +* Also, as long as domains are attached to a device through this interface, +* any tries to call iommu_attach_device() should fail (iommu_detach_device() +* can't fail, so we fail on the tryint to re-attach). This should make us safe when trying to re-attach. (perhaps?)
Yes. I will fix it.
Best regards,
Lu Baolu
Quoted 140 lines
Quoted 137 lines
drivers/iommu/iommu.c +36
-0+* against a device being attached to a guest as a whole while there are still +* pasid users on it (aux and sva). +*/ +int iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev) +{ +int ret = -ENODEV ; + +if (domain->ops->aux_attach_dev) + ret = domain->ops->aux_attach_dev (domain, dev); + +if (!ret) +trace_attach_device_to_domain (dev); + +return ret; +} +EXPORT_SYMBOL_GPL (iommu_aux_attach_device); + +void iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev) +{ +if (domain->ops->aux_detach_dev) { + domain->ops->aux_detach_dev (domain, dev); +trace_detach_device_from_domain (dev); + } +} +EXPORT_SYMBOL_GPL (iommu_aux_detach_device); + +int iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev) +{ +int ret = -ENODEV ; + +if (domain->ops->aux_get_pasid) + ret = domain->ops->aux_get_pasid (domain, dev); + +return ret; +} +EXPORT_SYMBOL_GPL (iommu_aux_get_pasid);include/linux/iommu.h +61
-0@@ -156,6 +156,11 @@ struct iommu_resv_region {enum iommu_resv_type type; }; +/* Per device IOMMU features */ +enum iommu_dev_features { +IOMMU_DEV_FEAT_AUX ,/* Aux-domain feature */ +}; +#ifdef CONFIG_IOMMU_API /** @@ -183,6 +188,10 @@ struct iommu_resv_region {* @domain_window_enable: Configure and enable a particular window for a domain * @domain_window_disable: Disable a particular window for a domain * @of_xlate: add OF master IDs to iommu grouping +* @dev_has/enable/disable_feat: per device entries to check/enable/disable +* iommu specific features. +* @aux_attach/detach_dev: aux-domain specific attach/detach entries. +* @aux_get_pasid: get the pasid given an aux-domain * @pgsize_bitmap: bitmap of all possible supported page sizes */ struct iommu_ops {@@ -226,6 +235,16 @@ struct iommu_ops {int (*of_xlate)(struct device *dev,struct of_phandle_args *args);bool (*is_attach_deferred)(struct iommu_domain *domain,struct device *dev); +/* Per device IOMMU features */ +bool (*dev_has_feat)(struct device *dev,enum iommu_dev_features f); +int (*dev_enable_feat)(struct device *dev,enum iommu_dev_features f); +int (*dev_disable_feat)(struct device *dev,enum iommu_dev_features f); + +/* Aux-domain specific attach/detach entries */ +int (*aux_attach_dev)(struct iommu_domain *domain,struct device *dev); +void (*aux_detach_dev)(struct iommu_domain *domain,struct device *dev); +int (*aux_get_pasid)(struct iommu_domain *domain,struct device *dev); +unsigned long pgsize_bitmap; };@@ -412,6 +431,13 @@ static inline void dev_iommu_fwspec_set(struct device *dev,int iommu_probe_device (struct device *dev);void iommu_release_device (struct device *dev); +bool iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat); +int iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat); +int iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat); +int iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev); +void iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev); +int iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev); +#else /* CONFIG_IOMMU_API */ struct iommu_ops {};@@ -696,6 +722,41 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)return NULL ; } +static inline bool +iommu_dev_has_feature (struct device *dev,enum iommu_dev_features feat) +{ +return false; +} + +static inline int +iommu_dev_enable_feature (struct device *dev,enum iommu_dev_features feat) +{ +return -ENODEV ; +} + +static inline int +iommu_dev_disable_feature (struct device *dev,enum iommu_dev_features feat) +{ +return -ENODEV ; +} + +static inline int +iommu_aux_attach_device (struct iommu_domain *domain,struct device *dev) +{ +return -ENODEV ; +} + +static inline void +iommu_aux_detach_device (struct iommu_domain *domain,struct device *dev) +{ +} + +static inline int +iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev) +{ +return -ENODEV ; +} +#endif /* CONFIG_IOMMU_API */ #ifdef CONFIG_IOMMU_DEBUGFS
PATCH v5 2/8 iommu/vt-d: Add per-device IOMMU feature ops entries
In reply to Lu BaoluThis adds the iommu ops entries for aux-domain per-device feature query and enable/disable.
--- drivers/iommu/intel-iommu.c | 86 +++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 1 + 2 files changed, 87 insertions(+)
drivers/iommu/intel-iommu.c +86 -0
@@ -2481,6 +2481,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, info->domain = domain; info->iommu = iommu; info->pasid_table =NULL ; + info->auxd_enabled =0 ;if (dev &&dev_is_pci (dev)) {struct pci_dev *pdev =to_pci_dev (info->dev);
@@ -5215,6 +5216,24 @@ static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,return phys; } +static inline bool scalable_mode_support (void ) +{ +struct dmar_drhd_unit *drhd; +struct intel_iommu *iommu; +bool ret = true; + +rcu_read_lock (); +for_each_active_iommu (iommu, drhd) { +if (!sm_supported (iommu)) { + ret = false; +break ; + } + } +rcu_read_unlock (); + +return ret; +} +static bool intel_iommu_capable (enum iommu_cap cap) {if (cap ==IOMMU_CAP_CACHE_COHERENCY )
@@ -5379,6 +5398,70 @@ struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) }#endif /* CONFIG_INTEL_IOMMU_SVM */ +static int intel_iommu_enable_auxd (struct device *dev) +{ +struct device_domain_info *info; +struct dmar_domain *domain; +unsigned long flags; + +if (!scalable_mode_support ()) +return -ENODEV ; + + domain =get_valid_domain_for_dev (dev); +if (!domain) +return -ENODEV ; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu; + info->auxd_enabled =1 ; +spin_unlock_irqrestore (&device_domain_lock, flags); + +return 0 ; +} + +static int intel_iommu_disable_auxd (struct device *dev) +{ +struct device_domain_info *info; +unsigned long flags; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu; +if (!WARN_ON (!info)) + info->auxd_enabled =0 ; +spin_unlock_irqrestore (&device_domain_lock, flags); + +return 0 ; +} + +static bool +intel_iommu_dev_has_feat (struct device *dev,enum iommu_dev_features feat) +{ +struct device_domain_info *info = dev->archdata.iommu; + +if (feat ==IOMMU_DEV_FEAT_AUX ) +return scalable_mode_support () && info && info->auxd_enabled; + +return false; +} + +static int +intel_iommu_dev_enable_feat (struct device *dev,enum iommu_dev_features feat) +{ +if (feat ==IOMMU_DEV_FEAT_AUX ) +return intel_iommu_enable_auxd (dev); + +return -ENODEV ; +} + +static int +intel_iommu_dev_disable_feat (struct device *dev,enum iommu_dev_features feat) +{ +if (feat ==IOMMU_DEV_FEAT_AUX ) +return intel_iommu_disable_auxd (dev); + +return -ENODEV ; +} +const struct iommu_ops intel_iommu_ops = { .capable = intel_iommu_capable, .domain_alloc = intel_iommu_domain_alloc,
@@ -5393,6 +5476,9 @@ const struct iommu_ops intel_iommu_ops = { .get_resv_regions = intel_iommu_get_resv_regions, .put_resv_regions = intel_iommu_put_resv_regions, .device_group = pci_device_group, + .dev_has_feat = intel_iommu_dev_has_feat, + .dev_enable_feat = intel_iommu_dev_enable_feat, + .dev_disable_feat = intel_iommu_dev_disable_feat, .pgsize_bitmap = INTEL_IOMMU_PGSIZES, };
include/linux/intel-iommu.h +1 -0
@@ -571,6 +571,7 @@ struct device_domain_info { u8 pri_enabled:1; u8 ats_supported:1; u8 ats_enabled:1; + u8 auxd_enabled:1; /* Multiple domains per device */u8 ats_qdep;struct device *dev;/* it's NULL for PCIe-to-PCI bridge */ struct intel_iommu *iommu;/* IOMMU used by this device */
Signature · 1 line
2.17.1
Joerg Roedel
maintainerRe: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi,
this looks a bit confusing to me because I can see no checking whether the device actually supports scalable mode. More below:
Quoted 10 lines · Lu Baolu
drivers/iommu/intel-iommu.c +10
-0+static int intel_iommu_enable_auxd (struct device *dev) +{ +struct device_domain_info *info; +struct dmar_domain *domain; +unsigned long flags; + +if (!scalable_mode_support ()) +return -ENODEV ; + + domain =get_valid_domain_for_dev (dev);
drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5420
Quoted 10 lines
drivers/iommu/intel-iommu.c +10
-0+if (!domain) +return -ENODEV ; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu; + info->auxd_enabled =1 ; +spin_unlock_irqrestore (&device_domain_lock, flags); + +return 0 ; +}
This code sets a flag to mark scalable mode enabled. Doesn't the device need some handling too, like enabling the PASID capability and all?
drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5445
Quoted 11 lines
drivers/iommu/intel-iommu.c +11
-0+ +static bool +intel_iommu_dev_has_feat (struct device *dev,enum iommu_dev_features feat) +{ +struct device_domain_info *info = dev->archdata.iommu; + +if (feat ==IOMMU_DEV_FEAT_AUX ) +return scalable_mode_support () && info && info->auxd_enabled; + +return false; +}
Why is this checking the auxd_enabled flag? The function should just return whether the device _supports_ scalable mode, not whether it is enabled.
Regards,
Joerg
Re: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi Joerg,
Thanks for reviewing my patch.
Quoted 4 lines · Joerg Roedel
Hi,
this looks a bit confusing to me because I can see no checking whether the device actually supports scalable mode.
Yes. I should put some checking there. Device scalable mode capability is exposed in PCI extended capability list.
Quoted 18 lines
More below:
Quoted 15 lines · Lu Baolu
drivers/iommu/intel-iommu.c +15
-0+static int intel_iommu_enable_auxd (struct device *dev) +{ +struct device_domain_info *info; +struct dmar_domain *domain; +unsigned long flags; + +if (!scalable_mode_support ()) +return -ENODEV ; + + domain =get_valid_domain_for_dev (dev); +if (!domain) +return -ENODEV ; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu;
Quoted 8 lines
drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5420
Quoted 5 lines
drivers/iommu/intel-iommu.c +5
-0+ info->auxd_enabled =1 ; +spin_unlock_irqrestore (&device_domain_lock, flags); + +return 0 ; +}This code sets a flag to mark scalable mode enabled. Doesn't the device need some handling too, like enabling the PASID capability and all?
Yes. My design was rough. We should prepare the device for scalable mode instead of assuming that everything is ready.
Quoted 14 lines
drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5445
Quoted 12 lines
drivers/iommu/intel-iommu.c +11
-0+ +static bool +intel_iommu_dev_has_feat (struct device *dev,enum iommu_dev_features feat) +{ +struct device_domain_info *info = dev->archdata.iommu; + +if (feat ==IOMMU_DEV_FEAT_AUX ) +return scalable_mode_support () && info && info->auxd_enabled; + +return false; +}Why is this checking the
auxd_enabledflag?
We need an API to check whether this feature is enabled. In vfio, it is used like below,
if (iommu_dev_has_feat(dev, FEAT_AUX_DOMAIN))
iommu_aux_attach_device(dev, domain)
else
iommu_attach_device(dev, domain)
Quoted 3 lines
The function should just
return whether the device _supports_ scalable mode, not whether it is enabled.
Do we want to have an API to tell whether device has aux-domain feature? It could be included in the enable API. The enable API returns failure if device doesn't support aux-domain.
Quoted 5 lines
Regards,
Joerg
Best regards,
Lu Baolu
Re: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
In reply to Joerg RoedelHi Joerg,
Quoted 6 lines · Joerg Roedel
Quoted 6 lines
drivers/iommu/intel-iommu.c +6
-0+ +static bool +intel_iommu_dev_has_feat (struct device *dev,enum iommu_dev_features feat) +{ +struct device_domain_info *info = dev->archdata.iommu; +
Quoted 8 lines
drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5445
Quoted 5 lines
drivers/iommu/intel-iommu.c +5
-0+if (feat ==IOMMU_DEV_FEAT_AUX ) +return scalable_mode_support () && info && info->auxd_enabled; + +return false; +}Why is this checking the
auxd_enabledflag? The function should just return whether thedevice_supports_scalable mode, not whether it is enabled.
Yes, as the API name implies, it should return the device capability instead of enable/disable status. I misused this API in the IOMMU driver.
Since we already have iommu_dev_enable/disable_feature() to enable and disable an iommu specific feature, is it possible to add another API to query whether a specific feature has been enabled?
How about
bool iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)?
This is necessary for the third party drivers (like vfio) to determine which domain attach interface it should use:
if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
iommmu_aux_attach_device(domain, dev)
else
iommu_attach_device(domain, dev)
Best regards,
Lu Baolu
Joerg Roedel
maintainerRe: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi Lu Baolu,
Quoted 1 line · Lu Baolu
bool
iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)?
Looks good. Having a function to check for enabled features is certainly a good thing.
Regards,
Joerg
PATCH v5 3/8 iommu/vt-d: Move common code out of iommu_attch_device()
In reply to Lu BaoluThis part of code could be used by both normal and aux domain specific attach entries. Hence move them into a common function to avoid duplication.
--- drivers/iommu/intel-iommu.c | 60 ++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 24 deletions(-)
drivers/iommu/intel-iommu.c +36 -24
@@ -5058,35 +5058,14 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)domain_exit (to_dmar_domain (domain)); } -static int intel_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +static int prepare_domain_attach_device (struct iommu_domain *domain, +struct device *dev) {struct dmar_domain *dmar_domain =to_dmar_domain (domain);struct intel_iommu *iommu;int addr_width;u8 bus, devfn; -if (device_is_rmrr_locked (dev)) { -dev_warn (dev,"Device is ineligible for IOMMU domain attach due to platform RMRR requirement. Contact your platform vendor.\n" ); -return -EPERM ; - } - -/* normally dev is not mapped */ -if (unlikely (domain_context_mapped (dev))) { -struct dmar_domain *old_domain; - - old_domain =find_domain (dev); -if (old_domain) { -rcu_read_lock (); -dmar_remove_one_dev_info (old_domain, dev); -rcu_read_unlock (); - -if (!domain_type_is_vm_or_si (old_domain) && -list_empty (&old_domain->devices)) -domain_exit (old_domain); - } - } - iommu =device_to_iommu (dev, &bus, &devfn);if (!iommu)return -ENODEV ;
@@ -5119,7 +5098,40 @@ static int intel_iommu_attach_device(struct iommu_domain *domain, dmar_domain->agaw--; } -return domain_add_dev_info (dmar_domain, dev); +return 0 ; +} + +static int intel_iommu_attach_device (struct iommu_domain *domain, +struct device *dev) +{ +int ret; + +if (device_is_rmrr_locked (dev)) { +dev_warn (dev,"Device is ineligible for IOMMU domain attach due to platform RMRR requirement. Contact your platform vendor.\n" ); +return -EPERM ; + } + +/* normally dev is not mapped */ +if (unlikely (domain_context_mapped (dev))) { +struct dmar_domain *old_domain; + + old_domain =find_domain (dev); +if (old_domain) { +rcu_read_lock (); +dmar_remove_one_dev_info (old_domain, dev); +rcu_read_unlock (); + +if (!domain_type_is_vm_or_si (old_domain) && +list_empty (&old_domain->devices)) +domain_exit (old_domain); + } + } + + ret =prepare_domain_attach_device (domain, dev); +if (ret) +return ret; + +return domain_add_dev_info (to_dmar_domain (domain), dev); } static void intel_iommu_detach_device(struct iommu_domain *domain,
Signature · 1 line
2.17.1
Re: [PATCH v5 3/8] iommu/vt-d: Move common code out of iommu_attch_device()
Quoted 8 lines · On Thu, 10 Jan 2019 11:00:22 +0800 Lu Baolu <[email protected]> wrote:
This part of code could be used by both normal and aux domain specific attach entries. Hence move them into a common function to avoid duplication.
Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Kevin Tian <[email protected]>Signed-off-by Lu Baolu <[email protected]>
Another trivial one (it's going to be one of those days). Typo in the patch title.
Quoted 88 lines
--- drivers/iommu/intel-iommu.c | 60 ++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 24 deletions(-)drivers/iommu/intel-iommu.c +36
-24@@ -5058,35 +5058,14 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)domain_exit (to_dmar_domain (domain)); } -static int intel_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +static int prepare_domain_attach_device (struct iommu_domain *domain, +struct device *dev) {struct dmar_domain *dmar_domain =to_dmar_domain (domain);struct intel_iommu *iommu;int addr_width;u8 bus, devfn; -if (device_is_rmrr_locked (dev)) { -dev_warn (dev,"Device is ineligible for IOMMU domain attach due to platform RMRR requirement. Contact your platform vendor.\n" ); -return -EPERM ; - } - -/* normally dev is not mapped */ -if (unlikely (domain_context_mapped (dev))) { -struct dmar_domain *old_domain; - - old_domain =find_domain (dev); -if (old_domain) { -rcu_read_lock (); -dmar_remove_one_dev_info (old_domain, dev); -rcu_read_unlock (); - -if (!domain_type_is_vm_or_si (old_domain) && -list_empty (&old_domain->devices)) -domain_exit (old_domain); - } - } - iommu =device_to_iommu (dev, &bus, &devfn);if (!iommu)return -ENODEV ;@@ -5119,7 +5098,40 @@ static int intel_iommu_attach_device(struct iommu_domain *domain, dmar_domain->agaw--; } -return domain_add_dev_info (dmar_domain, dev); +return 0 ; +} + +static int intel_iommu_attach_device (struct iommu_domain *domain, +struct device *dev) +{ +int ret; + +if (device_is_rmrr_locked (dev)) { +dev_warn (dev,"Device is ineligible for IOMMU domain attach due to platform RMRR requirement. Contact your platform vendor.\n" ); +return -EPERM ; + } + +/* normally dev is not mapped */ +if (unlikely (domain_context_mapped (dev))) { +struct dmar_domain *old_domain; + + old_domain =find_domain (dev); +if (old_domain) { +rcu_read_lock (); +dmar_remove_one_dev_info (old_domain, dev); +rcu_read_unlock (); + +if (!domain_type_is_vm_or_si (old_domain) && +list_empty (&old_domain->devices)) +domain_exit (old_domain); + } + } + + ret =prepare_domain_attach_device (domain, dev); +if (ret) +return ret; + +return domain_add_dev_info (to_dmar_domain (domain), dev); } static void intel_iommu_detach_device(struct iommu_domain *domain,
PATCH v5 4/8 iommu/vt-d: Aux-domain specific domain attach/detach
In reply to Lu BaoluWhen multiple domains per device has been enabled by the device driver, the device will tag the default PASID for the domain to all DMA traffics out of the subset of this device; and the IOMMU should translate the DMA requests in PASID granularity.
This adds the intel_iommu_aux_attach/detach_device() ops to support managing PASID granular translation structures when the device driver has enabled multiple domains per device.
--- drivers/iommu/intel-iommu.c | 152 ++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 10 +++ 2 files changed, 162 insertions(+)
drivers/iommu/intel-iommu.c +152 -0
@@ -2482,6 +2482,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, info->iommu = iommu; info->pasid_table =NULL ; info->auxd_enabled =0 ; +INIT_LIST_HEAD (&info->auxiliary_domains);if (dev &&dev_is_pci (dev)) {struct pci_dev *pdev =to_pci_dev (info->dev);
@@ -5058,6 +5059,131 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)domain_exit (to_dmar_domain (domain)); } +/* +* Check whether a @domain could be attached to the @dev through the +* aux-domain attach/detach APIs. +*/ +static inline bool +is_aux_domain (struct device *dev,struct iommu_domain *domain) +{ +struct device_domain_info *info = dev->archdata.iommu; + +return info && info->auxd_enabled && + domain->type ==IOMMU_DOMAIN_UNMANAGED ; +} + +static void auxiliary_link_device (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info = dev->archdata.iommu; + +assert_spin_locked (&device_domain_lock); +if (WARN_ON (!info)) +return ; + + domain->auxd_refcnt++; +list_add (&domain->auxd, &info->auxiliary_domains); +} + +static void auxiliary_unlink_device (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info = dev->archdata.iommu; + +assert_spin_locked (&device_domain_lock); +if (WARN_ON (!info)) +return ; + +list_del (&domain->auxd); + domain->auxd_refcnt--; + +if (!domain->auxd_refcnt && domain->default_pasid >0 ) +intel_pasid_free_id (domain->default_pasid); +} + +static int aux_domain_add_dev (struct dmar_domain *domain, +struct device *dev) +{ +int ret; +u8 bus, devfn; +unsigned long flags; +struct intel_iommu *iommu; + + iommu =device_to_iommu (dev, &bus, &devfn); +if (!iommu) +return -ENODEV ; + +if (domain->default_pasid <=0 ) { +int pasid; + + pasid =intel_pasid_alloc_id (domain,PASID_MIN , +pci_max_pasids (to_pci_dev (dev)), +GFP_KERNEL ); +if (pasid <=0 ) { +pr_err ("Can't allocate default pasid\n" ); +return -ENODEV ; + } + domain->default_pasid = pasid; + } + +spin_lock_irqsave (&device_domain_lock, flags); +/* +* iommu->lock must be held to attach domain to iommu and setup the +* pasid entry for second level translation. +*/ +spin_lock (&iommu->lock); + ret =domain_attach_iommu (domain, iommu); +if (ret) + goto attach_failed; + +/* Setup the PASID entry for mediated devices: */ + ret =intel_pasid_setup_second_level (iommu, domain, dev, + domain->default_pasid); +if (ret) + goto table_failed; +spin_unlock (&iommu->lock); + +auxiliary_link_device (domain, dev); + +spin_unlock_irqrestore (&device_domain_lock, flags); + +return 0 ; + +table_failed: +domain_detach_iommu (domain, iommu); +attach_failed: +spin_unlock (&iommu->lock); +spin_unlock_irqrestore (&device_domain_lock, flags); +if (!domain->auxd_refcnt && domain->default_pasid >0 ) +intel_pasid_free_id (domain->default_pasid); + +return ret; +} + +static void aux_domain_remove_dev (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info; +struct intel_iommu *iommu; +unsigned long flags; + +if (!is_aux_domain (dev, &domain->domain)) +return ; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu; + iommu = info->iommu; + +auxiliary_unlink_device (domain, dev); + +spin_lock (&iommu->lock); +intel_pasid_tear_down_entry (iommu, dev, domain->default_pasid); +domain_detach_iommu (domain, iommu); +spin_unlock (&iommu->lock); + +spin_unlock_irqrestore (&device_domain_lock, flags); +} +static int prepare_domain_attach_device (struct iommu_domain *domain,struct device *dev) {
@@ -5111,6 +5237,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,return -EPERM ; } +if (is_aux_domain (dev, domain)) +return -EPERM ; +/* normally dev is not mapped */ if (unlikely (domain_context_mapped (dev))) {struct dmar_domain *old_domain;
@@ -5134,12 +5263,33 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,return domain_add_dev_info (to_dmar_domain (domain), dev); } +static int intel_iommu_aux_attach_device (struct iommu_domain *domain, +struct device *dev) +{ +int ret; + +if (!is_aux_domain (dev, domain)) +return -EPERM ; + + ret =prepare_domain_attach_device (domain, dev); +if (ret) +return ret; + +return aux_domain_add_dev (to_dmar_domain (domain), dev); +} +static void intel_iommu_detach_device (struct iommu_domain *domain,struct device *dev) {dmar_remove_one_dev_info (to_dmar_domain (domain), dev); } +static void intel_iommu_aux_detach_device (struct iommu_domain *domain, +struct device *dev) +{ +aux_domain_remove_dev (to_dmar_domain (domain), dev); +} +static int intel_iommu_map (struct iommu_domain *domain,unsigned long iova,phys_addr_t hpa,size_t size,int iommu_prot)
@@ -5480,6 +5630,8 @@ const struct iommu_ops intel_iommu_ops = { .domain_free = intel_iommu_domain_free, .attach_dev = intel_iommu_attach_device, .detach_dev = intel_iommu_detach_device, + .aux_attach_dev = intel_iommu_aux_attach_device, + .aux_detach_dev = intel_iommu_aux_detach_device, .map = intel_iommu_map, .unmap = intel_iommu_unmap, .iova_to_phys = intel_iommu_iova_to_phys,
include/linux/intel-iommu.h +10 -0
@@ -492,9 +492,11 @@ struct dmar_domain {/* Domain ids per IOMMU. Use u16 since * domain ids are 16 bit wide according * to VT-d spec, section 9.3 */ +unsigned int auxd_refcnt;/* Refcount of auxiliary attaching */ bool has_iotlb_device;struct list_head devices;/* all devices' list */ +struct list_head auxd;/* link to device's auxiliary list */ struct iova_domain iovad;/* iova's that belong to this domain */ struct dma_pte *pgd;/* virtual address */
@@ -513,6 +515,11 @@ struct dmar_domain {2 == 1GiB, 3 == 512GiB, 4 == 1TiB */ u64 max_addr;/* maximum mapped address */ +int default_pasid;/* +* The default pasid used for non-SVM +* traffic on mediated devices. +*/ +struct iommu_domain domain;/* generic domain data structure for iommu core */ };
@@ -562,6 +569,9 @@ struct device_domain_info {struct list_head link;/* link to domain siblings */ struct list_head global;/* link to global list */ struct list_head table;/* link to pasid table */ +struct list_head auxiliary_domains;/* auxiliary domains +* attached to this device +*/ u8 bus;/* PCI bus number */ u8 devfn;/* PCI devfn number */ u16 pfsid;/* SRIOV physical function source ID */
Signature · 1 line
2.17.1
Re: [PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
Quoted 11 lines · On Thu, 10 Jan 2019 11:00:23 +0800 Lu Baolu <[email protected]> wrote:
When multiple domains per device has been enabled by the device driver, the device will tag the default PASID for the domain to all DMA traffics out of the subset of this device; and the IOMMU should translate the DMA requests in PASID granularity.
This adds the intel_iommu_aux_attach/detach_device() ops to support managing PASID granular translation structures when the device driver has enabled multiple domains per device.
Quoted 6 lines
Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Kevin Tian <[email protected]>Signed-off-by Sanjay Kumar <[email protected]>Signed-off-by Liu Yi L <[email protected]>Signed-off-by Lu Baolu <[email protected]>
The following is probably a rather naive review given I don't know the driver or hardware well at all. Still, it seems like things are a lot less balanced than I'd expect and isn't totally obvious to me why that is.
Quoted 18 lines
--- drivers/iommu/intel-iommu.c | 152 ++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 10 +++ 2 files changed, 162 insertions(+)drivers/iommu/intel-iommu.c +1
-0@@ -2482,6 +2482,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, info->iommu = iommu; info->pasid_table =NULL ; info->auxd_enabled =0 ; +INIT_LIST_HEAD (&info->auxiliary_domains);if (dev &&dev_is_pci (dev)) {struct pci_dev *pdev =to_pci_dev (info->dev);
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5067
Quoted 9 lines
drivers/iommu/intel-iommu.c +6
-0@@ -5058,6 +5059,131 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)domain_exit (to_dmar_domain (domain)); } +/* +* Check whether a @domain could be attached to the @dev through the +* aux-domain attach/detach APIs. +*/ +static inline bool +is_aux_domain (struct device *dev,struct iommu_domain *domain)
I'm finding the distinction between an aux domain capability on a given device and whether one is actually in use to be obscured slightly in the function naming.
This one for example is actually checking if we have a domain that is capable of being enabled for aux domain use, but not yet actually in that mode?
Mind you I'm not sure I have a better answer for the naming. can_aux_domain_be_enabled? is_unattached_aux_domain?
Quoted 24 lines
drivers/iommu/intel-iommu.c +24
-0+{ +struct device_domain_info *info = dev->archdata.iommu; + +return info && info->auxd_enabled && + domain->type ==IOMMU_DOMAIN_UNMANAGED ; +} + +static void auxiliary_link_device (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info = dev->archdata.iommu; + +assert_spin_locked (&device_domain_lock); +if (WARN_ON (!info)) +return ; + + domain->auxd_refcnt++; +list_add (&domain->auxd, &info->auxiliary_domains); +} + +static void auxiliary_unlink_device (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info = dev->archdata.iommu;
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5101
Quoted 10 lines
drivers/iommu/intel-iommu.c +10
-0+ +assert_spin_locked (&device_domain_lock); +if (WARN_ON (!info)) +return ; + +list_del (&domain->auxd); + domain->auxd_refcnt--; + +if (!domain->auxd_refcnt && domain->default_pasid >0 ) +intel_pasid_free_id (domain->default_pasid);
This seems unbalanced wrt to what is happening in auxiliary_link_device. If this is necessary then it would be good to have comments saying why. To my uniformed eye, looks like we could do this at the end of aux_domain_remove_dev, except that we need to hold the lock. As such perhaps it makes sense to do the pasid allocation under that lock in the first place?
I'm not 100% sure, but is there a race if you get the final remove running against a new add currently?
Quoted 5 lines
drivers/iommu/intel-iommu.c +5
-0+} + +static int aux_domain_add_dev (struct dmar_domain *domain, +struct device *dev) +{
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5116
Quoted 10 lines
drivers/iommu/intel-iommu.c +10
-0+int ret; +u8 bus, devfn; +unsigned long flags; +struct intel_iommu *iommu; + + iommu =device_to_iommu (dev, &bus, &devfn); +if (!iommu) +return -ENODEV ; + +if (domain->default_pasid <=0 ) {
device_domain_lock isn't held, so we might be in process of removing, see the pasid as set, just as it becomes unset and hence leave here without one set?
Quoted 32 lines
drivers/iommu/intel-iommu.c +32
-0+int pasid; + + pasid =intel_pasid_alloc_id (domain,PASID_MIN , +pci_max_pasids (to_pci_dev (dev)), +GFP_KERNEL ); +if (pasid <=0 ) { +pr_err ("Can't allocate default pasid\n" ); +return -ENODEV ; + } + domain->default_pasid = pasid; + } + +spin_lock_irqsave (&device_domain_lock, flags); +/* +* iommu->lock must be held to attach domain to iommu and setup the +* pasid entry for second level translation. +*/ +spin_lock (&iommu->lock); + ret =domain_attach_iommu (domain, iommu); +if (ret) + goto attach_failed; + +/* Setup the PASID entry for mediated devices: */ + ret =intel_pasid_setup_second_level (iommu, domain, dev, + domain->default_pasid); +if (ret) + goto table_failed; +spin_unlock (&iommu->lock); + +auxiliary_link_device (domain, dev); + +spin_unlock_irqrestore (&device_domain_lock, flags);
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5158
Quoted 10 lines
drivers/iommu/intel-iommu.c +10
-0+ +return 0 ; + +table_failed: +domain_detach_iommu (domain, iommu); +attach_failed: +spin_unlock (&iommu->lock); +spin_unlock_irqrestore (&device_domain_lock, flags); +if (!domain->auxd_refcnt && domain->default_pasid >0 ) +intel_pasid_free_id (domain->default_pasid);
It would be odd for this to race against a remove, but in theory it 'might' I think, potentially giving a double free.
Quoted 122 lines
drivers/iommu/intel-iommu.c +54
-0+ +return ret; +} + +static void aux_domain_remove_dev (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info; +struct intel_iommu *iommu; +unsigned long flags; + +if (!is_aux_domain (dev, &domain->domain)) +return ; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu; + iommu = info->iommu; + +auxiliary_unlink_device (domain, dev); + +spin_lock (&iommu->lock); +intel_pasid_tear_down_entry (iommu, dev, domain->default_pasid); +domain_detach_iommu (domain, iommu); +spin_unlock (&iommu->lock); + +spin_unlock_irqrestore (&device_domain_lock, flags); +} +static int prepare_domain_attach_device (struct iommu_domain *domain,struct device *dev) {@@ -5111,6 +5237,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,return -EPERM ; } +if (is_aux_domain (dev, domain)) +return -EPERM ; +/* normally dev is not mapped */ if (unlikely (domain_context_mapped (dev))) {struct dmar_domain *old_domain;@@ -5134,12 +5263,33 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,return domain_add_dev_info (to_dmar_domain (domain), dev); } +static int intel_iommu_aux_attach_device (struct iommu_domain *domain, +struct device *dev) +{ +int ret; + +if (!is_aux_domain (dev, domain)) +return -EPERM ; + + ret =prepare_domain_attach_device (domain, dev); +if (ret) +return ret; + +return aux_domain_add_dev (to_dmar_domain (domain), dev); +} +static void intel_iommu_detach_device (struct iommu_domain *domain,struct device *dev) {dmar_remove_one_dev_info (to_dmar_domain (domain), dev); } +static void intel_iommu_aux_detach_device (struct iommu_domain *domain, +struct device *dev) +{ +aux_domain_remove_dev (to_dmar_domain (domain), dev); +} +static int intel_iommu_map (struct iommu_domain *domain,unsigned long iova,phys_addr_t hpa,size_t size,int iommu_prot)@@ -5480,6 +5630,8 @@ const struct iommu_ops intel_iommu_ops = { .domain_free = intel_iommu_domain_free, .attach_dev = intel_iommu_attach_device, .detach_dev = intel_iommu_detach_device, + .aux_attach_dev = intel_iommu_aux_attach_device, + .aux_detach_dev = intel_iommu_aux_detach_device, .map = intel_iommu_map, .unmap = intel_iommu_unmap, .iova_to_phys = intel_iommu_iova_to_phys,include/linux/intel-iommu.h +10
-0@@ -492,9 +492,11 @@ struct dmar_domain {/* Domain ids per IOMMU. Use u16 since * domain ids are 16 bit wide according * to VT-d spec, section 9.3 */ +unsigned int auxd_refcnt;/* Refcount of auxiliary attaching */ bool has_iotlb_device;struct list_head devices;/* all devices' list */ +struct list_head auxd;/* link to device's auxiliary list */ struct iova_domain iovad;/* iova's that belong to this domain */ struct dma_pte *pgd;/* virtual address */ @@ -513,6 +515,11 @@ struct dmar_domain {2 == 1GiB, 3 == 512GiB, 4 == 1TiB */ u64 max_addr;/* maximum mapped address */ +int default_pasid;/* +* The default pasid used for non-SVM +* traffic on mediated devices. +*/ +struct iommu_domain domain;/* generic domain data structure for iommu core */ };@@ -562,6 +569,9 @@ struct device_domain_info {struct list_head link;/* link to domain siblings */ struct list_head global;/* link to global list */ struct list_head table;/* link to pasid table */ +struct list_head auxiliary_domains;/* auxiliary domains +* attached to this device +*/ u8 bus;/* PCI bus number */ u8 devfn;/* PCI devfn number */ u16 pfsid;/* SRIOV physical function source ID */
Re: [PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
Hi,
Quoted 21 lines · Jonathan Cameron
Quoted 11 lines · On Thu, 10 Jan 2019 11:00:23 +0800 Lu Baolu <[email protected]> wrote:
When multiple domains per device has been enabled by the device driver, the device will tag the default PASID for the domain to all DMA traffics out of the subset of this device; and the IOMMU should translate the DMA requests in PASID granularity.
This adds the intel_iommu_aux_attach/detach_device() ops to support managing PASID granular translation structures when the device driver has enabled multiple domains per device.
Quoted 6 lines
Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Kevin Tian <[email protected]>Signed-off-by Sanjay Kumar <[email protected]>Signed-off-by Liu Yi L <[email protected]>Signed-off-by Lu Baolu <[email protected]>
Quoted 4 lines
The following is probably a rather naive review given I don't know the driver or hardware well at all. Still, it seems like things are a lot less balanced than I'd expect and isn't totally obvious to me why that is.
Thank you!
Quoted 33 lines
Quoted 19 lines
--- drivers/iommu/intel-iommu.c | 152 ++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 10 +++ 2 files changed, 162 insertions(+)drivers/iommu/intel-iommu.c +1
-0@@ -2482,6 +2482,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, info->iommu = iommu; info->pasid_table =NULL ; info->auxd_enabled =0 ; +INIT_LIST_HEAD (&info->auxiliary_domains);if (dev &&dev_is_pci (dev)) {struct pci_dev *pdev =to_pci_dev (info->dev);drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5067
Quoted 9 lines
drivers/iommu/intel-iommu.c +6
-0@@ -5058,6 +5059,131 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)domain_exit (to_dmar_domain (domain)); } +/* +* Check whether a @domain could be attached to the @dev through the +* aux-domain attach/detach APIs. +*/ +static inline bool +is_aux_domain (struct device *dev,struct iommu_domain *domain)I'm finding the distinction between an aux domain capability on a given device and whether one is actually in use to be obscured slightly in the function naming.
Quoted 8 lines
This one for example is actually checking if we have a domain that is capable of being enabled for aux domain use, but not yet actually in that mode?
Mind you I'm not sure I have a better answer for the naming.
can_aux_domain_be_enabled?is_unattached_aux_domain?
device aux mode vs. normal mode
===============================
When we talk about the auxiliary mode (simply aux-mode), it means "the device works in aux-mode or normal mode". "normal mode" means that the device (and it's corresponding IOMMU) supports only RID (PCI Request ID) based DMA translation; while, aux-mode means the the device (and it's IOMMU) supports fine-grained DMA translation, like PASID based DMA translation with Intel VT-d scalable mode.
We are adding below APIs to switch a device between these two modes:
int iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX)
And this API (still under discussion) to check which mode the device is working in:
bool iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
aux-domain
==========
If a device is working in aux-mode and we are going to attach a domain to this device, we say "this domain will be attached to the device in aux mode", and simply "aux domain". So a domain is "normal" when it is going to attach to a device in normal mode; and is "aux-domain" when it is going to attach to a device in aux mode.
Quoted 43 lines
Quoted 25 lines
drivers/iommu/intel-iommu.c +24
-0+{ +struct device_domain_info *info = dev->archdata.iommu; + +return info && info->auxd_enabled && + domain->type ==IOMMU_DOMAIN_UNMANAGED ; +} + +static void auxiliary_link_device (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info = dev->archdata.iommu; + +assert_spin_locked (&device_domain_lock); +if (WARN_ON (!info)) +return ; + + domain->auxd_refcnt++; +list_add (&domain->auxd, &info->auxiliary_domains); +} + +static void auxiliary_unlink_device (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info = dev->archdata.iommu;drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5101
Quoted 10 lines
drivers/iommu/intel-iommu.c +10
-0+ +assert_spin_locked (&device_domain_lock); +if (WARN_ON (!info)) +return ; + +list_del (&domain->auxd); + domain->auxd_refcnt--; + +if (!domain->auxd_refcnt && domain->default_pasid >0 ) +intel_pasid_free_id (domain->default_pasid);This seems unbalanced wrt to what is happening in
auxiliary_link_device. If this is necessary then it would be good to have comments saying why. To my uniformed eye, looks like we could do this at the end ofaux_domain_remove_dev, except that we need to hold the lock. As such perhaps it makes sense to do the pasid allocation under that lock in the first place?
Quoted 2 lines
I'm not 100% sure, but is there a race if you get the final remove running against a new add currently?
Yes. This is unbalanced.
I will move pasid free out here and it should be done with the lock held to avoid race.
Quoted 12 lines
Quoted 12 lines
drivers/iommu/intel-iommu.c +11
-0+} + +static int aux_domain_add_dev (struct dmar_domain *domain, +struct device *dev) +{ +int ret; +u8 bus, devfn; +unsigned long flags; +struct intel_iommu *iommu; + + iommu =device_to_iommu (dev, &bus, &devfn);
Quoted 8 lines
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5116
Quoted 4 lines
drivers/iommu/intel-iommu.c +4
-0+if (!iommu) +return -ENODEV ; + +if (domain->default_pasid <=0 ) {
device_domain_lockisn't held, so we might be in process of removing, see the pasid as set, just as it becomes unset and hence leave here without one set?
Good catch, the lock should be held.
Quoted 38 lines
Quoted 38 lines
drivers/iommu/intel-iommu.c +37
-0+int pasid; + + pasid =intel_pasid_alloc_id (domain,PASID_MIN , +pci_max_pasids (to_pci_dev (dev)), +GFP_KERNEL ); +if (pasid <=0 ) { +pr_err ("Can't allocate default pasid\n" ); +return -ENODEV ; + } + domain->default_pasid = pasid; + } + +spin_lock_irqsave (&device_domain_lock, flags); +/* +* iommu->lock must be held to attach domain to iommu and setup the +* pasid entry for second level translation. +*/ +spin_lock (&iommu->lock); + ret =domain_attach_iommu (domain, iommu); +if (ret) + goto attach_failed; + +/* Setup the PASID entry for mediated devices: */ + ret =intel_pasid_setup_second_level (iommu, domain, dev, + domain->default_pasid); +if (ret) + goto table_failed; +spin_unlock (&iommu->lock); + +auxiliary_link_device (domain, dev); + +spin_unlock_irqrestore (&device_domain_lock, flags); + +return 0 ; + +table_failed: +domain_detach_iommu (domain, iommu);
Quoted 8 lines
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5158
Quoted 5 lines
drivers/iommu/intel-iommu.c +5
-0+attach_failed: +spin_unlock (&iommu->lock); +spin_unlock_irqrestore (&device_domain_lock, flags); +if (!domain->auxd_refcnt && domain->default_pasid >0 ) +intel_pasid_free_id (domain->default_pasid);It would be odd for this to race against a remove, but in theory it 'might' I think, potentially giving a double free.
Yes. Should be done with the lock held.
Best regards,
Lu Baolu
Quoted 126 lines
Quoted 123 lines
drivers/iommu/intel-iommu.c +54
-0+ +return ret; +} + +static void aux_domain_remove_dev (struct dmar_domain *domain, +struct device *dev) +{ +struct device_domain_info *info; +struct intel_iommu *iommu; +unsigned long flags; + +if (!is_aux_domain (dev, &domain->domain)) +return ; + +spin_lock_irqsave (&device_domain_lock, flags); + info = dev->archdata.iommu; + iommu = info->iommu; + +auxiliary_unlink_device (domain, dev); + +spin_lock (&iommu->lock); +intel_pasid_tear_down_entry (iommu, dev, domain->default_pasid); +domain_detach_iommu (domain, iommu); +spin_unlock (&iommu->lock); + +spin_unlock_irqrestore (&device_domain_lock, flags); +} +static int prepare_domain_attach_device (struct iommu_domain *domain,struct device *dev) {@@ -5111,6 +5237,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,return -EPERM ; } +if (is_aux_domain (dev, domain)) +return -EPERM ; +/* normally dev is not mapped */ if (unlikely (domain_context_mapped (dev))) {struct dmar_domain *old_domain;@@ -5134,12 +5263,33 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,return domain_add_dev_info (to_dmar_domain (domain), dev); } +static int intel_iommu_aux_attach_device (struct iommu_domain *domain, +struct device *dev) +{ +int ret; + +if (!is_aux_domain (dev, domain)) +return -EPERM ; + + ret =prepare_domain_attach_device (domain, dev); +if (ret) +return ret; + +return aux_domain_add_dev (to_dmar_domain (domain), dev); +} +static void intel_iommu_detach_device (struct iommu_domain *domain,struct device *dev) {dmar_remove_one_dev_info (to_dmar_domain (domain), dev); } +static void intel_iommu_aux_detach_device (struct iommu_domain *domain, +struct device *dev) +{ +aux_domain_remove_dev (to_dmar_domain (domain), dev); +} +static int intel_iommu_map (struct iommu_domain *domain,unsigned long iova,phys_addr_t hpa,size_t size,int iommu_prot)@@ -5480,6 +5630,8 @@ const struct iommu_ops intel_iommu_ops = { .domain_free = intel_iommu_domain_free, .attach_dev = intel_iommu_attach_device, .detach_dev = intel_iommu_detach_device, + .aux_attach_dev = intel_iommu_aux_attach_device, + .aux_detach_dev = intel_iommu_aux_detach_device, .map = intel_iommu_map, .unmap = intel_iommu_unmap, .iova_to_phys = intel_iommu_iova_to_phys,include/linux/intel-iommu.h +10
-0@@ -492,9 +492,11 @@ struct dmar_domain {/* Domain ids per IOMMU. Use u16 since * domain ids are 16 bit wide according * to VT-d spec, section 9.3 */ +unsigned int auxd_refcnt;/* Refcount of auxiliary attaching */ bool has_iotlb_device;struct list_head devices;/* all devices' list */ +struct list_head auxd;/* link to device's auxiliary list */ struct iova_domain iovad;/* iova's that belong to this domain */ struct dma_pte *pgd;/* virtual address */ @@ -513,6 +515,11 @@ struct dmar_domain {2 == 1GiB, 3 == 512GiB, 4 == 1TiB */ u64 max_addr;/* maximum mapped address */ +int default_pasid;/* +* The default pasid used for non-SVM +* traffic on mediated devices. +*/ +struct iommu_domain domain;/* generic domain data structure for iommu core */ };@@ -562,6 +569,9 @@ struct device_domain_info {struct list_head link;/* link to domain siblings */ struct list_head global;/* link to global list */ struct list_head table;/* link to pasid table */ +struct list_head auxiliary_domains;/* auxiliary domains +* attached to this device +*/ u8 bus;/* PCI bus number */ u8 devfn;/* PCI devfn number */ u16 pfsid;/* SRIOV physical function source ID */
Re: [PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
Quoted 24 lines · On Tue, 15 Jan 2019 10:10:21 +0800 Lu Baolu <[email protected]> wrote:
Hi,
Quoted 21 lines · Jonathan Cameron
Quoted 17 lines · On Thu, 10 Jan 2019 11:00:23 +0800 Lu Baolu <[email protected]> wrote:
When multiple domains per device has been enabled by the device driver, the device will tag the default PASID for the domain to all DMA traffics out of the subset of this device; and the IOMMU should translate the DMA requests in PASID granularity.
This adds the intel_iommu_aux_attach/detach_device() ops to support managing PASID granular translation structures when the device driver has enabled multiple domains per device.
Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Kevin Tian <[email protected]>Signed-off-by Sanjay Kumar <[email protected]>Signed-off-by Liu Yi L <[email protected]>Signed-off-by Lu Baolu <[email protected]>
Quoted 6 lines
Quoted 4 lines
The following is probably a rather naive review given I don't know the driver or hardware well at all. Still, it seems like things are a lot less balanced than I'd expect and isn't totally obvious to me why that is.
Thank you!
You are welcome.
...
Quoted 39 lines
Quoted 11 lines
drivers/iommu/intel-iommu.c static void intel_iommu_domain_free(struct iommu_domain *domain) · line 5067
Quoted 6 lines
drivers/iommu/intel-iommu.c +6
-0+/* +* Check whether a @domain could be attached to the @dev through the +* aux-domain attach/detach APIs. +*/ +static inline bool +is_aux_domain (struct device *dev,struct iommu_domain *domain)I'm finding the distinction between an aux domain capability on a given device and whether one is actually in use to be obscured slightly in the function naming.
Quoted 8 lines
This one for example is actually checking if we have a domain that is capable of being enabled for aux domain use, but not yet actually in that mode?
Mind you I'm not sure I have a better answer for the naming.
can_aux_domain_be_enabled?is_unattached_aux_domain?device aux mode vs. normal mode
===============================When we talk about the auxiliary mode (simply aux-mode), it means "the device works in aux-mode or normal mode". "normal mode" means that the device (and it's corresponding IOMMU) supports only RID (PCI Request ID) based DMA translation; while, aux-mode means the the device (and it's IOMMU) supports fine-grained DMA translation, like PASID based DMA translation with Intel VT-d scalable mode.
We are adding below APIs to switch a device between these two modes:
int iommu_dev_enable/disable_feature(dev,
IOMMU_DEV_FEAT_AUX)And this API (still under discussion) to check which mode the device is working in:
bool
iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
Quoted 8 lines
aux-domain
==========If a device is working in aux-mode and we are going to attach a domain to this device, we say "this domain will be attached to the device in aux mode", and simply "aux domain". So a domain is "normal" when it is going to attach to a device in normal mode; and is "aux-domain" when it is going to attach to a device in aux mode.
Hmm.. OK I guess. It still feels like there is more need to refer to the docs than there should be. Still, your code and I may well never read it again so I don't mind :)
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
+{
PATCH v5 5/8 iommu/vt-d: Return ID associated with an auxiliary domain
In reply to Lu BaoluThis adds support to return the default pasid associated with an auxiliary domain. The PCI device which is bound with this domain should use this value as the pasid for all DMA requests of the subset of device which is isolated and protected with this domain.
--- drivers/iommu/intel-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/iommu/intel-iommu.c +10 -0
@@ -5624,6 +5624,15 @@ intel_iommu_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)return -ENODEV ; } +static int +intel_iommu_aux_get_pasid (struct iommu_domain *domain,struct device *dev) +{ +struct dmar_domain *dmar_domain =to_dmar_domain (domain); + +return dmar_domain->default_pasid >0 ? + dmar_domain->default_pasid : -EINVAL ; +} +const struct iommu_ops intel_iommu_ops = { .capable = intel_iommu_capable, .domain_alloc = intel_iommu_domain_alloc,
@@ -5632,6 +5641,7 @@ const struct iommu_ops intel_iommu_ops = { .detach_dev = intel_iommu_detach_device, .aux_attach_dev = intel_iommu_aux_attach_device, .aux_detach_dev = intel_iommu_aux_detach_device, + .aux_get_pasid = intel_iommu_aux_get_pasid, .map = intel_iommu_map, .unmap = intel_iommu_unmap, .iova_to_phys = intel_iommu_iova_to_phys,
Signature · 1 line
2.17.1
PATCH v5 6/8 vfio/mdev: Add iommu related member in mdev_device
In reply to Lu BaoluA 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.
--- drivers/vfio/mdev/mdev_core.c | 18 ++++++++++++++++++ drivers/vfio/mdev/mdev_private.h | 1 + include/linux/mdev.h | 14 ++++++++++++++ 3 files changed, 33 insertions(+)
drivers/vfio/mdev/mdev_core.c +18 -0
@@ -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); + +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); +static int __init mdev_init (void ) {return mdev_bus_register ();
drivers/vfio/mdev/mdev_private.h +1 -0
@@ -34,6 +34,7 @@ struct mdev_device {struct list_head next;struct kobject *type_kobj;bool active; +struct device *iommu_device; };#define to_mdev_device (dev) container_of(dev, struct mdev_device, dev)
include/linux/mdev.h +14 -0
@@ -15,6 +15,20 @@struct mdev_device ; +/* +* Called by the parent device driver to set the device which represents +* 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); +/** * struct mdev_parent_ops - Structure to be registered for each parent device to * register the device to mdev module.
Signature · 1 line
2.17.1
PATCH v5 7/8 vfio/type1: Add domain at(de)taching group helpers
In reply to Lu BaoluThis adds helpers to attach or detach a domain to a group. This will replace iommu_attach_group() which only works for non-mdev devices.
If a domain is attaching to a group which includes the mediated devices, it should attach to the iommu device (a pci device which represents the mdev in iommu scope) instead. The added helper supports attaching domain to groups for both pci and mdev devices.
--- drivers/vfio/vfio_iommu_type1.c | 84 ++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 7 deletions(-)
drivers/vfio/vfio_iommu_type1.c +77 -7
@@ -91,6 +91,7 @@ struct vfio_dma {struct vfio_group {struct iommu_group *iommu_group;struct list_head next; +bool mdev_group;/* An mdev group */ };/*
@@ -1298,6 +1299,75 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base)return ret; } +static struct device *vfio_mdev_get_iommu_device (struct device *dev) +{ +struct device *(*fn)(struct device *dev); +struct device *iommu_device; + + fn =symbol_get (mdev_get_iommu_device); +if (fn) { + iommu_device =fn (dev); +symbol_put (mdev_get_iommu_device); + +return iommu_device; + } + +return NULL ; +} + +static int vfio_mdev_attach_domain (struct device *dev,void *data) +{ +struct iommu_domain *domain = data; +struct device *iommu_device; + + iommu_device =vfio_mdev_get_iommu_device (dev); +if (iommu_device) { +if (iommu_dev_has_feature (iommu_device,IOMMU_DEV_FEAT_AUX )) +return iommu_aux_attach_device (domain, iommu_device); +else +return iommu_attach_device (domain, iommu_device); + } + +return -EINVAL ; +} + +static int vfio_mdev_detach_domain (struct device *dev,void *data) +{ +struct iommu_domain *domain = data; +struct device *iommu_device; + + iommu_device =vfio_mdev_get_iommu_device (dev); +if (iommu_device) { +if (iommu_dev_has_feature (iommu_device,IOMMU_DEV_FEAT_AUX )) +iommu_aux_detach_device (domain, iommu_device); +else +iommu_detach_device (domain, iommu_device); + } + +return 0 ; +} + +static int vfio_iommu_attach_group (struct vfio_domain *domain, +struct vfio_group *group) +{ +if (group->mdev_group) +return iommu_group_for_each_dev (group->iommu_group, + domain->domain, + vfio_mdev_attach_domain); +else +return iommu_attach_group (domain->domain, group->iommu_group); +} + +static void vfio_iommu_detach_group (struct vfio_domain *domain, +struct vfio_group *group) +{ +if (group->mdev_group) +iommu_group_for_each_dev (group->iommu_group, domain->domain, + vfio_mdev_detach_domain); +else +iommu_detach_group (domain->domain, group->iommu_group); +} +static int vfio_iommu_type1_attach_group (void *iommu_data,struct iommu_group *iommu_group) {
@@ -1373,7 +1443,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, goto out_domain; } - ret =iommu_attach_group (domain->domain, iommu_group); + ret =vfio_iommu_attach_group (domain, group);if (ret) goto out_domain;
@@ -1405,8 +1475,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,list_for_each_entry (d, &iommu->domain_list, next) {if (d->domain->ops == domain->domain->ops && d->prot == domain->prot) { -iommu_detach_group (domain->domain, iommu_group); -if (!iommu_attach_group (d->domain, iommu_group)) { +vfio_iommu_detach_group (domain, group); +if (!vfio_iommu_attach_group (d, group)) {list_add (&group->next, &d->group_list);iommu_domain_free (domain->domain);kfree (domain);
@@ -1414,7 +1484,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,return 0 ; } - ret =iommu_attach_group (domain->domain, iommu_group); + ret =vfio_iommu_attach_group (domain, group);if (ret) goto out_domain; }
@@ -1440,7 +1510,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,return 0 ; out_detach: -iommu_detach_group (domain->domain, iommu_group); +vfio_iommu_detach_group (domain, group); out_domain:iommu_domain_free (domain->domain); out_free:
@@ -1531,7 +1601,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,if (!group)continue ; -iommu_detach_group (domain->domain, iommu_group); +vfio_iommu_detach_group (domain, group);list_del (&group->next);kfree (group);/*
@@ -1596,7 +1666,7 @@ static void vfio_release_domain(struct vfio_domain *domain, bool external)list_for_each_entry_safe (group, group_tmp, &domain->group_list, next) {if (!external) -iommu_detach_group (domain->domain, group->iommu_group); +vfio_iommu_detach_group (domain, group);list_del (&group->next);kfree (group); }
Signature · 1 line
2.17.1
PATCH v5 8/8 vfio/type1: Handle different mdev isolation type
In reply to Lu BaoluThis adds the support to determine the isolation type of a mediated device group by checking whether it has an iommu device. If an iommu device exists, an iommu domain will be allocated and then attached to the iommu device. Otherwise, keep the same behavior as it is.
--- drivers/vfio/vfio_iommu_type1.c | 48 ++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 7 deletions(-)
drivers/vfio/vfio_iommu_type1.c +41 -7
@@ -1368,13 +1368,40 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain,iommu_detach_group (domain->domain, group->iommu_group); } +static bool vfio_bus_is_mdev (struct bus_type *bus) +{ +struct bus_type *mdev_bus; +bool ret = false; + + mdev_bus =symbol_get (mdev_bus_type); +if (mdev_bus) { + ret = (bus == mdev_bus); +symbol_put (mdev_bus_type); + } + +return ret; +} + +static int vfio_mdev_iommu_device (struct device *dev,void *data) +{ +struct device **old = data, *new; + + new =vfio_mdev_get_iommu_device (dev); +if (!new || (*old && *old != new)) +return -EINVAL ; + + *old = new; + +return 0 ; +} +static int vfio_iommu_type1_attach_group (void *iommu_data,struct iommu_group *iommu_group) {struct vfio_iommu *iommu = iommu_data;struct vfio_group *group;struct vfio_domain *domain, *d; -struct bus_type *bus =NULL , *mdev_bus; +struct bus_type *bus =NULL ;int ret;bool resv_msi, msi_remap;phys_addr_t resv_msi_base;
@@ -1409,23 +1436,30 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,if (ret) goto out_free; - mdev_bus =symbol_get (mdev_bus_type); +if (vfio_bus_is_mdev (bus)) { +struct device *iommu_device =NULL ; -if (mdev_bus) { -if ((bus == mdev_bus) && !iommu_present (bus)) { -symbol_put (mdev_bus_type); + group->mdev_group = true; + +/* Determine the isolation type */ + ret =iommu_group_for_each_dev (iommu_group, &iommu_device, + vfio_mdev_iommu_device); +if (ret || !iommu_device) {if (!iommu->external_domain) {INIT_LIST_HEAD (&domain->group_list); iommu->external_domain = domain; - }else + }else {kfree (domain); + }list_add (&group->next, &iommu->external_domain->group_list);mutex_unlock (&iommu->lock); +return 0 ; } -symbol_put (mdev_bus_type); + + bus = iommu_device->bus; } domain->domain =iommu_domain_alloc (bus);
Signature · 1 line
2.17.1