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

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

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.

/*
  * Set up dirty tracking on a second only or nested translation type.
  */
int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
                                      struct device *dev, u32 pasid,
                                      bool enabled)
{
[...]
         pgtt = pasid_pte_get_pgtt(pte);
         if (pgtt != PASID_ENTRY_PGTT_SL_ONLY &&
             pgtt != PASID_ENTRY_PGTT_NESTED) {
                 spin_unlock(&iommu->lock);
                 dev_err_ratelimited(
                         dev,
                         "Dirty tracking not supported on translation 
type %d\n",
                         pgtt);
                 return -EOPNOTSUPP;
         }

Thanks,
baolu