[Qemu-devel] [PATCH qom-cpu v3 0/9] dump: Build cleanups redone

[Qemu-devel] [PATCH qom-cpu v3 0/9] dump: Build cleanups redone

[Andreas Färber]

Hello,

This series is an alternative to patches previously queued or posted,
based on virgin master.

As requested by Paolo, this replaces Kate's previous memory_mapping split
and my follow-ups and instead goes directly for moving things to CPUState.

All knowledge about dump / memory mapping are moved away from configure.

v3 adds an additional patch proposing a semantic change to facilitate CPU
handling and it prepends another bugfix to avoid merge conflicts.

Regards,
Andreas

v2 -> v3:
* Incorporate Luiz' x86 bugfix.
* Append a patch to resolve the open-coded CPU loops.
* Massage CONFIG_HAVE_* commit messages (they were previously reordered).

v1 -> v2:
* Dropped Kate's memory_mapping split
* Dropped target_ulong cleanup and replaced with typedef
* Amended CPUArchState cleanups with introducing hooks in CPUClass
* Drop memory_memory stubs instead of moving them

Cc: Wen Congyang <wency@cn.fujitsu.com>
Cc: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
Cc: Jens Freimann <jfrei@linux.vnet.ibm.com>
Cc: Vincent Rabin <rabin@rab.in>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Luiz Capitulino <lcapitulino@redhat.com>

Andreas Färber (7):
  dump: Move stubs into libqemustub.a
  cpu: Turn cpu_paging_enabled() into a CPUState hook
  memory_mapping: Move MemoryMappingList typedef to qemu/typedefs.h
  cpu: Turn cpu_get_memory_mapping() into a CPUState hook
  memory_mapping: Drop qemu_get_memory_mapping() stub
  dump: Unconditionally compile
  memory_mapping: Change qemu_get_guest_memory_mapping() semantics

Luiz Capitulino (1):
  target-i386: walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE
    addresses

Qiao Nuohan (1):
  target-i386: Fix mask of pte index in memory mapping

 Makefile.target                   |  8 ++------
 configure                         |  8 --------
 hmp-commands.hx                   |  2 --
 include/qemu/typedefs.h           |  2 ++
 include/qom/cpu.h                 | 21 ++++++++++++++++++++
 include/sysemu/memory_mapping.h   |  8 +++-----
 memory_mapping-stub.c             | 33 ------------------------------
 memory_mapping.c                  | 42 +++++++++++++++++++++------------------
 qom/cpu.c                         | 27 +++++++++++++++++++++++++
 stubs/Makefile.objs               |  1 +
 dump-stub.c => stubs/dump.c       |  8 --------
 target-i386/arch_memory_mapping.c | 23 +++++++++++----------
 target-i386/cpu-qom.h             |  2 ++
 target-i386/cpu.c                 | 12 +++++++++--
 14 files changed, 103 insertions(+), 94 deletions(-)
 delete mode 100644 memory_mapping-stub.c
 rename dump-stub.c => stubs/dump.c (65%)

-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 1/9] dump: Move stubs into libqemustub.a

[Andreas Färber]

This allows us to drop CONFIG_NO_CORE_DUMP with its indirect dependency
on CONFIG_HAVE_CORE_DUMP.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Makefile.target             | 2 --
 stubs/Makefile.objs         | 1 +
 dump-stub.c => stubs/dump.c | 0
 3 files changed, 1 insertion(+), 2 deletions(-)
 rename dump-stub.c => stubs/dump.c (100%)

diff --git a/Makefile.target b/Makefile.target
index ce4391f..1cafb17 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -64,7 +64,6 @@ CONFIG_NO_PCI = $(if $(subst n,,$(CONFIG_PCI)),n,y)
 CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
 CONFIG_NO_XEN = $(if $(subst n,,$(CONFIG_XEN)),n,y)
 CONFIG_NO_GET_MEMORY_MAPPING = $(if $(subst n,,$(CONFIG_HAVE_GET_MEMORY_MAPPING)),n,y)
-CONFIG_NO_CORE_DUMP = $(if $(subst n,,$(CONFIG_HAVE_CORE_DUMP)),n,y)
 
 #########################################################
 # cpu emulator library
@@ -114,7 +113,6 @@ obj-y += memory.o savevm.o cputlb.o
 obj-$(CONFIG_HAVE_GET_MEMORY_MAPPING) += memory_mapping.o
 obj-$(CONFIG_HAVE_CORE_DUMP) += dump.o
 obj-$(CONFIG_NO_GET_MEMORY_MAPPING) += memory_mapping-stub.o
-obj-$(CONFIG_NO_CORE_DUMP) += dump-stub.o
 LIBS+=$(libs_softmmu)
 
 # xen support
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 03dff20..9b701b4 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -2,6 +2,7 @@ stub-obj-y += arch-query-cpu-def.o
 stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
 stub-obj-y += cpu-get-icount.o
+stub-obj-y += dump.o
 stub-obj-y += fdset-add-fd.o
 stub-obj-y += fdset-find-fd.o
 stub-obj-y += fdset-get-fd.o
diff --git a/dump-stub.c b/stubs/dump.c
similarity index 100%
rename from dump-stub.c
rename to stubs/dump.c
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 2/9] target-i386: Fix mask of pte index in memory mapping

[Andreas Färber]

From: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>

Function walk_pte() needs pte index to calculate virtual address.
However, pte index of PAE paging or IA-32e paging is 9 bit, so the mask
should be 0x1ff.

Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
Reviewed-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/arch_memory_mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
index 844893f..a2eb7e7 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -38,7 +38,7 @@ static void walk_pte(MemoryMappingList *list, hwaddr pte_start_addr,
             continue;
         }
 
-        start_vaddr = start_line_addr | ((i & 0x1fff) << 12);
+        start_vaddr = start_line_addr | ((i & 0x1ff) << 12);
         memory_mapping_list_add_merge_sorted(list, start_paddr,
                                              start_vaddr, 1 << 12);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 3/9] target-i386: walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses

[Andreas Färber]

From: Luiz Capitulino <lcapitulino@redhat.com>

The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.

However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xfffffffffffff000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.

This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/arch_memory_mapping.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
index a2eb7e7..5096fbd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
 }
 
 /* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xffffffffff000 /* selects bits 51:12 */
+
 static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
                      int32_t a20_mask, target_ulong start_line_addr)
 {
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
             continue;
         }
 
-        pte_start_addr = (pde & ~0xfff) & a20_mask;
+        pte_start_addr = (pde & PLM4_ADDR_MASK) & a20_mask;
         walk_pte(list, pte_start_addr, a20_mask, line_addr);
     }
 }
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
             continue;
         }
 
-        pde_start_addr = (pdpe & ~0xfff) & a20_mask;
+        pde_start_addr = (pdpe & PLM4_ADDR_MASK) & a20_mask;
         walk_pde(list, pde_start_addr, a20_mask, line_addr);
     }
 }
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
         }
 
         line_addr = ((i & 0x1ffULL) << 39) | (0xffffULL << 48);
-        pdpe_start_addr = (pml4e & ~0xfff) & a20_mask;
+        pdpe_start_addr = (pml4e & PLM4_ADDR_MASK) & a20_mask;
         walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
     }
 }
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
         if (env->hflags & HF_LMA_MASK) {
             hwaddr pml4e_addr;
 
-            pml4e_addr = (env->cr[3] & ~0xfff) & env->a20_mask;
+            pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask;
             walk_pml4e(list, pml4e_addr, env->a20_mask);
         } else
 #endif
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/cpu.h                 | 10 ++++++++++
 include/sysemu/memory_mapping.h   |  1 -
 memory_mapping-stub.c             |  6 ------
 memory_mapping.c                  |  2 +-
 qom/cpu.c                         | 13 +++++++++++++
 target-i386/arch_memory_mapping.c |  6 +-----
 target-i386/cpu.c                 | 11 +++++++++--
 7 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7cd9442..cf5fec2 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -48,6 +48,7 @@ typedef struct CPUState CPUState;
  * @reset: Callback to reset the #CPUState to its initial state.
  * @do_interrupt: Callback for interrupt handling.
  * @get_arch_id: Callback for getting architecture-dependent CPU ID.
+ * @get_paging_enabled: Callback for inquiring whether paging is enabled.
  * @vmsd: State description for migration.
  *
  * Represents a CPU family or model.
@@ -62,6 +63,7 @@ typedef struct CPUClass {
     void (*reset)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
     int64_t (*get_arch_id)(CPUState *cpu);
+    bool (*get_paging_enabled)(CPUState *cpu);
 
     const struct VMStateDescription *vmsd;
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
@@ -138,6 +140,14 @@ struct CPUState {
 };
 
 /**
+ * cpu_paging_enabled:
+ * @cpu: The CPU whose state is to be inspected.
+ *
+ * Returns: %true if paging is enabled, %false otherwise.
+ */
+bool cpu_paging_enabled(CPUState *cpu);
+
+/**
  * cpu_write_elf64_note:
  * @f: pointer to a function that writes memory to a file
  * @cpu: The CPU whose memory is to be dumped
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 1256125..6f01524 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -31,7 +31,6 @@ typedef struct MemoryMappingList {
 } MemoryMappingList;
 
 int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
-bool cpu_paging_enabled(CPUArchState *env);
 
 /*
  * add or merge the memory region [phys_addr, phys_addr + length) into the
diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
index 24d5d67..6c0dfeb 100644
--- a/memory_mapping-stub.c
+++ b/memory_mapping-stub.c
@@ -25,9 +25,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
 {
     return -1;
 }
-
-bool cpu_paging_enabled(CPUArchState *env)
-{
-    return true;
-}
-
diff --git a/memory_mapping.c b/memory_mapping.c
index ff45b3a..0790aac 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -170,7 +170,7 @@ static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
     CPUArchState *env;
 
     for (env = start_cpu; env != NULL; env = env->next_cpu) {
-        if (cpu_paging_enabled(env)) {
+        if (cpu_paging_enabled(ENV_GET_CPU(env))) {
             return env;
         }
     }
diff --git a/qom/cpu.c b/qom/cpu.c
index 04aefbb..ea7e676 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -50,6 +50,18 @@ bool cpu_exists(int64_t id)
     return data.found;
 }
 
+bool cpu_paging_enabled(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    return cc->get_paging_enabled(cpu);
+}
+
+static bool cpu_common_get_paging_enabled(CPUState *cpu)
+{
+    return true;
+}
+
 /* CPU hot-plug notifiers */
 static NotifierList cpu_added_notifiers =
     NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
@@ -176,6 +188,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->class_by_name = cpu_common_class_by_name;
     k->reset = cpu_common_reset;
     k->get_arch_id = cpu_common_get_arch_id;
+    k->get_paging_enabled = cpu_common_get_paging_enabled;
     k->write_elf32_qemunote = cpu_common_write_elf32_qemunote;
     k->write_elf32_note = cpu_common_write_elf32_note;
     k->write_elf64_qemunote = cpu_common_write_elf64_qemunote;
diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
index 5096fbd..c5a10ec 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -241,7 +241,7 @@ static void walk_pml4e(MemoryMappingList *list,
 
 int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
 {
-    if (!cpu_paging_enabled(env)) {
+    if (!cpu_paging_enabled(ENV_GET_CPU(env))) {
         /* paging is disabled */
         return 0;
     }
@@ -273,7 +273,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
     return 0;
 }
 
-bool cpu_paging_enabled(CPUArchState *env)
-{
-    return env->cr[0] & CR0_PG_MASK;
-}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1a501d9..7364e3b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2505,6 +2505,13 @@ static int64_t x86_cpu_get_arch_id(CPUState *cs)
     return env->cpuid_apic_id;
 }
 
+static bool x86_cpu_get_paging_enabled(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    return cpu->env.cr[0] & CR0_PG_MASK;
+}
+
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 {
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
@@ -2519,6 +2526,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->reset = x86_cpu_reset;
 
     cc->do_interrupt = x86_cpu_do_interrupt;
+    cc->get_arch_id = x86_cpu_get_arch_id;
+    cc->get_paging_enabled = x86_cpu_get_paging_enabled;
 #ifndef CONFIG_USER_ONLY
     cc->write_elf64_note = x86_cpu_write_elf64_note;
     cc->write_elf64_qemunote = x86_cpu_write_elf64_qemunote;
@@ -2526,8 +2535,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->write_elf32_qemunote = x86_cpu_write_elf32_qemunote;
 #endif
     cpu_class_set_vmsd(cc, &vmstate_x86_cpu);
-
-    cc->get_arch_id = x86_cpu_get_arch_id;
 }
 
 static const TypeInfo x86_cpu_type_info = {
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 5/9] memory_mapping: Move MemoryMappingList typedef to qemu/typedefs.h

[Andreas Färber]

This will avoid issues with hwaddr and ram_addr_t when including
sysemu/memory_mapping.h for CONFIG_USER_ONLY.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qemu/typedefs.h         | 2 ++
 include/sysemu/memory_mapping.h | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 93aae81..1218a61 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -22,6 +22,8 @@ typedef struct AddressSpace AddressSpace;
 typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionSection MemoryRegionSection;
 
+typedef struct MemoryMappingList MemoryMappingList;
+
 typedef struct NICInfo NICInfo;
 typedef struct HCIInfo HCIInfo;
 typedef struct AudioState AudioState;
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 6f01524..1f71c32 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -15,6 +15,7 @@
 #define MEMORY_MAPPING_H
 
 #include "qemu/queue.h"
+#include "qemu/typedefs.h"
 
 /* The physical and virtual address in the memory mapping are contiguous. */
 typedef struct MemoryMapping {
@@ -24,11 +25,11 @@ typedef struct MemoryMapping {
     QTAILQ_ENTRY(MemoryMapping) next;
 } MemoryMapping;
 
-typedef struct MemoryMappingList {
+struct MemoryMappingList {
     unsigned int num;
     MemoryMapping *last_mapping;
     QTAILQ_HEAD(, MemoryMapping) head;
-} MemoryMappingList;
+};
 
 int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
 
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 6/9] cpu: Turn cpu_get_memory_mapping() into a CPUState hook

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/cpu.h                 | 11 +++++++++++
 include/sysemu/memory_mapping.h   |  2 --
 memory_mapping-stub.c             |  6 ------
 memory_mapping.c                  |  2 +-
 qom/cpu.c                         | 14 ++++++++++++++
 target-i386/arch_memory_mapping.c |  7 +++++--
 target-i386/cpu-qom.h             |  2 ++
 target-i386/cpu.c                 |  1 +
 8 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index cf5fec2..93a4612 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -23,6 +23,7 @@
 #include <signal.h>
 #include "hw/qdev-core.h"
 #include "qemu/thread.h"
+#include "qemu/typedefs.h"
 
 typedef int (*WriteCoreDumpFunction)(void *buf, size_t size, void *opaque);
 
@@ -49,6 +50,7 @@ typedef struct CPUState CPUState;
  * @do_interrupt: Callback for interrupt handling.
  * @get_arch_id: Callback for getting architecture-dependent CPU ID.
  * @get_paging_enabled: Callback for inquiring whether paging is enabled.
+ * @get_memory_mapping: Callback for obtaining the memory mappings.
  * @vmsd: State description for migration.
  *
  * Represents a CPU family or model.
@@ -64,6 +66,7 @@ typedef struct CPUClass {
     void (*do_interrupt)(CPUState *cpu);
     int64_t (*get_arch_id)(CPUState *cpu);
     bool (*get_paging_enabled)(CPUState *cpu);
+    int (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list);
 
     const struct VMStateDescription *vmsd;
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
@@ -148,6 +151,14 @@ struct CPUState {
 bool cpu_paging_enabled(CPUState *cpu);
 
 /**
+ * @cpu: The CPU whose memory mappings are to be obtained.
+ * @list: Where to write the memory mappings to.
+ *
+ * Returns: 0 if successful.
+ */
+int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
+
+/**
  * cpu_write_elf64_note:
  * @f: pointer to a function that writes memory to a file
  * @cpu: The CPU whose memory is to be dumped
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 1f71c32..c47e6ee 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -31,8 +31,6 @@ struct MemoryMappingList {
     QTAILQ_HEAD(, MemoryMapping) head;
 };
 
-int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
-
 /*
  * add or merge the memory region [phys_addr, phys_addr + length) into the
  * memory mapping's list. The region's virtual address starts with virt_addr,
diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
index 6c0dfeb..989dc00 100644
--- a/memory_mapping-stub.c
+++ b/memory_mapping-stub.c
@@ -19,9 +19,3 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
 {
     return -2;
 }
-
-int cpu_get_memory_mapping(MemoryMappingList *list,
-					                                          CPUArchState *env)
-{
-    return -1;
-}
diff --git a/memory_mapping.c b/memory_mapping.c
index 0790aac..481530a 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -188,7 +188,7 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
     first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
     if (first_paging_enabled_cpu) {
         for (env = first_paging_enabled_cpu; env != NULL; env = env->next_cpu) {
-            ret = cpu_get_memory_mapping(list, env);
+            ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
             if (ret < 0) {
                 return -1;
             }
diff --git a/qom/cpu.c b/qom/cpu.c
index ea7e676..e7e1c25 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -62,6 +62,19 @@ static bool cpu_common_get_paging_enabled(CPUState *cpu)
     return true;
 }
 
+int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    return cc->get_memory_mapping(cpu, list);
+}
+
+static int cpu_common_get_memory_mapping(CPUState *cpu,
+                                         MemoryMappingList *list)
+{
+    return -1;
+}
+
 /* CPU hot-plug notifiers */
 static NotifierList cpu_added_notifiers =
     NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
@@ -189,6 +202,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->reset = cpu_common_reset;
     k->get_arch_id = cpu_common_get_arch_id;
     k->get_paging_enabled = cpu_common_get_paging_enabled;
+    k->get_memory_mapping = cpu_common_get_memory_mapping;
     k->write_elf32_qemunote = cpu_common_write_elf32_qemunote;
     k->write_elf32_note = cpu_common_write_elf32_note;
     k->write_elf64_qemunote = cpu_common_write_elf64_qemunote;
diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
index c5a10ec..b117068 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -239,9 +239,12 @@ static void walk_pml4e(MemoryMappingList *list,
 }
 #endif
 
-int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
+int x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list)
 {
-    if (!cpu_paging_enabled(ENV_GET_CPU(env))) {
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+
+    if (!cpu_paging_enabled(cs)) {
         /* paging is disabled */
         return 0;
     }
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 849cedf..11a4b10 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -98,4 +98,6 @@ int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
 int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
                                  void *opaque);
 
+int x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
+
 #endif
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7364e3b..1303892 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2529,6 +2529,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->get_arch_id = x86_cpu_get_arch_id;
     cc->get_paging_enabled = x86_cpu_get_paging_enabled;
 #ifndef CONFIG_USER_ONLY
+    cc->get_memory_mapping = x86_cpu_get_memory_mapping;
     cc->write_elf64_note = x86_cpu_write_elf64_note;
     cc->write_elf64_qemunote = x86_cpu_write_elf64_qemunote;
     cc->write_elf32_note = x86_cpu_write_elf32_note;
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 7/9] memory_mapping: Drop qemu_get_memory_mapping() stub

[Andreas Färber]

dump.c:dump_init() never checked for the return code anyway.
If paging is not enabled, it will fall back to an identity map.
If paging is enabled and getting memory mapping list is not
implemented, qemu_get_guest_memory_mapping() will return an error.

Since the targets not implementing memory mapping also don't implement
dump support, we will not reach this code today and can worry about
changing cpu_paging_enabled() default when the need arises.

This allows us to drop CONFIG_HAVE_GET_MEMORY_SUPPORT.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Makefile.target       |  4 +---
 configure             |  4 ----
 memory_mapping-stub.c | 21 ---------------------
 3 files changed, 1 insertion(+), 28 deletions(-)
 delete mode 100644 memory_mapping-stub.c

diff --git a/Makefile.target b/Makefile.target
index 1cafb17..f9e1d89 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -63,7 +63,6 @@ all: $(PROGS) stap
 CONFIG_NO_PCI = $(if $(subst n,,$(CONFIG_PCI)),n,y)
 CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
 CONFIG_NO_XEN = $(if $(subst n,,$(CONFIG_XEN)),n,y)
-CONFIG_NO_GET_MEMORY_MAPPING = $(if $(subst n,,$(CONFIG_HAVE_GET_MEMORY_MAPPING)),n,y)
 
 #########################################################
 # cpu emulator library
@@ -110,9 +109,8 @@ obj-y += hw/
 obj-$(CONFIG_FDT) += device_tree.o
 obj-$(CONFIG_KVM) += kvm-all.o
 obj-y += memory.o savevm.o cputlb.o
-obj-$(CONFIG_HAVE_GET_MEMORY_MAPPING) += memory_mapping.o
+obj-y += memory_mapping.o
 obj-$(CONFIG_HAVE_CORE_DUMP) += dump.o
-obj-$(CONFIG_NO_GET_MEMORY_MAPPING) += memory_mapping-stub.o
 LIBS+=$(libs_softmmu)
 
 # xen support
diff --git a/configure b/configure
index eb74510..11e186e 100755
--- a/configure
+++ b/configure
@@ -4339,10 +4339,6 @@ case "$target_arch2" in
       fi
     fi
 esac
-case "$target_arch2" in
-  i386|x86_64)
-    echo "CONFIG_HAVE_GET_MEMORY_MAPPING=y" >> $config_target_mak
-esac
 if test "$target_bigendian" = "yes" ; then
   echo "TARGET_WORDS_BIGENDIAN=y" >> $config_target_mak
 fi
diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
deleted file mode 100644
index 989dc00..0000000
--- a/memory_mapping-stub.c
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * QEMU memory mapping
- *
- * Copyright Fujitsu, Corp. 2011, 2012
- *
- * Authors:
- *     Wen Congyang <wency@cn.fujitsu.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#include "cpu.h"
-#include "exec/cpu-all.h"
-#include "sysemu/memory_mapping.h"
-
-int qemu_get_guest_memory_mapping(MemoryMappingList *list)
-{
-    return -2;
-}
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 8/9] dump: Unconditionally compile

[Andreas Färber]

qmp_dump_guest_memory() calls dump_init() and returns an Error when
cpu_get_dump_info() returns an error, as done by the stub.
So there is no need to have a stub for qmp_dump_guest_memory().

Enable the documentation of the always-present dump-guest-memory command.

That way we can drop CONFIG_HAVE_CORE_DUMP and leave configure
completely out of the picture for target CPU features.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Makefile.target | 2 +-
 configure       | 4 ----
 hmp-commands.hx | 2 --
 stubs/dump.c    | 8 --------
 4 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index f9e1d89..b0be124 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -110,7 +110,7 @@ obj-$(CONFIG_FDT) += device_tree.o
 obj-$(CONFIG_KVM) += kvm-all.o
 obj-y += memory.o savevm.o cputlb.o
 obj-y += memory_mapping.o
-obj-$(CONFIG_HAVE_CORE_DUMP) += dump.o
+obj-y += dump.o
 LIBS+=$(libs_softmmu)
 
 # xen support
diff --git a/configure b/configure
index 11e186e..33e9435 100755
--- a/configure
+++ b/configure
@@ -4344,10 +4344,6 @@ if test "$target_bigendian" = "yes" ; then
 fi
 if test "$target_softmmu" = "yes" ; then
   echo "CONFIG_SOFTMMU=y" >> $config_target_mak
-  case "$target_arch2" in
-    i386|x86_64)
-      echo "CONFIG_HAVE_CORE_DUMP=y" >> $config_target_mak
-  esac
 fi
 if test "$target_user_only" = "yes" ; then
   echo "CONFIG_USER_ONLY=y" >> $config_target_mak
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..074dbe1 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -989,7 +989,6 @@ server will ask the spice/vnc client to automatically reconnect using the
 new parameters (if specified) once the vm migration finished successfully.
 ETEXI
 
-#if defined(CONFIG_HAVE_CORE_DUMP)
     {
         .name       = "dump-guest-memory",
         .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
@@ -1013,7 +1012,6 @@ gdb.
     length: the memory size, in bytes. It's optional, and should be specified
             with begin together.
 ETEXI
-#endif
 
     {
         .name       = "snapshot_blkdev",
diff --git a/stubs/dump.c b/stubs/dump.c
index b3f42cb..43c9a3f 100644
--- a/stubs/dump.c
+++ b/stubs/dump.c
@@ -16,14 +16,6 @@
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
 
-/* we need this function in hmp.c */
-void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
-                           int64_t begin, bool has_length, int64_t length,
-                           Error **errp)
-{
-    error_set(errp, QERR_UNSUPPORTED);
-}
-
 int cpu_get_dump_info(ArchDumpInfo *info)
 {
     return -1;
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu v3 9/9] memory_mapping: Change qemu_get_guest_memory_mapping() semantics

[Andreas Färber]

Previously it would search for the first CPU with paging enabled and
retrieve memory mappings from this and any following CPUs or return an
error if that fails.

Instead walk all CPUs and if paging is enabled retrieve the memory
mappings. Fail only if retrieving memory mappings on a CPU with paging
enabled fails.

This not only allows to more easily use one qemu_for_each_cpu() instead
of open-coding two CPU loops and drops find_first_paging_enabled_cpu()
but removes the implicit assumption of heterogeneity between CPUs n..N
in having paging enabled.

Cc: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 memory_mapping.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/memory_mapping.c b/memory_mapping.c
index 481530a..55ac2b8 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -165,35 +165,39 @@ void memory_mapping_list_init(MemoryMappingList *list)
     QTAILQ_INIT(&list->head);
 }
 
-static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
+typedef struct GetGuestMemoryMappingData {
+    MemoryMappingList *list;
+    int ret;
+} GetGuestMemoryMappingData;
+
+static void qemu_get_one_guest_memory_mapping(CPUState *cpu, void *data)
 {
-    CPUArchState *env;
+    GetGuestMemoryMappingData *s = data;
+    int ret;
 
-    for (env = start_cpu; env != NULL; env = env->next_cpu) {
-        if (cpu_paging_enabled(ENV_GET_CPU(env))) {
-            return env;
-        }
+    if (!cpu_paging_enabled(cpu) || s->ret == -1) {
+        return;
+    }
+    ret = cpu_get_memory_mapping(cpu, s->list);
+    if (ret < 0) {
+        s->ret = -1;
+    } else {
+        s->ret = 0;
     }
-
-    return NULL;
 }
 
 int qemu_get_guest_memory_mapping(MemoryMappingList *list)
 {
-    CPUArchState *env, *first_paging_enabled_cpu;
+    GetGuestMemoryMappingData s = {
+        .list = list,
+        .ret = -2,
+    };
     RAMBlock *block;
     ram_addr_t offset, length;
-    int ret;
 
-    first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
-    if (first_paging_enabled_cpu) {
-        for (env = first_paging_enabled_cpu; env != NULL; env = env->next_cpu) {
-            ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
-            if (ret < 0) {
-                return -1;
-            }
-        }
-        return 0;
+    qemu_for_each_cpu(qemu_get_one_guest_memory_mapping, &s);
+    if (s.ret != -2) {
+        return s.ret;
     }
 
     /*
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook

[Luiz Capitulino]

On Thu, 30 May 2013 17:07:56 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Signed-off-by: Andreas Färber <afaerber@suse.de>

Nitpick alarm on.

> ---
>  include/qom/cpu.h                 | 10 ++++++++++
>  include/sysemu/memory_mapping.h   |  1 -
>  memory_mapping-stub.c             |  6 ------
>  memory_mapping.c                  |  2 +-
>  qom/cpu.c                         | 13 +++++++++++++
>  target-i386/arch_memory_mapping.c |  6 +-----
>  target-i386/cpu.c                 | 11 +++++++++--
>  7 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7cd9442..cf5fec2 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -48,6 +48,7 @@ typedef struct CPUState CPUState;
>   * @reset: Callback to reset the #CPUState to its initial state.
>   * @do_interrupt: Callback for interrupt handling.
>   * @get_arch_id: Callback for getting architecture-dependent CPU ID.
> + * @get_paging_enabled: Callback for inquiring whether paging is enabled.
>   * @vmsd: State description for migration.
>   *
>   * Represents a CPU family or model.
> @@ -62,6 +63,7 @@ typedef struct CPUClass {
>      void (*reset)(CPUState *cpu);
>      void (*do_interrupt)(CPUState *cpu);
>      int64_t (*get_arch_id)(CPUState *cpu);
> +    bool (*get_paging_enabled)(CPUState *cpu);

Argument could be const?

>  
>      const struct VMStateDescription *vmsd;
>      int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
> @@ -138,6 +140,14 @@ struct CPUState {
>  };
>  
>  /**
> + * cpu_paging_enabled:
> + * @cpu: The CPU whose state is to be inspected.
> + *
> + * Returns: %true if paging is enabled, %false otherwise.
> + */
> +bool cpu_paging_enabled(CPUState *cpu);
> +
> +/**
>   * cpu_write_elf64_note:
>   * @f: pointer to a function that writes memory to a file
>   * @cpu: The CPU whose memory is to be dumped
> diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
> index 1256125..6f01524 100644
> --- a/include/sysemu/memory_mapping.h
> +++ b/include/sysemu/memory_mapping.h
> @@ -31,7 +31,6 @@ typedef struct MemoryMappingList {
>  } MemoryMappingList;
>  
>  int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
> -bool cpu_paging_enabled(CPUArchState *env);
>  
>  /*
>   * add or merge the memory region [phys_addr, phys_addr + length) into the
> diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
> index 24d5d67..6c0dfeb 100644
> --- a/memory_mapping-stub.c
> +++ b/memory_mapping-stub.c
> @@ -25,9 +25,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
>  {
>      return -1;
>  }
> -
> -bool cpu_paging_enabled(CPUArchState *env)
> -{
> -    return true;
> -}
> -
> diff --git a/memory_mapping.c b/memory_mapping.c
> index ff45b3a..0790aac 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -170,7 +170,7 @@ static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
>      CPUArchState *env;
>  
>      for (env = start_cpu; env != NULL; env = env->next_cpu) {
> -        if (cpu_paging_enabled(env)) {
> +        if (cpu_paging_enabled(ENV_GET_CPU(env))) {
>              return env;
>          }
>      }
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 04aefbb..ea7e676 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -50,6 +50,18 @@ bool cpu_exists(int64_t id)
>      return data.found;
>  }
>  
> +bool cpu_paging_enabled(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    return cc->get_paging_enabled(cpu);
> +}
> +
> +static bool cpu_common_get_paging_enabled(CPUState *cpu)
> +{
> +    return true;
> +}

Not sure if this is important, but I wonder if we want to do this

I mean, for all cases where you want to know if paging is enabled, what
will happen if this default method says "yes, it's enabled" but it
actually isn't?

> +
>  /* CPU hot-plug notifiers */
>  static NotifierList cpu_added_notifiers =
>      NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
> @@ -176,6 +188,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->class_by_name = cpu_common_class_by_name;
>      k->reset = cpu_common_reset;
>      k->get_arch_id = cpu_common_get_arch_id;
> +    k->get_paging_enabled = cpu_common_get_paging_enabled;
>      k->write_elf32_qemunote = cpu_common_write_elf32_qemunote;
>      k->write_elf32_note = cpu_common_write_elf32_note;
>      k->write_elf64_qemunote = cpu_common_write_elf64_qemunote;
> diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
> index 5096fbd..c5a10ec 100644
> --- a/target-i386/arch_memory_mapping.c
> +++ b/target-i386/arch_memory_mapping.c
> @@ -241,7 +241,7 @@ static void walk_pml4e(MemoryMappingList *list,
>  
>  int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
>  {
> -    if (!cpu_paging_enabled(env)) {
> +    if (!cpu_paging_enabled(ENV_GET_CPU(env))) {
>          /* paging is disabled */
>          return 0;
>      }
> @@ -273,7 +273,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
>      return 0;
>  }
>  
> -bool cpu_paging_enabled(CPUArchState *env)
> -{
> -    return env->cr[0] & CR0_PG_MASK;
> -}
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1a501d9..7364e3b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2505,6 +2505,13 @@ static int64_t x86_cpu_get_arch_id(CPUState *cs)
>      return env->cpuid_apic_id;
>  }
>  
> +static bool x86_cpu_get_paging_enabled(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +
> +    return cpu->env.cr[0] & CR0_PG_MASK;
> +}
> +
>  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> @@ -2519,6 +2526,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->reset = x86_cpu_reset;
>  
>      cc->do_interrupt = x86_cpu_do_interrupt;
> +    cc->get_arch_id = x86_cpu_get_arch_id;

Unrelated change?

> +    cc->get_paging_enabled = x86_cpu_get_paging_enabled;
>  #ifndef CONFIG_USER_ONLY
>      cc->write_elf64_note = x86_cpu_write_elf64_note;
>      cc->write_elf64_qemunote = x86_cpu_write_elf64_qemunote;
> @@ -2526,8 +2535,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->write_elf32_qemunote = x86_cpu_write_elf32_qemunote;
>  #endif
>      cpu_class_set_vmsd(cc, &vmstate_x86_cpu);
> -
> -    cc->get_arch_id = x86_cpu_get_arch_id;
>  }
>  
>  static const TypeInfo x86_cpu_type_info = {

Re: [Qemu-devel] [PATCH qom-cpu v3 6/9] cpu: Turn cpu_get_memory_mapping() into a CPUState hook

[Luiz Capitulino]

On Thu, 30 May 2013 17:07:58 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qom/cpu.h                 | 11 +++++++++++
>  include/sysemu/memory_mapping.h   |  2 --
>  memory_mapping-stub.c             |  6 ------
>  memory_mapping.c                  |  2 +-
>  qom/cpu.c                         | 14 ++++++++++++++
>  target-i386/arch_memory_mapping.c |  7 +++++--
>  target-i386/cpu-qom.h             |  2 ++
>  target-i386/cpu.c                 |  1 +
>  8 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index cf5fec2..93a4612 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -23,6 +23,7 @@
>  #include <signal.h>
>  #include "hw/qdev-core.h"
>  #include "qemu/thread.h"
> +#include "qemu/typedefs.h"
>  
>  typedef int (*WriteCoreDumpFunction)(void *buf, size_t size, void *opaque);
>  
> @@ -49,6 +50,7 @@ typedef struct CPUState CPUState;
>   * @do_interrupt: Callback for interrupt handling.
>   * @get_arch_id: Callback for getting architecture-dependent CPU ID.
>   * @get_paging_enabled: Callback for inquiring whether paging is enabled.
> + * @get_memory_mapping: Callback for obtaining the memory mappings.
>   * @vmsd: State description for migration.
>   *
>   * Represents a CPU family or model.
> @@ -64,6 +66,7 @@ typedef struct CPUClass {
>      void (*do_interrupt)(CPUState *cpu);
>      int64_t (*get_arch_id)(CPUState *cpu);
>      bool (*get_paging_enabled)(CPUState *cpu);
> +    int (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list);

Would be nice to take an Error argument and fill it properly when
get_memory_mapping() is not implemented.

>  
>      const struct VMStateDescription *vmsd;
>      int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
> @@ -148,6 +151,14 @@ struct CPUState {
>  bool cpu_paging_enabled(CPUState *cpu);
>  
>  /**
> + * @cpu: The CPU whose memory mappings are to be obtained.
> + * @list: Where to write the memory mappings to.
> + *
> + * Returns: 0 if successful.
> + */
> +int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
> +
> +/**
>   * cpu_write_elf64_note:
>   * @f: pointer to a function that writes memory to a file
>   * @cpu: The CPU whose memory is to be dumped
> diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
> index 1f71c32..c47e6ee 100644
> --- a/include/sysemu/memory_mapping.h
> +++ b/include/sysemu/memory_mapping.h
> @@ -31,8 +31,6 @@ struct MemoryMappingList {
>      QTAILQ_HEAD(, MemoryMapping) head;
>  };
>  
> -int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
> -
>  /*
>   * add or merge the memory region [phys_addr, phys_addr + length) into the
>   * memory mapping's list. The region's virtual address starts with virt_addr,
> diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
> index 6c0dfeb..989dc00 100644
> --- a/memory_mapping-stub.c
> +++ b/memory_mapping-stub.c
> @@ -19,9 +19,3 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
>  {
>      return -2;
>  }
> -
> -int cpu_get_memory_mapping(MemoryMappingList *list,
> -					                                          CPUArchState *env)
> -{
> -    return -1;
> -}
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 0790aac..481530a 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -188,7 +188,7 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
>      first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
>      if (first_paging_enabled_cpu) {
>          for (env = first_paging_enabled_cpu; env != NULL; env = env->next_cpu) {
> -            ret = cpu_get_memory_mapping(list, env);
> +            ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
>              if (ret < 0) {
>                  return -1;
>              }
> diff --git a/qom/cpu.c b/qom/cpu.c
> index ea7e676..e7e1c25 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -62,6 +62,19 @@ static bool cpu_common_get_paging_enabled(CPUState *cpu)
>      return true;
>  }
>  
> +int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    return cc->get_memory_mapping(cpu, list);
> +}
> +
> +static int cpu_common_get_memory_mapping(CPUState *cpu,
> +                                         MemoryMappingList *list)
> +{
> +    return -1;
> +}
> +
>  /* CPU hot-plug notifiers */
>  static NotifierList cpu_added_notifiers =
>      NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
> @@ -189,6 +202,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->reset = cpu_common_reset;
>      k->get_arch_id = cpu_common_get_arch_id;
>      k->get_paging_enabled = cpu_common_get_paging_enabled;
> +    k->get_memory_mapping = cpu_common_get_memory_mapping;
>      k->write_elf32_qemunote = cpu_common_write_elf32_qemunote;
>      k->write_elf32_note = cpu_common_write_elf32_note;
>      k->write_elf64_qemunote = cpu_common_write_elf64_qemunote;
> diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
> index c5a10ec..b117068 100644
> --- a/target-i386/arch_memory_mapping.c
> +++ b/target-i386/arch_memory_mapping.c
> @@ -239,9 +239,12 @@ static void walk_pml4e(MemoryMappingList *list,
>  }
>  #endif
>  
> -int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
> +int x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list)
>  {
> -    if (!cpu_paging_enabled(ENV_GET_CPU(env))) {
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +
> +    if (!cpu_paging_enabled(cs)) {
>          /* paging is disabled */
>          return 0;
>      }
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 849cedf..11a4b10 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -98,4 +98,6 @@ int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
>  int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
>                                   void *opaque);
>  
> +int x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
> +
>  #endif
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7364e3b..1303892 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2529,6 +2529,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->get_arch_id = x86_cpu_get_arch_id;
>      cc->get_paging_enabled = x86_cpu_get_paging_enabled;
>  #ifndef CONFIG_USER_ONLY
> +    cc->get_memory_mapping = x86_cpu_get_memory_mapping;
>      cc->write_elf64_note = x86_cpu_write_elf64_note;
>      cc->write_elf64_qemunote = x86_cpu_write_elf64_qemunote;
>      cc->write_elf32_note = x86_cpu_write_elf32_note;

Re: [Qemu-devel] [PATCH qom-cpu v3 9/9] memory_mapping: Change qemu_get_guest_memory_mapping() semantics

[Luiz Capitulino]

On Thu, 30 May 2013 17:08:01 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Previously it would search for the first CPU with paging enabled and
> retrieve memory mappings from this and any following CPUs or return an
> error if that fails.
> 
> Instead walk all CPUs and if paging is enabled retrieve the memory
> mappings. Fail only if retrieving memory mappings on a CPU with paging
> enabled fails.
> 
> This not only allows to more easily use one qemu_for_each_cpu() instead
> of open-coding two CPU loops and drops find_first_paging_enabled_cpu()
> but removes the implicit assumption of heterogeneity between CPUs n..N
> in having paging enabled.
> 
> Cc: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  memory_mapping.c | 42 +++++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 481530a..55ac2b8 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -165,35 +165,39 @@ void memory_mapping_list_init(MemoryMappingList *list)
>      QTAILQ_INIT(&list->head);
>  }
>  
> -static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
> +typedef struct GetGuestMemoryMappingData {
> +    MemoryMappingList *list;
> +    int ret;
> +} GetGuestMemoryMappingData;
> +
> +static void qemu_get_one_guest_memory_mapping(CPUState *cpu, void *data)
>  {
> -    CPUArchState *env;
> +    GetGuestMemoryMappingData *s = data;
> +    int ret;
>  
> -    for (env = start_cpu; env != NULL; env = env->next_cpu) {
> -        if (cpu_paging_enabled(ENV_GET_CPU(env))) {
> -            return env;
> -        }
> +    if (!cpu_paging_enabled(cpu) || s->ret == -1) {
> +        return;
> +    }
> +    ret = cpu_get_memory_mapping(cpu, s->list);
> +    if (ret < 0) {
> +        s->ret = -1;
> +    } else {
> +        s->ret = 0;
>      }

Does cpu_get_memory_mapping() ever return a positive value or a negative
value != -1 ?

It it doesn't, I'd do:

s->ret = cpu_get_memory_mapping();
assert(s->ret == 0 || s->ret == -1);

> -
> -    return NULL;
>  }
>  
>  int qemu_get_guest_memory_mapping(MemoryMappingList *list)
>  {
> -    CPUArchState *env, *first_paging_enabled_cpu;
> +    GetGuestMemoryMappingData s = {
> +        .list = list,
> +        .ret = -2,
> +    };
>      RAMBlock *block;
>      ram_addr_t offset, length;
> -    int ret;
>  
> -    first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
> -    if (first_paging_enabled_cpu) {
> -        for (env = first_paging_enabled_cpu; env != NULL; env = env->next_cpu) {
> -            ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
> -            if (ret < 0) {
> -                return -1;
> -            }
> -        }
> -        return 0;
> +    qemu_for_each_cpu(qemu_get_one_guest_memory_mapping, &s);
> +    if (s.ret != -2) {
> +        return s.ret;
>      }

I see we ignore error in dump_init(). Doesn't matter today because
x86_cpu_get_memory_mapping() never fails. But as you're doing this cleanup,
shouldn't you add proper error handling?

>  
>      /*

Re: [Qemu-devel] [PATCH qom-cpu v3 0/9] dump: Build cleanups redone

[Luiz Capitulino]

On Thu, 30 May 2013 17:07:52 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Hello,
> 
> This series is an alternative to patches previously queued or posted,
> based on virgin master.
> 
> As requested by Paolo, this replaces Kate's previous memory_mapping split
> and my follow-ups and instead goes directly for moving things to CPUState.
> 
> All knowledge about dump / memory mapping are moved away from configure.
> 
> v3 adds an additional patch proposing a semantic change to facilitate CPU
> handling and it prepends another bugfix to avoid merge conflicts.

I had some minor comments, but series looks good to me.

Also, Andreas, I'm going to cherry-pick mine and Qiao's fixes into QMP
tree so that they're included in my today's pull request.

> 
> Regards,
> Andreas
> 
> v2 -> v3:
> * Incorporate Luiz' x86 bugfix.
> * Append a patch to resolve the open-coded CPU loops.
> * Massage CONFIG_HAVE_* commit messages (they were previously reordered).
> 
> v1 -> v2:
> * Dropped Kate's memory_mapping split
> * Dropped target_ulong cleanup and replaced with typedef
> * Amended CPUArchState cleanups with introducing hooks in CPUClass
> * Drop memory_memory stubs instead of moving them
> 
> Cc: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> Cc: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Cc: Vincent Rabin <rabin@rab.in>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> 
> Andreas Färber (7):
>   dump: Move stubs into libqemustub.a
>   cpu: Turn cpu_paging_enabled() into a CPUState hook
>   memory_mapping: Move MemoryMappingList typedef to qemu/typedefs.h
>   cpu: Turn cpu_get_memory_mapping() into a CPUState hook
>   memory_mapping: Drop qemu_get_memory_mapping() stub
>   dump: Unconditionally compile
>   memory_mapping: Change qemu_get_guest_memory_mapping() semantics
> 
> Luiz Capitulino (1):
>   target-i386: walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE
>     addresses
> 
> Qiao Nuohan (1):
>   target-i386: Fix mask of pte index in memory mapping
> 
>  Makefile.target                   |  8 ++------
>  configure                         |  8 --------
>  hmp-commands.hx                   |  2 --
>  include/qemu/typedefs.h           |  2 ++
>  include/qom/cpu.h                 | 21 ++++++++++++++++++++
>  include/sysemu/memory_mapping.h   |  8 +++-----
>  memory_mapping-stub.c             | 33 ------------------------------
>  memory_mapping.c                  | 42 +++++++++++++++++++++------------------
>  qom/cpu.c                         | 27 +++++++++++++++++++++++++
>  stubs/Makefile.objs               |  1 +
>  dump-stub.c => stubs/dump.c       |  8 --------
>  target-i386/arch_memory_mapping.c | 23 +++++++++++----------
>  target-i386/cpu-qom.h             |  2 ++
>  target-i386/cpu.c                 | 12 +++++++++--
>  14 files changed, 103 insertions(+), 94 deletions(-)
>  delete mode 100644 memory_mapping-stub.c
>  rename dump-stub.c => stubs/dump.c (65%)
> 

Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook

[Andreas Färber]

Am 31.05.2013 15:33, schrieb Luiz Capitulino:
> On Thu, 30 May 2013 17:07:56 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Nitpick alarm on.

Very welcome :)

>> ---
>>  include/qom/cpu.h                 | 10 ++++++++++
>>  include/sysemu/memory_mapping.h   |  1 -
>>  memory_mapping-stub.c             |  6 ------
>>  memory_mapping.c                  |  2 +-
>>  qom/cpu.c                         | 13 +++++++++++++
>>  target-i386/arch_memory_mapping.c |  6 +-----
>>  target-i386/cpu.c                 | 11 +++++++++--
>>  7 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 7cd9442..cf5fec2 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -48,6 +48,7 @@ typedef struct CPUState CPUState;
>>   * @reset: Callback to reset the #CPUState to its initial state.
>>   * @do_interrupt: Callback for interrupt handling.
>>   * @get_arch_id: Callback for getting architecture-dependent CPU ID.
>> + * @get_paging_enabled: Callback for inquiring whether paging is enabled.
>>   * @vmsd: State description for migration.
>>   *
>>   * Represents a CPU family or model.
>> @@ -62,6 +63,7 @@ typedef struct CPUClass {
>>      void (*reset)(CPUState *cpu);
>>      void (*do_interrupt)(CPUState *cpu);
>>      int64_t (*get_arch_id)(CPUState *cpu);
>> +    bool (*get_paging_enabled)(CPUState *cpu);
> 
> Argument could be const?

I haven't seen any other such example in QOM, but don't see why not,
changed [1].

[...]
>> diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
>> index 24d5d67..6c0dfeb 100644
>> --- a/memory_mapping-stub.c
>> +++ b/memory_mapping-stub.c
>> @@ -25,9 +25,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
>>  {
>>      return -1;
>>  }
>> -
>> -bool cpu_paging_enabled(CPUArchState *env)
>> -{
>> -    return true;
>> -}
>> -
[...]
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 04aefbb..ea7e676 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -50,6 +50,18 @@ bool cpu_exists(int64_t id)
>>      return data.found;
>>  }
>>  
>> +bool cpu_paging_enabled(CPUState *cpu)
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> +
>> +    return cc->get_paging_enabled(cpu);
>> +}
>> +
>> +static bool cpu_common_get_paging_enabled(CPUState *cpu)
>> +{
>> +    return true;
>> +}
> 
> Not sure if this is important, but I wonder if we want to do this
> 
> I mean, for all cases where you want to know if paging is enabled, what
> will happen if this default method says "yes, it's enabled" but it
> actually isn't?

As you can see, this is a direct conversation of today's stub into a
CPUClass callback. If we want to change the default, which I believe I
have advocated elsewhere, we should do so in a follow-up patch.

[...]
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 1a501d9..7364e3b 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
[...]
>> @@ -2519,6 +2526,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>>      cc->reset = x86_cpu_reset;
>>  
>>      cc->do_interrupt = x86_cpu_do_interrupt;
>> +    cc->get_arch_id = x86_cpu_get_arch_id;
> 
> Unrelated change?
> 
>> +    cc->get_paging_enabled = x86_cpu_get_paging_enabled;
>>  #ifndef CONFIG_USER_ONLY
>>      cc->write_elf64_note = x86_cpu_write_elf64_note;
>>      cc->write_elf64_qemunote = x86_cpu_write_elf64_qemunote;
>> @@ -2526,8 +2535,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>>      cc->write_elf32_qemunote = x86_cpu_write_elf32_qemunote;
>>  #endif
>>      cpu_class_set_vmsd(cc, &vmstate_x86_cpu);
>> -
>> -    cc->get_arch_id = x86_cpu_get_arch_id;

As maintainer of target-i386/cpu.c I took the liberty of grouping the
get_* callbacks together - there is no reason to separate this one out,
and one of the following patches adds a get_memory_mapping field that
needs to be assigned  inside !CONFIG_USER_ONLY, thus get_paging_enabled
before the #ifndef.
And I think moving one line in its own patch would be overkill, even by
my standards. ;) But I should mention it in the commit message then.

Andreas

>>  }
>>  
>>  static const TypeInfo x86_cpu_type_info = {

[1] Diff:

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index cf5fec2..1f70240 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -63,7 +63,7 @@ typedef struct CPUClass {
     void (*reset)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
     int64_t (*get_arch_id)(CPUState *cpu);
-    bool (*get_paging_enabled)(CPUState *cpu);
+    bool (*get_paging_enabled)(const CPUState *cpu);

     const struct VMStateDescription *vmsd;
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
@@ -145,7 +145,7 @@ struct CPUState {
  *
  * Returns: %true if paging is enabled, %false otherwise.
  */
-bool cpu_paging_enabled(CPUState *cpu);
+bool cpu_paging_enabled(const CPUState *cpu);

 /**
  * cpu_write_elf64_note:
diff --git a/qom/cpu.c b/qom/cpu.c
index ea7e676..9f6da0f 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -50,14 +50,14 @@ bool cpu_exists(int64_t id)
     return data.found;
 }

-bool cpu_paging_enabled(CPUState *cpu)
+bool cpu_paging_enabled(const CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);

     return cc->get_paging_enabled(cpu);
 }

-static bool cpu_common_get_paging_enabled(CPUState *cpu)
+static bool cpu_common_get_paging_enabled(const CPUState *cpu)
 {
     return true;
 }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c717ce7..f6fa7fa 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2505,7 +2505,7 @@ static int64_t x86_cpu_get_arch_id(CPUState *cs)
     return env->cpuid_apic_id;
 }

-static bool x86_cpu_get_paging_enabled(CPUState *cs)
+static bool x86_cpu_get_paging_enabled(const CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu v3 6/9] cpu: Turn cpu_get_memory_mapping() into a CPUState hook

[Andreas Färber]

Am 31.05.2013 15:48, schrieb Luiz Capitulino:
> On Thu, 30 May 2013 17:07:58 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  include/qom/cpu.h                 | 11 +++++++++++
>>  include/sysemu/memory_mapping.h   |  2 --
>>  memory_mapping-stub.c             |  6 ------
>>  memory_mapping.c                  |  2 +-
>>  qom/cpu.c                         | 14 ++++++++++++++
>>  target-i386/arch_memory_mapping.c |  7 +++++--
>>  target-i386/cpu-qom.h             |  2 ++
>>  target-i386/cpu.c                 |  1 +
>>  8 files changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index cf5fec2..93a4612 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -23,6 +23,7 @@
>>  #include <signal.h>
>>  #include "hw/qdev-core.h"
>>  #include "qemu/thread.h"
>> +#include "qemu/typedefs.h"
>>  
>>  typedef int (*WriteCoreDumpFunction)(void *buf, size_t size, void *opaque);
>>  
>> @@ -49,6 +50,7 @@ typedef struct CPUState CPUState;
>>   * @do_interrupt: Callback for interrupt handling.
>>   * @get_arch_id: Callback for getting architecture-dependent CPU ID.
>>   * @get_paging_enabled: Callback for inquiring whether paging is enabled.
>> + * @get_memory_mapping: Callback for obtaining the memory mappings.
>>   * @vmsd: State description for migration.
>>   *
>>   * Represents a CPU family or model.
>> @@ -64,6 +66,7 @@ typedef struct CPUClass {
>>      void (*do_interrupt)(CPUState *cpu);
>>      int64_t (*get_arch_id)(CPUState *cpu);
>>      bool (*get_paging_enabled)(CPUState *cpu);
>> +    int (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list);
> 
> Would be nice to take an Error argument and fill it properly when
> get_memory_mapping() is not implemented.

Done.

Andreas

diff --cc include/qom/cpu.h
index 1f70240,93a4612..0000000
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@@ -63,7 -65,8 +65,9 @@@ typedef struct CPUClass
      void (*reset)(CPUState *cpu);
      void (*do_interrupt)(CPUState *cpu);
      int64_t (*get_arch_id)(CPUState *cpu);
 -    bool (*get_paging_enabled)(CPUState *cpu);
 -    int (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list);
 +    bool (*get_paging_enabled)(const CPUState *cpu);
++    void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
++                               Error **errp);

      const struct VMStateDescription *vmsd;
      int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
@@@ -145,9 -148,17 +149,19 @@@ struct CPUState
   *
   * Returns: %true if paging is enabled, %false otherwise.
   */
 -bool cpu_paging_enabled(CPUState *cpu);
 +bool cpu_paging_enabled(const CPUState *cpu);

  /**
+  * @cpu: The CPU whose memory mappings are to be obtained.
+  * @list: Where to write the memory mappings to.
++ * @errp: Pointer for reporting an #Error.
+  *
+  * Returns: 0 if successful.
+  */
 -int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
++void cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
++                            Error **errp);
+
+ /**
   * cpu_write_elf64_note:
   * @f: pointer to a function that writes memory to a file
   * @cpu: The CPU whose memory is to be dumped
diff --git a/memory_mapping.c b/memory_mapping.c
index 481530a..9bd24ce 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -183,13 +183,14 @@ int
qemu_get_guest_memory_mapping(MemoryMappingList *list)
     CPUArchState *env, *first_paging_enabled_cpu;
     RAMBlock *block;
     ram_addr_t offset, length;
-    int ret;

     first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
     if (first_paging_enabled_cpu) {
         for (env = first_paging_enabled_cpu; env != NULL; env =
env->next_cpu) {
-            ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
-            if (ret < 0) {
+            Error *err = NULL;
+            cpu_get_memory_mapping(ENV_GET_CPU(env), list, &err);
+            if (err) {
+                error_free(err);
                 return -1;
             }
         }
diff --git a/qom/cpu.c b/qom/cpu.c
index 97063e1..b25fbc9 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -62,17 +62,19 @@ static bool cpu_common_get_paging_enabled(const
CPUState *cpu)
     return true;
 }

-int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list)
+void cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
+                            Error **errp)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);

-    return cc->get_memory_mapping(cpu, list);
+    return cc->get_memory_mapping(cpu, list, errp);
 }

-static int cpu_common_get_memory_mapping(CPUState *cpu,
-                                         MemoryMappingList *list)
+static void cpu_common_get_memory_mapping(CPUState *cpu,
+                                          MemoryMappingList *list,
+                                          Error **errp)
 {
-    return -1;
+    error_setg(errp, "Obtaining memory mappings is unsupported on this
CPU.");
 }

 /* CPU hot-plug notifiers */
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index b117068..2566a04 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -239,14 +239,15 @@ static void walk_pml4e(MemoryMappingList *list,
 }
 #endif

-int x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list)
+void x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list,
+                                Error **errp)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;

     if (!cpu_paging_enabled(cs)) {
         /* paging is disabled */
-        return 0;
+        return;
     }

     if (env->cr[4] & CR4_PAE_MASK) {
@@ -272,7 +273,5 @@ int x86_cpu_get_memory_mapping(CPUState *cs,
MemoryMappingList *list)
         pse = !!(env->cr[4] & CR4_PSE_MASK);
         walk_pde2(list, pde_addr, env->a20_mask, pse);
     }
-
-    return 0;
 }

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 11a4b10..e0ac072 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -98,6 +98,7 @@ int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction
f, CPUState *cpu,
 int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
                                  void *opaque);

-int x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
+void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
+                                Error **errp);

 #endif

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu v3 9/9] memory_mapping: Change qemu_get_guest_memory_mapping() semantics

[Andreas Färber]

Am 31.05.2013 16:14, schrieb Luiz Capitulino:
> On Thu, 30 May 2013 17:08:01 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Previously it would search for the first CPU with paging enabled and
>> retrieve memory mappings from this and any following CPUs or return an
>> error if that fails.
>>
>> Instead walk all CPUs and if paging is enabled retrieve the memory
>> mappings. Fail only if retrieving memory mappings on a CPU with paging
>> enabled fails.
>>
>> This not only allows to more easily use one qemu_for_each_cpu() instead
>> of open-coding two CPU loops and drops find_first_paging_enabled_cpu()
>> but removes the implicit assumption of heterogeneity between CPUs n..N
>> in having paging enabled.
>>
>> Cc: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  memory_mapping.c | 42 +++++++++++++++++++++++-------------------
>>  1 file changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/memory_mapping.c b/memory_mapping.c
>> index 481530a..55ac2b8 100644
>> --- a/memory_mapping.c
>> +++ b/memory_mapping.c
>> @@ -165,35 +165,39 @@ void memory_mapping_list_init(MemoryMappingList *list)
>>      QTAILQ_INIT(&list->head);
>>  }
>>  
>> -static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
>> +typedef struct GetGuestMemoryMappingData {
>> +    MemoryMappingList *list;
>> +    int ret;
>> +} GetGuestMemoryMappingData;
>> +
>> +static void qemu_get_one_guest_memory_mapping(CPUState *cpu, void *data)
>>  {
>> -    CPUArchState *env;
>> +    GetGuestMemoryMappingData *s = data;
>> +    int ret;
>>  
>> -    for (env = start_cpu; env != NULL; env = env->next_cpu) {
>> -        if (cpu_paging_enabled(ENV_GET_CPU(env))) {
>> -            return env;
>> -        }
>> +    if (!cpu_paging_enabled(cpu) || s->ret == -1) {
>> +        return;
>> +    }
>> +    ret = cpu_get_memory_mapping(cpu, s->list);
>> +    if (ret < 0) {
>> +        s->ret = -1;
>> +    } else {
>> +        s->ret = 0;
>>      }
> 
> Does cpu_get_memory_mapping() ever return a positive value or a negative
> value != -1 ?
> 
> It it doesn't, I'd do:
> 
> s->ret = cpu_get_memory_mapping();
> assert(s->ret == 0 || s->ret == -1);

This no longer applies after returning an Error* from
cpu_get_memory_mapping() instead. :)

> 
>> -
>> -    return NULL;
>>  }
>>  
>>  int qemu_get_guest_memory_mapping(MemoryMappingList *list)
>>  {
>> -    CPUArchState *env, *first_paging_enabled_cpu;
>> +    GetGuestMemoryMappingData s = {
>> +        .list = list,
>> +        .ret = -2,
>> +    };
>>      RAMBlock *block;
>>      ram_addr_t offset, length;
>> -    int ret;
>>  
>> -    first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
>> -    if (first_paging_enabled_cpu) {
>> -        for (env = first_paging_enabled_cpu; env != NULL; env = env->next_cpu) {
>> -            ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
>> -            if (ret < 0) {
>> -                return -1;
>> -            }
>> -        }
>> -        return 0;
>> +    qemu_for_each_cpu(qemu_get_one_guest_memory_mapping, &s);
>> +    if (s.ret != -2) {
>> +        return s.ret;
>>      }
> 
> I see we ignore error in dump_init(). Doesn't matter today because
> x86_cpu_get_memory_mapping() never fails. But as you're doing this cleanup,
> shouldn't you add proper error handling?

This patch is not needed for arm/s390x, so we can easily postpone it. I
included it here for early feedback since my series building on this
still needs to be tested and tweaked for bsd-user.

I figure passing through Error** would be the logical follow-up here?
Yet s.ret is being reused to check if any CPU provided any mappings at
all - we could instead do two loops, one for checking if any CPU has
paging enabled and then one passing out any Error* and propagating that.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg