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

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

On 2024/10/10 14:40, Baolu Lu wrote:
> On 2024/10/9 20:15, Jason Gunthorpe wrote:
>> 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:
> 
> This will break the existing dirty page tracking functionality. Intel
> IOMMU only supports enabling or disabling dirty page tracking at the
> second-stage page table.

So, perhaps something like below?

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 1497f3112b12..e11dde259afa 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -544,6 +544,8 @@ enum {
                                 ecap_slads((iommu)->ecap))
  #define nested_supported(iommu)        (sm_supported(iommu) && 
         \
                                  ecap_nest((iommu)->ecap))
+#define flts_supported(iommu)  (sm_supported(iommu) &&                 \
+                                ecap_flts((iommu)->ecap))

  struct pasid_entry;
  struct pasid_state_entry;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9f6b0780f2ef..d1a7378489a4 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3530,6 +3530,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_stage;

         /* Must be NESTING domain */
         if (parent) {
@@ -3546,8 +3547,14 @@ intel_iommu_domain_alloc_user(struct device *dev, 
u32 flags,
         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);
+       /*
+        * Always allocate the guest compatible page table unless
+        * IOMMU_HWPT_ALLOC_NEST_PARENT or IOMMU_HWPT_ALLOC_DIRTY_TRACKING
+        * is specified.
+        */
+       first_stage = (nested_parent || dirty_tracking) ?
+                       false : flts_supported(iommu);
+       dmar_domain = paging_domain_alloc(dev, first_stage);
         if (IS_ERR(dmar_domain))
                 return ERR_CAST(dmar_domain);
         domain = &dmar_domain->domain;

Thanks,
baolu