[PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Zhang, Xiantao]

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

>From d184d9b0a91ca674961000ed3d35b7fc25d29e03 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Tue, 28 Apr 2009 16:59:36 +0800
Subject: [PATCH] qemu-kvm: other archs should maintain memory mapping also.
 
Currently, use TARGET_I386 to comment out the mapping machanism
for other archs, but mapping machanism should be useful for other archs
to maintain guest's memory mapping.
 
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 qemu-kvm.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)
 
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 68a9218..4f71192 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -846,6 +846,7 @@ static int must_use_aliases_target(target_phys_addr_t addr)
         return true;
     return false;
 }
+#endif
 
 static struct mapping {
     target_phys_addr_t phys;
@@ -885,7 +886,6 @@ static void drop_mapping(target_phys_addr_t start_addr)
     if (p)
         *p = mappings[--nr_mappings];
 }
-#endif
 
 void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long size,
@@ -893,9 +893,7 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
 {
     int r = 0;
     unsigned long area_flags;
-#ifdef TARGET_I386
     struct mapping *p;
-#endif
 
     if (start_addr + size > phys_ram_size) {
         phys_ram_size = start_addr + size;
@@ -955,13 +953,11 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
         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;
 }
-- 
1.6.0
 

[-- Attachment #2: 0004-qemu-kvm-other-archs-should-maintain-memory-mapping.patch --]
[-- Type: application/octet-stream, Size: 1698 bytes --]

From d184d9b0a91ca674961000ed3d35b7fc25d29e03 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Tue, 28 Apr 2009 16:59:36 +0800
Subject: [PATCH] qemu-kvm: other archs should maintain memory mapping also.

Currently, use TARGET_I386 to comment out the mapping machanism
for other archs, but mapping machanism should be useful for other archs
to maintain guest's memory mapping.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 qemu-kvm.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 68a9218..4f71192 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -846,6 +846,7 @@ static int must_use_aliases_target(target_phys_addr_t addr)
         return true;
     return false;
 }
+#endif
 
 static struct mapping {
     target_phys_addr_t phys;
@@ -885,7 +886,6 @@ static void drop_mapping(target_phys_addr_t start_addr)
     if (p)
         *p = mappings[--nr_mappings];
 }
-#endif
 
 void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long size,
@@ -893,9 +893,7 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
 {
     int r = 0;
     unsigned long area_flags;
-#ifdef TARGET_I386
     struct mapping *p;
-#endif
 
     if (start_addr + size > phys_ram_size) {
         phys_ram_size = start_addr + size;
@@ -955,13 +953,11 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
         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;
 }
-- 
1.6.0

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Avi Kivity]

Zhang, Xiantao wrote:
> From d184d9b0a91ca674961000ed3d35b7fc25d29e03 Mon Sep 17 00:00:00 2001
> From: Xiantao Zhang <xiantao.zhang@intel.com>
> Date: Tue, 28 Apr 2009 16:59:36 +0800
> Subject: [PATCH] qemu-kvm: other archs should maintain memory mapping also.
>  
> Currently, use TARGET_I386 to comment out the mapping machanism
> for other archs, but mapping machanism should be useful for other archs
> to maintain guest's memory mapping.
>  
>   

Hollis, does this work for you?

If now, you can add a new define KVM_WANT_MAPPING or something, and 
define it for I386 and IA64.

> Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> ---
>  qemu-kvm.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
>  
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 68a9218..4f71192 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -846,6 +846,7 @@ static int must_use_aliases_target(target_phys_addr_t addr)
>          return true;
>      return false;
>  }
> +#endif
>  
>  static struct mapping {
>      target_phys_addr_t phys;
> @@ -885,7 +886,6 @@ static void drop_mapping(target_phys_addr_t start_addr)
>      if (p)
>          *p = mappings[--nr_mappings];
>  }
> -#endif
>  
>  void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>                                        unsigned long size,
> @@ -893,9 +893,7 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>  {
>      int r = 0;
>      unsigned long area_flags;
> -#ifdef TARGET_I386
>      struct mapping *p;
> -#endif
>  
>      if (start_addr + size > phys_ram_size) {
>          phys_ram_size = start_addr + size;
> @@ -955,13 +953,11 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>          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;
>  }
>   


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Jes Sorensen]

>>>>> "Avi" == Avi Kivity <avi@redhat.com> writes:

Avi> Zhang, Xiantao wrote:
>> From d184d9b0a91ca674961000ed3d35b7fc25d29e03 Mon Sep 17 00:00:00
>> 2001 From: Xiantao Zhang <xiantao.zhang@intel.com> Date: Tue, 28
>> Apr 2009 16:59:36 +0800 Subject: [PATCH] qemu-kvm: other archs
>> should maintain memory mapping also.  Currently, use TARGET_I386 to
>> comment out the mapping machanism for other archs, but mapping
>> machanism should be useful for other archs to maintain guest's
>> memory mapping.
>> 

Avi> Hollis, does this work for you?

Avi> If now, you can add a new define KVM_WANT_MAPPING or something,
Avi> and define it for I386 and IA64.

I have gone ahead and implemented this - however I also redid the
must_use_alias code in the process, removing a ton of gross
TARGET_I386 #ifdefs.

The must_use_alias patch needs to be applied first as it introduces
target-<arch>/qemu-kvm-arch.h which is needed for the KVM_WANT_MAPPING
patch.

Look for follow up mails to this one in a couple of minutes.

Cheers,
Jes

Re: Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Jes Sorensen]

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

Avi Kivity wrote:
>> Currently, use TARGET_I386 to comment out the mapping machanism
>> for other archs, but mapping machanism should be useful for other archs
>> to maintain guest's memory mapping. 
> 
> Hollis, does this work for you?
> 
> If now, you can add a new define KVM_WANT_MAPPING or something, and 
> define it for I386 and IA64.

Hi,

This is the must_use_alias patch mentioned in my previous email.

Jes


[-- Attachment #2: 0004-qemu-kvm-arch-alias-handling.patch --]
[-- Type: text/x-patch, Size: 6235 bytes --]

Move must_use_aliases_{source,target} functions to qemu-kvm-<arch> and
introduce qemu-kvm-arch.h header files, making it possible to inline
functions that a noops on a given architecture.

This removes a lot of ugly #ifdef TARGET_I386 from qemu-kvm.c

Signed-off-by: Jes Sorensen <jes@sgi.com>
---
 qemu-kvm-x86.c              |   20 +++++++++++++++++
 qemu-kvm.c                  |   50 +++++++-------------------------------------
 qemu-kvm.h                  |    1 
 target-i386/qemu-kvm-arch.h |   17 ++++++++++++++
 target-ia64/qemu-kvm-arch.h |   22 +++++++++++++++++++
 5 files changed, 69 insertions(+), 41 deletions(-)

Index: qemu-kvm/qemu-kvm-x86.c
===================================================================
--- qemu-kvm.orig/qemu-kvm-x86.c
+++ qemu-kvm/qemu-kvm-x86.c
@@ -851,8 +851,28 @@
 }
 #endif
 
+int destroy_region_works = 0;
+
 void kvm_arch_do_ioperm(void *_data)
 {
     struct ioperm_data *data = _data;
     ioperm(data->start_port, data->num, data->turn_on);
 }
+
+int kvm_arch_must_use_aliases_source(target_phys_addr_t addr)
+{
+    if (destroy_region_works)
+        return false;
+    if (addr == 0xa0000 || addr == 0xa8000)
+        return true;
+    return false;
+}
+
+int kvm_arch_must_use_aliases_target(target_phys_addr_t addr)
+{
+    if (destroy_region_works)
+        return false;
+    if (addr >= 0xe0000000 && addr < 0x100000000ull)
+        return true;
+    return false;
+}
Index: qemu-kvm/qemu-kvm.c
===================================================================
--- qemu-kvm.orig/qemu-kvm.c
+++ qemu-kvm/qemu-kvm.c
@@ -767,10 +767,6 @@
     return 0;
 }
 
-#ifdef TARGET_I386
-static int destroy_region_works = 0;
-#endif
-
 int kvm_qemu_create_context(void)
 {
     int r;
@@ -829,24 +825,6 @@
 }
 
 #ifdef TARGET_I386
-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 >= 0xe0000000 && addr < 0x100000000ull)
-        return true;
-    return false;
-}
-
 static struct mapping {
     target_phys_addr_t phys;
     ram_addr_t ram;
@@ -905,14 +883,13 @@
     area_flags = phys_offset & ~TARGET_PAGE_MASK;
 
     if (area_flags != IO_MEM_RAM) {
-#ifdef TARGET_I386
-        if (must_use_aliases_source(start_addr)) {
+        if (kvm_arch_must_use_aliases_source(start_addr)) {
             kvm_destroy_memory_alias(kvm_context, start_addr);
             return;
         }
-        if (must_use_aliases_target(start_addr))
+        if (kvm_arch_must_use_aliases_target(start_addr))
             return;
-#endif
+
         while (size > 0) {
             p = find_mapping(start_addr);
             if (p) {
@@ -936,8 +913,7 @@
     if (area_flags >= TLB_MMIO)
         return;
 
-#ifdef TARGET_I386
-    if (must_use_aliases_source(start_addr)) {
+    if (kvm_arch_must_use_aliases_source(start_addr)) {
         p = find_ram_mapping(phys_offset);
         if (p) {
             kvm_create_memory_alias(kvm_context, start_addr, size,
@@ -945,7 +921,6 @@
         }
         return;
     }
-#endif
 
     r = kvm_register_phys_mem(kvm_context, start_addr,
                               qemu_get_ram_ptr(phys_offset),
@@ -1256,10 +1231,9 @@
     if (log)
 	kvm_dirty_pages_log_enable_slot(kvm_context, start, size);
     else {
-#ifdef TARGET_I386
-        if (must_use_aliases_target(start))
+        if (kvm_arch_must_use_aliases_target(start))
             return;
-#endif
+
 	kvm_dirty_pages_log_disable_slot(kvm_context, start, size);
     }
 }
@@ -1375,10 +1349,8 @@
 #ifndef TARGET_IA64
     void *buf;
 
-#ifdef TARGET_I386
-    if (must_use_aliases_source(start_addr))
+    if (kvm_arch_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,
@@ -1389,10 +1361,8 @@
 
 int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len)
 {
-#ifdef TARGET_I386
-    if (must_use_aliases_source(phys_addr))
+    if (kvm_arch_must_use_aliases_source(phys_addr))
         return 0;
-#endif
 
 #ifndef TARGET_IA64
     kvm_qemu_log_memory(phys_addr, len, 1);
@@ -1402,10 +1372,8 @@
 
 int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len)
 {
-#ifdef TARGET_I386
-    if (must_use_aliases_source(phys_addr))
+    if (kvm_arch_must_use_aliases_source(phys_addr))
         return 0;
-#endif
 
 #ifndef TARGET_IA64
     kvm_qemu_log_memory(phys_addr, len, 0);
Index: qemu-kvm/qemu-kvm.h
===================================================================
--- qemu-kvm.orig/qemu-kvm.h
+++ qemu-kvm/qemu-kvm.h
@@ -9,6 +9,7 @@
 #define THE_ORIGINAL_AND_TRUE_QEMU_KVM_H
 
 #include "cpu.h"
+#include "qemu-kvm-arch.h"
 
 #include <signal.h>
 
Index: qemu-kvm/target-i386/qemu-kvm-arch.h
===================================================================
--- /dev/null
+++ qemu-kvm/target-i386/qemu-kvm-arch.h
@@ -0,0 +1,17 @@
+/*
+ * qemu/kvm x86 integration
+ *
+ * Copyright (C) 2006-2008 Qumranet Technologies
+ * Copyright (C) 2009 Silicon Graphics Inc.
+ *
+ * Licensed under the terms of the GNU GPL version 2 or higher.
+ */
+#ifndef QEMU_KVM_ARCH_H
+#define QEMU_KVM_ARCH_H
+
+extern int destroy_region_works;
+
+extern int kvm_arch_must_use_aliases_source(target_phys_addr_t addr);
+extern int kvm_arch_must_use_aliases_target(target_phys_addr_t addr);
+
+#endif
Index: qemu-kvm/target-ia64/qemu-kvm-arch.h
===================================================================
--- /dev/null
+++ qemu-kvm/target-ia64/qemu-kvm-arch.h
@@ -0,0 +1,22 @@
+/*
+ * qemu/kvm ia64 integration
+ *
+ * Copyright (C) 2006-2008 Qumranet Technologies
+ * Copyright (C) 2009 Silicon Graphics Inc.
+ *
+ * Licensed under the terms of the GNU GPL version 2 or higher.
+ */
+#ifndef QEMU_KVM_ARCH_H
+#define QEMU_KVM_ARCH_H
+
+static inline int kvm_arch_must_use_aliases_source(target_phys_addr_t addr)
+{
+    return 0;
+}
+
+static inline int kvm_arch_must_use_aliases_target(target_phys_addr_t addr)
+{
+    return 0;
+}
+
+#endif

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Jes Sorensen]

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

Avi Kivity wrote:
> Hollis, does this work for you?
> 
> If now, you can add a new define KVM_WANT_MAPPING or something, and 
> define it for I386 and IA64.


Hi,

This is the one implementing the KVM_WANT_MAPPING change.

Cheers,
Jes



[-- Attachment #2: 0005-qemu-kvm-arch-mapping.patch --]
[-- Type: text/x-patch, Size: 3063 bytes --]

Introduce KVM_WANT_MAPPING define to switch on mapping code, instead
of relying on TARGET_<arch> defines.

Signed-off-by: Jes Sorensen <jes-sJ/iWh9BUns@public.gmane.org>

---
 qemu-kvm.c                  |   42 ++++++++++++++++++++++++++----------------
 target-i386/qemu-kvm-arch.h |    2 ++
 target-ia64/qemu-kvm-arch.h |    2 ++
 3 files changed, 30 insertions(+), 16 deletions(-)

Index: qemu-kvm/qemu-kvm.c
===================================================================
--- qemu-kvm.orig/qemu-kvm.c
+++ qemu-kvm/qemu-kvm.c
@@ -824,7 +824,7 @@
     kvm_finalize(kvm_context);
 }
 
-#ifdef TARGET_I386
+#ifdef KVM_WANT_MAPPING
 static struct mapping {
     target_phys_addr_t phys;
     ram_addr_t ram;
@@ -863,6 +863,26 @@
     if (p)
         *p = mappings[--nr_mappings];
 }
+
+static inline void kvm_cpu_drop_mappings(target_phys_addr_t start_addr,
+                                         unsigned long size)
+{
+    struct mapping *p;
+
+    while (size > 0) {
+        p = find_mapping(start_addr);
+        if (p) {
+            kvm_unregister_memory_area(kvm_context, p->phys, p->len);
+            drop_mapping(p->phys);
+        }
+        start_addr += TARGET_PAGE_SIZE;
+        if (size > TARGET_PAGE_SIZE) {
+            size -= TARGET_PAGE_SIZE;
+        } else {
+            size = 0;
+        }
+    }
+}
 #endif
 
 void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
@@ -871,7 +891,7 @@
 {
     int r = 0;
     unsigned long area_flags;
-#ifdef TARGET_I386
+#ifdef KVM_WANT_MAPPING
     struct mapping *p;
 #endif
 
@@ -890,19 +910,9 @@
         if (kvm_arch_must_use_aliases_target(start_addr))
             return;
 
-        while (size > 0) {
-            p = find_mapping(start_addr);
-            if (p) {
-                kvm_unregister_memory_area(kvm_context, p->phys, p->len);
-                drop_mapping(p->phys);
-            }
-            start_addr += TARGET_PAGE_SIZE;
-            if (size > TARGET_PAGE_SIZE) {
-                size -= TARGET_PAGE_SIZE;
-            } else {
-                size = 0;
-            }
-        }
+#ifdef KVM_WANT_MAPPING
+        kvm_cpu_drop_mappings(start_addr, size);
+#endif
         return;
     }
 
@@ -930,7 +940,7 @@
         exit(1);
     }
 
-#ifdef TARGET_I386
+#ifdef KVM_WANT_MAPPING
     drop_mapping(start_addr);
     p = &mappings[nr_mappings++];
     p->phys = start_addr;
Index: qemu-kvm/target-i386/qemu-kvm-arch.h
===================================================================
--- qemu-kvm.orig/target-i386/qemu-kvm-arch.h
+++ qemu-kvm/target-i386/qemu-kvm-arch.h
@@ -14,4 +14,6 @@
 extern int kvm_arch_must_use_aliases_source(target_phys_addr_t addr);
 extern int kvm_arch_must_use_aliases_target(target_phys_addr_t addr);
 
+#define KVM_WANT_MAPPING	1
+
 #endif
Index: qemu-kvm/target-ia64/qemu-kvm-arch.h
===================================================================
--- qemu-kvm.orig/target-ia64/qemu-kvm-arch.h
+++ qemu-kvm/target-ia64/qemu-kvm-arch.h
@@ -19,4 +19,6 @@
     return 0;
 }
 
+#define KVM_WANT_MAPPING	1
+
 #endif

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Avi Kivity]

Jes Sorensen wrote:
> Avi Kivity wrote:
>>> Currently, use TARGET_I386 to comment out the mapping machanism
>>> for other archs, but mapping machanism should be useful for other archs
>>> to maintain guest's memory mapping. 
>>
>> Hollis, does this work for you?
>>
>> If now, you can add a new define KVM_WANT_MAPPING or something, and 
>> define it for I386 and IA64.
>
> Hi,
>
> This is the must_use_alias patch mentioned in my previous email.


> +int destroy_region_works = 0;
> +
>   

Global name, prefix with kvm_.  Does it actually need to be global?

>  
> Index: qemu-kvm/target-i386/qemu-kvm-arch.h
> ===================================================================
> --- /dev/null
> +++ qemu-kvm/target-i386/qemu-kvm-arch.h
> @@ -0,0 +1,17 @@
> +/*
> + * qemu/kvm x86 integration
> + *
> + * Copyright (C) 2006-2008 Qumranet Technologies
> + * Copyright (C) 2009 Silicon Graphics Inc.
> + *
> + * Licensed under the terms of the GNU GPL version 2 or higher.
> + */
> +#ifndef QEMU_KVM_ARCH_H
> +#define QEMU_KVM_ARCH_H
> +
> +extern int destroy_region_works;
> +
> +extern int kvm_arch_must_use_aliases_source(target_phys_addr_t addr);
> +extern int kvm_arch_must_use_aliases_target(target_phys_addr_t addr);
>   

The header depends on target_phys_addr_t, so it must include whatever 
defines it.

> --- /dev/null
> +++ qemu-kvm/target-ia64/qemu-kvm-arch.h
> @@ -0,0 +1,22 @@
> +/*
> + * qemu/kvm ia64 integration
> + *
> + * Copyright (C) 2006-2008 Qumranet Technologies
> + * Copyright (C) 2009 Silicon Graphics Inc.
> + *
> + * Licensed under the terms of the GNU GPL version 2 or higher.
> + */
> +#ifndef QEMU_KVM_ARCH_H
> +#define QEMU_KVM_ARCH_H
> +
> +static inline int kvm_arch_must_use_aliases_source(target_phys_addr_t addr)
> +{
> +    return 0;
> +}
> +
> +static inline int kvm_arch_must_use_aliases_target(target_phys_addr_t addr)
> +{
> +    return 0;
> +}
> +
> +#endif
>   

Missing other archs...

Instead of duplicating this for every arch, you can have a #define that 
tells you if you want non-trivial arch definitions, and supply the 
trivial definitions in qemu-kvm.h.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Avi Kivity]

Avi Kivity wrote:
> Jes Sorensen wrote:
>> Avi Kivity wrote:
>>> Hollis, does this work for you?
>>>
>>> If now, you can add a new define KVM_WANT_MAPPING or something, and 
>>> define it for I386 and IA64.
>>
>>
>> Hi,
>>
>> This is the one implementing the KVM_WANT_MAPPING change.
>>
>> Cheers,
>> Jes
>>
>>
>
> There is in fact a call to drop_mapping() outside any #ifdef (in 
> kvm_cpu_register_physical_memory()).  I'm confused... maybe we should 
> make this code unconditional.
>


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Jes Sorensen]

Avi Kivity wrote:
> Avi Kivity wrote:
>>> This is the one implementing the KVM_WANT_MAPPING change.
>>>
>> There is in fact a call to drop_mapping() outside any #ifdef (in 
>> kvm_cpu_register_physical_memory()).  I'm confused... maybe we should 
>> make this code unconditional.

Hi Avi,

I don't follow this - if you apply my patch
0005-qemu-kvm-arch-mapping.patch this is no longer the case.

Cheers,
Jes

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Jes Sorensen]

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

Avi Kivity wrote:
> Jes Sorensen wrote:
>> +int destroy_region_works = 0;
> 
> Global name, prefix with kvm_.  Does it actually need to be global?

Gone, now local to qemu-kvm-x86.c. I moved the initializer into
kvm_arch_create_context() instead.

> The header depends on target_phys_addr_t, so it must include whatever 
> defines it.

Added an #include "cpu-all.h" which defines it.

> Missing other archs...
> 
> Instead of duplicating this for every arch, you can have a #define that 
> tells you if you want non-trivial arch definitions, and supply the 
> trivial definitions in qemu-kvm.h.

Done, I also added a PPC header file - which may or may not be wanted
at this point. You can just cut it out if you don't think it should be
added.

What do you think of this version then?

Cheers,
Jes



[-- Attachment #2: 0004-qemu-kvm-arch-alias-handling.patch --]
[-- Type: text/x-patch, Size: 7631 bytes --]

Move must_use_aliases_{source,target} functions to qemu-kvm-<arch> and
introduce qemu-kvm-arch.h header files, making it possible to inline
functions that a noops on a given architecture.

This removes a lot of ugly #ifdef TARGET_I386 from qemu-kvm.c

Signed-off-by: Jes Sorensen <jes-sJ/iWh9BUns@public.gmane.org>
---
 qemu-kvm-x86.c              |   25 +++++++++++++++++++-
 qemu-kvm.c                  |   53 +++++++-------------------------------------
 qemu-kvm.h                  |   13 ++++++++++
 target-i386/qemu-kvm-arch.h |   17 ++++++++++++++
 target-ia64/qemu-kvm-arch.h |   14 +++++++++++
 target-ppc/qemu-kvm-arch.h  |   14 +++++++++++
 6 files changed, 91 insertions(+), 45 deletions(-)

Index: qemu-kvm/qemu-kvm-x86.c
===================================================================
--- qemu-kvm.orig/qemu-kvm-x86.c
+++ qemu-kvm/qemu-kvm-x86.c
@@ -28,6 +28,8 @@
 
 static int lm_capable_kernel;
 
+static int destroy_region_works = 0;
+
 int kvm_qemu_create_memory_alias(uint64_t phys_start,
                                  uint64_t len,
                                  uint64_t target_phys)
@@ -57,7 +59,10 @@
     for (i = 0; i < kvm_msr_list->nmsrs; ++i)
 	if (kvm_msr_list->indices[i] == MSR_STAR)
 	    kvm_has_msr_star = 1;
-	return 0;
+
+    destroy_region_works = kvm_destroy_memory_region_works(kvm_context);
+
+    return 0;
 }
 
 static void set_msr_entry(struct kvm_msr_entry *entry, uint32_t index,
@@ -856,3 +861,21 @@
     struct ioperm_data *data = _data;
     ioperm(data->start_port, data->num, data->turn_on);
 }
+
+int kvm_arch_must_use_aliases_source(target_phys_addr_t addr)
+{
+    if (destroy_region_works)
+        return false;
+    if (addr == 0xa0000 || addr == 0xa8000)
+        return true;
+    return false;
+}
+
+int kvm_arch_must_use_aliases_target(target_phys_addr_t addr)
+{
+    if (destroy_region_works)
+        return false;
+    if (addr >= 0xe0000000 && addr < 0x100000000ull)
+        return true;
+    return false;
+}
Index: qemu-kvm/qemu-kvm.c
===================================================================
--- qemu-kvm.orig/qemu-kvm.c
+++ qemu-kvm/qemu-kvm.c
@@ -767,10 +767,6 @@
     return 0;
 }
 
-#ifdef TARGET_I386
-static int destroy_region_works = 0;
-#endif
-
 int kvm_qemu_create_context(void)
 {
     int r;
@@ -795,9 +791,6 @@
             return -1;
         }
     }
-#ifdef TARGET_I386
-    destroy_region_works = kvm_destroy_memory_region_works(kvm_context);
-#endif
 
     if (kvm_irqchip && kvm_has_gsi_routing(kvm_context)) {
         kvm_clear_gsi_routes(kvm_context);
@@ -829,24 +822,6 @@
 }
 
 #ifdef TARGET_I386
-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 >= 0xe0000000 && addr < 0x100000000ull)
-        return true;
-    return false;
-}
-
 static struct mapping {
     target_phys_addr_t phys;
     ram_addr_t ram;
@@ -905,14 +880,13 @@
     area_flags = phys_offset & ~TARGET_PAGE_MASK;
 
     if (area_flags != IO_MEM_RAM) {
-#ifdef TARGET_I386
-        if (must_use_aliases_source(start_addr)) {
+        if (kvm_arch_must_use_aliases_source(start_addr)) {
             kvm_destroy_memory_alias(kvm_context, start_addr);
             return;
         }
-        if (must_use_aliases_target(start_addr))
+        if (kvm_arch_must_use_aliases_target(start_addr))
             return;
-#endif
+
         while (size > 0) {
             p = find_mapping(start_addr);
             if (p) {
@@ -936,8 +910,7 @@
     if (area_flags >= TLB_MMIO)
         return;
 
-#ifdef TARGET_I386
-    if (must_use_aliases_source(start_addr)) {
+    if (kvm_arch_must_use_aliases_source(start_addr)) {
         p = find_ram_mapping(phys_offset);
         if (p) {
             kvm_create_memory_alias(kvm_context, start_addr, size,
@@ -945,7 +918,6 @@
         }
         return;
     }
-#endif
 
     r = kvm_register_phys_mem(kvm_context, start_addr,
                               qemu_get_ram_ptr(phys_offset),
@@ -1256,10 +1228,9 @@
     if (log)
 	kvm_dirty_pages_log_enable_slot(kvm_context, start, size);
     else {
-#ifdef TARGET_I386
-        if (must_use_aliases_target(start))
+        if (kvm_arch_must_use_aliases_target(start))
             return;
-#endif
+
 	kvm_dirty_pages_log_disable_slot(kvm_context, start, size);
     }
 }
@@ -1375,10 +1346,8 @@
 #ifndef TARGET_IA64
     void *buf;
 
-#ifdef TARGET_I386
-    if (must_use_aliases_source(start_addr))
+    if (kvm_arch_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,
@@ -1389,10 +1358,8 @@
 
 int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len)
 {
-#ifdef TARGET_I386
-    if (must_use_aliases_source(phys_addr))
+    if (kvm_arch_must_use_aliases_source(phys_addr))
         return 0;
-#endif
 
 #ifndef TARGET_IA64
     kvm_qemu_log_memory(phys_addr, len, 1);
@@ -1402,10 +1369,8 @@
 
 int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len)
 {
-#ifdef TARGET_I386
-    if (must_use_aliases_source(phys_addr))
+    if (kvm_arch_must_use_aliases_source(phys_addr))
         return 0;
-#endif
 
 #ifndef TARGET_IA64
     kvm_qemu_log_memory(phys_addr, len, 0);
Index: qemu-kvm/qemu-kvm.h
===================================================================
--- qemu-kvm.orig/qemu-kvm.h
+++ qemu-kvm/qemu-kvm.h
@@ -9,6 +9,7 @@
 #define THE_ORIGINAL_AND_TRUE_QEMU_KVM_H
 
 #include "cpu.h"
+#include "qemu-kvm-arch.h"
 
 #include <signal.h>
 
@@ -233,4 +234,16 @@
     }
 }
 
+#ifdef KVM_ARCH_NO_ADDR_ALIASING
+static inline int kvm_arch_must_use_aliases_source(target_phys_addr_t addr)
+{
+    return 0;
+}
+
+static inline int kvm_arch_must_use_aliases_target(target_phys_addr_t addr)
+{
+    return 0;
+}
+#endif
+
 #endif
Index: qemu-kvm/target-i386/qemu-kvm-arch.h
===================================================================
--- /dev/null
+++ qemu-kvm/target-i386/qemu-kvm-arch.h
@@ -0,0 +1,17 @@
+/*
+ * qemu/kvm x86 integration
+ *
+ * Copyright (C) 2006-2008 Qumranet Technologies
+ * Copyright (C) 2009 Silicon Graphics Inc.
+ *
+ * Licensed under the terms of the GNU GPL version 2 or higher.
+ */
+#ifndef QEMU_KVM_ARCH_H
+#define QEMU_KVM_ARCH_H
+
+#include "cpu-all.h"
+
+extern int kvm_arch_must_use_aliases_source(target_phys_addr_t addr);
+extern int kvm_arch_must_use_aliases_target(target_phys_addr_t addr);
+
+#endif
Index: qemu-kvm/target-ia64/qemu-kvm-arch.h
===================================================================
--- /dev/null
+++ qemu-kvm/target-ia64/qemu-kvm-arch.h
@@ -0,0 +1,14 @@
+/*
+ * qemu/kvm ia64 integration
+ *
+ * Copyright (C) 2006-2008 Qumranet Technologies
+ * Copyright (C) 2009 SGI
+ *
+ * Licensed under the terms of the GNU GPL version 2 or higher.
+ */
+#ifndef QEMU_KVM_ARCH_H
+#define QEMU_KVM_ARCH_H
+
+#define KVM_ARCH_NO_ADDR_ALIASING    1
+
+#endif
Index: qemu-kvm/target-ppc/qemu-kvm-arch.h
===================================================================
--- /dev/null
+++ qemu-kvm/target-ppc/qemu-kvm-arch.h
@@ -0,0 +1,14 @@
+/*
+ * qemu/kvm PPC integration
+ *
+ * Copyright (C) 2006-2008 Qumranet Technologies
+ * Copyright (C) 2009 SGI
+ *
+ * Licensed under the terms of the GNU GPL version 2 or higher.
+ */
+#ifndef QEMU_KVM_ARCH_H
+#define QEMU_KVM_ARCH_H
+
+#define KVM_ARCH_NO_ADDR_ALIASING    1
+
+#endif

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mappingalso.

[Hollis Blanchard]

On Mon, 2009-05-04 at 11:25 +0200, Jes Sorensen wrote:
> Avi Kivity wrote:
> > Jes Sorensen wrote:
> >> +int destroy_region_works = 0;
> > 
> > Global name, prefix with kvm_.  Does it actually need to be global?
> 
> Gone, now local to qemu-kvm-x86.c. I moved the initializer into
> kvm_arch_create_context() instead.
> 
> > The header depends on target_phys_addr_t, so it must include whatever 
> > defines it.
> 
> Added an #include "cpu-all.h" which defines it.
> 
> > Missing other archs...
> > 
> > Instead of duplicating this for every arch, you can have a #define that 
> > tells you if you want non-trivial arch definitions, and supply the 
> > trivial definitions in qemu-kvm.h.
> 
> Done, I also added a PPC header file - which may or may not be wanted
> at this point. You can just cut it out if you don't think it should be
> added.

I don't understand the code being moved, but I guess I don't want it, so
your patch is fine with me.

(Wtf are those magic addresses? And not a single comment?? Aren't we
better than this?)

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Zhang, Xiantao]

Any conclusion on this thread ? 
Xiantao

Zhang, Xiantao wrote:
> From d184d9b0a91ca674961000ed3d35b7fc25d29e03 Mon Sep 17 00:00:00 2001
> From: Xiantao Zhang <xiantao.zhang@intel.com>
> Date: Tue, 28 Apr 2009 16:59:36 +0800
> Subject: [PATCH] qemu-kvm: other archs should maintain memory mapping
> also. 
> 
> Currently, use TARGET_I386 to comment out the mapping machanism
> for other archs, but mapping machanism should be useful for other
> archs 
> to maintain guest's memory mapping.
> 
> Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> ---
>  qemu-kvm.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 68a9218..4f71192 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -846,6 +846,7 @@ static int
>          must_use_aliases_target(target_phys_addr_t addr) return true;
>      return false;
>  }
> +#endif
> 
>  static struct mapping {
>      target_phys_addr_t phys;
> @@ -885,7 +886,6 @@ static void drop_mapping(target_phys_addr_t
>      start_addr) if (p)
>          *p = mappings[--nr_mappings];
>  }
> -#endif
> 
>  void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>                                        unsigned long size,
> @@ -893,9 +893,7 @@ void
>  kvm_cpu_register_physical_memory(target_phys_addr_t start_addr, {
>      int r = 0;
>      unsigned long area_flags;
> -#ifdef TARGET_I386
>      struct mapping *p;
> -#endif
> 
>      if (start_addr + size > phys_ram_size) {
>          phys_ram_size = start_addr + size;
> @@ -955,13 +953,11 @@ void
>          kvm_cpu_register_physical_memory(target_phys_addr_t
>      start_addr, 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;
>  }

Re: [PATCH 04/04] qemu-kvm: other archs should maintain memory mapping also.

[Jes Sorensen]

Zhang, Xiantao wrote:
> Any conclusion on this thread ? 
> Xiantao

Hi Xiantao,

Not that I am aware of, I suspect it's still pending in Avi's tree.

Cheers,
Jes


> Zhang, Xiantao wrote:
>> From d184d9b0a91ca674961000ed3d35b7fc25d29e03 Mon Sep 17 00:00:00 2001
>> From: Xiantao Zhang <xiantao.zhang@intel.com>
>> Date: Tue, 28 Apr 2009 16:59:36 +0800
>> Subject: [PATCH] qemu-kvm: other archs should maintain memory mapping
>> also. 
>>
>> Currently, use TARGET_I386 to comment out the mapping machanism
>> for other archs, but mapping machanism should be useful for other
>> archs 
>> to maintain guest's memory mapping.
>>
>> Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
>> ---
>>  qemu-kvm.c |    6 +-----
>>  1 files changed, 1 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-kvm.c b/qemu-kvm.c
>> index 68a9218..4f71192 100644
>> --- a/qemu-kvm.c
>> +++ b/qemu-kvm.c
>> @@ -846,6 +846,7 @@ static int
>>          must_use_aliases_target(target_phys_addr_t addr) return true;
>>      return false;
>>  }
>> +#endif
>>
>>  static struct mapping {
>>      target_phys_addr_t phys;
>> @@ -885,7 +886,6 @@ static void drop_mapping(target_phys_addr_t
>>      start_addr) if (p)
>>          *p = mappings[--nr_mappings];
>>  }
>> -#endif
>>
>>  void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>>                                        unsigned long size,
>> @@ -893,9 +893,7 @@ void
>>  kvm_cpu_register_physical_memory(target_phys_addr_t start_addr, {
>>      int r = 0;
>>      unsigned long area_flags;
>> -#ifdef TARGET_I386
>>      struct mapping *p;
>> -#endif
>>
>>      if (start_addr + size > phys_ram_size) {
>>          phys_ram_size = start_addr + size;
>> @@ -955,13 +953,11 @@ void
>>          kvm_cpu_register_physical_memory(target_phys_addr_t
>>      start_addr, 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;
>>  }