Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:

> > This change might cause a functional regression when it comes to nested
> > translation. In nested translation mode, the user page table (e.g.,
> > created and managed by a guest VM for guest kernel DMA) must be in the
> > first-stage page table format. Then, it can be nested on a second-stage
> > page table managed by the host kernel.
> > 
> > Currently, the kernel automatically selects the page table formats. For
> > example, the Intel IOMMU driver always uses the first-stage page table
> > for guest kernel DMA. After this change, this assumption no longer holds
> > true. This means the kernel might use a second-stage page table for
> > guest kernel DMA, breaking nested translation.
> 
> Hmmm. I assumed after discussion in v1 series you are fine. Looks like I misread it?

It is a bug in the implementation in the intel driver.

intel_iommu_domain_alloc_user() should always allocate a page table
that works, if you are in a guest context it must allocate a first
level/guest page table otherwise nested VFIO will be broken.

For this reason Intel driver should always allocate the guest
compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
specified.

Something like this:

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9f6b0780f2ef5e..cfae6c2973e0ee 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1440,7 +1440,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
  * Check and return whether first level is used by default for
  * DMA translation.
  */
-static bool first_level_by_default(unsigned int type)
+static bool first_level_by_default(void)
 {
 	/* Only SL is available in legacy mode */
 	if (!scalable_mode_support())
@@ -1450,8 +1450,8 @@ static bool first_level_by_default(unsigned int type)
 	if (intel_cap_flts_sanity() ^ intel_cap_slts_sanity())
 		return intel_cap_flts_sanity();
 
-	/* Both levels are available, decide it based on domain type */
-	return type != IOMMU_DOMAIN_UNMANAGED;
+	/* Both levels are available, use FL */
+	return true;
 }
 
 static struct dmar_domain *alloc_domain(unsigned int type)
@@ -1463,7 +1463,7 @@ static struct dmar_domain *alloc_domain(unsigned int type)
 		return NULL;
 
 	domain->nid = NUMA_NO_NODE;
-	if (first_level_by_default(type))
+	if (first_level_by_default())
 		domain->use_first_level = true;
 	INIT_LIST_HEAD(&domain->devices);
 	INIT_LIST_HEAD(&domain->dev_pasids);
@@ -3287,8 +3287,7 @@ int __init intel_iommu_init(void)
 		 * is likely to be much lower than the overhead of synchronizing
 		 * the virtual and physical IOMMU page-tables.
 		 */
-		if (cap_caching_mode(iommu->cap) &&
-		    !first_level_by_default(IOMMU_DOMAIN_DMA)) {
+		if (cap_caching_mode(iommu->cap) && !first_level_by_default()) {
 			pr_info_once("IOMMU batching disallowed due to virtualization\n");
 			iommu_set_dma_strict();
 		}
@@ -3530,6 +3529,7 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	struct intel_iommu *iommu = info->iommu;
 	struct dmar_domain *dmar_domain;
 	struct iommu_domain *domain;
+	bool first_level;
 
 	/* Must be NESTING domain */
 	if (parent) {
@@ -3541,13 +3541,18 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	if (flags &
 	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
 		return ERR_PTR(-EOPNOTSUPP);
+	if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !intel_cap_slts_sanity())
+		return ERR_PTR(-EOPNOTSUPP);
 	if (nested_parent && !nested_supported(iommu))
 		return ERR_PTR(-EOPNOTSUPP);
 	if (user_data || (dirty_tracking && !ssads_supported(iommu)))
 		return ERR_PTR(-EOPNOTSUPP);
 
-	/* Do not use first stage for user domain translation. */
-	dmar_domain = paging_domain_alloc(dev, false);
+	if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT)
+		first_level = false;
+	else
+		first_level = first_level_by_default();
+	dmar_domain = paging_domain_alloc(dev, first_level);
 	if (IS_ERR(dmar_domain))
 		return ERR_CAST(dmar_domain);
 	domain = &dmar_domain->domain;