Re: [PATCH] On unmap, flush IOMMU TLB and return correct size

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, 2013-09-05 at 04:05 +0000, Shankar, Hari wrote:
> Hi Alex,
> See inline,
> 
> On 9/2/13 9:25 PM, "Alex Williamson" <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> >Hi Hari,
> >
> >On Mon, 2013-09-02 at 02:24 +0000, Shankar, Hari wrote:
> >> Patch Description: The patch is for Intel IOMMU driver. It forces IOMMU
> >>TLB flush for the particular domain and return correct unmap size when
> >>intel_iommu_unmap() is called
> >
> >"Patch Description:" is unnecessary.  Look at other commits that have
> >touched this file to get an idea how to format the subject and commit
> >log:
> >
> >http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/drivers
> >/iommu/intel-iommu.c
> >
> >Sign-off should go here, not below the patch.  Subject should include a
> >subsystem, such as "iommu/intel:" or "intel-iommu:".  Also a good idea
> >to provide some justification why this is necessary for stable (unmaps
> >from userspace through vfio results in coherency issue...)
> >
> >> The patch is generated on Linux kernel version 3.6.11
> >
> >This should be noted, but not part of the commit message.  See section
> >15 of SubmittingPatches and note the additional comments between the
> >"---" marker and the beginning of the patch.
> >
> >> --- linux/drivers/iommu/intel-iommu.c.orig      2013-09-01
> >>10:10:14.723958000 -0700
> >> +++ linux/drivers/iommu/intel-iommu.c   2013-09-01 18:22:58.497578000
> >>-0700
> >> @@ -4060,14 +4060,31 @@ static size_t intel_iommu_unmap(struct i
> >
> >Tabs seem to have been clobbered in your patch, scripts/checkpatch.pl
> >will note this for you.
> >
> >>  {
> >>         struct dmar_domain *dmar_domain = domain->priv;
> >>         int order;
> >> +       struct intel_iommu *iommu;
> >> +       unsigned long start_pfn, last_pfn;
> >> +       unsigned int npages;
> >> +       int iommu_id, num, ndomains;
> >> 
> >> -       order = dma_pte_clear_range(dmar_domain, iova >> VTD_PAGE_SHIFT,
> >> -                           (iova + size - 1) >> VTD_PAGE_SHIFT);
> >> +       start_pfn = iova >> VTD_PAGE_SHIFT;
> >> +       last_pfn = (iova + size - 1) >> VTD_PAGE_SHIFT;
> >> +       order = dma_pte_clear_range(dmar_domain, start_pfn, last_pfn);
> >> +       last_pfn |= (1UL << order) - 1;
> >> +       npages = last_pfn - start_pfn + 1;
> >
> >Doesn't order tell us npages more directly?  npages = 1UL << order?
> 
> 
> [Hari] 1UL << order formula will not work for all cases. Jeff Kimmel
> (cc'ed) has a good explanation for that. Jeff's response below,
> 
> <Jeff>
> If start_pfn is always aligned modulo (1 << order),
> then '1UL << order' would get the same npages value, but if
> start_pfn is not guaranteed to have such alignment, then '1UL << order'
> would
> produce an incorrect npages value.  For example, start_pfn could fall in
> some range that was mapped with small pages, but the range could end in a
> region mapped with large pages.
> 
> </Jeff>

I think iommu_map()/iommu_unmap() only passes naturally aligned regions
to the iommu drivers, so I don't think we need to worry about that.
Feel free to verify.

> >> +       for_each_set_bit(iommu_id, dmar_domain->iommu_bmp,
> >>g_num_of_iommus) {
> >> +               iommu = g_iommus[iommu_id];
> >> 
> >> -       if (dmar_domain->max_addr == iova + size)
> >> +               /*
> >> +                * find bit position of dmar_domain
> >> +                */
> >> +               ndomains = cap_ndoms(iommu->cap);
> >> +               for_each_set_bit(num, iommu->domain_ids, ndomains)
> >> +                       if (iommu->domains[num] == dmar_domain)
> >> +                               iommu_flush_iotlb_psi(iommu, num,
> >> +                                                       start_pfn,
> >>npages, 0);
> >
> >This second bitmap search seems unnecessary, I think
> >iommu_flush_iotlb_psi wants dmar_domain->id as the second parameter in
> >all cases, not num.  So this could be reduced to :
> >
> >for_each_set_bit(iommu_id, dmar_domain->iommu_bmp, g_num_of_iommus)
> >	iommu_flush_iotlb_psi(g_iommus[iommu_id], dmar_domain->id, start_pfn,
> >npages, 0);
> 
> [Hari] For VFIO, iommu_alloc_vm_domain() is called to get VM domain and
> has DOMAIN_FLAG_VIRTUAL_MACHINE flag set. In this case, domain->id is
> derived from static variable 'vm_domid' and not by using
> find_first_zero_bit in iommu->domain_ids, which in-turn also claims a slot
> on iommu->domains[] array, see iommu_attach_domain(). For domain with
> DOMAIN_FLAG_VIRTUAL_MACHINE flag, domain id is actually derived later in
> domain_context_mapping_one(), logic under 'if (domain->flags &
> DOMAIN_FLAG_VIRTUAL_MACHINE'. As you can see domain->id for VM domain is
> not the actual domain id and can also conflict with non VM domain as
> vm_domid starts from 0. Using domain->id in the loop will give wrong
> result. Using second loop and passing 'num' makes sure we always select
> the right domain to flush.

Ok, that does sound familiar.  I agree, you do need to search the
bitmaps.  Thanks,

Alex

> >> +       }
> >> +       if (dmar_domain->max_addr <= iova + (npages << VTD_PAGE_SHIFT))
> >>                 dmar_domain->max_addr = iova;
> >> -
> >> -       return PAGE_SIZE << order;
> >> +       return npages << VTD_PAGE_SHIFT;
> >>  }
> >> 
> >>  static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain
> >>*domain,
> >> 
> >> Signed-off-by: Hari Shankar <hshankar-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
> >> cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >
> >Please configure your mailer to drop the html version.  Thanks,
> >
> >Alex
> >
>