Re: [PATCH] kvm: work around inability of older kvm modules to destroy memory regions

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

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

Hi, this patch breaks all non x86 architectures as libkvm/libkvm-x86.c 
has the only implementation of the alias functionality.
Until now only qemu-kvm-x86 has called that functions, but since this 
patch the generic qemu-kvm.c calls them which leads to unresolved 
symbols for powerpc, s390 and surely ia64 too.

Well we could insert stubs for these call, but when looking on the 
kernel side x86 is also the only implementer of the KVM_SET_MEMORY_ALIAS 
ioctl. Until more arch support that there is no reason to create these 
functions for non-x86 in libkvm. Also the assumptions which addresses 
must be aliased base on hardware specific assumptions e.g. vga card -> 
arch specific too.

For now I hold a no-op stub in my private queue to test powerpc, but 
eventually this mechanism should be arch dependent and this 
implementation x86 only.
Avi could you modify your patch to work for the other arch's too ?
I don't want to foist more urgent work on you, so if you are too busy 
for the right patch now and unapplying is no option because it is too 
important for x86 you can use the ugly++ patch I attached as interim 
solution. It is my test patch and uses ifdef's enabling this code only 
for x86, ugly but works until we have found the right split.

regards,
Christian

Avi Kivity wrote:
> From: Avi Kivity <avi@redhat.com>
>
> instead of removing and adding memory regions, keep the vga framebuffer mapped,
> and use aliases for the vga windows at 0xa0000.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
>
> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
> index 619a166..067cf03 100644
> --- a/qemu/qemu-kvm.c
> +++ b/qemu/qemu-kvm.c
> @@ -766,6 +766,8 @@ int kvm_qemu_init()
>      return 0;
>  }
>
> +static int destroy_region_works = 0;
> +
>  int kvm_qemu_create_context(void)
>  {
>      int r;
> @@ -782,6 +784,7 @@ int kvm_qemu_create_context(void)
>      r = kvm_arch_qemu_create_context();
>      if(r <0)
>  	kvm_qemu_destroy();
> +    destroy_region_works = kvm_destroy_memory_region_works(kvm_context);
>      return 0;
>  }
>
> @@ -790,16 +793,80 @@ void kvm_qemu_destroy(void)
>      kvm_finalize(kvm_context);
>  }
>
> +static int must_use_aliases_source(target_phys_addr_t addr)
> +{
> +    if (destroy_region_works)
> +        return false;
> +    if (addr == 0xa0000 || addr == 0xa8000)
> +        return true;
> +    return false;
> +}
> +
> +static int must_use_aliases_target(target_phys_addr_t addr)
> +{
> +    if (destroy_region_works)
> +        return false;
> +    if (addr >= 0xe000000 && addr < 0x100000000ull)
> +        return true;
> +    return false;
> +}
> +
> +static struct mapping {
> +    target_phys_addr_t phys;
> +    ram_addr_t ram;
> +    ram_addr_t len;
> +} mappings[50];
> +static int nr_mappings;
> +
> +static struct mapping *find_ram_mapping(ram_addr_t ram_addr)
> +{
> +    struct mapping *p;
> +
> +    for (p = mappings; p < mappings + nr_mappings; ++p) {
> +        if (p->ram <= ram_addr && ram_addr < p->ram + p->len) {
> +            return p;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct mapping *find_mapping(target_phys_addr_t start_addr)
> +{
> +    struct mapping *p;
> +
> +    for (p = mappings; p < mappings + nr_mappings; ++p) {
> +        if (p->phys <= start_addr && start_addr < p->phys + p->len) {
> +            return p;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void drop_mapping(target_phys_addr_t start_addr)
> +{
> +    struct mapping *p = find_mapping(start_addr);
> +
> +    if (p)
> +        *p = mappings[--nr_mappings];
> +}
> +
>  void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>                                        unsigned long size,
>                                        unsigned long phys_offset)
>  {
>      int r = 0;
>      unsigned long area_flags = phys_offset & ~TARGET_PAGE_MASK;
> +    struct mapping *p;
>
>      phys_offset &= ~IO_MEM_ROM;
>
>      if (area_flags == IO_MEM_UNASSIGNED) {
> +        if (must_use_aliases_source(start_addr)) {
> +            kvm_destroy_memory_alias(kvm_context, start_addr);
> +            return;
> +        }
> +        if (must_use_aliases_target(start_addr))
> +            return;
>          kvm_unregister_memory_area(kvm_context, start_addr, size);
>          return;
>      }
> @@ -811,6 +878,15 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>      if (area_flags >= TLB_MMIO)
>          return;
>
> +    if (must_use_aliases_source(start_addr)) {
> +        p = find_ram_mapping(phys_offset);
> +        if (p) {
> +            kvm_create_memory_alias(kvm_context, start_addr, size,
> +                                    p->phys + (phys_offset - p->ram));
> +        }
> +        return;
> +    }
> +
>      r = kvm_register_phys_mem(kvm_context, start_addr,
>                                phys_ram_base + phys_offset,
>                                size, 0);
> @@ -818,6 +894,13 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>          printf("kvm_cpu_register_physical_memory: failed\n");
>          exit(1);
>      }
> +
> +    drop_mapping(start_addr);
> +    p = &mappings[nr_mappings++];
> +    p->phys = start_addr;
> +    p->ram = phys_offset;
> +    p->len = size;
> +
>      return;
>  }
>
> @@ -984,8 +1067,11 @@ void kvm_qemu_log_memory(target_phys_addr_t start, target_phys_addr_t size,
>  {
>      if (log)
>  	kvm_dirty_pages_log_enable_slot(kvm_context, start, size);
> -    else
> +    else {
> +        if (must_use_aliases_target(start))
> +            return;
>  	kvm_dirty_pages_log_disable_slot(kvm_context, start, size);
> +    }
>  }
>
>  int kvm_get_phys_ram_page_bitmap(unsigned char *bitmap)
> @@ -1071,6 +1157,9 @@ void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_a
>  {
>      void *buf;
>
> +    if (must_use_aliases_source(start_addr))
> +        return;
> +
>      buf = qemu_malloc((end_addr - start_addr) / 8 + 2);
>      kvm_get_dirty_pages_range(kvm_context, start_addr, end_addr - start_addr,
>  			      buf, NULL, kvm_get_dirty_bitmap_cb);
> @@ -1080,6 +1169,8 @@ void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_a
>  int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len)
>  {
>  #ifndef TARGET_IA64
> +    if (must_use_aliases_source(phys_addr))
> +        return 0;
>      kvm_qemu_log_memory(phys_addr, len, 1);
>  #endif
>      return 0;
> @@ -1088,6 +1179,8 @@ int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len)
>  int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len)
>  {
>  #ifndef TARGET_IA64
> +    if (must_use_aliases_source(phys_addr))
> +        return 0;
>      kvm_qemu_log_memory(phys_addr, len, 0);
>  #endif
>      return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-commits" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization


[-- Attachment #2: memory-aliases.diff --]
[-- Type: text/x-diff, Size: 3545 bytes --]

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -766,7 +766,9 @@ int kvm_qemu_init()
     return 0;
 }
 
+#ifdef TARGET_I386
 static int destroy_region_works = 0;
+#endif
 
 int kvm_qemu_create_context(void)
 {
@@ -784,7 +786,9 @@ int kvm_qemu_create_context(void)
     r = kvm_arch_qemu_create_context();
     if(r <0)
 	kvm_qemu_destroy();
+#ifdef TARGET_I386
     destroy_region_works = kvm_destroy_memory_region_works(kvm_context);
+#endif
     return 0;
 }
 
@@ -793,6 +797,7 @@ void kvm_qemu_destroy(void)
     kvm_finalize(kvm_context);
 }
 
+#ifdef TARGET_I386
 static int must_use_aliases_source(target_phys_addr_t addr)
 {
     if (destroy_region_works)
@@ -849,6 +854,7 @@ static void drop_mapping(target_phys_add
     if (p)
         *p = mappings[--nr_mappings];
 }
+#endif
 
 void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long size,
@@ -856,17 +862,21 @@ void kvm_cpu_register_physical_memory(ta
 {
     int r = 0;
     unsigned long area_flags = phys_offset & ~TARGET_PAGE_MASK;
+#ifdef TARGET_I386
     struct mapping *p;
+#endif
 
     phys_offset &= ~IO_MEM_ROM;
 
     if (area_flags == IO_MEM_UNASSIGNED) {
+#ifdef TARGET_I386
         if (must_use_aliases_source(start_addr)) {
             kvm_destroy_memory_alias(kvm_context, start_addr);
             return;
         }
         if (must_use_aliases_target(start_addr))
             return;
+#endif
         kvm_unregister_memory_area(kvm_context, start_addr, size);
         return;
     }
@@ -878,6 +888,7 @@ void kvm_cpu_register_physical_memory(ta
     if (area_flags >= TLB_MMIO)
         return;
 
+#ifdef TARGET_I386
     if (must_use_aliases_source(start_addr)) {
         p = find_ram_mapping(phys_offset);
         if (p) {
@@ -886,6 +897,7 @@ void kvm_cpu_register_physical_memory(ta
         }
         return;
     }
+#endif
 
     r = kvm_register_phys_mem(kvm_context, start_addr,
                               phys_ram_base + phys_offset,
@@ -895,11 +907,13 @@ void kvm_cpu_register_physical_memory(ta
         exit(1);
     }
 
+#ifdef TARGET_I386
     drop_mapping(start_addr);
     p = &mappings[nr_mappings++];
     p->phys = start_addr;
     p->ram = phys_offset;
     p->len = size;
+#endif
 
     return;
 }
@@ -1068,8 +1082,10 @@ void kvm_qemu_log_memory(target_phys_add
     if (log)
 	kvm_dirty_pages_log_enable_slot(kvm_context, start, size);
     else {
+#ifdef TARGET_I386
         if (must_use_aliases_target(start))
             return;
+#endif
 	kvm_dirty_pages_log_disable_slot(kvm_context, start, size);
     }
 }
@@ -1157,8 +1173,10 @@ void kvm_physical_sync_dirty_bitmap(targ
 {
     void *buf;
 
+#ifdef TARGET_I386
     if (must_use_aliases_source(start_addr))
         return;
+#endif
 
     buf = qemu_malloc((end_addr - start_addr) / 8 + 2);
     kvm_get_dirty_pages_range(kvm_context, start_addr, end_addr - start_addr,
@@ -1168,21 +1186,21 @@ void kvm_physical_sync_dirty_bitmap(targ
 
 int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len)
 {
-#ifndef TARGET_IA64
+#ifdef TARGET_I386
     if (must_use_aliases_source(phys_addr))
         return 0;
+#endif
     kvm_qemu_log_memory(phys_addr, len, 1);
-#endif
     return 0;
 }
 
 int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len)
 {
-#ifndef TARGET_IA64
+#ifdef TARGET_I386
     if (must_use_aliases_source(phys_addr))
         return 0;
+#endif
     kvm_qemu_log_memory(phys_addr, len, 0);
-#endif
     return 0;
 }