Re: [PATCH 4/4] Hack around IOMMU changes

[Zachary Amsden <zamsden@redhat.com>]

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<zamsden@redhat.com>
>> 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<zamsden@redhat.com>
>> ---
>>   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
>
>