| Message ID | 49C94A2F.9070706@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
@@ -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."
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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." > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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." >> > >
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)?
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