Re: [PATCH v6 1/9] iommu: Add APIs for multiple domains per device

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi Jean,

On 2/13/19 7:55 PM, Jean-Philippe Brucker wrote:
> Hi,
> 
> I have a few boring nits and one question below

Thanks a lot for reviewing my patch.

> 
> On 13/02/2019 04:02, Lu Baolu 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.
> 
> That last sentence seems strange, how about "With finer-granularity DMA
> isolation, subsets of a PCI function can be isolated from each others by
> the IOMMU."

Yours looks better. Thanks!

> 
>> 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.
> 
> s/after/if/ since calling has_feature() shouldn't be a prerequisite to
> using the aux-domain interface (unlike calling enable_feature()).

After reconsideration, I think my comments about this API isn't
correct. It should be like,

* iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
   - Detect both IOMMU and PCI endpoint devices supporting the feature
     (aux-domain here) without host driver dependency.

* iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)
   - Check the enabling status of the feature (aux-domain here). The
     aux-domain interfaces are available only if this returns true.

> 
>>
>> * iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX)
>>    - Enable/disable device specific aux-domain feature.
>>
>> * iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)
>>    - Check whether the aux domain specific feature enabled or
>>      not.
> 
> "is enabled"

Sure.

> 
>>
>> * 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 <kevin.tian@intel.com>
>> Liu Yi L <yi.l.liu@intel.com>
>> Ashok Raj <ashok.raj@intel.com>
>> Sanjay Kumar <sanjay.k.kumar@intel.com>
>> Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Alex Williamson <alex.williamson@redhat.com>
>> Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> Joerg Roedel <joro@8bytes.org>
>>
>> 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
>>
>> Cc: Ashok Raj <ashok.raj@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Liu Yi L <yi.l.liu@intel.com>
>> Suggested-by: Kevin Tian <kevin.tian@intel.com>
>> Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> Suggested-by: Joerg Roedel <jroedel@suse.de>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 91 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/iommu.h | 70 +++++++++++++++++++++++++++++++++
>>   2 files changed, 161 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 3ed4db334341..d0b323e8357f 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2033,3 +2033,94 @@ 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);
>> +
>> +bool iommu_dev_feature_enabled(struct device *dev, enum
>> iommu_dev_features feat)
>> +{
>> +       const struct iommu_ops *ops = dev->bus->iommu_ops;
>> +
>> +       if (ops && ops->dev_feat_enabled)
>> +               return ops->dev_feat_enabled(dev, feat);
>> +
>> +       return false;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
>> +
>> +/*
>> + * Aux-domain specific attach/detach.
>> + *
>> + * Only works if iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX) returns
>> true.
> 
> Rather "Only works if iommu_dev_feature_enabled(dev, AUX) returns true"?

Yes. I will change this and above.

> 
>> + * 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
> 
> "when trying to re-attach"

Sure.

> 
>> us safe
>> + * against a device being attached to a guest as a whole while there
>> are still
>> + * pasid users on it (aux and sva).
> 
> A related question: is calling iommu_dev_disable_feat(dev, AUX) enough
> to detach all auxiliary domains? I guess device drivers will always need
> to tidy contexts up one by one, so maybe iommu_dev_disable_feat(dev,
> AUX) can return -EBUSY in this case? I think we need to specify this in
> a comment so device drivers know how to clean up.

Yes, -EBUSY sounds better.

iommu_dev_disable_feat() will not detach all aux-domains, it requires
the device drivers to do so before calling it. So if it sees anything
not cleaned up, it should return -EBUSY. And we should put it as a
comment for this API so that the device drivers know it.

> 
> Thanks,
> Jean

Best regards,
Lu Baolu

> 
>> + */
>> +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);
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index e90da6b6f3d1..831eb29b35c5 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -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,11 @@ 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.
>> + * @dev_feat_enabled: check enabled feature
>> + * @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 +236,17 @@ 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);
>> +       bool (*dev_feat_enabled)(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 +433,14 @@ 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 f);
>> +int iommu_dev_enable_feature(struct device *dev, enum
>> iommu_dev_features f);
>> +int iommu_dev_disable_feature(struct device *dev, enum
>> iommu_dev_features f);
>> +bool iommu_dev_feature_enabled(struct device *dev, enum
>> iommu_dev_features f);
>> +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 +725,47 @@ 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 bool
>> +iommu_dev_feature_enabled(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
>> -- 
>> 2.17.1
>>
> 
>