From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zachary Amsden Subject: Re: [PATCH 4/4] Hack around IOMMU changes Date: Wed, 26 May 2010 08:28:50 -1000 Message-ID: <4BFD6862.6010107@redhat.com> References: <4BFC974D.8010206@redhat.com> <4BFC9765.6070200@redhat.com> <4BFCD05D.4000603@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm , Avi Kivity , Gleb Natapov To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60839 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751936Ab0EZS2y (ORCPT ); Wed, 26 May 2010 14:28:54 -0400 In-Reply-To: <4BFCD05D.4000603@web.de> Sender: kvm-owner@vger.kernel.org List-ID: On 05/25/2010 09:40 PM, Jan Kiszka wrote: > Zachary Amsden wrote: > >> On 05/25/2010 05:36 PM, Zachary Amsden wrote: >> >>> Not for the faint of heart, this patch subverts the code by >>> reassigning a local variable from a macro. >>> >> This time, with patch. >> >> >> From 97b9230f699aba1c5f47972032b2d4d935a83054 Mon Sep 17 00:00:00 2001 >> From: Zachary Amsden >> Date: Tue, 25 May 2010 17:17:32 -1000 >> Subject: [PATCH 4/5] IOMMU API changed >> >> Ugly, dirty, disease ridden fix for IOMMU changes; the module >> is now trying to use larger IOMMU intervals; deny it this, and >> stick to page size. This requires forcibly setting page_size >> variable through knowledge of the code. Yuck. If you have a >> better solution, implement it. >> >> Signed-off-by: Zachary Amsden >> --- >> external-module-compat-comm.h | 12 ++++++++++++ >> 1 files changed, 12 insertions(+), 0 deletions(-) >> >> diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h >> index c5284e5..708019e 100644 >> --- a/external-module-compat-comm.h >> +++ b/external-module-compat-comm.h >> @@ -1128,3 +1128,15 @@ perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) >> #if LINUX_VERSION_CODE< KERNEL_VERSION(2,6,32) >> #define lockdep_is_held(m) (1) >> #endif >> + >> +#if LINUX_VERSION_CODE< KERNEL_VERSION(2,6,34) >> +/* This is a dirty, nasty trick */ >> +#define iommu_map(domain, iova, paddr, gfp_order, prot) \ >> +({ \ >> + int _r = iommu_map_range(domain, iova, paddr, PAGE_SIZE, prot); \ >> > This should be (PAGE_SIZE<< gfp_order) according to my current > understanding. > It should, but can't be. > >> + page_size = PAGE_SIZE; \ >> > And what is this for? > iommu_unmap is now passed an unreasonable value for order: PAGE_SIZE. The way the code work, that ends up becoming zero, then -1, for an infinite sized unmap (if I'm reading correctly). Then iommu_unmap_range returns the gfp_order of the unmap which it actually did. Since we have no way to know the size of the original map, I deliberately force all mappings to be in units of PAGE_SIZE. This requires changing the local variable page_size in the following code. (Note the pin is unchanged, so we are doubling pinning... maybe this is causing my bug). /* * Pin all pages we are about to map in memory. This is * important because we unmap and unpin in 4kb steps later. */ pfn = kvm_pin_pages(kvm, slot, gfn, page_size); if (is_error_pfn(pfn)) { gfn += 1; continue; } /* Map into IO address space */ r = iommu_map(domain, gfn_to_gpa(gfn), pfn_to_hpa(pfn), get_order(page_size), flags); if (r) { printk(KERN_ERR "kvm_iommu_map_address:" "iommu failed to map pfn=%lx\n", pfn); goto unmap_pages; } gfn += page_size >> PAGE_SHIFT; In any case, fixing the gfn computation is needed so the unmap can work: while (gfn < end_gfn) { unsigned long unmap_pages; int order; /* Get physical address */ phys = iommu_iova_to_phys(domain, gfn_to_gpa(gfn)); pfn = phys >> PAGE_SHIFT; /* Unmap address from IO address space */ order = iommu_unmap(domain, gfn_to_gpa(gfn), PAGE_SIZE); unmap_pages = 1ULL << order; /* Unpin all pages we just unmapped to not leak any memory */ kvm_unpin_pages(kvm, pfn, unmap_pages); gfn += unmap_pages; } Note iommu_unmap(,,PAGE_SIZE) is a bogus argument. > >> + _r; \ >> +}) >> +#define iommu_unmap(domain, iova, gfp_order) \ >> + (iommu_unmap_range(domain, iova, PAGE_SIZE),1) >> +#endif >> -- 1.7.0.1 >> > Besides that, putting this under CONFIG_IOMMU_API and making the > functions static inline would be preferred. > > Jan > >