[PATCH 4/4] Hack around IOMMU changes

[PATCH 4/4] Hack around IOMMU changes

[Zachary Amsden]

Not for the faint of heart, this patch subverts the code by reassigning 
a local variable from a macro.

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

[Zachary Amsden]

[-- Attachment #1: Type: text/plain, Size: 178 bytes --]

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.


[-- Attachment #2: 0004-IOMMU-API-changed.patch --]
[-- Type: text/plain, Size: 1374 bytes --]

>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); \
+	page_size = PAGE_SIZE; \
+	_r; \
+})
+#define iommu_unmap(domain, iova, gfp_order) \
+	(iommu_unmap_range(domain, iova, PAGE_SIZE),1)
+#endif
-- 
1.7.0.1

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

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.

> +	page_size = PAGE_SIZE; \

And what is this for?

> +	_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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Zachary Amsden]

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
>
>    

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

Zachary Amsden wrote:
> 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. 

Indeed. This is an upstream bug and has to be fixed instead of worked
around. I'll send a patch.

Jan

PS: If there were already a KVM release with this included, the more
appropriate way for kvm-kmod to deal with it would be patching the
particular code line during sync.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]