[Qemu-devel] [PATCH v2 0/6] target-arm: enable qmp-dump-guest-memory

[Qemu-devel] [PATCH v2 0/6] target-arm: enable qmp-dump-guest-memory

[Andrew Jones]

This series brings qmp-dump-guest-memory to arm and aarch64
targets. I've detailed my testing and the results in the
following table.

v2: changes thanks to Peter's review questions
 - Threw in the FP registers. Can view them with gdb on elf
   formatted dumps.
 - Added comments stating where the register structs come from.
 - Fixed determination of 32 vs. 64-bit and LE vs. BE formats.
   Added a new config to the test matrix below to test this fix.
 - Added a couple R-b's


arm/aarch64 kvm guest kdump testing (P - PASS, F - FAIL). Testing done
with a latest mainline crash utility (all new patches needed are now in
master).

.-------------------------------------------------------------------------------.
|				Host	| arm32	| arm64	| arm64 | arm64	| arm64	|
|---------------------------------------|-------|-------|-------|-------|-------|
|				Guest	| arm32	| arm64	| arm64 | arm64	| arm32	|
|---------------------------------------|-------|-------|-------|-------|-------|
|			     User binary| arm32	| arm64	| arm32 | arm64	| arm32	|
|---------------------------------------|-------|-------|-------|-------|-------|
|				Pagesize| 4K	| 4K	| 4K	| 64K	| 4K	|
|===============================================================================|
| kdump in guest			| F[1]	| P[2]	| P[2]	| P[3]	| F[1]	|
|---------------------------------------|-------|-------|-------|-------|-------|
| qmp-dump-guest-memory <filename>[4]	| P	| P	| P	| P	| P	|
|---------------------------------------|-------|-------|-------|-------|-------|
| qmp-dump-guest-memory -z <filename>[5]| F[8]	| P	| P	| P	| F[8]	|
|---------------------------------------|-------|-------|-------|-------|-------|
| qmp-dump-guest-memory -l <filename>[6]| F[8]	| P	| P	| P	| F[8]	|
|---------------------------------------|-------|-------|-------|-------|-------|
| qmp-dump-guest-memory -s <filename>[7]| F[8]	| P	| P	| P	| F[8]	|
.-------------------------------------------------------------------------------.

[1] Kernel v4.4-rc1 crashes with a NULL pointer dereference at virtual
    address 00000000 in a memcpy (crash_kexec/machine_kexec/fncpy/memcpy).
    Needs kernel debugging.
[2] Not sure about mainline, but works with the RHEL kernel,
    makedumpfile does not yet support arm64 with 4K pages, but using
    'core_collector cp' in /etc/kdump.conf allows saving an uncompressed
    elf file.
[3] Not sure about mainline, but works with the RHEL kernel,
    uses makedumpfile, thus generates a makedumpfile formatted file
    using zlib compression.
[4] No format specified, creates an uncompressed elf formatted file.
[5] makedumpfile format, with zlib compression
[6] makedumpfile format, with lzo compression
[7] makedumpfile format, with snappy compression
[8] The crash utility doesn't seem to like arm32 dumps in makedumpfile
    format. Looks like the physical page bitmap is all zeros? Needs
    qemu and crash debugging.

Additional notes:
1) QEMU also has scripts/dump-guest-memory.py, which can and should be
   updated to support multiple architectures, pagesizes, and physbases.
   This is currently left as future work.


Andrew Jones (6):
  qapi-schema: dump-guest-memory: Improve text
  dump: qemunotes aren't commonly needed
  dump: allow target to set the page size
  dump: allow target to set the physical base
  target-arm: support QMP dump-guest-memory
  target-arm: dump-guest-memory: add fpregset notes

 dump.c                      | 131 ++++++++++-------
 include/sysemu/dump-arch.h  |   9 +-
 include/sysemu/dump.h       |  11 +-
 qapi-schema.json            |   4 +-
 qom/cpu.c                   |   4 +-
 target-arm/Makefile.objs    |   3 +-
 target-arm/arch_dump.c      | 347 ++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu-qom.h        |   5 +
 target-arm/cpu.c            |   3 +
 target-ppc/arch_dump.c      |   6 -
 target-ppc/cpu-qom.h        |   2 -
 target-ppc/translate_init.c |   1 -
 target-s390x/arch_dump.c    |   6 -
 target-s390x/cpu-qom.h      |   2 -
 target-s390x/cpu.c          |   1 -
 15 files changed, 448 insertions(+), 87 deletions(-)
 create mode 100644 target-arm/arch_dump.c

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/6] qapi-schema: dump-guest-memory: Improve text

[Andrew Jones]

dump-guest-memory is supported by more than just x86, however
the paging option is not.

(No functional change.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi-schema.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423fa7973..ebbad01c40948 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2089,8 +2089,7 @@
 # @dump-guest-memory
 #
 # Dump guest's memory to vmcore. It is a synchronous operation that can take
-# very long depending on the amount of guest memory. This command is only
-# supported on i386 and x86_64.
+# very long depending on the amount of guest memory.
 #
 # @paging: if true, do paging to get guest's memory mapping. This allows
 #          using gdb to process the core file.
@@ -2106,6 +2105,7 @@
 #             2. The guest can be in real-mode even if paging is enabled. For
 #                example, the guest uses ACPI to sleep, and ACPI sleep state
 #                goes in real-mode
+#             3. Currently only supported on i386 and x86_64.
 #
 # @protocol: the filename or file descriptor of the vmcore. The supported
 #            protocols are:
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/6] dump: qemunotes aren't commonly needed

[Andrew Jones]

Only one of three architectures implementing qmp-dump-guest-memory write
qemu notes. And, another architecture (arm/aarch64) is coming, which
won't use them either. Make the common implementation truly common.

(No functional change.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 qom/cpu.c                   | 4 ++--
 target-ppc/arch_dump.c      | 6 ------
 target-ppc/cpu-qom.h        | 2 --
 target-ppc/translate_init.c | 1 -
 target-s390x/arch_dump.c    | 6 ------
 target-s390x/cpu-qom.h      | 2 --
 target-s390x/cpu.c          | 1 -
 7 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/qom/cpu.c b/qom/cpu.c
index fb80d13a3f491..8f537a470f3c4 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -130,7 +130,7 @@ int cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
 static int cpu_common_write_elf32_qemunote(WriteCoreDumpFunction f,
                                            CPUState *cpu, void *opaque)
 {
-    return -1;
+    return 0;
 }
 
 int cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cpu,
@@ -159,7 +159,7 @@ int cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
 static int cpu_common_write_elf64_qemunote(WriteCoreDumpFunction f,
                                            CPUState *cpu, void *opaque)
 {
-    return -1;
+    return 0;
 }
 
 int cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cpu,
diff --git a/target-ppc/arch_dump.c b/target-ppc/arch_dump.c
index 5acafc68a4e39..816dbc21340d9 100644
--- a/target-ppc/arch_dump.c
+++ b/target-ppc/arch_dump.c
@@ -278,9 +278,3 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     return ppc64_write_all_elf64_notes("CORE", f, cpu, cpuid, opaque);
 }
-
-int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
-                                   CPUState *cpu, void *opaque)
-{
-    return 0;
-}
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index bc20504b3d55a..7d5e2b36a997c 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -125,8 +125,6 @@ int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int ppc_cpu_gdb_read_register_apple(CPUState *cpu, uint8_t *buf, int reg);
 int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
-int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
-                                   CPUState *cpu, void *opaque);
 int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                                int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index e88dc7fc7aa38..4ab2d927b01bb 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9712,7 +9712,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->vmsd = &vmstate_ppc_cpu;
 #if defined(TARGET_PPC64)
     cc->write_elf64_note = ppc64_cpu_write_elf64_note;
-    cc->write_elf64_qemunote = ppc64_cpu_write_elf64_qemunote;
 #endif
 #endif
     cc->cpu_exec_enter = ppc_cpu_exec_enter;
diff --git a/target-s390x/arch_dump.c b/target-s390x/arch_dump.c
index dab63eb44f270..549a99b357e3f 100644
--- a/target-s390x/arch_dump.c
+++ b/target-s390x/arch_dump.c
@@ -246,9 +246,3 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
 
     return (elf_note_size) * nr_cpus;
 }
-
-int s390_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
-                                  CPUState *cpu, void *opaque)
-{
-    return 0;
-}
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 491c1b8769669..029a44af246cc 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -91,8 +91,6 @@ void s390_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
 int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                               int cpuid, void *opaque);
 
-int s390_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
-                                  CPUState *cpu, void *opaque);
 hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
 int s390_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 189a2afc0fe1f..e5a3f65029075 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -353,7 +353,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     cc->get_phys_page_debug = s390_cpu_get_phys_page_debug;
     cc->vmsd = &vmstate_s390_cpu;
     cc->write_elf64_note = s390_cpu_write_elf64_note;
-    cc->write_elf64_qemunote = s390_cpu_write_elf64_qemunote;
     cc->cpu_exec_interrupt = s390_cpu_exec_interrupt;
     cc->debug_excp_handler = s390x_cpu_debug_excp_handler;
 #endif
-- 
2.4.3

[Qemu-devel] [PATCH v2 3/6] dump: allow target to set the page size

[Andrew Jones]

This is necessary for targets that don't have TARGET_PAGE_SIZE ==
real-target-page-size. The target should set the page size to the
correct one, if known, or, if not known, to the maximum page size
it supports.

(No functional change.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 dump.c                     | 127 ++++++++++++++++++++++++++++-----------------
 include/sysemu/dump-arch.h |   8 +--
 include/sysemu/dump.h      |  10 +---
 3 files changed, 85 insertions(+), 60 deletions(-)

diff --git a/dump.c b/dump.c
index 78b7d843ceb29..e1d9bae9e89d1 100644
--- a/dump.c
+++ b/dump.c
@@ -347,18 +347,18 @@ static void write_memory(DumpState *s, GuestPhysBlock *block, ram_addr_t start,
     int64_t i;
     Error *local_err = NULL;
 
-    for (i = 0; i < size / TARGET_PAGE_SIZE; i++) {
-        write_data(s, block->host_addr + start + i * TARGET_PAGE_SIZE,
-                   TARGET_PAGE_SIZE, &local_err);
+    for (i = 0; i < size / s->dump_info.page_size; i++) {
+        write_data(s, block->host_addr + start + i * s->dump_info.page_size,
+                   s->dump_info.page_size, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
         }
     }
 
-    if ((size % TARGET_PAGE_SIZE) != 0) {
-        write_data(s, block->host_addr + start + i * TARGET_PAGE_SIZE,
-                   size % TARGET_PAGE_SIZE, &local_err);
+    if ((size % s->dump_info.page_size) != 0) {
+        write_data(s, block->host_addr + start + i * s->dump_info.page_size,
+                   size % s->dump_info.page_size, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
@@ -737,7 +737,7 @@ static void create_header32(DumpState *s, Error **errp)
 
     strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
     dh->header_version = cpu_to_dump32(s, 6);
-    block_size = TARGET_PAGE_SIZE;
+    block_size = s->dump_info.page_size;
     dh->block_size = cpu_to_dump32(s, block_size);
     sub_hdr_size = sizeof(struct KdumpSubHeader32) + s->note_size;
     sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size);
@@ -837,7 +837,7 @@ static void create_header64(DumpState *s, Error **errp)
 
     strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
     dh->header_version = cpu_to_dump32(s, 6);
-    block_size = TARGET_PAGE_SIZE;
+    block_size = s->dump_info.page_size;
     dh->block_size = cpu_to_dump32(s, block_size);
     sub_hdr_size = sizeof(struct KdumpSubHeader64) + s->note_size;
     sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size);
@@ -933,6 +933,11 @@ static void write_dump_header(DumpState *s, Error **errp)
     }
 }
 
+static size_t dump_bitmap_get_bufsize(DumpState *s)
+{
+    return s->dump_info.page_size;
+}
+
 /*
  * set dump_bitmap sequencely. the bit before last_pfn is not allowed to be
  * rewritten, so if need to set the first bit, set last_pfn and pfn to 0.
@@ -946,6 +951,8 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
     off_t old_offset, new_offset;
     off_t offset_bitmap1, offset_bitmap2;
     uint32_t byte, bit;
+    size_t bitmap_bufsize = dump_bitmap_get_bufsize(s);
+    size_t bits_per_buf = bitmap_bufsize * CHAR_BIT;
 
     /* should not set the previous place */
     assert(last_pfn <= pfn);
@@ -956,14 +963,14 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
      * making new_offset be bigger than old_offset can also sync remained data
      * into vmcore.
      */
-    old_offset = BUFSIZE_BITMAP * (last_pfn / PFN_BUFBITMAP);
-    new_offset = BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP);
+    old_offset = bitmap_bufsize * (last_pfn / bits_per_buf);
+    new_offset = bitmap_bufsize * (pfn / bits_per_buf);
 
     while (old_offset < new_offset) {
         /* calculate the offset and write dump_bitmap */
         offset_bitmap1 = s->offset_dump_bitmap + old_offset;
         if (write_buffer(s->fd, offset_bitmap1, buf,
-                         BUFSIZE_BITMAP) < 0) {
+                         bitmap_bufsize) < 0) {
             return -1;
         }
 
@@ -971,17 +978,17 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
         offset_bitmap2 = s->offset_dump_bitmap + s->len_dump_bitmap +
                          old_offset;
         if (write_buffer(s->fd, offset_bitmap2, buf,
-                         BUFSIZE_BITMAP) < 0) {
+                         bitmap_bufsize) < 0) {
             return -1;
         }
 
-        memset(buf, 0, BUFSIZE_BITMAP);
-        old_offset += BUFSIZE_BITMAP;
+        memset(buf, 0, bitmap_bufsize);
+        old_offset += bitmap_bufsize;
     }
 
     /* get the exact place of the bit in the buf, and set it */
-    byte = (pfn % PFN_BUFBITMAP) / CHAR_BIT;
-    bit = (pfn % PFN_BUFBITMAP) % CHAR_BIT;
+    byte = (pfn % bits_per_buf) / CHAR_BIT;
+    bit = (pfn % bits_per_buf) % CHAR_BIT;
     if (value) {
         buf[byte] |= 1u << bit;
     } else {
@@ -991,6 +998,20 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
     return 0;
 }
 
+static uint64_t dump_paddr_to_pfn(DumpState *s, uint64_t addr)
+{
+    int target_page_shift = ctz32(s->dump_info.page_size);
+
+    return (addr >> target_page_shift) - ARCH_PFN_OFFSET;
+}
+
+static uint64_t dump_pfn_to_paddr(DumpState *s, uint64_t pfn)
+{
+    int target_page_shift = ctz32(s->dump_info.page_size);
+
+    return (pfn + ARCH_PFN_OFFSET) << target_page_shift;
+}
+
 /*
  * exam every page and return the page frame number and the address of the page.
  * bufptr can be NULL. note: the blocks here is supposed to reflect guest-phys
@@ -1001,16 +1022,16 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
                           uint8_t **bufptr, DumpState *s)
 {
     GuestPhysBlock *block = *blockptr;
-    hwaddr addr;
+    hwaddr addr, target_page_mask = ~((hwaddr)s->dump_info.page_size - 1);
     uint8_t *buf;
 
     /* block == NULL means the start of the iteration */
     if (!block) {
         block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         *blockptr = block;
-        assert((block->target_start & ~TARGET_PAGE_MASK) == 0);
-        assert((block->target_end & ~TARGET_PAGE_MASK) == 0);
-        *pfnptr = paddr_to_pfn(block->target_start);
+        assert((block->target_start & ~target_page_mask) == 0);
+        assert((block->target_end & ~target_page_mask) == 0);
+        *pfnptr = dump_paddr_to_pfn(s, block->target_start);
         if (bufptr) {
             *bufptr = block->host_addr;
         }
@@ -1018,10 +1039,10 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
     }
 
     *pfnptr = *pfnptr + 1;
-    addr = pfn_to_paddr(*pfnptr);
+    addr = dump_pfn_to_paddr(s, *pfnptr);
 
     if ((addr >= block->target_start) &&
-        (addr + TARGET_PAGE_SIZE <= block->target_end)) {
+        (addr + s->dump_info.page_size <= block->target_end)) {
         buf = block->host_addr + (addr - block->target_start);
     } else {
         /* the next page is in the next block */
@@ -1030,9 +1051,9 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
         if (!block) {
             return false;
         }
-        assert((block->target_start & ~TARGET_PAGE_MASK) == 0);
-        assert((block->target_end & ~TARGET_PAGE_MASK) == 0);
-        *pfnptr = paddr_to_pfn(block->target_start);
+        assert((block->target_start & ~target_page_mask) == 0);
+        assert((block->target_end & ~target_page_mask) == 0);
+        *pfnptr = dump_paddr_to_pfn(s, block->target_start);
         buf = block->host_addr;
     }
 
@@ -1050,9 +1071,11 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
     void *dump_bitmap_buf;
     size_t num_dumpable;
     GuestPhysBlock *block_iter = NULL;
+    size_t bitmap_bufsize = dump_bitmap_get_bufsize(s);
+    size_t bits_per_buf = bitmap_bufsize * CHAR_BIT;
 
     /* dump_bitmap_buf is used to store dump_bitmap temporarily */
-    dump_bitmap_buf = g_malloc0(BUFSIZE_BITMAP);
+    dump_bitmap_buf = g_malloc0(bitmap_bufsize);
 
     num_dumpable = 0;
     last_pfn = 0;
@@ -1074,11 +1097,11 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
 
     /*
      * set_dump_bitmap will always leave the recently set bit un-sync. Here we
-     * set last_pfn + PFN_BUFBITMAP to 0 and those set but un-sync bit will be
-     * synchronized into vmcore.
+     * set the remaining bits from last_pfn to the end of the bitmap buffer to
+     * 0. With those set, the un-sync bit will be synchronized into the vmcore.
      */
     if (num_dumpable > 0) {
-        ret = set_dump_bitmap(last_pfn, last_pfn + PFN_BUFBITMAP, false,
+        ret = set_dump_bitmap(last_pfn, last_pfn + bits_per_buf, false,
                               dump_bitmap_buf, s);
         if (ret < 0) {
             dump_error(s, "dump: failed to sync dump_bitmap", errp);
@@ -1098,8 +1121,8 @@ static void prepare_data_cache(DataCache *data_cache, DumpState *s,
 {
     data_cache->fd = s->fd;
     data_cache->data_size = 0;
-    data_cache->buf_size = BUFSIZE_DATA_CACHE;
-    data_cache->buf = g_malloc0(BUFSIZE_DATA_CACHE);
+    data_cache->buf_size = 4 * dump_bitmap_get_bufsize(s);
+    data_cache->buf = g_malloc0(data_cache->buf_size);
     data_cache->offset = offset;
 }
 
@@ -1193,7 +1216,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
     prepare_data_cache(&page_data, s, offset_data);
 
     /* prepare buffer to store compressed data */
-    len_buf_out = get_len_buf_out(TARGET_PAGE_SIZE, s->flag_compress);
+    len_buf_out = get_len_buf_out(s->dump_info.page_size, s->flag_compress);
     assert(len_buf_out != 0);
 
 #ifdef CONFIG_LZO
@@ -1206,19 +1229,19 @@ static void write_dump_pages(DumpState *s, Error **errp)
      * init zero page's page_desc and page_data, because every zero page
      * uses the same page_data
      */
-    pd_zero.size = cpu_to_dump32(s, TARGET_PAGE_SIZE);
+    pd_zero.size = cpu_to_dump32(s, s->dump_info.page_size);
     pd_zero.flags = cpu_to_dump32(s, 0);
     pd_zero.offset = cpu_to_dump64(s, offset_data);
     pd_zero.page_flags = cpu_to_dump64(s, 0);
-    buf = g_malloc0(TARGET_PAGE_SIZE);
-    ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
+    buf = g_malloc0(s->dump_info.page_size);
+    ret = write_cache(&page_data, buf, s->dump_info.page_size, false);
     g_free(buf);
     if (ret < 0) {
         dump_error(s, "dump: failed to write page data (zero page)", errp);
         goto out;
     }
 
-    offset_data += TARGET_PAGE_SIZE;
+    offset_data += s->dump_info.page_size;
 
     /*
      * dump memory to vmcore page by page. zero page will all be resided in the
@@ -1226,7 +1249,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
      */
     while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
         /* check zero page */
-        if (is_zero_page(buf, TARGET_PAGE_SIZE)) {
+        if (is_zero_page(buf, s->dump_info.page_size)) {
             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
                               false);
             if (ret < 0) {
@@ -1248,8 +1271,8 @@ static void write_dump_pages(DumpState *s, Error **errp)
              size_out = len_buf_out;
              if ((s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) &&
                     (compress2(buf_out, (uLongf *)&size_out, buf,
-                               TARGET_PAGE_SIZE, Z_BEST_SPEED) == Z_OK) &&
-                    (size_out < TARGET_PAGE_SIZE)) {
+                               s->dump_info.page_size, Z_BEST_SPEED) == Z_OK) &&
+                    (size_out < s->dump_info.page_size)) {
                 pd.flags = cpu_to_dump32(s, DUMP_DH_COMPRESSED_ZLIB);
                 pd.size  = cpu_to_dump32(s, size_out);
 
@@ -1260,9 +1283,9 @@ static void write_dump_pages(DumpState *s, Error **errp)
                 }
 #ifdef CONFIG_LZO
             } else if ((s->flag_compress & DUMP_DH_COMPRESSED_LZO) &&
-                    (lzo1x_1_compress(buf, TARGET_PAGE_SIZE, buf_out,
+                    (lzo1x_1_compress(buf, s->dump_info.page_size, buf_out,
                     (lzo_uint *)&size_out, wrkmem) == LZO_E_OK) &&
-                    (size_out < TARGET_PAGE_SIZE)) {
+                    (size_out < s->dump_info.page_size)) {
                 pd.flags = cpu_to_dump32(s, DUMP_DH_COMPRESSED_LZO);
                 pd.size  = cpu_to_dump32(s, size_out);
 
@@ -1274,9 +1297,9 @@ static void write_dump_pages(DumpState *s, Error **errp)
 #endif
 #ifdef CONFIG_SNAPPY
             } else if ((s->flag_compress & DUMP_DH_COMPRESSED_SNAPPY) &&
-                    (snappy_compress((char *)buf, TARGET_PAGE_SIZE,
+                    (snappy_compress((char *)buf, s->dump_info.page_size,
                     (char *)buf_out, &size_out) == SNAPPY_OK) &&
-                    (size_out < TARGET_PAGE_SIZE)) {
+                    (size_out < s->dump_info.page_size)) {
                 pd.flags = cpu_to_dump32(s, DUMP_DH_COMPRESSED_SNAPPY);
                 pd.size  = cpu_to_dump32(s, size_out);
 
@@ -1289,13 +1312,14 @@ static void write_dump_pages(DumpState *s, Error **errp)
             } else {
                 /*
                  * fall back to save in plaintext, size_out should be
-                 * assigned TARGET_PAGE_SIZE
+                 * assigned the target's page size
                  */
                 pd.flags = cpu_to_dump32(s, 0);
-                size_out = TARGET_PAGE_SIZE;
+                size_out = s->dump_info.page_size;
                 pd.size = cpu_to_dump32(s, size_out);
 
-                ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
+                ret = write_cache(&page_data, buf,
+                                  s->dump_info.page_size, false);
                 if (ret < 0) {
                     dump_error(s, "dump: failed to write page data", errp);
                     goto out;
@@ -1430,7 +1454,7 @@ static void get_max_mapnr(DumpState *s)
     GuestPhysBlock *last_block;
 
     last_block = QTAILQ_LAST(&s->guest_phys_blocks.head, GuestPhysBlockHead);
-    s->max_mapnr = paddr_to_pfn(last_block->target_end);
+    s->max_mapnr = dump_paddr_to_pfn(s, last_block->target_end);
 }
 
 static void dump_init(DumpState *s, int fd, bool has_format,
@@ -1489,6 +1513,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         goto cleanup;
     }
 
+    if (!s->dump_info.page_size) {
+        s->dump_info.page_size = TARGET_PAGE_SIZE;
+    }
+
     s->note_size = cpu_get_note_size(s->dump_info.d_class,
                                      s->dump_info.d_machine, nr_cpus);
     if (s->note_size < 0) {
@@ -1512,8 +1540,9 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     get_max_mapnr(s);
 
     uint64_t tmp;
-    tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), TARGET_PAGE_SIZE);
-    s->len_dump_bitmap = tmp * TARGET_PAGE_SIZE;
+    tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT),
+                       s->dump_info.page_size);
+    s->len_dump_bitmap = tmp * s->dump_info.page_size;
 
     /* init for kdump-compressed format */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
index 9c95cede3d3b7..43358396ea81a 100644
--- a/include/sysemu/dump-arch.h
+++ b/include/sysemu/dump-arch.h
@@ -15,9 +15,11 @@
 #define DUMP_ARCH_H
 
 typedef struct ArchDumpInfo {
-    int d_machine;  /* Architecture */
-    int d_endian;   /* ELFDATA2LSB or ELFDATA2MSB */
-    int d_class;    /* ELFCLASS32 or ELFCLASS64 */
+    int d_machine;           /* Architecture */
+    int d_endian;            /* ELFDATA2LSB or ELFDATA2MSB */
+    int d_class;             /* ELFCLASS32 or ELFCLASS64 */
+    uint32_t page_size;      /* The target's page size. If it's variable and
+                              * unknown, then this should be the maximum. */
 } ArchDumpInfo;
 
 struct GuestPhysBlockList; /* memory_mapping.h */
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7e4ec5c7d96fb..16cbd8d881fd6 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -20,12 +20,9 @@
 #define VERSION_FLAT_HEADER         (1)    /* version of flattened format */
 #define END_FLAG_FLAT_HEADER        (-1)
 
+#ifndef ARCH_PFN_OFFSET
 #define ARCH_PFN_OFFSET             (0)
-
-#define paddr_to_pfn(X) \
-    (((unsigned long long)(X) >> TARGET_PAGE_BITS) - ARCH_PFN_OFFSET)
-#define pfn_to_paddr(X) \
-    (((unsigned long long)(X) + ARCH_PFN_OFFSET) << TARGET_PAGE_BITS)
+#endif
 
 /*
  * flag for compressed format
@@ -39,9 +36,6 @@
 #define PHYS_BASE                   (0)
 #define DUMP_LEVEL                  (1)
 #define DISKDUMP_HEADER_BLOCKS      (1)
-#define BUFSIZE_BITMAP              (TARGET_PAGE_SIZE)
-#define PFN_BUFBITMAP               (CHAR_BIT * BUFSIZE_BITMAP)
-#define BUFSIZE_DATA_CACHE          (TARGET_PAGE_SIZE * 4)
 
 #include "sysemu/dump-arch.h"
 #include "sysemu/memory_mapping.h"
-- 
2.4.3

[Qemu-devel] [PATCH v2 4/6] dump: allow target to set the physical base

[Andrew Jones]

crash assumes the physical base in the kdump subheader of
makedumpfile formatted dumps is correct. Zero is not correct
for all architectures, so allow it to be changed.

(No functional change.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 dump.c                     | 4 ++--
 include/sysemu/dump-arch.h | 1 +
 include/sysemu/dump.h      | 1 -
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/dump.c b/dump.c
index e1d9bae9e89d1..2d4892bec2672 100644
--- a/dump.c
+++ b/dump.c
@@ -775,7 +775,7 @@ static void create_header32(DumpState *s, Error **errp)
 
     /* 64bit max_mapnr_64 */
     kh->max_mapnr_64 = cpu_to_dump64(s, s->max_mapnr);
-    kh->phys_base = cpu_to_dump32(s, PHYS_BASE);
+    kh->phys_base = cpu_to_dump32(s, s->dump_info.phys_base);
     kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL);
 
     offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
@@ -875,7 +875,7 @@ static void create_header64(DumpState *s, Error **errp)
 
     /* 64bit max_mapnr_64 */
     kh->max_mapnr_64 = cpu_to_dump64(s, s->max_mapnr);
-    kh->phys_base = cpu_to_dump64(s, PHYS_BASE);
+    kh->phys_base = cpu_to_dump64(s, s->dump_info.phys_base);
     kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL);
 
     offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
index 43358396ea81a..e25b02e99013c 100644
--- a/include/sysemu/dump-arch.h
+++ b/include/sysemu/dump-arch.h
@@ -20,6 +20,7 @@ typedef struct ArchDumpInfo {
     int d_class;             /* ELFCLASS32 or ELFCLASS64 */
     uint32_t page_size;      /* The target's page size. If it's variable and
                               * unknown, then this should be the maximum. */
+    uint64_t phys_base;      /* The target's physmem base. */
 } ArchDumpInfo;
 
 struct GuestPhysBlockList; /* memory_mapping.h */
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 16cbd8d881fd6..2f04b247bed1b 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -33,7 +33,6 @@
 
 #define KDUMP_SIGNATURE             "KDUMP   "
 #define SIG_LEN                     (sizeof(KDUMP_SIGNATURE) - 1)
-#define PHYS_BASE                   (0)
 #define DUMP_LEVEL                  (1)
 #define DISKDUMP_HEADER_BLOCKS      (1)
 
-- 
2.4.3

[Qemu-devel] [PATCH v2 5/6] target-arm: support QMP dump-guest-memory

[Andrew Jones]

Add the support needed for creating prstatus elf notes. This
allows us to use QMP dump-guest-memory.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/Makefile.objs |   3 +-
 target-arm/arch_dump.c   | 230 +++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu-qom.h     |   5 ++
 target-arm/cpu.c         |   3 +
 4 files changed, 239 insertions(+), 2 deletions(-)
 create mode 100644 target-arm/arch_dump.c

diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
index 9460b409a5a1c..a80eb39743a78 100644
--- a/target-arm/Makefile.objs
+++ b/target-arm/Makefile.objs
@@ -1,5 +1,5 @@
 obj-y += arm-semi.o
-obj-$(CONFIG_SOFTMMU) += machine.o
+obj-$(CONFIG_SOFTMMU) += machine.o psci.o arch_dump.o
 obj-$(CONFIG_KVM) += kvm.o
 obj-$(call land,$(CONFIG_KVM),$(call lnot,$(TARGET_AARCH64))) += kvm32.o
 obj-$(call land,$(CONFIG_KVM),$(TARGET_AARCH64)) += kvm64.o
@@ -7,6 +7,5 @@ obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
 obj-y += translate.o op_helper.o helper.o cpu.o
 obj-y += neon_helper.o iwmmxt_helper.o
 obj-y += gdbstub.o
-obj-$(CONFIG_SOFTMMU) += psci.o
 obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
 obj-y += crypto_helper.o
diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
new file mode 100644
index 0000000000000..5debe549d721d
--- /dev/null
+++ b/target-arm/arch_dump.c
@@ -0,0 +1,230 @@
+/* Support for writing ELF notes for ARM architectures
+ *
+ * Copyright (C) 2015 Red Hat Inc.
+ *
+ * Author: Andrew Jones <drjones@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "cpu.h"
+#include "elf.h"
+#include "sysemu/dump.h"
+
+#define NOTE_NAME       "CORE"
+#define NOTE_NAMESZ     5
+
+/* struct user_pt_regs from arch/arm64/include/uapi/asm/ptrace.h */
+struct aarch64_user_regs {
+    uint64_t regs[31];
+    uint64_t sp;
+    uint64_t pc;
+    uint64_t pstate;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_regs) != 272);
+
+/* struct elf_prstatus from include/uapi/linux/elfcore.h */
+struct aarch64_elf_prstatus {
+    char pad1[32]; /* 32 == offsetof(struct elf_prstatus, pr_pid) */
+    uint32_t pr_pid;
+    char pad2[76]; /* 76 == offsetof(struct elf_prstatus, pr_reg) -
+                            offsetof(struct elf_prstatus, pr_ppid) */
+    struct aarch64_user_regs pr_reg;
+    int pr_fpvalid;
+    char pad3[4];
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
+
+struct aarch64_note {
+    Elf64_Nhdr hdr;
+    char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
+    struct aarch64_elf_prstatus prstatus;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
+
+static int
+aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
+                         int id, DumpState *s)
+{
+    struct aarch64_note note;
+    int ret, i;
+
+    memset(&note, 0, sizeof(note));
+
+    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
+    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
+    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
+
+    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
+    note.prstatus.pr_pid = cpu_to_dump32(s, id);
+
+    if (is_a64(env)) {
+        for (i = 0; i < 31; ++i) {
+            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
+        }
+        note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
+        note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
+        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate_read(env));
+    } else {
+        aarch64_sync_64_to_32(env);
+        for (i = 0; i < 16; ++i) {
+            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->regs[i]);
+        }
+        note.prstatus.pr_reg.sp = note.prstatus.pr_reg.regs[13];
+        note.prstatus.pr_reg.pc = note.prstatus.pr_reg.regs[15];
+        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
+    }
+
+    ret = f(&note, sizeof(note), s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
+/* struct pt_regs from arch/arm/include/asm/ptrace.h */
+struct arm_user_regs {
+    uint32_t regs[17];
+    char pad[4];
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct arm_user_regs) != 72);
+
+/* struct elf_prstatus from include/uapi/linux/elfcore.h */
+struct arm_elf_prstatus {
+    char pad1[24]; /* 24 == offsetof(struct elf_prstatus, pr_pid) */
+    uint32_t pr_pid;
+    char pad2[44]; /* 44 == offsetof(struct elf_prstatus, pr_reg) -
+                            offsetof(struct elf_prstatus, pr_ppid) */
+    struct arm_user_regs pr_reg;
+    int pr_fpvalid;
+} QEMU_PACKED arm_elf_prstatus;
+
+QEMU_BUILD_BUG_ON(sizeof(struct arm_elf_prstatus) != 148);
+
+struct arm_note {
+    Elf32_Nhdr hdr;
+    char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
+    struct arm_elf_prstatus prstatus;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct arm_note) != 168);
+
+static int
+arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
+                     int id, DumpState *s)
+{
+    struct arm_note note;
+    int ret, i;
+
+    memset(&note, 0, sizeof(note));
+
+    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
+    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
+    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
+
+    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
+    note.prstatus.pr_pid = cpu_to_dump32(s, id);
+
+    for (i = 0; i < 16; ++i) {
+        note.prstatus.pr_reg.regs[i] = cpu_to_dump32(s, env->regs[i]);
+    }
+    note.prstatus.pr_reg.regs[16] = cpu_to_dump32(s, cpsr_read(env));
+
+    ret = f(&note, sizeof(note), s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
+int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque)
+{
+    CPUARMState *env = &ARM_CPU(cs)->env;
+    int ret;
+
+    if (arm_el_is_aa64(env, 1)) {
+        ret = aarch64_write_elf64_note(f, env, cpuid, opaque);
+    } else {
+        ret = arm_write_elf32_note(f, env, cpuid, opaque);
+    }
+    return ret;
+}
+
+int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque)
+{
+    return arm_write_elf32_note(f, &ARM_CPU(cs)->env, cpuid, opaque);
+}
+
+int cpu_get_dump_info(ArchDumpInfo *info,
+                      const GuestPhysBlockList *guest_phys_blocks)
+{
+    ARMCPU *cpu = ARM_CPU(first_cpu);
+    CPUARMState *env = &cpu->env;
+    GuestPhysBlock *block;
+    hwaddr lowest_addr = ULLONG_MAX;
+
+    /* Take a best guess at the phys_base. If we get it wrong then crash
+     * will need '--machdep phys_offset=<phys-offset>' added to its command
+     * line, which isn't any worse than assuming we can use zero, but being
+     * wrong. This is the same algorithm the crash utility uses when
+     * attempting to guess as it loads non-dumpfile formatted files.
+     */
+    QTAILQ_FOREACH(block, &guest_phys_blocks->head, next) {
+        if (block->target_start < lowest_addr) {
+            lowest_addr = block->target_start;
+        }
+    }
+
+    if (arm_el_is_aa64(env, 1)) {
+        info->d_machine = EM_AARCH64;
+        info->d_class = ELFCLASS64;
+        info->page_size = (1 << 16); /* aarch64 max pagesize */
+        if (lowest_addr != ULLONG_MAX) {
+            info->phys_base = lowest_addr;
+        }
+    } else {
+        info->d_machine = EM_ARM;
+        info->d_class = ELFCLASS32;
+        info->page_size = (1 << 12);
+        if (lowest_addr < UINT_MAX) {
+            info->phys_base = lowest_addr;
+        }
+    }
+
+    info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0
+                     ? ELFDATA2MSB : ELFDATA2LSB;
+
+    return 0;
+}
+
+ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
+{
+    size_t note_size;
+
+    if (class == ELFCLASS64) {
+        note_size = sizeof(struct aarch64_note);
+    } else {
+        note_size = sizeof(struct arm_note);
+    }
+
+    return note_size * nr_cpus;
+}
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 25fb1ce0f3f3d..5bd9b7bb9fa7e 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -221,6 +221,11 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int arm_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 
+int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque);
+int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque);
+
 /* Callback functions for the generic timer's timers. */
 void arm_gt_ptimer_cb(void *opaque);
 void arm_gt_vtimer_cb(void *opaque);
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 30739fc0dfa74..db91a3f9eb467 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1428,6 +1428,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->disas_set_info = arm_disas_set_info;
 
+    cc->write_elf64_note = arm_cpu_write_elf64_note;
+    cc->write_elf32_note = arm_cpu_write_elf32_note;
+
     /*
      * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
      * the object in cpus -> dangling pointer after final
-- 
2.4.3

[Qemu-devel] [PATCH v2 6/6] target-arm: dump-guest-memory: add fpregset notes

[Andrew Jones]

Also refactors note init code to avoid code duplication.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/arch_dump.c | 161 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 139 insertions(+), 22 deletions(-)

diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
index 5debe549d721d..8d74788411d79 100644
--- a/target-arm/arch_dump.c
+++ b/target-arm/arch_dump.c
@@ -35,6 +35,16 @@ struct aarch64_user_regs {
 
 QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_regs) != 272);
 
+/* struct user_fpsimd_state from arch/arm64/include/uapi/asm/ptrace.h */
+struct aarch64_user_vfp_state {
+    uint64_t vregs[64];
+    uint32_t fpsr;
+    uint32_t fpcr;
+    char pad[8];
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_vfp_state) != 528);
+
 /* struct elf_prstatus from include/uapi/linux/elfcore.h */
 struct aarch64_elf_prstatus {
     char pad1[32]; /* 32 == offsetof(struct elf_prstatus, pr_pid) */
@@ -51,10 +61,77 @@ QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
 struct aarch64_note {
     Elf64_Nhdr hdr;
     char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
-    struct aarch64_elf_prstatus prstatus;
+    union {
+        struct aarch64_elf_prstatus prstatus;
+        struct aarch64_user_vfp_state vfp;
+    };
 } QEMU_PACKED;
 
-QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
+#define AARCH64_NOTE_HEADER_SIZE offsetof(struct aarch64_note, prstatus)
+#define AARCH64_PRSTATUS_NOTE_SIZE \
+            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_elf_prstatus))
+#define AARCH64_FPREGSET_NOTE_SIZE \
+            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_user_vfp_state))
+
+static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
+                              Elf64_Word type, Elf64_Word descsz)
+{
+    memset(note, 0, sizeof(*note));
+
+    note->hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
+    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
+    note->hdr.n_type = cpu_to_dump32(s, type);
+
+    memcpy(note->name, NOTE_NAME, NOTE_NAMESZ);
+}
+
+static void arm_write_vregs(uint64_t *vregs, int num_regs,
+                            CPUARMState *env, DumpState *s)
+{
+    int i;
+
+    for (i = 0; i < num_regs; ++i) {
+        vregs[i] = cpu_to_dump64(s, env->vfp.regs[i]);
+    }
+
+    if (s->dump_info.d_endian == ELFDATA2MSB) {
+        /* We must always swap vfp.regs's 2n and 2n+1 entries when
+         * generating BE notes, because even big endian hosts use
+         * 2n+1 for the high half.
+         */
+        for (i = 0; i < num_regs/2; ++i) {
+            uint64_t tmp = vregs[2*i];
+            vregs[2*i] = vregs[2*i+1];
+            vregs[2*i+1] = tmp;
+        }
+    }
+}
+
+static int
+aarch64_write_elf64_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
+                             int id, DumpState *s)
+{
+    struct aarch64_note note;
+    int ret;
+
+    aarch64_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
+
+    if (is_a64(env)) {
+        arm_write_vregs(note.vfp.vregs, 64, env, s);
+    } else {
+        arm_write_vregs(note.vfp.vregs, 32, env, s);
+    }
+
+    note.vfp.fpsr = cpu_to_dump32(s, vfp_get_fpsr(env));
+    note.vfp.fpcr = cpu_to_dump32(s, vfp_get_fpcr(env));
+
+    ret = f(&note, AARCH64_FPREGSET_NOTE_SIZE, s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
 
 static int
 aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
@@ -63,13 +140,8 @@ aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
     struct aarch64_note note;
     int ret, i;
 
-    memset(&note, 0, sizeof(note));
-
-    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
-    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
-    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
+    aarch64_note_init(&note, s, NT_PRSTATUS, sizeof(note.prstatus));
 
-    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
     note.prstatus.pr_pid = cpu_to_dump32(s, id);
 
     if (is_a64(env)) {
@@ -89,12 +161,12 @@ aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
         note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
     }
 
-    ret = f(&note, sizeof(note), s);
+    ret = f(&note, AARCH64_PRSTATUS_NOTE_SIZE, s);
     if (ret < 0) {
         return -1;
     }
 
-    return 0;
+    return aarch64_write_elf64_fpregset(f, env, id, s);
 }
 
 /* struct pt_regs from arch/arm/include/asm/ptrace.h */
@@ -105,6 +177,14 @@ struct arm_user_regs {
 
 QEMU_BUILD_BUG_ON(sizeof(struct arm_user_regs) != 72);
 
+/* struct user_vfp from arch/arm/include/asm/user.h */
+struct arm_user_vfp_state {
+    uint64_t vregs[32];
+    uint32_t fpscr;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct arm_user_vfp_state) != 260);
+
 /* struct elf_prstatus from include/uapi/linux/elfcore.h */
 struct arm_elf_prstatus {
     char pad1[24]; /* 24 == offsetof(struct elf_prstatus, pr_pid) */
@@ -120,10 +200,50 @@ QEMU_BUILD_BUG_ON(sizeof(struct arm_elf_prstatus) != 148);
 struct arm_note {
     Elf32_Nhdr hdr;
     char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
-    struct arm_elf_prstatus prstatus;
+    union {
+        struct arm_elf_prstatus prstatus;
+        struct arm_user_vfp_state vfp;
+    };
 } QEMU_PACKED;
 
-QEMU_BUILD_BUG_ON(sizeof(struct arm_note) != 168);
+#define ARM_NOTE_HEADER_SIZE offsetof(struct arm_note, prstatus)
+#define ARM_PRSTATUS_NOTE_SIZE \
+            (ARM_NOTE_HEADER_SIZE + sizeof(struct arm_elf_prstatus))
+#define ARM_FPREGSET_NOTE_SIZE \
+            (ARM_NOTE_HEADER_SIZE + sizeof(struct arm_user_vfp_state))
+
+static void arm_note_init(struct arm_note *note, DumpState *s,
+                          Elf32_Word type, Elf32_Word descsz)
+{
+    memset(note, 0, sizeof(*note));
+
+    note->hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
+    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
+    note->hdr.n_type = cpu_to_dump32(s, type);
+
+    memcpy(note->name, NOTE_NAME, NOTE_NAMESZ);
+}
+
+static int
+arm_write_elf32_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
+                         int id, DumpState *s)
+{
+    struct arm_note note;
+    int ret;
+
+    arm_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
+
+    arm_write_vregs(note.vfp.vregs, 32, env, s);
+
+    note.vfp.fpscr = cpu_to_dump32(s, vfp_get_fpscr(env));
+
+    ret = f(&note, ARM_FPREGSET_NOTE_SIZE, s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
 
 static int
 arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
@@ -132,13 +252,8 @@ arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
     struct arm_note note;
     int ret, i;
 
-    memset(&note, 0, sizeof(note));
-
-    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
-    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
-    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
+    arm_note_init(&note, s, NT_PRSTATUS, sizeof(note.prstatus));
 
-    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
     note.prstatus.pr_pid = cpu_to_dump32(s, id);
 
     for (i = 0; i < 16; ++i) {
@@ -146,12 +261,12 @@ arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
     }
     note.prstatus.pr_reg.regs[16] = cpu_to_dump32(s, cpsr_read(env));
 
-    ret = f(&note, sizeof(note), s);
+    ret = f(&note, ARM_PRSTATUS_NOTE_SIZE, s);
     if (ret < 0) {
         return -1;
     }
 
-    return 0;
+    return arm_write_elf32_fpregset(f, env, id, s);
 }
 
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
@@ -221,9 +336,11 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
     size_t note_size;
 
     if (class == ELFCLASS64) {
-        note_size = sizeof(struct aarch64_note);
+        note_size = AARCH64_PRSTATUS_NOTE_SIZE;
+        note_size += AARCH64_FPREGSET_NOTE_SIZE;
     } else {
-        note_size = sizeof(struct arm_note);
+        note_size = ARM_PRSTATUS_NOTE_SIZE;
+        note_size += ARM_FPREGSET_NOTE_SIZE;
     }
 
     return note_size * nr_cpus;
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 5/6] target-arm: support QMP dump-guest-memory

[Peter Maydell]

On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> Add the support needed for creating prstatus elf notes. This
> allows us to use QMP dump-guest-memory.

> +
> +    if (is_a64(env)) {
> +        for (i = 0; i < 31; ++i) {
> +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
> +        }
> +        note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
> +        note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
> +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate_read(env));
> +    } else {
> +        aarch64_sync_64_to_32(env);
> +        for (i = 0; i < 16; ++i) {
> +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->regs[i]);
> +        }
> +        note.prstatus.pr_reg.sp = note.prstatus.pr_reg.regs[13];
> +        note.prstatus.pr_reg.pc = note.prstatus.pr_reg.regs[15];
> +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
> +    }

This doesn't look right. sync_64_to_32 is copying the state held
in the 64-bit env->xregs etc into the 32-bit env->regs. But if we're
in 32-bit state then the true state is in the 32-bit fields and
this will trash it. You want to sync_32_to_64 here, and then the
code to write the values to the dump is the same either way
(except for pstate vs cpsr which we haven't managed to clean up
and unify yet, sadly).

I think you want
   uint64_t pstate;
   [...]

   if (!is_a64(env)) {
       aarch64_sync_32_to_64(env);
       pstate = cpsr_read(env);
   } else {
       pstate = pstate_read(env);
   }
   for (i = 0; i < 31; ++i) {
       note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
   }
   note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
   note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
   note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate);

(Note that the 32-bit SP is not architecturally in X31;
it's in one of the other xregs, depending what mode the CPU
was in. For 32-bit userspace that will be USR and it's in X13.)


> +
> +    ret = f(&note, sizeof(note), s);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/* struct pt_regs from arch/arm/include/asm/ptrace.h */
> +struct arm_user_regs {
> +    uint32_t regs[17];
> +    char pad[4];
> +} QEMU_PACKED;
> +
> +QEMU_BUILD_BUG_ON(sizeof(struct arm_user_regs) != 72);
> +
> +/* struct elf_prstatus from include/uapi/linux/elfcore.h */
> +struct arm_elf_prstatus {
> +    char pad1[24]; /* 24 == offsetof(struct elf_prstatus, pr_pid) */
> +    uint32_t pr_pid;
> +    char pad2[44]; /* 44 == offsetof(struct elf_prstatus, pr_reg) -
> +                            offsetof(struct elf_prstatus, pr_ppid) */
> +    struct arm_user_regs pr_reg;
> +    int pr_fpvalid;
> +} QEMU_PACKED arm_elf_prstatus;
> +
> +QEMU_BUILD_BUG_ON(sizeof(struct arm_elf_prstatus) != 148);
> +
> +struct arm_note {
> +    Elf32_Nhdr hdr;
> +    char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
> +    struct arm_elf_prstatus prstatus;
> +} QEMU_PACKED;
> +
> +QEMU_BUILD_BUG_ON(sizeof(struct arm_note) != 168);
> +
> +static int
> +arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
> +                     int id, DumpState *s)
> +{
> +    struct arm_note note;
> +    int ret, i;
> +
> +    memset(&note, 0, sizeof(note));
> +
> +    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
> +    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
> +    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
> +
> +    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
> +    note.prstatus.pr_pid = cpu_to_dump32(s, id);
> +
> +    for (i = 0; i < 16; ++i) {
> +        note.prstatus.pr_reg.regs[i] = cpu_to_dump32(s, env->regs[i]);
> +    }
> +    note.prstatus.pr_reg.regs[16] = cpu_to_dump32(s, cpsr_read(env));
> +
> +    ret = f(&note, sizeof(note), s);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> +                             int cpuid, void *opaque)
> +{
> +    CPUARMState *env = &ARM_CPU(cs)->env;
> +    int ret;
> +
> +    if (arm_el_is_aa64(env, 1)) {
> +        ret = aarch64_write_elf64_note(f, env, cpuid, opaque);
> +    } else {
> +        ret = arm_write_elf32_note(f, env, cpuid, opaque);
> +    }

This might produce the wrong kind of dump if we're in EL2
or EL3 at the point we take it (can only happen in emulation
and only once we add EL2 and EL3 emulation support, which isn't
active yet). Do we care?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 6/6] target-arm: dump-guest-memory: add fpregset notes

[Peter Maydell]

On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> Also refactors note init code to avoid code duplication.

Can you squash those parts down into the preceding patch?

>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/arch_dump.c | 161 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 139 insertions(+), 22 deletions(-)
>
> diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
> index 5debe549d721d..8d74788411d79 100644
> --- a/target-arm/arch_dump.c
> +++ b/target-arm/arch_dump.c
> @@ -35,6 +35,16 @@ struct aarch64_user_regs {
>
>  QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_regs) != 272);
>
> +/* struct user_fpsimd_state from arch/arm64/include/uapi/asm/ptrace.h */

The kernel struct uses __uint128_t, not an array of uint64_t; that's
worth commenting because it clarifies what the expected format is.

> +struct aarch64_user_vfp_state {
> +    uint64_t vregs[64];
> +    uint32_t fpsr;
> +    uint32_t fpcr;
> +    char pad[8];
> +} QEMU_PACKED;
> +
> +QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_vfp_state) != 528);
> +
>  /* struct elf_prstatus from include/uapi/linux/elfcore.h */
>  struct aarch64_elf_prstatus {
>      char pad1[32]; /* 32 == offsetof(struct elf_prstatus, pr_pid) */
> @@ -51,10 +61,77 @@ QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
>  struct aarch64_note {
>      Elf64_Nhdr hdr;
>      char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
> -    struct aarch64_elf_prstatus prstatus;
> +    union {
> +        struct aarch64_elf_prstatus prstatus;
> +        struct aarch64_user_vfp_state vfp;
> +    };
>  } QEMU_PACKED;
>
> -QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
> +#define AARCH64_NOTE_HEADER_SIZE offsetof(struct aarch64_note, prstatus)
> +#define AARCH64_PRSTATUS_NOTE_SIZE \
> +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_elf_prstatus))
> +#define AARCH64_FPREGSET_NOTE_SIZE \
> +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_user_vfp_state))
> +
> +static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
> +                              Elf64_Word type, Elf64_Word descsz)
> +{
> +    memset(note, 0, sizeof(*note));
> +
> +    note->hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
> +    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
> +    note->hdr.n_type = cpu_to_dump32(s, type);
> +
> +    memcpy(note->name, NOTE_NAME, NOTE_NAMESZ);
> +}
> +
> +static void arm_write_vregs(uint64_t *vregs, int num_regs,
> +                            CPUARMState *env, DumpState *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < num_regs; ++i) {
> +        vregs[i] = cpu_to_dump64(s, env->vfp.regs[i]);
> +    }
> +
> +    if (s->dump_info.d_endian == ELFDATA2MSB) {
> +        /* We must always swap vfp.regs's 2n and 2n+1 entries when
> +         * generating BE notes, because even big endian hosts use
> +         * 2n+1 for the high half.
> +         */
> +        for (i = 0; i < num_regs/2; ++i) {
> +            uint64_t tmp = vregs[2*i];
> +            vregs[2*i] = vregs[2*i+1];
> +            vregs[2*i+1] = tmp;
> +        }

This swapping is correct for AArch64, where the core dump format
is an array of 128 bit Qn register values (which in QEMU
are stored in vfp.regs[2n+1]:vfp.regs[2n] as a pair of 64
bit values). However it's wrong for AArch32, where the core
dump format is an array of 64-bit Dn register values, which
in QEMU are just in vfp.regs[n].

(See the comment in target-arm/cpu.h about VFP/Neon register
state and different views of the register bank.)

> +static int
> +arm_write_elf32_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
> +                         int id, DumpState *s)
> +{
> +    struct arm_note note;
> +    int ret;
> +
> +    arm_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
> +
> +    arm_write_vregs(note.vfp.vregs, 32, env, s);
> +
> +    note.vfp.fpscr = cpu_to_dump32(s, vfp_get_fpscr(env));

Do consumers care if we write out a dump with FP register
state for a CPU which doesn't implement FP? (For AArch64
FP is always present, but for AArch32 it may not be.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/6] dump: allow target to set the physical base

[Peter Maydell]

On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> crash assumes the physical base in the kdump subheader of
> makedumpfile formatted dumps is correct. Zero is not correct
> for all architectures, so allow it to be changed.
>
> (No functional change.)
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 5/6] target-arm: support QMP dump-guest-memory

[Andrew Jones]

On Thu, Dec 03, 2015 at 11:44:05AM +0000, Peter Maydell wrote:
> On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> > Add the support needed for creating prstatus elf notes. This
> > allows us to use QMP dump-guest-memory.
> 
> > +
> > +    if (is_a64(env)) {
> > +        for (i = 0; i < 31; ++i) {
> > +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
> > +        }
> > +        note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
> > +        note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
> > +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate_read(env));
> > +    } else {
> > +        aarch64_sync_64_to_32(env);
> > +        for (i = 0; i < 16; ++i) {
> > +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->regs[i]);
> > +        }
> > +        note.prstatus.pr_reg.sp = note.prstatus.pr_reg.regs[13];
> > +        note.prstatus.pr_reg.pc = note.prstatus.pr_reg.regs[15];
> > +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
> > +    }
> 
> This doesn't look right. sync_64_to_32 is copying the state held
> in the 64-bit env->xregs etc into the 32-bit env->regs. But if we're
> in 32-bit state then the true state is in the 32-bit fields and
> this will trash it. You want to sync_32_to_64 here, and then the
> code to write the values to the dump is the same either way
> (except for pstate vs cpsr which we haven't managed to clean up
> and unify yet, sadly).

Besides the unnecessary call to aarch64_sync_64_to_32(), then, for the
KVM case, the above code is correct. However, for the TCG case, I now
see why it's wrong.

The KVM case starts with 64-bit state, because this function is dealing
with 64-bit guest kernels. The TCG case, when userspace is running a
32-bit binary, starts with 32-bit state. In both cases we want to get
32-bit state into a 64-bit elf note. KVM needs aarch64_sync_64_to_32(),
which is actually already done by cpu_synchronize_all_states(), and
then to shoehorn the 32-bit registers into the 64-bit elf note, as done
above. TCG, on the other hand, doesn't need to sync any state, it just
needs to shoehorn. So the above aarch64_sync_64_to_32() call, which I
actually added *for* TCG (since I misunderstood your comment on v1),
actually makes it wrong. Needless to say, I didn't test TCG :-)

Now, to fix it, we could do what you have here below

> 
> I think you want
>    uint64_t pstate;
>    [...]
> 
>    if (!is_a64(env)) {
>        aarch64_sync_32_to_64(env);
>        pstate = cpsr_read(env);
>    } else {
>        pstate = pstate_read(env);
>    }
>    for (i = 0; i < 31; ++i) {
>        note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
>    }

But, this adds an unnecessary aarch64_sync_32_to_64() call to the kvm
case (although it wouldn't hurt, as aarch64_sync_32_to_64 is the inverse
of aarch64_sync_64_to_32, which we've already done earlier). It also
always adds register values 16..30 to the elf note (which may not always
be zero in the 32-bit userspace case?). The way I have it above makes
sure those registers are zero in that case.

So, how about we just remove the aarch64_sync_64_to_32() from the code
I have above? Won't that make it work for both KVM and TCG?


>    note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
>    note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
>    note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate);
> 
> (Note that the 32-bit SP is not architecturally in X31;
> it's in one of the other xregs, depending what mode the CPU
> was in. For 32-bit userspace that will be USR and it's in X13.)

Yup, that's why I was pulling it from x13 in the above code. In your
version you can now use x31, due to the aarch64_sync_32_to_64().

Anyway, I'll actually test with TCG for v3.

> 
> 
> > +
> > +    ret = f(&note, sizeof(note), s);
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/* struct pt_regs from arch/arm/include/asm/ptrace.h */
> > +struct arm_user_regs {
> > +    uint32_t regs[17];
> > +    char pad[4];
> > +} QEMU_PACKED;
> > +
> > +QEMU_BUILD_BUG_ON(sizeof(struct arm_user_regs) != 72);
> > +
> > +/* struct elf_prstatus from include/uapi/linux/elfcore.h */
> > +struct arm_elf_prstatus {
> > +    char pad1[24]; /* 24 == offsetof(struct elf_prstatus, pr_pid) */
> > +    uint32_t pr_pid;
> > +    char pad2[44]; /* 44 == offsetof(struct elf_prstatus, pr_reg) -
> > +                            offsetof(struct elf_prstatus, pr_ppid) */
> > +    struct arm_user_regs pr_reg;
> > +    int pr_fpvalid;
> > +} QEMU_PACKED arm_elf_prstatus;
> > +
> > +QEMU_BUILD_BUG_ON(sizeof(struct arm_elf_prstatus) != 148);
> > +
> > +struct arm_note {
> > +    Elf32_Nhdr hdr;
> > +    char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
> > +    struct arm_elf_prstatus prstatus;
> > +} QEMU_PACKED;
> > +
> > +QEMU_BUILD_BUG_ON(sizeof(struct arm_note) != 168);
> > +
> > +static int
> > +arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
> > +                     int id, DumpState *s)
> > +{
> > +    struct arm_note note;
> > +    int ret, i;
> > +
> > +    memset(&note, 0, sizeof(note));
> > +
> > +    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
> > +    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
> > +    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
> > +
> > +    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
> > +    note.prstatus.pr_pid = cpu_to_dump32(s, id);
> > +
> > +    for (i = 0; i < 16; ++i) {
> > +        note.prstatus.pr_reg.regs[i] = cpu_to_dump32(s, env->regs[i]);
> > +    }
> > +    note.prstatus.pr_reg.regs[16] = cpu_to_dump32(s, cpsr_read(env));
> > +
> > +    ret = f(&note, sizeof(note), s);
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> > +                             int cpuid, void *opaque)
> > +{
> > +    CPUARMState *env = &ARM_CPU(cs)->env;
> > +    int ret;
> > +
> > +    if (arm_el_is_aa64(env, 1)) {
> > +        ret = aarch64_write_elf64_note(f, env, cpuid, opaque);
> > +    } else {
> > +        ret = arm_write_elf32_note(f, env, cpuid, opaque);
> > +    }
> 
> This might produce the wrong kind of dump if we're in EL2
> or EL3 at the point we take it (can only happen in emulation
> and only once we add EL2 and EL3 emulation support, which isn't
> active yet). Do we care?

"care" is loaded word :-) If I can tweak this in some easy way now to get
it ready for el2/el3 emulation, then I'm happy to do so. However, without
a test environment, and without strong motivation to use this feature on
emulation in the first place, then I'd rather not bother for the initial
introduction of it. We can always modify it later.

Thanks,
drew

Re: [Qemu-devel] [PATCH v2 6/6] target-arm: dump-guest-memory: add fpregset notes

[Andrew Jones]

On Thu, Dec 03, 2015 at 12:27:34PM +0000, Peter Maydell wrote:
> On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> > Also refactors note init code to avoid code duplication.
> 
> Can you squash those parts down into the preceding patch?

Sure, but there wasn't any duplication of code in the first patch, only
the FP register notes introduce that possibility.

> 
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target-arm/arch_dump.c | 161 ++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 139 insertions(+), 22 deletions(-)
> >
> > diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
> > index 5debe549d721d..8d74788411d79 100644
> > --- a/target-arm/arch_dump.c
> > +++ b/target-arm/arch_dump.c
> > @@ -35,6 +35,16 @@ struct aarch64_user_regs {
> >
> >  QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_regs) != 272);
> >
> > +/* struct user_fpsimd_state from arch/arm64/include/uapi/asm/ptrace.h */
> 
> The kernel struct uses __uint128_t, not an array of uint64_t; that's
> worth commenting because it clarifies what the expected format is.

OK

> 
> > +struct aarch64_user_vfp_state {
> > +    uint64_t vregs[64];
> > +    uint32_t fpsr;
> > +    uint32_t fpcr;
> > +    char pad[8];
> > +} QEMU_PACKED;
> > +
> > +QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_vfp_state) != 528);
> > +
> >  /* struct elf_prstatus from include/uapi/linux/elfcore.h */
> >  struct aarch64_elf_prstatus {
> >      char pad1[32]; /* 32 == offsetof(struct elf_prstatus, pr_pid) */
> > @@ -51,10 +61,77 @@ QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
> >  struct aarch64_note {
> >      Elf64_Nhdr hdr;
> >      char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
> > -    struct aarch64_elf_prstatus prstatus;
> > +    union {
> > +        struct aarch64_elf_prstatus prstatus;
> > +        struct aarch64_user_vfp_state vfp;
> > +    };
> >  } QEMU_PACKED;
> >
> > -QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
> > +#define AARCH64_NOTE_HEADER_SIZE offsetof(struct aarch64_note, prstatus)
> > +#define AARCH64_PRSTATUS_NOTE_SIZE \
> > +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_elf_prstatus))
> > +#define AARCH64_FPREGSET_NOTE_SIZE \
> > +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_user_vfp_state))
> > +
> > +static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
> > +                              Elf64_Word type, Elf64_Word descsz)
> > +{
> > +    memset(note, 0, sizeof(*note));
> > +
> > +    note->hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
> > +    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
> > +    note->hdr.n_type = cpu_to_dump32(s, type);
> > +
> > +    memcpy(note->name, NOTE_NAME, NOTE_NAMESZ);
> > +}
> > +
> > +static void arm_write_vregs(uint64_t *vregs, int num_regs,
> > +                            CPUARMState *env, DumpState *s)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < num_regs; ++i) {
> > +        vregs[i] = cpu_to_dump64(s, env->vfp.regs[i]);
> > +    }
> > +
> > +    if (s->dump_info.d_endian == ELFDATA2MSB) {
> > +        /* We must always swap vfp.regs's 2n and 2n+1 entries when
> > +         * generating BE notes, because even big endian hosts use
> > +         * 2n+1 for the high half.
> > +         */
> > +        for (i = 0; i < num_regs/2; ++i) {
> > +            uint64_t tmp = vregs[2*i];
> > +            vregs[2*i] = vregs[2*i+1];
> > +            vregs[2*i+1] = tmp;
> > +        }
> 
> This swapping is correct for AArch64, where the core dump format
> is an array of 128 bit Qn register values (which in QEMU
> are stored in vfp.regs[2n+1]:vfp.regs[2n] as a pair of 64
> bit values). However it's wrong for AArch32, where the core
> dump format is an array of 64-bit Dn register values, which
> in QEMU are just in vfp.regs[n].
> 
> (See the comment in target-arm/cpu.h about VFP/Neon register
> state and different views of the register bank.)

OK, I'll fix that. I thought I tested this already, but maybe not, as
I didn't check FP registers for every test case in the matrix in the
cover letter.

> 
> > +static int
> > +arm_write_elf32_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
> > +                         int id, DumpState *s)
> > +{
> > +    struct arm_note note;
> > +    int ret;
> > +
> > +    arm_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
> > +
> > +    arm_write_vregs(note.vfp.vregs, 32, env, s);
> > +
> > +    note.vfp.fpscr = cpu_to_dump32(s, vfp_get_fpscr(env));
> 
> Do consumers care if we write out a dump with FP register
> state for a CPU which doesn't implement FP? (For AArch64
> FP is always present, but for AArch32 it may not be.)

I don't know, but it should be easy to avoid writing them when FP
isn't present, based on some status, so I'll look into doing that.

Thanks,
drew

Re: [Qemu-devel] [PATCH v2 5/6] target-arm: support QMP dump-guest-memory

[Peter Maydell]

On 3 December 2015 at 18:55, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Dec 03, 2015 at 11:44:05AM +0000, Peter Maydell wrote:
>> On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
>> > Add the support needed for creating prstatus elf notes. This
>> > allows us to use QMP dump-guest-memory.
>>
>> > +
>> > +    if (is_a64(env)) {
>> > +        for (i = 0; i < 31; ++i) {
>> > +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
>> > +        }
>> > +        note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
>> > +        note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
>> > +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate_read(env));
>> > +    } else {
>> > +        aarch64_sync_64_to_32(env);
>> > +        for (i = 0; i < 16; ++i) {
>> > +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->regs[i]);
>> > +        }
>> > +        note.prstatus.pr_reg.sp = note.prstatus.pr_reg.regs[13];
>> > +        note.prstatus.pr_reg.pc = note.prstatus.pr_reg.regs[15];
>> > +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
>> > +    }
>>
>> This doesn't look right. sync_64_to_32 is copying the state held
>> in the 64-bit env->xregs etc into the 32-bit env->regs. But if we're
>> in 32-bit state then the true state is in the 32-bit fields and
>> this will trash it. You want to sync_32_to_64 here, and then the
>> code to write the values to the dump is the same either way
>> (except for pstate vs cpsr which we haven't managed to clean up
>> and unify yet, sadly).
>
> Besides the unnecessary call to aarch64_sync_64_to_32(), then, for the
> KVM case, the above code is correct. However, for the TCG case, I now
> see why it's wrong.
>
> The KVM case starts with 64-bit state, because this function is dealing
> with 64-bit guest kernels. The TCG case, when userspace is running a
> 32-bit binary, starts with 32-bit state. In both cases we want to get
> 32-bit state into a 64-bit elf note. KVM needs aarch64_sync_64_to_32(),
> which is actually already done by cpu_synchronize_all_states(), and
> then to shoehorn the 32-bit registers into the 64-bit elf note, as done
> above. TCG, on the other hand, doesn't need to sync any state, it just
> needs to shoehorn. So the above aarch64_sync_64_to_32() call, which I
> actually added *for* TCG (since I misunderstood your comment on v1),
> actually makes it wrong. Needless to say, I didn't test TCG :-)
>
> Now, to fix it, we could do what you have here below
>
>>
>> I think you want
>>    uint64_t pstate;
>>    [...]
>>
>>    if (!is_a64(env)) {
>>        aarch64_sync_32_to_64(env);
>>        pstate = cpsr_read(env);
>>    } else {
>>        pstate = pstate_read(env);
>>    }
>>    for (i = 0; i < 31; ++i) {
>>        note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
>>    }
>
> But, this adds an unnecessary aarch64_sync_32_to_64() call to the kvm
> case (although it wouldn't hurt, as aarch64_sync_32_to_64 is the inverse
> of aarch64_sync_64_to_32, which we've already done earlier). It also
> always adds register values 16..30 to the elf note (which may not always
> be zero in the 32-bit userspace case?). The way I have it above makes
> sure those registers are zero in that case.

If you do that then you'll lose the information about the other
32-bit registers (the svc/irq/etc banked versions). Those aren't
relevant if the 32-bit code is in usermode, but probably you want
them if you're doing a dump of a 64-bit (emulated) hypervisor
that happens to be running a 32-bit guest kernel at point of dump.

> So, how about we just remove the aarch64_sync_64_to_32() from the code
> I have above? Won't that make it work for both KVM and TCG?
>
>
>>    note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
>>    note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
>>    note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate);
>>
>> (Note that the 32-bit SP is not architecturally in X31;
>> it's in one of the other xregs, depending what mode the CPU
>> was in. For 32-bit userspace that will be USR and it's in X13.)
>
> Yup, that's why I was pulling it from x13 in the above code. In your
> version you can now use x31, due to the aarch64_sync_32_to_64().

Note that sync_32_to_64 does not copy regs[13] into x31, which was
my point. In a 64-bit-format dump of a VM that happens to be
running 32 bit code you should not expect pstate.sp to be the
32-bit process's SP...

>> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>> > +                             int cpuid, void *opaque)
>> > +{
>> > +    CPUARMState *env = &ARM_CPU(cs)->env;
>> > +    int ret;
>> > +
>> > +    if (arm_el_is_aa64(env, 1)) {
>> > +        ret = aarch64_write_elf64_note(f, env, cpuid, opaque);
>> > +    } else {
>> > +        ret = arm_write_elf32_note(f, env, cpuid, opaque);
>> > +    }
>>
>> This might produce the wrong kind of dump if we're in EL2
>> or EL3 at the point we take it (can only happen in emulation
>> and only once we add EL2 and EL3 emulation support, which isn't
>> active yet). Do we care?
>
> "care" is loaded word :-) If I can tweak this in some easy way now to get
> it ready for el2/el3 emulation, then I'm happy to do so. However, without
> a test environment, and without strong motivation to use this feature on
> emulation in the first place, then I'd rather not bother for the initial
> introduction of it. We can always modify it later.

For this test I think you can just say
  if (arm_feature(env, ARM_FEATURE_AARCH64)) {

which basically says "64-bit dump if the CPU supports 64-bit" (32-bit
KVM VMs won't have this feature bit). The other awkward part is
figuring out which endianness to use. I think there we can just put
in a comment
    /* We assume the relevant endianness is that of EL1; this is right
     * for kernels but might give the wrong answer if you're trying to
     * take a dump of a hypervisor that happens currently to be running
     * a wrong-endian kernel.
     */
and leave it for somebody who cares to try to figure out the right
semantics.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 6/6] target-arm: dump-guest-memory: add fpregset notes

[Peter Maydell]

On 3 December 2015 at 19:00, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Dec 03, 2015 at 12:27:34PM +0000, Peter Maydell wrote:
>> Do consumers care if we write out a dump with FP register
>> state for a CPU which doesn't implement FP? (For AArch64
>> FP is always present, but for AArch32 it may not be.)
>
> I don't know, but it should be easy to avoid writing them when FP
> isn't present, based on some status, so I'll look into doing that.

if (arm_feature(env, ARM_FEATURE_VFP)) is probably what you want.

thanks
-- PMM