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" <[email protected]> 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 <[email protected]>
> >> cc: [email protected]
> >
> >Please configure your mailer to drop the html version. Thanks,
> >
> >Alex
> >
>
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
