diff mbox series

[v3,06/10] iommu/vt-d: Set the nested domain to a device

Message ID 20230511145110.27707-7-yi.l.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series Add Intel VT-d nested translation | expand

Commit Message

Yi Liu May 11, 2023, 2:51 p.m. UTC 
hence enable nested domain usage on Intel VT-d.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/nested.c | 47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Comments

Tian, Kevin May 24, 2023, 7:22 a.m. UTC | #1 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, May 11, 2023 10:51 PM
> 
> +
> +static int intel_nested_attach_dev(struct iommu_domain *domain,
> +				   struct device *dev)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> +	struct intel_iommu *iommu = info->iommu;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (info->domain)
> +		device_block_translation(dev);
> +
> +	/* Is s2_domain compatible with this IOMMU? */
> +	ret = prepare_domain_attach_device(&dmar_domain->s2_domain-
> >domain, dev);
> +	if (ret) {
> +		dev_err_ratelimited(dev, "s2 domain is not compatible\n");
> +		return ret;
> +	}

this also includes logic to trim higher page levels:

	/*
	 * Knock out extra levels of page tables if necessary
	 */
	while (iommu->agaw < dmar_domain->agaw) {
		struct dma_pte *pte;

		pte = dmar_domain->pgd;
		if (dma_pte_present(pte)) {
			dmar_domain->pgd = phys_to_virt(dma_pte_addr(pte));
			free_pgtable_page(pte);
		}
		dmar_domain->agaw--;
	}

What's the background of doing such truncation instead of simply
failing the request?

In any means it's probably fine before the domain includes any mapping
but really unreasonable to apply it to an existing s2 when it's used as
a parent.
Baolu Lu May 26, 2023, 4:24 a.m. UTC | #2 
On 5/24/23 3:22 PM, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, May 11, 2023 10:51 PM
>>
>> +
>> +static int intel_nested_attach_dev(struct iommu_domain *domain,
>> +				   struct device *dev)
>> +{
>> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
>> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> +	struct intel_iommu *iommu = info->iommu;
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	if (info->domain)
>> +		device_block_translation(dev);
>> +
>> +	/* Is s2_domain compatible with this IOMMU? */
>> +	ret = prepare_domain_attach_device(&dmar_domain->s2_domain-
>>> domain, dev);
>> +	if (ret) {
>> +		dev_err_ratelimited(dev, "s2 domain is not compatible\n");
>> +		return ret;
>> +	}
> 
> this also includes logic to trim higher page levels:
> 
> 	/*
> 	 * Knock out extra levels of page tables if necessary
> 	 */
> 	while (iommu->agaw < dmar_domain->agaw) {
> 		struct dma_pte *pte;
> 
> 		pte = dmar_domain->pgd;
> 		if (dma_pte_present(pte)) {
> 			dmar_domain->pgd = phys_to_virt(dma_pte_addr(pte));
> 			free_pgtable_page(pte);
> 		}
> 		dmar_domain->agaw--;
> 	}
> 
> What's the background of doing such truncation instead of simply
> failing the request?

This code existed a long time ago. I'm not sure if it's still reasonable
so far.

> In any means it's probably fine before the domain includes any mapping
> but really unreasonable to apply it to an existing s2 when it's used as
> a parent.

But for the new nested translation, it is obviously unreasonable.

Let me revisit it.

Best regards,
baolu
diff mbox series

Patch

diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index f83931bb44b6..fd38424b78f0 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -11,8 +11,53 @@ 
 #define pr_fmt(fmt)	"DMAR: " fmt
 
 #include <linux/iommu.h>
+#include <linux/pci.h>
+#include <linux/pci-ats.h>
 
 #include "iommu.h"
+#include "pasid.h"
+
+static int intel_nested_attach_dev(struct iommu_domain *domain,
+				   struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	if (info->domain)
+		device_block_translation(dev);
+
+	/* Is s2_domain compatible with this IOMMU? */
+	ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev);
+	if (ret) {
+		dev_err_ratelimited(dev, "s2 domain is not compatible\n");
+		return ret;
+	}
+
+	ret = domain_attach_iommu(dmar_domain, iommu);
+	if (ret) {
+		dev_err_ratelimited(dev, "Failed to attach domain to iommu\n");
+		return ret;
+	}
+
+	ret = intel_pasid_setup_nested(iommu, dev,
+				       PASID_RID2PASID, dmar_domain);
+	if (ret) {
+		domain_detach_iommu(dmar_domain, iommu);
+		dev_err_ratelimited(dev, "Failed to setup pasid entry\n");
+		return ret;
+	}
+
+	info->domain = dmar_domain;
+	spin_lock_irqsave(&dmar_domain->lock, flags);
+	list_add(&info->link, &dmar_domain->devices);
+	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+	domain_update_iommu_cap(dmar_domain);
+
+	return 0;
+}
 
 static void intel_nested_domain_free(struct iommu_domain *domain)
 {
@@ -20,7 +65,9 @@  static void intel_nested_domain_free(struct iommu_domain *domain)
 }
 
 static const struct iommu_domain_ops intel_nested_domain_ops = {
+	.attach_dev		= intel_nested_attach_dev,
 	.free			= intel_nested_domain_free,
+	.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
 };
 
 struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *s2_domain,