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

[Qemu-devel] [PATCH v3 0/9] 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.

v3:
 - more changes thanks to Peter's review comments
   - fix 32-to-64 shoehorning to work better with EL2/EL3
   - stole comment from Peter for best-effort at endianness selection
   - only make ARM VFP notes when the guest has VFP registers
   - some more patch squashing and code commenting
   - another r-b added
 - collapsed a level of indirection, because if we're writing elf64 notes
   then we'll always call the aarch64 function [drew]
 - fixed the note type for ARM VFP notes (+ a tiny bit of refactoring) [drew]
 - set prstatus.pr_fpvalid [drew]
 - tested on tcg [drew]

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 (9):
  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: introduce aarch64_compat_sp
  target-arm: support QMP dump-guest-memory
  target-arm: dump-guest-memory: add prfpreg notes for aarch64
  elf: add arm note types
  target-arm: dump-guest-memory: add vfp notes for arm

 dump.c                      | 131 ++++++++++-------
 include/elf.h               |   5 +
 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      | 336 ++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu-qom.h        |   5 +
 target-arm/cpu.c            |   3 +
 target-arm/cpu.h            |  45 ++++++
 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 -
 17 files changed, 487 insertions(+), 87 deletions(-)
 create mode 100644 target-arm/arch_dump.c

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/9] 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 v3 2/9] 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 v3 3/9] 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 v3 4/9] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 v3 5/9] target-arm: introduce aarch64_compat_sp

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/cpu.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 815fef8a30663..9231a9ff3b9c2 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -863,6 +863,51 @@ enum arm_cpu_mode {
 #define ARM_IWMMXT_wCGR2	10
 #define ARM_IWMMXT_wCGR3	11
 
+/* AArch64 to AArch32 register mappings */
+#define compat_fp_usr   xregs[11]
+#define compat_sp_usr   xregs[13]
+#define compat_lr_usr   xregs[14]
+#define compat_sp_hyp   xregs[15]
+#define compat_lr_irq   xregs[16]
+#define compat_sp_irq   xregs[17]
+#define compat_lr_svc   xregs[18]
+#define compat_sp_svc   xregs[19]
+#define compat_lr_abt   xregs[20]
+#define compat_sp_abt   xregs[21]
+#define compat_lr_und   xregs[22]
+#define compat_sp_und   xregs[23]
+#define compat_r8_fiq   xregs[24]
+#define compat_r9_fiq   xregs[25]
+#define compat_r10_fiq  xregs[26]
+#define compat_r11_fiq  xregs[27]
+#define compat_r12_fiq  xregs[28]
+#define compat_sp_fiq   xregs[29]
+#define compat_lr_fiq   xregs[30]
+
+static inline uint64_t aarch64_compat_sp(CPUARMState *env)
+{
+    uint32_t mode = env->uncached_cpsr & CPSR_M;
+
+    switch (mode) {
+    case ARM_CPU_MODE_USR:
+    case ARM_CPU_MODE_SYS:
+        return env->compat_sp_usr;
+    case ARM_CPU_MODE_FIQ:
+        return env->compat_sp_fiq;
+    case ARM_CPU_MODE_IRQ:
+        return env->compat_sp_irq;
+    case ARM_CPU_MODE_SVC:
+        return env->compat_sp_svc;
+    case ARM_CPU_MODE_ABT:
+        return env->compat_sp_abt;
+    case ARM_CPU_MODE_HYP:
+        return env->compat_sp_hyp;
+    case ARM_CPU_MODE_UND:
+        return env->compat_sp_und;
+    }
+    g_assert_not_reached();
+}
+
 /* If adding a feature bit which corresponds to a Linux ELF
  * HWCAP bit, remember to update the feature-bit-to-hwcap
  * mapping in linux-user/elfload.c:get_elf_hwcap().
-- 
2.4.3

[Qemu-devel] [PATCH v3 6/9] 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..dc32d98101004
--- /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"
+
+/* 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;
+    uint32_t 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[8]; /* align_up(sizeof("CORE"), 4) */
+    struct aarch64_elf_prstatus prstatus;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
+
+static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
+                              const char *name, Elf64_Word namesz,
+                              Elf64_Word type, Elf64_Word descsz)
+{
+    memset(note, 0, sizeof(*note));
+
+    note->hdr.n_namesz = cpu_to_dump32(s, namesz);
+    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
+    note->hdr.n_type = cpu_to_dump32(s, type);
+
+    memcpy(note->name, name, namesz);
+}
+
+int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque)
+{
+    struct aarch64_note note;
+    CPUARMState *env = &ARM_CPU(cs)->env;
+    DumpState *s = opaque;
+    uint64_t pstate, sp;
+    int ret, i;
+
+    aarch64_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
+
+    note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
+
+    if (!is_a64(env)) {
+        aarch64_sync_32_to_64(env);
+        pstate = cpsr_read(env);
+        sp = aarch64_compat_sp(env);
+    } else {
+        pstate = pstate_read(env);
+        sp = env->xregs[31];
+    }
+
+    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, sp);
+    note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
+    note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate);
+
+    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;
+    uint32_t 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[8]; /* align_up(sizeof("CORE"), 4) */
+    struct arm_elf_prstatus prstatus;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct arm_note) != 168);
+
+static void arm_note_init(struct arm_note *note, DumpState *s,
+                          const char *name, Elf32_Word namesz,
+                          Elf32_Word type, Elf32_Word descsz)
+{
+    memset(note, 0, sizeof(*note));
+
+    note->hdr.n_namesz = cpu_to_dump32(s, namesz);
+    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
+    note->hdr.n_type = cpu_to_dump32(s, type);
+
+    memcpy(note->name, name, namesz);
+}
+
+int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque)
+{
+    struct arm_note note;
+    CPUARMState *env = &ARM_CPU(cs)->env;
+    DumpState *s = opaque;
+    int ret, i;
+
+    arm_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
+
+    note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
+
+    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 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_feature(env, ARM_FEATURE_AARCH64)) {
+        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;
+        }
+    }
+
+    /* 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
+     * dump a hypervisor that happens to be running an opposite-endian
+     * kernel.
+     */
+    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 v3 7/9] target-arm: dump-guest-memory: add prfpreg notes for aarch64

[Andrew Jones]

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

diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
index dc32d98101004..6cbba92f3d014 100644
--- a/target-arm/arch_dump.c
+++ b/target-arm/arch_dump.c
@@ -45,13 +45,36 @@ struct aarch64_elf_prstatus {
 
 QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
 
+/* struct user_fpsimd_state from arch/arm64/include/uapi/asm/ptrace.h
+ *
+ * While the vregs member of user_fpsimd_state is of type __uint128_t,
+ * QEMU uses an array of uint64_t, where the high half of the 128-bit
+ * value is always in the 2n+1'th index. Thus we also break the 128-
+ * bit values into two halves in this reproduction of user_fpsimd_state.
+ */
+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 aarch64_note {
     Elf64_Nhdr hdr;
     char name[8]; /* align_up(sizeof("CORE"), 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_PRFPREG_NOTE_SIZE \
+            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_user_vfp_state))
 
 static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
                               const char *name, Elf64_Word namesz,
@@ -66,6 +89,42 @@ static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
     memcpy(note->name, name, namesz);
 }
 
+static int
+aarch64_write_elf64_prfpreg(WriteCoreDumpFunction f, CPUARMState *env,
+                            int cpuid, DumpState *s)
+{
+    struct aarch64_note note;
+    int ret, i;
+
+    aarch64_note_init(&note, s, "CORE", 5, NT_PRFPREG, sizeof(note.vfp));
+
+    for (i = 0; i < 64; ++i) {
+        note.vfp.vregs[i] = cpu_to_dump64(s, float64_val(env->vfp.regs[i]));
+    }
+
+    if (s->dump_info.d_endian == ELFDATA2MSB) {
+        /* For AArch4 we must always swap the 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 < 32; ++i) {
+            uint64_t tmp = note.vfp.vregs[2*i];
+            note.vfp.vregs[2*i] = note.vfp.vregs[2*i+1];
+            note.vfp.vregs[2*i+1] = tmp;
+        }
+    }
+
+    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_PRFPREG_NOTE_SIZE, s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                              int cpuid, void *opaque)
 {
@@ -78,6 +137,7 @@ int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
     aarch64_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
 
     note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
+    note.prstatus.pr_fpvalid = cpu_to_dump32(s, 1);
 
     if (!is_a64(env)) {
         aarch64_sync_32_to_64(env);
@@ -95,12 +155,12 @@ int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
     note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
     note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate);
 
-    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_prfpreg(f, env, cpuid, s);
 }
 
 /* struct pt_regs from arch/arm/include/asm/ptrace.h */
@@ -129,7 +189,9 @@ struct arm_note {
     struct arm_elf_prstatus prstatus;
 } 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))
 
 static void arm_note_init(struct arm_note *note, DumpState *s,
                           const char *name, Elf32_Word namesz,
@@ -161,7 +223,7 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
     }
     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;
     }
@@ -221,9 +283,10 @@ 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_PRFPREG_NOTE_SIZE;
     } else {
-        note_size = sizeof(struct arm_note);
+        note_size = ARM_PRSTATUS_NOTE_SIZE;
     }
 
     return note_size * nr_cpus;
-- 
2.4.3

[Qemu-devel] [PATCH v3 8/9] elf: add arm note types

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 include/elf.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/elf.h b/include/elf.h
index 66add810df144..1098d217ec7ff 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -1471,6 +1471,11 @@ typedef struct elf64_shdr {
 #define NT_PPC_VMX       0x100          /* PowerPC Altivec/VMX registers */
 #define NT_PPC_SPE       0x101          /* PowerPC SPE/EVR registers */
 #define NT_PPC_VSX       0x102          /* PowerPC VSX registers */
+#define NT_ARM_VFP      0x400           /* ARM VFP/NEON registers */
+#define NT_ARM_TLS      0x401           /* ARM TLS register */
+#define NT_ARM_HW_BREAK 0x402           /* ARM hardware breakpoint registers */
+#define NT_ARM_HW_WATCH 0x403           /* ARM hardware watchpoint registers */
+#define NT_ARM_SYSTEM_CALL      0x404   /* ARM system call number */
 
 
 /* Note header in a PT_NOTE section */
-- 
2.4.3

[Qemu-devel] [PATCH v3 9/9] target-arm: dump-guest-memory: add vfp notes for arm

[Andrew Jones]

gdb won't actually dump these with 'info all-registers' since
it first tries to confirm that it should by checking the VFP
hwcap in the .auxv note. Well, we don't generate an .auxv note.

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

diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
index 6cbba92f3d014..f2bc5c6f604a2 100644
--- a/target-arm/arch_dump.c
+++ b/target-arm/arch_dump.c
@@ -183,15 +183,28 @@ struct arm_elf_prstatus {
 
 QEMU_BUILD_BUG_ON(sizeof(struct arm_elf_prstatus) != 148);
 
+/* 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 arm_note {
     Elf32_Nhdr hdr;
-    char name[8]; /* align_up(sizeof("CORE"), 4) */
-    struct arm_elf_prstatus prstatus;
+    char name[8]; /* align_up(sizeof("LINUX"), 4) */
+    union {
+        struct arm_elf_prstatus prstatus;
+        struct arm_user_vfp_state vfp;
+    };
 } QEMU_PACKED;
 
 #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_VFP_NOTE_SIZE \
+            (ARM_NOTE_HEADER_SIZE + sizeof(struct arm_user_vfp_state))
 
 static void arm_note_init(struct arm_note *note, DumpState *s,
                           const char *name, Elf32_Word namesz,
@@ -206,17 +219,40 @@ static void arm_note_init(struct arm_note *note, DumpState *s,
     memcpy(note->name, name, namesz);
 }
 
+static int arm_write_elf32_vfp(WriteCoreDumpFunction f, CPUARMState *env,
+                               int cpuid, DumpState *s)
+{
+    struct arm_note note;
+    int ret, i;
+
+    arm_note_init(&note, s, "LINUX", 6, NT_ARM_VFP, sizeof(note.vfp));
+
+    for (i = 0; i < 32; ++i) {
+        note.vfp.vregs[i] = cpu_to_dump64(s, float64_val(env->vfp.regs[i]));
+    }
+
+    note.vfp.fpscr = cpu_to_dump32(s, vfp_get_fpscr(env));
+
+    ret = f(&note, ARM_VFP_NOTE_SIZE, s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
 int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
                              int cpuid, void *opaque)
 {
     struct arm_note note;
     CPUARMState *env = &ARM_CPU(cs)->env;
     DumpState *s = opaque;
-    int ret, i;
+    int ret, i, fpvalid = !!arm_feature(env, ARM_FEATURE_VFP);
 
     arm_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
 
     note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
+    note.prstatus.pr_fpvalid = cpu_to_dump32(s, fpvalid);
 
     for (i = 0; i < 16; ++i) {
         note.prstatus.pr_reg.regs[i] = cpu_to_dump32(s, env->regs[i]);
@@ -226,6 +262,8 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
     ret = f(&note, ARM_PRSTATUS_NOTE_SIZE, s);
     if (ret < 0) {
         return -1;
+    } else if (fpvalid) {
+        return arm_write_elf32_vfp(f, env, cpuid, s);
     }
 
     return 0;
@@ -280,6 +318,8 @@ int cpu_get_dump_info(ArchDumpInfo *info,
 
 ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
 {
+    ARMCPU *cpu = ARM_CPU(first_cpu);
+    CPUARMState *env = &cpu->env;
     size_t note_size;
 
     if (class == ELFCLASS64) {
@@ -287,6 +327,9 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
         note_size += AARCH64_PRFPREG_NOTE_SIZE;
     } else {
         note_size = ARM_PRSTATUS_NOTE_SIZE;
+        if (arm_feature(env, ARM_FEATURE_VFP)) {
+            note_size += ARM_VFP_NOTE_SIZE;
+        }
     }
 
     return note_size * nr_cpus;
-- 
2.4.3

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

[Peter Maydell]

On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> 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>

> +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> +                             int cpuid, void *opaque)
> +{
> +    struct aarch64_note note;
> +    CPUARMState *env = &ARM_CPU(cs)->env;
> +    DumpState *s = opaque;
> +    uint64_t pstate, sp;
> +    int ret, i;
> +
> +    aarch64_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
> +
> +    note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
> +
> +    if (!is_a64(env)) {
> +        aarch64_sync_32_to_64(env);
> +        pstate = cpsr_read(env);
> +        sp = aarch64_compat_sp(env);

I don't understand why we need to do this. If this is an
AArch64 dump then we should just treat it as an AArch64
dump, and presumably the consumer of the dump knows enough
to know what the "hypervisor view" of a CPU that's currently
in 32-bit mode is. It has to anyway to be able to figure
out where all the other registers are, so why can't it
also figure out what mode the CPU is currently in and thus
where r13 is in the xregs array?

> +    } else {
> +        pstate = pstate_read(env);
> +        sp = env->xregs[31];
> +    }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 9/9] target-arm: dump-guest-memory: add vfp notes for arm

[Peter Maydell]

On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> gdb won't actually dump these with 'info all-registers' since
> it first tries to confirm that it should by checking the VFP
> hwcap in the .auxv note. Well, we don't generate an .auxv note.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 8/9] elf: add arm note types

[Peter Maydell]

On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  include/elf.h | 5 +++++
>  1 file changed, 5 insertions(+)

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 7/9] target-arm: dump-guest-memory: add prfpreg notes for aarch64

[Peter Maydell]

On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/arch_dump.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 71 insertions(+), 8 deletions(-)

> +static int

We don't usually do the 'linebreak before function name' style.

> +aarch64_write_elf64_prfpreg(WriteCoreDumpFunction f, CPUARMState *env,
> +                            int cpuid, DumpState *s)
> +{
> +    struct aarch64_note note;
> +    int ret, i;
> +
> +    aarch64_note_init(&note, s, "CORE", 5, NT_PRFPREG, sizeof(note.vfp));
> +
> +    for (i = 0; i < 64; ++i) {
> +        note.vfp.vregs[i] = cpu_to_dump64(s, float64_val(env->vfp.regs[i]));
> +    }
> +
> +    if (s->dump_info.d_endian == ELFDATA2MSB) {
> +        /* For AArch4 we must always swap the vfp.regs's 2n and 2n+1

"AArch64".

> +         * entries when generating BE notes, because even big endian
> +         * hosts use 2n+1 for the high half.
> +         */

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/9] dump: allow target to set the page size

[Peter Maydell]

On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> 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>

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

thanks
-- PMM

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

[Andrew Jones]

On Fri, Dec 18, 2015 at 11:59:39AM +0000, Peter Maydell wrote:
> On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> > 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>
> 
> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> > +                             int cpuid, void *opaque)
> > +{
> > +    struct aarch64_note note;
> > +    CPUARMState *env = &ARM_CPU(cs)->env;
> > +    DumpState *s = opaque;
> > +    uint64_t pstate, sp;
> > +    int ret, i;
> > +
> > +    aarch64_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
> > +
> > +    note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
> > +
> > +    if (!is_a64(env)) {
> > +        aarch64_sync_32_to_64(env);
> > +        pstate = cpsr_read(env);
> > +        sp = aarch64_compat_sp(env);
> 
> I don't understand why we need to do this. If this is an
> AArch64 dump then we should just treat it as an AArch64
> dump, and presumably the consumer of the dump knows enough
> to know what the "hypervisor view" of a CPU that's currently
> in 32-bit mode is. It has to anyway to be able to figure
> out where all the other registers are, so why can't it
> also figure out what mode the CPU is currently in and thus
> where r13 is in the xregs array?

You're probably right that this shouldn't be necessary. But, in order for
it not to be necessary, I'll need to write another crash patch. Currently,
if you do a dump-guest-memory on a running guest, i.e. one where the kernel
has not called panic(), and thus the cpus are actually in 32-bit usermode,
rather than in the 64-bit cpu-stop IPI handler, then the crash utility
segfaults if sp == xregs[31]. crash does properly decode the registers
it digs out of the stack frame on a panic'ed cpu though, and setting sp
to aarch64_compat_sp here also allows crash to work properly in the non-
panic'd case.

So, I could teach crash to do what I'm doing here in qemu instead, but
there's still one more reason why it may make sense to do it here. That
reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
so why not set sp to the correct userspace sp?

Thanks,
drew

> 
> > +    } else {
> > +        pstate = pstate_read(env);
> > +        sp = env->xregs[31];
> > +    }
> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On 18 December 2015 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
> On Fri, Dec 18, 2015 at 11:59:39AM +0000, Peter Maydell wrote:
>> On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
>> > 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>
>>
>> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>> > +                             int cpuid, void *opaque)
>> > +{
>> > +    struct aarch64_note note;
>> > +    CPUARMState *env = &ARM_CPU(cs)->env;
>> > +    DumpState *s = opaque;
>> > +    uint64_t pstate, sp;
>> > +    int ret, i;
>> > +
>> > +    aarch64_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
>> > +
>> > +    note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
>> > +
>> > +    if (!is_a64(env)) {
>> > +        aarch64_sync_32_to_64(env);
>> > +        pstate = cpsr_read(env);
>> > +        sp = aarch64_compat_sp(env);
>>
>> I don't understand why we need to do this. If this is an
>> AArch64 dump then we should just treat it as an AArch64
>> dump, and presumably the consumer of the dump knows enough
>> to know what the "hypervisor view" of a CPU that's currently
>> in 32-bit mode is. It has to anyway to be able to figure
>> out where all the other registers are, so why can't it
>> also figure out what mode the CPU is currently in and thus
>> where r13 is in the xregs array?
>
> You're probably right that this shouldn't be necessary. But, in order for
> it not to be necessary, I'll need to write another crash patch. Currently,
> if you do a dump-guest-memory on a running guest, i.e. one where the kernel
> has not called panic(), and thus the cpus are actually in 32-bit usermode,
> rather than in the 64-bit cpu-stop IPI handler, then the crash utility
> segfaults if sp == xregs[31]. crash does properly decode the registers
> it digs out of the stack frame on a panic'ed cpu though, and setting sp
> to aarch64_compat_sp here also allows crash to work properly in the non-
> panic'd case.

If crash segfaults then that's clearly a bug in crash...
What is it expecting to see in the SP field?

> So, I could teach crash to do what I'm doing here in qemu instead, but
> there's still one more reason why it may make sense to do it here. That
> reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
> xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
> correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
> so why not set sp to the correct userspace sp?

Well, what spec are we following here? The most logical view
to me seems to be to say "you get the view of the system
that you get from a JTAG debugger or a hypervisor", which
is to say you see a 64-bit set of registers and it's the
debugger's job to decide which bits of those might be
interesting to view as 32-bit and what is actually "live"
32 bit state.

>From that point of view there is no valid AArch64 SP register
at this point in execution (xregs[31] for QEMU would be stale
state, so not a good choice I think).

thanks
-- PMM

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

[Andrew Jones]

On Fri, Dec 18, 2015 at 04:31:13PM +0000, Peter Maydell wrote:
> On 18 December 2015 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
> > On Fri, Dec 18, 2015 at 11:59:39AM +0000, Peter Maydell wrote:
> >> On 15 December 2015 at 22:51, Andrew Jones <drjones@redhat.com> wrote:
> >> > 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>
> >>
> >> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> >> > +                             int cpuid, void *opaque)
> >> > +{
> >> > +    struct aarch64_note note;
> >> > +    CPUARMState *env = &ARM_CPU(cs)->env;
> >> > +    DumpState *s = opaque;
> >> > +    uint64_t pstate, sp;
> >> > +    int ret, i;
> >> > +
> >> > +    aarch64_note_init(&note, s, "CORE", 5, NT_PRSTATUS, sizeof(note.prstatus));
> >> > +
> >> > +    note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
> >> > +
> >> > +    if (!is_a64(env)) {
> >> > +        aarch64_sync_32_to_64(env);
> >> > +        pstate = cpsr_read(env);
> >> > +        sp = aarch64_compat_sp(env);
> >>
> >> I don't understand why we need to do this. If this is an
> >> AArch64 dump then we should just treat it as an AArch64
> >> dump, and presumably the consumer of the dump knows enough
> >> to know what the "hypervisor view" of a CPU that's currently
> >> in 32-bit mode is. It has to anyway to be able to figure
> >> out where all the other registers are, so why can't it
> >> also figure out what mode the CPU is currently in and thus
> >> where r13 is in the xregs array?
> >
> > You're probably right that this shouldn't be necessary. But, in order for
> > it not to be necessary, I'll need to write another crash patch. Currently,
> > if you do a dump-guest-memory on a running guest, i.e. one where the kernel
> > has not called panic(), and thus the cpus are actually in 32-bit usermode,
> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
> > segfaults if sp == xregs[31]. crash does properly decode the registers
> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
> > to aarch64_compat_sp here also allows crash to work properly in the non-
> > panic'd case.
> 
> If crash segfaults then that's clearly a bug in crash...
> What is it expecting to see in the SP field?

A valid stack pointer

> 
> > So, I could teach crash to do what I'm doing here in qemu instead, but
> > there's still one more reason why it may make sense to do it here. That
> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
> > so why not set sp to the correct userspace sp?
> 
> Well, what spec are we following here? The most logical view

With the shoehorning approach we're following the aarch64 ptrace spec,
but putting arm32 registers in it. We then rely on the analysis tools
to do the right thing when they see pstate has PSR_MODE32_BIT set, which
all cpsr modes do.

The ptrace code in the kernel would return a real aarch32 view, i.e.
only registers up to r15 and a cpsr. We can't do that here because we've
committed to an EM_AARCH64 formatted core, and we only have one type of
PRSTATUS note for that core type. Furthermore, we want to be able to
return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.

> to me seems to be to say "you get the view of the system
> that you get from a JTAG debugger or a hypervisor", which
> is to say you see a 64-bit set of registers and it's the
> debugger's job to decide which bits of those might be
> interesting to view as 32-bit and what is actually "live"
> 32 bit state.

It appears that PRSTATUS aware tools don't currently work this way.

> 
> From that point of view there is no valid AArch64 SP register
> at this point in execution (xregs[31] for QEMU would be stale
> state, so not a good choice I think).

I suppose zero or all 1's are the safest choices for "undefined", but
being undefined actually gives us freedom to use aarch64_compat_sp as
well, which, to me, looks like a safer and more useful value.

Thanks,
drew

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

[Peter Maydell]

On 18 December 2015 at 18:05, Andrew Jones <drjones@redhat.com> wrote:
> On Fri, Dec 18, 2015 at 04:31:13PM +0000, Peter Maydell wrote:
>> On 18 December 2015 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
>> > On Fri, Dec 18, 2015 at 11:59:39AM +0000, Peter Maydell wrote:
>> >> I don't understand why we need to do this. If this is an
>> >> AArch64 dump then we should just treat it as an AArch64
>> >> dump, and presumably the consumer of the dump knows enough
>> >> to know what the "hypervisor view" of a CPU that's currently
>> >> in 32-bit mode is. It has to anyway to be able to figure
>> >> out where all the other registers are, so why can't it
>> >> also figure out what mode the CPU is currently in and thus
>> >> where r13 is in the xregs array?
>> >
>> > You're probably right that this shouldn't be necessary. But, in order for
>> > it not to be necessary, I'll need to write another crash patch. Currently,
>> > if you do a dump-guest-memory on a running guest, i.e. one where the kernel
>> > has not called panic(), and thus the cpus are actually in 32-bit usermode,
>> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
>> > segfaults if sp == xregs[31]. crash does properly decode the registers
>> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
>> > to aarch64_compat_sp here also allows crash to work properly in the non-
>> > panic'd case.
>>
>> If crash segfaults then that's clearly a bug in crash...
>> What is it expecting to see in the SP field?
>
> A valid stack pointer

But why? What does it do with it? (In any case a dump of a crashed
system could have any random rubbish in SP so the tool has to handle
it being something weird.)

>> > So, I could teach crash to do what I'm doing here in qemu instead, but
>> > there's still one more reason why it may make sense to do it here. That
>> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
>> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
>> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
>> > so why not set sp to the correct userspace sp?
>>
>> Well, what spec are we following here? The most logical view
>
> With the shoehorning approach we're following the aarch64 ptrace spec,
> but putting arm32 registers in it. We then rely on the analysis tools
> to do the right thing when they see pstate has PSR_MODE32_BIT set, which
> all cpsr modes do.

Right, so the analysis tool already has to cope with the MODE32 bit
being set, and it can also figure out where the SP is (and which
other registers are currently live for 32-bit).

> The ptrace code in the kernel would return a real aarch32 view, i.e.
> only registers up to r15 and a cpsr. We can't do that here because we've
> committed to an EM_AARCH64 formatted core, and we only have one type of
> PRSTATUS note for that core type. Furthermore, we want to be able to
> return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.
>
>> to me seems to be to say "you get the view of the system
>> that you get from a JTAG debugger or a hypervisor", which
>> is to say you see a 64-bit set of registers and it's the
>> debugger's job to decide which bits of those might be
>> interesting to view as 32-bit and what is actually "live"
>> 32 bit state.
>
> It appears that PRSTATUS aware tools don't currently work this way.
>
>>
>> From that point of view there is no valid AArch64 SP register
>> at this point in execution (xregs[31] for QEMU would be stale
>> state, so not a good choice I think).
>
> I suppose zero or all 1's are the safest choices for "undefined", but
> being undefined actually gives us freedom to use aarch64_compat_sp as
> well, which, to me, looks like a safer and more useful value.

It just doesn't really make sense to me to do this one bit of
work for the debug analysis tools when they already have to know
that 32-bit modes are special. We seem to end up with a function
in QEMU whose only purpose is working around a bug in the thing
consuming the coredump.

thanks
-- PMM

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

[Andrew Jones]

On Fri, Dec 18, 2015 at 06:46:14PM +0000, Peter Maydell wrote:
> On 18 December 2015 at 18:05, Andrew Jones <drjones@redhat.com> wrote:
> > On Fri, Dec 18, 2015 at 04:31:13PM +0000, Peter Maydell wrote:
> >> On 18 December 2015 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
> >> > On Fri, Dec 18, 2015 at 11:59:39AM +0000, Peter Maydell wrote:
> >> >> I don't understand why we need to do this. If this is an
> >> >> AArch64 dump then we should just treat it as an AArch64
> >> >> dump, and presumably the consumer of the dump knows enough
> >> >> to know what the "hypervisor view" of a CPU that's currently
> >> >> in 32-bit mode is. It has to anyway to be able to figure
> >> >> out where all the other registers are, so why can't it
> >> >> also figure out what mode the CPU is currently in and thus
> >> >> where r13 is in the xregs array?
> >> >
> >> > You're probably right that this shouldn't be necessary. But, in order for
> >> > it not to be necessary, I'll need to write another crash patch. Currently,
> >> > if you do a dump-guest-memory on a running guest, i.e. one where the kernel
> >> > has not called panic(), and thus the cpus are actually in 32-bit usermode,
> >> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
> >> > segfaults if sp == xregs[31]. crash does properly decode the registers
> >> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
> >> > to aarch64_compat_sp here also allows crash to work properly in the non-
> >> > panic'd case.
> >>
> >> If crash segfaults then that's clearly a bug in crash...
> >> What is it expecting to see in the SP field?
> >
> > A valid stack pointer
> 
> But why? What does it do with it? (In any case a dump of a crashed
> system could have any random rubbish in SP so the tool has to handle
> it being something weird.)

The reason crash segfaults is because sp (xregs[31]) wasn't a user stack
address, and thus it expected the stack to include at least two frames,
which would mean fp would be non-zero, but it's not, and that leads to the
calculation of a bad stack pointer which then gets used as an offset into
the stack buffer, overflowing it. This is definitely a crash bug that
should be fixed. I'll report it.

Also, crash's arm64_get_dumpfile_stackframe() simply assumes sp will be
the stack pointer, and it doesn't check pstate for the MODE32 bit first.
I think it should be easy to fix this, as I only found two places that
should be changed. I'll send a patch for that.

> 
> >> > So, I could teach crash to do what I'm doing here in qemu instead, but
> >> > there's still one more reason why it may make sense to do it here. That
> >> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
> >> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
> >> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
> >> > so why not set sp to the correct userspace sp?
> >>
> >> Well, what spec are we following here? The most logical view
> >
> > With the shoehorning approach we're following the aarch64 ptrace spec,
> > but putting arm32 registers in it. We then rely on the analysis tools
> > to do the right thing when they see pstate has PSR_MODE32_BIT set, which
> > all cpsr modes do.
> 
> Right, so the analysis tool already has to cope with the MODE32 bit
> being set, and it can also figure out where the SP is (and which
> other registers are currently live for 32-bit).
> 
> > The ptrace code in the kernel would return a real aarch32 view, i.e.
> > only registers up to r15 and a cpsr. We can't do that here because we've
> > committed to an EM_AARCH64 formatted core, and we only have one type of
> > PRSTATUS note for that core type. Furthermore, we want to be able to
> > return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.
> >
> >> to me seems to be to say "you get the view of the system
> >> that you get from a JTAG debugger or a hypervisor", which
> >> is to say you see a 64-bit set of registers and it's the
> >> debugger's job to decide which bits of those might be
> >> interesting to view as 32-bit and what is actually "live"
> >> 32 bit state.
> >
> > It appears that PRSTATUS aware tools don't currently work this way.
> >
> >>
> >> From that point of view there is no valid AArch64 SP register
> >> at this point in execution (xregs[31] for QEMU would be stale
> >> state, so not a good choice I think).
> >
> > I suppose zero or all 1's are the safest choices for "undefined", but
> > being undefined actually gives us freedom to use aarch64_compat_sp as
> > well, which, to me, looks like a safer and more useful value.
> 
> It just doesn't really make sense to me to do this one bit of
> work for the debug analysis tools when they already have to know
> that 32-bit modes are special. We seem to end up with a function
> in QEMU whose only purpose is working around a bug in the thing
> consuming the coredump.

OK, I've come around to your point of view. What value would you like
me to put in sp? 0, 1's, or ??

Thanks,
drew