diff mbox series

[v5,2/8] iommu/vt-d: Add per-device IOMMU feature ops entries

Message ID 20190110030027.31447-3-baolu.lu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series vfio/mdev: IOMMU aware mediated device | expand

Commit Message

Baolu Lu Jan. 10, 2019, 3 a.m. UTC 
This adds the iommu ops entries for aux-domain per-device
feature query and enable/disable.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 86 +++++++++++++++++++++++++++++++++++++
 include/linux/intel-iommu.h |  1 +
 2 files changed, 87 insertions(+)

Comments

Joerg Roedel Jan. 11, 2019, 11:16 a.m. UTC | #1 
Hi,

this looks a bit confusing to me because I can see no checking whether
the device actually supports scalable mode. More below:

On Thu, Jan 10, 2019 at 11:00:21AM +0800, Lu Baolu wrote:
> +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;
> +}

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?

> +
> +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
Baolu Lu Jan. 14, 2019, 5:30 a.m. UTC | #2 
Hi Joerg,

Thanks for reviewing my patch.

On 1/11/19 7:16 PM, Joerg Roedel wrote:
> 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.

> More below:
> 
> On Thu, Jan 10, 2019 at 11:00:21AM +0800, Lu Baolu wrote:
>> +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;
>> +}
> 
> 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.

> 
>> +
>> +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?

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)

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

> 
> Regards,
> 
> 	Joerg
> 

Best regards,
Lu Baolu
Baolu Lu Jan. 24, 2019, 6:47 a.m. UTC | #3 
Hi Joerg,

On 1/11/19 7:16 PM, Joerg Roedel wrote:
>> +
>> +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.

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 Jan. 24, 2019, 1:20 p.m. UTC | #4 
Hi Lu Baolu,

On Thu, Jan 24, 2019 at 02:47:39PM +0800, Lu Baolu wrote:
> 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
diff mbox series

Patch

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2bd9ac285c0d..ee8832d26f7e 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -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,
 };
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 0605f3bf6e79..7cf9f7f3724a 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -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 */