[PATCH] fix bad physical address in kvm_update_dirty_pages_log()

[PATCH] fix bad physical address in kvm_update_dirty_pages_log()

[Yaniv Kamay]

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

Hi,

Attaching patch that fix updating qemu dirty region. Previous 
kvm_update_dirty_pages_log() imp treat physical
ram as if it is linear mapped to guest physical memory. This patch fix 
it by mapping physical ram to guest physical
memory areas and for etch area call kvm_get_dirty_pages_range() with the 
correct address and size.


Thanks,
Yaniv




[-- Attachment #2: 0001-fix-bad-physical-address-in-kvm_update_dirty_pages_l.patch --]
[-- Type: text/x-patch, Size: 3027 bytes --]

>From 1179bb5123a53392d705801dc37e491f0766957c Mon Sep 17 00:00:00 2001
From: Yaniv Kamay <yaniv@qumranet.com>
Date: Tue, 24 Mar 2009 20:49:07 +0200
Subject: [PATCH] fix bad physical address in kvm_update_dirty_pages_log()

---
 qemu/qemu-kvm.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 4164368..71cd3dc 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -1216,6 +1216,27 @@ static int kvm_get_dirty_bitmap_cb(unsigned long start, unsigned long len,
     return kvm_get_dirty_pages_log_range(start, bitmap, start, len);
 }
 
+static int find_phys_area(ram_addr_t phys_ram_offset, ram_addr_t *offset, target_phys_addr_t *start,
+                              ram_addr_t *size)
+{
+    struct mapping *now = NULL;
+    struct mapping *map;
+
+    for (map = mappings; map < mappings + nr_mappings; ++map) {
+        if (map->ram >= phys_ram_offset && (!now || map->ram < now->ram)) {
+            now = map;
+        }
+    }
+
+    if (!now) {
+        return -1;
+    }
+
+    *offset = now->ram - phys_ram_offset;
+    *start = now->phys;
+    *size = now->len;
+    return 0;
+}
 /* 
  * get kvm's dirty pages bitmap and update qemu's
  * we only care about physical ram, which resides in slots 0 and 3
@@ -1223,12 +1244,44 @@ static int kvm_get_dirty_bitmap_cb(unsigned long start, unsigned long len,
 int kvm_update_dirty_pages_log(void)
 {
     int r = 0;
+    ram_addr_t now = 0;
+    ram_addr_t end = phys_ram_size;
+    ram_addr_t offset;
+    target_phys_addr_t area_start;
+    ram_addr_t area_size;
+    unsigned char *dirty_bitmap = kvm_dirty_bitmap;
+
+    if (!dirty_bitmap) {
+        printf("%s: no dirty bitmap\n", __FUNCTION__);
+        return -1;
+    }
 
+    while (now < end && !find_phys_area(now, &offset, &area_start, &area_size)) {
+        if ((offset & ~TARGET_PAGE_MASK) || (area_start & ~TARGET_PAGE_MASK) || 
+                                                         (area_size & ~TARGET_PAGE_MASK)) {
+            printf("%s: invalid mem area\n", __FUNCTION__);
+            return -1;
+        }
 
-    r = kvm_get_dirty_pages_range(kvm_context, 0, phys_ram_size,
-                                  kvm_dirty_bitmap, NULL,
-                                  kvm_get_dirty_bitmap_cb);
-    return r;
+        if ((now += offset) >= end) {
+            break;
+        }
+        
+        if (area_size > end - now) {
+            return -1;
+        }
+        dirty_bitmap += offset / TARGET_PAGE_SIZE / 8;
+        if ((r = kvm_get_dirty_pages_range(kvm_context, area_start, area_size, dirty_bitmap, NULL,
+                                      kvm_get_dirty_bitmap_cb))) {
+            return r;
+        }
+        dirty_bitmap += area_size / TARGET_PAGE_SIZE / 8;
+        now += area_size;
+        if (!now) {
+            break;
+        }
+    }
+    return 0;
 }
 
 void kvm_qemu_log_memory(target_phys_addr_t start, target_phys_addr_t size,
-- 
1.6.0.2

Re: [PATCH] fix bad physical address in kvm_update_dirty_pages_log()

[Glauber Costa]

@@ -1223,12 +1244,44 @@ static int kvm_get_dirty_bitmap_cb(unsigned
long start, unsigned long len,
 int kvm_update_dirty_pages_log(void)
 {
     int r = 0;
+    ram_addr_t now = 0;
+    ram_addr_t end = phys_ram_size;
+    ram_addr_t offset;
+    target_phys_addr_t area_start;
+    ram_addr_t area_size;
+    unsigned char *dirty_bitmap = kvm_dirty_bitmap;
+
+    if (!dirty_bitmap) {
+        printf("%s: no dirty bitmap\n", __FUNCTION__);
+        return -1;
+    }
nitpick: you probably want fprintf(stderr...).


+    while (now < end && !find_phys_area(now, &offset, &area_start,
&area_size)) {
+        if ((offset & ~TARGET_PAGE_MASK) || (area_start &
~TARGET_PAGE_MASK) ||
+                                                         (area_size &
~TARGET_PAGE_MASK)) {
+            printf("%s: invalid mem area\n", __FUNCTION__);
same here.



+        if ((now += offset) >= end) {
+            break;
+        }
+
+        if (area_size > end - now) {
+            return -1;
+        }
any reason why you're handling those two differently?

--
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] fix bad physical address in kvm_update_dirty_pages_log()

[Yaniv Kamay]

Glauber Costa wrote:
> @@ -1223,12 +1244,44 @@ static int kvm_get_dirty_bitmap_cb(unsigned
> long start, unsigned long len,
>  int kvm_update_dirty_pages_log(void)
>  {
>      int r = 0;
> +    ram_addr_t now = 0;
> +    ram_addr_t end = phys_ram_size;
> +    ram_addr_t offset;
> +    target_phys_addr_t area_start;
> +    ram_addr_t area_size;
> +    unsigned char *dirty_bitmap = kvm_dirty_bitmap;
> +
> +    if (!dirty_bitmap) {
> +        printf("%s: no dirty bitmap\n", __FUNCTION__);
> +        return -1;
> +    }
> nitpick: you probably want fprintf(stderr...).
>
>
> +    while (now < end && !find_phys_area(now, &offset, &area_start,
> &area_size)) {
> +        if ((offset & ~TARGET_PAGE_MASK) || (area_start &
> ~TARGET_PAGE_MASK) ||
> +                                                         (area_size &
> ~TARGET_PAGE_MASK)) {
> +            printf("%s: invalid mem area\n", __FUNCTION__);
> same here.
>
>
>
> +        if ((now += offset) >= end) {
> +            break;
> +        }
> +
> +        if (area_size > end - now) {
> +            return -1;
> +        }
> any reason why you're handling those two differently?
>   
The first is orderly end of mappings and the later is error.
> --
> Glauber  Costa.
> "Free as in Freedom"
> http://glommer.net
>
> "The less confident you are, the more serious you have to act."
>   

Re: [PATCH] fix bad physical address in kvm_update_dirty_pages_log()

[Glauber Costa]

On Tue, Mar 24, 2009 at 6:27 PM, Yaniv Kamay <ykamay@redhat.com> wrote:
> Glauber Costa wrote:
>>
>> @@ -1223,12 +1244,44 @@ static int kvm_get_dirty_bitmap_cb(unsigned
>> long start, unsigned long len,
>>  int kvm_update_dirty_pages_log(void)
>>  {
>>     int r = 0;
>> +    ram_addr_t now = 0;
>> +    ram_addr_t end = phys_ram_size;
>> +    ram_addr_t offset;
>> +    target_phys_addr_t area_start;
>> +    ram_addr_t area_size;
>> +    unsigned char *dirty_bitmap = kvm_dirty_bitmap;
>> +
>> +    if (!dirty_bitmap) {
>> +        printf("%s: no dirty bitmap\n", __FUNCTION__);
>> +        return -1;
>> +    }
>> nitpick: you probably want fprintf(stderr...).
>>
>>
>> +    while (now < end && !find_phys_area(now, &offset, &area_start,
>> &area_size)) {
>> +        if ((offset & ~TARGET_PAGE_MASK) || (area_start &
>> ~TARGET_PAGE_MASK) ||
>> +                                                         (area_size &
>> ~TARGET_PAGE_MASK)) {
>> +            printf("%s: invalid mem area\n", __FUNCTION__);
>> same here.
>>
>>
>>
>> +        if ((now += offset) >= end) {
>> +            break;
>> +        }
>> +
>> +        if (area_size > end - now) {
>> +            return -1;
>> +        }
>> any reason why you're handling those two differently?
>>
>
> The first is orderly end of mappings and the later is error.
ok. I believe the code reads better if you handle all the errors together.

but this is nitpick too. The idea of the patch looks correct to me.
>>
>> --
>> Glauber  Costa.
>> "Free as in Freedom"
>> http://glommer.net
>>
>> "The less confident you are, the more serious you have to act."
>>
>
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] fix bad physical address in kvm_update_dirty_pages_log()

[Avi Kivity]

Yaniv Kamay wrote:
> Hi,
>
> Attaching patch that fix updating qemu dirty region. Previous 
> kvm_update_dirty_pages_log() imp treat physical
> ram as if it is linear mapped to guest physical memory. This patch fix 
> it by mapping physical ram to guest physical
> memory areas and for etch area call kvm_get_dirty_pages_range() with 
> the correct address and size.
>

kvm_get_dirty_pages_range() already operates on guest physical memory, 
and kvm_get_dirty_pages_log_range() (which is called by the callback) 
translates it to ram_addr, so it should work, no?  Maybe change the 
range parameters from (0, phys_ram_size) to (0, ~0UL)?


-- 
error compiling committee.c: too many arguments to function