[Qemu-devel] [PATCH v3 0/4] little-endian dump for ppc64

[Qemu-devel] [PATCH v3 0/4] little-endian dump for ppc64

[Greg Kurz]

Hi,

Please find the latest patch set for dump-guest-memory to support
little-endian ppc64 guest kernels. The only change since the last
post is a better naming of the dump endian helpers. Since it
affects patches 1 & 2, I resend the full serie, with Alex's
Reviewed-by tags from the last review.

Cheers.

---

Bharata B Rao (3):
      dump: Make DumpState and endian conversion routines available for arch-specific dump code
      ppc64-dump: Support dump for little endian ppc64
      ppc64 dump: Set the correct endianness in ELF dump header

Greg Kurz (1):
      target-ppc: ppc can be either endian


 dump.c                      |  228 ++++++++++++++++---------------------------
 include/sysemu/dump-arch.h  |   28 +++++
 include/sysemu/dump.h       |   48 +++++++--
 stubs/dump.c                |    2 
 target-ppc/arch_dump.c      |   94 +++++++++++-------
 target-ppc/cpu-qom.h        |    1 
 target-ppc/translate_init.c |   16 +++
 7 files changed, 228 insertions(+), 189 deletions(-)
 create mode 100644 include/sysemu/dump-arch.h

-- 
Greg

[Qemu-devel] [PATCH v3 1/4] dump: Make DumpState and endian conversion routines available for arch-specific dump code

[Greg Kurz]

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

Make DumpState and endian conversion routines available for arch-specific dump
code by moving into dump.h. DumpState will be needed by arch-specific dump
code to access target endian information from DumpState->ArchDumpInfo. Also
break the dependency of dump.h from stubs/dump.c by creating a separate
dump-arch.h.

This patch doesn't change any functionality.

Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
[ rebased on top of current master branch,
  renamed endian helpers to cpu_to_dump{16,32,64},
  pass a DumpState * argument to endian helpers,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

Changes in v3:
- better taste with the endian helpers naming

 dump.c                     |  228 +++++++++++++++++---------------------------
 include/sysemu/dump-arch.h |   28 +++++
 include/sysemu/dump.h      |   48 +++++++--
 stubs/dump.c               |    2 
 4 files changed, 154 insertions(+), 152 deletions(-)
 create mode 100644 include/sysemu/dump-arch.h

diff --git a/dump.c b/dump.c
index 14b3d1d..d24b865 100644
--- a/dump.c
+++ b/dump.c
@@ -36,9 +36,9 @@
 #define ELF_MACHINE_UNAME "Unknown"
 #endif
 
-static uint16_t cpu_convert_to_target16(uint16_t val, int endian)
+uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
 {
-    if (endian == ELFDATA2LSB) {
+    if (s->dump_info.d_endian == ELFDATA2LSB) {
         val = cpu_to_le16(val);
     } else {
         val = cpu_to_be16(val);
@@ -47,9 +47,9 @@ static uint16_t cpu_convert_to_target16(uint16_t val, int endian)
     return val;
 }
 
-static uint32_t cpu_convert_to_target32(uint32_t val, int endian)
+uint32_t cpu_to_dump32(DumpState *s, uint32_t val)
 {
-    if (endian == ELFDATA2LSB) {
+    if (s->dump_info.d_endian == ELFDATA2LSB) {
         val = cpu_to_le32(val);
     } else {
         val = cpu_to_be32(val);
@@ -58,9 +58,9 @@ static uint32_t cpu_convert_to_target32(uint32_t val, int endian)
     return val;
 }
 
-static uint64_t cpu_convert_to_target64(uint64_t val, int endian)
+uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
 {
-    if (endian == ELFDATA2LSB) {
+    if (s->dump_info.d_endian == ELFDATA2LSB) {
         val = cpu_to_le64(val);
     } else {
         val = cpu_to_be64(val);
@@ -69,39 +69,6 @@ static uint64_t cpu_convert_to_target64(uint64_t val, int endian)
     return val;
 }
 
-typedef struct DumpState {
-    GuestPhysBlockList guest_phys_blocks;
-    ArchDumpInfo dump_info;
-    MemoryMappingList list;
-    uint16_t phdr_num;
-    uint32_t sh_info;
-    bool have_section;
-    bool resume;
-    ssize_t note_size;
-    hwaddr memory_offset;
-    int fd;
-
-    GuestPhysBlock *next_block;
-    ram_addr_t start;
-    bool has_filter;
-    int64_t begin;
-    int64_t length;
-    Error **errp;
-
-    uint8_t *note_buf;          /* buffer for notes */
-    size_t note_buf_offset;     /* the writing place in note_buf */
-    uint32_t nr_cpus;           /* number of guest's cpu */
-    size_t page_size;           /* guest's page size */
-    uint32_t page_shift;        /* guest's page shift */
-    uint64_t max_mapnr;         /* the biggest guest's phys-mem's number */
-    size_t len_dump_bitmap;     /* the size of the place used to store
-                                   dump_bitmap in vmcore */
-    off_t offset_dump_bitmap;   /* offset of dump_bitmap part in vmcore */
-    off_t offset_page;          /* offset of page part in vmcore */
-    size_t num_dumpable;        /* number of page that can be dumped */
-    uint32_t flag_compress;     /* indicate the compression format */
-} DumpState;
-
 static int dump_cleanup(DumpState *s)
 {
     int ret = 0;
@@ -140,29 +107,25 @@ static int write_elf64_header(DumpState *s)
 {
     Elf64_Ehdr elf_header;
     int ret;
-    int endian = s->dump_info.d_endian;
 
     memset(&elf_header, 0, sizeof(Elf64_Ehdr));
     memcpy(&elf_header, ELFMAG, SELFMAG);
     elf_header.e_ident[EI_CLASS] = ELFCLASS64;
     elf_header.e_ident[EI_DATA] = s->dump_info.d_endian;
     elf_header.e_ident[EI_VERSION] = EV_CURRENT;
-    elf_header.e_type = cpu_convert_to_target16(ET_CORE, endian);
-    elf_header.e_machine = cpu_convert_to_target16(s->dump_info.d_machine,
-                                                   endian);
-    elf_header.e_version = cpu_convert_to_target32(EV_CURRENT, endian);
-    elf_header.e_ehsize = cpu_convert_to_target16(sizeof(elf_header), endian);
-    elf_header.e_phoff = cpu_convert_to_target64(sizeof(Elf64_Ehdr), endian);
-    elf_header.e_phentsize = cpu_convert_to_target16(sizeof(Elf64_Phdr),
-                                                     endian);
-    elf_header.e_phnum = cpu_convert_to_target16(s->phdr_num, endian);
+    elf_header.e_type = cpu_to_dump16(s, ET_CORE);
+    elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
+    elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
+    elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
+    elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
+    elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
+    elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
     if (s->have_section) {
         uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->sh_info;
 
-        elf_header.e_shoff = cpu_convert_to_target64(shoff, endian);
-        elf_header.e_shentsize = cpu_convert_to_target16(sizeof(Elf64_Shdr),
-                                                         endian);
-        elf_header.e_shnum = cpu_convert_to_target16(1, endian);
+        elf_header.e_shoff = cpu_to_dump64(s, shoff);
+        elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
+        elf_header.e_shnum = cpu_to_dump16(s, 1);
     }
 
     ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
@@ -178,29 +141,25 @@ static int write_elf32_header(DumpState *s)
 {
     Elf32_Ehdr elf_header;
     int ret;
-    int endian = s->dump_info.d_endian;
 
     memset(&elf_header, 0, sizeof(Elf32_Ehdr));
     memcpy(&elf_header, ELFMAG, SELFMAG);
     elf_header.e_ident[EI_CLASS] = ELFCLASS32;
-    elf_header.e_ident[EI_DATA] = endian;
+    elf_header.e_ident[EI_DATA] = s->dump_info.d_endian;
     elf_header.e_ident[EI_VERSION] = EV_CURRENT;
-    elf_header.e_type = cpu_convert_to_target16(ET_CORE, endian);
-    elf_header.e_machine = cpu_convert_to_target16(s->dump_info.d_machine,
-                                                   endian);
-    elf_header.e_version = cpu_convert_to_target32(EV_CURRENT, endian);
-    elf_header.e_ehsize = cpu_convert_to_target16(sizeof(elf_header), endian);
-    elf_header.e_phoff = cpu_convert_to_target32(sizeof(Elf32_Ehdr), endian);
-    elf_header.e_phentsize = cpu_convert_to_target16(sizeof(Elf32_Phdr),
-                                                     endian);
-    elf_header.e_phnum = cpu_convert_to_target16(s->phdr_num, endian);
+    elf_header.e_type = cpu_to_dump16(s, ET_CORE);
+    elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
+    elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
+    elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
+    elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
+    elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
+    elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
     if (s->have_section) {
         uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->sh_info;
 
-        elf_header.e_shoff = cpu_convert_to_target32(shoff, endian);
-        elf_header.e_shentsize = cpu_convert_to_target16(sizeof(Elf32_Shdr),
-                                                         endian);
-        elf_header.e_shnum = cpu_convert_to_target16(1, endian);
+        elf_header.e_shoff = cpu_to_dump32(s, shoff);
+        elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
+        elf_header.e_shnum = cpu_to_dump16(s, 1);
     }
 
     ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
@@ -218,15 +177,14 @@ static int write_elf64_load(DumpState *s, MemoryMapping *memory_mapping,
 {
     Elf64_Phdr phdr;
     int ret;
-    int endian = s->dump_info.d_endian;
 
     memset(&phdr, 0, sizeof(Elf64_Phdr));
-    phdr.p_type = cpu_convert_to_target32(PT_LOAD, endian);
-    phdr.p_offset = cpu_convert_to_target64(offset, endian);
-    phdr.p_paddr = cpu_convert_to_target64(memory_mapping->phys_addr, endian);
-    phdr.p_filesz = cpu_convert_to_target64(filesz, endian);
-    phdr.p_memsz = cpu_convert_to_target64(memory_mapping->length, endian);
-    phdr.p_vaddr = cpu_convert_to_target64(memory_mapping->virt_addr, endian);
+    phdr.p_type = cpu_to_dump32(s, PT_LOAD);
+    phdr.p_offset = cpu_to_dump64(s, offset);
+    phdr.p_paddr = cpu_to_dump64(s, memory_mapping->phys_addr);
+    phdr.p_filesz = cpu_to_dump64(s, filesz);
+    phdr.p_memsz = cpu_to_dump64(s, memory_mapping->length);
+    phdr.p_vaddr = cpu_to_dump64(s, memory_mapping->virt_addr);
 
     assert(memory_mapping->length >= filesz);
 
@@ -245,15 +203,14 @@ static int write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
 {
     Elf32_Phdr phdr;
     int ret;
-    int endian = s->dump_info.d_endian;
 
     memset(&phdr, 0, sizeof(Elf32_Phdr));
-    phdr.p_type = cpu_convert_to_target32(PT_LOAD, endian);
-    phdr.p_offset = cpu_convert_to_target32(offset, endian);
-    phdr.p_paddr = cpu_convert_to_target32(memory_mapping->phys_addr, endian);
-    phdr.p_filesz = cpu_convert_to_target32(filesz, endian);
-    phdr.p_memsz = cpu_convert_to_target32(memory_mapping->length, endian);
-    phdr.p_vaddr = cpu_convert_to_target32(memory_mapping->virt_addr, endian);
+    phdr.p_type = cpu_to_dump32(s, PT_LOAD);
+    phdr.p_offset = cpu_to_dump32(s, offset);
+    phdr.p_paddr = cpu_to_dump32(s, memory_mapping->phys_addr);
+    phdr.p_filesz = cpu_to_dump32(s, filesz);
+    phdr.p_memsz = cpu_to_dump32(s, memory_mapping->length);
+    phdr.p_vaddr = cpu_to_dump32(s, memory_mapping->virt_addr);
 
     assert(memory_mapping->length >= filesz);
 
@@ -269,16 +226,15 @@ static int write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
 static int write_elf64_note(DumpState *s)
 {
     Elf64_Phdr phdr;
-    int endian = s->dump_info.d_endian;
     hwaddr begin = s->memory_offset - s->note_size;
     int ret;
 
     memset(&phdr, 0, sizeof(Elf64_Phdr));
-    phdr.p_type = cpu_convert_to_target32(PT_NOTE, endian);
-    phdr.p_offset = cpu_convert_to_target64(begin, endian);
+    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
+    phdr.p_offset = cpu_to_dump64(s, begin);
     phdr.p_paddr = 0;
-    phdr.p_filesz = cpu_convert_to_target64(s->note_size, endian);
-    phdr.p_memsz = cpu_convert_to_target64(s->note_size, endian);
+    phdr.p_filesz = cpu_to_dump64(s, s->note_size);
+    phdr.p_memsz = cpu_to_dump64(s, s->note_size);
     phdr.p_vaddr = 0;
 
     ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
@@ -325,15 +281,14 @@ static int write_elf32_note(DumpState *s)
 {
     hwaddr begin = s->memory_offset - s->note_size;
     Elf32_Phdr phdr;
-    int endian = s->dump_info.d_endian;
     int ret;
 
     memset(&phdr, 0, sizeof(Elf32_Phdr));
-    phdr.p_type = cpu_convert_to_target32(PT_NOTE, endian);
-    phdr.p_offset = cpu_convert_to_target32(begin, endian);
+    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
+    phdr.p_offset = cpu_to_dump32(s, begin);
     phdr.p_paddr = 0;
-    phdr.p_filesz = cpu_convert_to_target32(s->note_size, endian);
-    phdr.p_memsz = cpu_convert_to_target32(s->note_size, endian);
+    phdr.p_filesz = cpu_to_dump32(s, s->note_size);
+    phdr.p_memsz = cpu_to_dump32(s, s->note_size);
     phdr.p_vaddr = 0;
 
     ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
@@ -375,7 +330,6 @@ static int write_elf_section(DumpState *s, int type)
 {
     Elf32_Shdr shdr32;
     Elf64_Shdr shdr64;
-    int endian = s->dump_info.d_endian;
     int shdr_size;
     void *shdr;
     int ret;
@@ -383,12 +337,12 @@ static int write_elf_section(DumpState *s, int type)
     if (type == 0) {
         shdr_size = sizeof(Elf32_Shdr);
         memset(&shdr32, 0, shdr_size);
-        shdr32.sh_info = cpu_convert_to_target32(s->sh_info, endian);
+        shdr32.sh_info = cpu_to_dump32(s, s->sh_info);
         shdr = &shdr32;
     } else {
         shdr_size = sizeof(Elf64_Shdr);
         memset(&shdr64, 0, shdr_size);
-        shdr64.sh_info = cpu_convert_to_target32(s->sh_info, endian);
+        shdr64.sh_info = cpu_to_dump32(s, s->sh_info);
         shdr = &shdr64;
     }
 
@@ -796,7 +750,6 @@ static int create_header32(DumpState *s)
     DiskDumpHeader32 *dh = NULL;
     KdumpSubHeader32 *kh = NULL;
     size_t size;
-    int endian = s->dump_info.d_endian;
     uint32_t block_size;
     uint32_t sub_hdr_size;
     uint32_t bitmap_blocks;
@@ -808,18 +761,17 @@ static int create_header32(DumpState *s)
     dh = g_malloc0(size);
 
     strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
-    dh->header_version = cpu_convert_to_target32(6, endian);
+    dh->header_version = cpu_to_dump32(s, 6);
     block_size = s->page_size;
-    dh->block_size = cpu_convert_to_target32(block_size, endian);
+    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);
-    dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
+    dh->sub_hdr_size = cpu_to_dump32(s, sub_hdr_size);
     /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
-    dh->max_mapnr = cpu_convert_to_target32(MIN(s->max_mapnr, UINT_MAX),
-                                            endian);
-    dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian);
+    dh->max_mapnr = cpu_to_dump32(s, MIN(s->max_mapnr, UINT_MAX));
+    dh->nr_cpus = cpu_to_dump32(s, s->nr_cpus);
     bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, block_size) * 2;
-    dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks, endian);
+    dh->bitmap_blocks = cpu_to_dump32(s, bitmap_blocks);
     strncpy(dh->utsname.machine, ELF_MACHINE_UNAME, sizeof(dh->utsname.machine));
 
     if (s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) {
@@ -835,7 +787,7 @@ static int create_header32(DumpState *s)
         status |= DUMP_DH_COMPRESSED_SNAPPY;
     }
 #endif
-    dh->status = cpu_convert_to_target32(status, endian);
+    dh->status = cpu_to_dump32(s, status);
 
     if (write_buffer(s->fd, 0, dh, size) < 0) {
         dump_error(s, "dump: failed to write disk dump header.\n");
@@ -848,13 +800,13 @@ static int create_header32(DumpState *s)
     kh = g_malloc0(size);
 
     /* 64bit max_mapnr_64 */
-    kh->max_mapnr_64 = cpu_convert_to_target64(s->max_mapnr, endian);
-    kh->phys_base = cpu_convert_to_target32(PHYS_BASE, endian);
-    kh->dump_level = cpu_convert_to_target32(DUMP_LEVEL, endian);
+    kh->max_mapnr_64 = cpu_to_dump64(s, s->max_mapnr);
+    kh->phys_base = cpu_to_dump32(s, PHYS_BASE);
+    kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL);
 
     offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
-    kh->offset_note = cpu_convert_to_target64(offset_note, endian);
-    kh->note_size = cpu_convert_to_target32(s->note_size, endian);
+    kh->offset_note = cpu_to_dump64(s, offset_note);
+    kh->note_size = cpu_to_dump32(s, s->note_size);
 
     if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
@@ -903,7 +855,6 @@ static int create_header64(DumpState *s)
     DiskDumpHeader64 *dh = NULL;
     KdumpSubHeader64 *kh = NULL;
     size_t size;
-    int endian = s->dump_info.d_endian;
     uint32_t block_size;
     uint32_t sub_hdr_size;
     uint32_t bitmap_blocks;
@@ -915,18 +866,17 @@ static int create_header64(DumpState *s)
     dh = g_malloc0(size);
 
     strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
-    dh->header_version = cpu_convert_to_target32(6, endian);
+    dh->header_version = cpu_to_dump32(s, 6);
     block_size = s->page_size;
-    dh->block_size = cpu_convert_to_target32(block_size, endian);
+    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);
-    dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
+    dh->sub_hdr_size = cpu_to_dump32(s, sub_hdr_size);
     /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
-    dh->max_mapnr = cpu_convert_to_target32(MIN(s->max_mapnr, UINT_MAX),
-                                            endian);
-    dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian);
+    dh->max_mapnr = cpu_to_dump32(s, MIN(s->max_mapnr, UINT_MAX));
+    dh->nr_cpus = cpu_to_dump32(s, s->nr_cpus);
     bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, block_size) * 2;
-    dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks, endian);
+    dh->bitmap_blocks = cpu_to_dump32(s, bitmap_blocks);
     strncpy(dh->utsname.machine, ELF_MACHINE_UNAME, sizeof(dh->utsname.machine));
 
     if (s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) {
@@ -942,7 +892,7 @@ static int create_header64(DumpState *s)
         status |= DUMP_DH_COMPRESSED_SNAPPY;
     }
 #endif
-    dh->status = cpu_convert_to_target32(status, endian);
+    dh->status = cpu_to_dump32(s, status);
 
     if (write_buffer(s->fd, 0, dh, size) < 0) {
         dump_error(s, "dump: failed to write disk dump header.\n");
@@ -955,13 +905,13 @@ static int create_header64(DumpState *s)
     kh = g_malloc0(size);
 
     /* 64bit max_mapnr_64 */
-    kh->max_mapnr_64 = cpu_convert_to_target64(s->max_mapnr, endian);
-    kh->phys_base = cpu_convert_to_target64(PHYS_BASE, endian);
-    kh->dump_level = cpu_convert_to_target32(DUMP_LEVEL, endian);
+    kh->max_mapnr_64 = cpu_to_dump64(s, s->max_mapnr);
+    kh->phys_base = cpu_to_dump64(s, PHYS_BASE);
+    kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL);
 
     offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
-    kh->offset_note = cpu_convert_to_target64(offset_note, endian);
-    kh->note_size = cpu_convert_to_target64(s->note_size, endian);
+    kh->offset_note = cpu_to_dump64(s, offset_note);
+    kh->note_size = cpu_to_dump64(s, s->note_size);
 
     if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
@@ -1283,7 +1233,6 @@ static int write_dump_pages(DumpState *s)
     off_t offset_desc, offset_data;
     PageDescriptor pd, pd_zero;
     uint8_t *buf;
-    int endian = s->dump_info.d_endian;
     GuestPhysBlock *block_iter = NULL;
     uint64_t pfn_iter;
 
@@ -1311,10 +1260,10 @@ static int write_dump_pages(DumpState *s)
      * init zero page's page_desc and page_data, because every zero page
      * uses the same page_data
      */
-    pd_zero.size = cpu_convert_to_target32(s->page_size, endian);
-    pd_zero.flags = cpu_convert_to_target32(0, endian);
-    pd_zero.offset = cpu_convert_to_target64(offset_data, endian);
-    pd_zero.page_flags = cpu_convert_to_target64(0, endian);
+    pd_zero.size = cpu_to_dump32(s, s->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(s->page_size);
     ret = write_cache(&page_data, buf, s->page_size, false);
     g_free(buf);
@@ -1354,9 +1303,8 @@ static int write_dump_pages(DumpState *s)
              if ((s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) &&
                     (compress2(buf_out, (uLongf *)&size_out, buf, s->page_size,
                     Z_BEST_SPEED) == Z_OK) && (size_out < s->page_size)) {
-                pd.flags = cpu_convert_to_target32(DUMP_DH_COMPRESSED_ZLIB,
-                                                   endian);
-                pd.size  = cpu_convert_to_target32(size_out, endian);
+                pd.flags = cpu_to_dump32(s, DUMP_DH_COMPRESSED_ZLIB);
+                pd.size  = cpu_to_dump32(s, size_out);
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
                 if (ret < 0) {
@@ -1368,9 +1316,8 @@ static int write_dump_pages(DumpState *s)
                     (lzo1x_1_compress(buf, s->page_size, buf_out,
                     (lzo_uint *)&size_out, wrkmem) == LZO_E_OK) &&
                     (size_out < s->page_size)) {
-                pd.flags = cpu_convert_to_target32(DUMP_DH_COMPRESSED_LZO,
-                                                   endian);
-                pd.size  = cpu_convert_to_target32(size_out, endian);
+                pd.flags = cpu_to_dump32(s, DUMP_DH_COMPRESSED_LZO);
+                pd.size  = cpu_to_dump32(s, size_out);
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
                 if (ret < 0) {
@@ -1383,9 +1330,8 @@ static int write_dump_pages(DumpState *s)
                     (snappy_compress((char *)buf, s->page_size,
                     (char *)buf_out, &size_out) == SNAPPY_OK) &&
                     (size_out < s->page_size)) {
-                pd.flags = cpu_convert_to_target32(
-                                        DUMP_DH_COMPRESSED_SNAPPY, endian);
-                pd.size  = cpu_convert_to_target32(size_out, endian);
+                pd.flags = cpu_to_dump32(s, DUMP_DH_COMPRESSED_SNAPPY);
+                pd.size  = cpu_to_dump32(s, size_out);
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
                 if (ret < 0) {
@@ -1398,9 +1344,9 @@ static int write_dump_pages(DumpState *s)
                  * fall back to save in plaintext, size_out should be
                  * assigned to s->page_size
                  */
-                pd.flags = cpu_convert_to_target32(0, endian);
+                pd.flags = cpu_to_dump32(s, 0);
                 size_out = s->page_size;
-                pd.size = cpu_convert_to_target32(size_out, endian);
+                pd.size = cpu_to_dump32(s, size_out);
 
                 ret = write_cache(&page_data, buf, s->page_size, false);
                 if (ret < 0) {
@@ -1410,8 +1356,8 @@ static int write_dump_pages(DumpState *s)
             }
 
             /* get and write page desc here */
-            pd.page_flags = cpu_convert_to_target64(0, endian);
-            pd.offset = cpu_convert_to_target64(offset_data, endian);
+            pd.page_flags = cpu_to_dump64(s, 0);
+            pd.offset = cpu_to_dump64(s, offset_data);
             offset_data += size_out;
 
             ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false);
diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
new file mode 100644
index 0000000..9c95ced
--- /dev/null
+++ b/include/sysemu/dump-arch.h
@@ -0,0 +1,28 @@
+/*
+ * QEMU dump
+ *
+ * Copyright Fujitsu, Corp. 2011, 2012
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef DUMP_ARCH_H
+#define DUMP_ARCH_H
+
+typedef struct ArchDumpInfo {
+    int d_machine;  /* Architecture */
+    int d_endian;   /* ELFDATA2LSB or ELFDATA2MSB */
+    int d_class;    /* ELFCLASS32 or ELFCLASS64 */
+} ArchDumpInfo;
+
+struct GuestPhysBlockList; /* memory_mapping.h */
+int cpu_get_dump_info(ArchDumpInfo *info,
+                      const struct GuestPhysBlockList *guest_phys_blocks);
+ssize_t cpu_get_note_size(int class, int machine, int nr_cpus);
+
+#endif
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index efab7a3..e8d5a82 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -43,11 +43,8 @@
 #define PFN_BUFBITMAP               (CHAR_BIT * BUFSIZE_BITMAP)
 #define BUFSIZE_DATA_CACHE          (TARGET_PAGE_SIZE * 4)
 
-typedef struct ArchDumpInfo {
-    int d_machine;  /* Architecture */
-    int d_endian;   /* ELFDATA2LSB or ELFDATA2MSB */
-    int d_class;    /* ELFCLASS32 or ELFCLASS64 */
-} ArchDumpInfo;
+#include "sysemu/dump-arch.h"
+#include "sysemu/memory_mapping.h"
 
 typedef struct QEMU_PACKED MakedumpfileHeader {
     char signature[16];     /* = "makedumpfile" */
@@ -158,9 +155,40 @@ typedef struct QEMU_PACKED PageDescriptor {
     uint64_t page_flags;            /* page flags */
 } PageDescriptor;
 
-struct GuestPhysBlockList; /* memory_mapping.h */
-int cpu_get_dump_info(ArchDumpInfo *info,
-                      const struct GuestPhysBlockList *guest_phys_blocks);
-ssize_t cpu_get_note_size(int class, int machine, int nr_cpus);
-
+typedef struct DumpState {
+    GuestPhysBlockList guest_phys_blocks;
+    ArchDumpInfo dump_info;
+    MemoryMappingList list;
+    uint16_t phdr_num;
+    uint32_t sh_info;
+    bool have_section;
+    bool resume;
+    ssize_t note_size;
+    hwaddr memory_offset;
+    int fd;
+
+    GuestPhysBlock *next_block;
+    ram_addr_t start;
+    bool has_filter;
+    int64_t begin;
+    int64_t length;
+    Error **errp;
+
+    uint8_t *note_buf;          /* buffer for notes */
+    size_t note_buf_offset;     /* the writing place in note_buf */
+    uint32_t nr_cpus;           /* number of guest's cpu */
+    size_t page_size;           /* guest's page size */
+    uint32_t page_shift;        /* guest's page shift */
+    uint64_t max_mapnr;         /* the biggest guest's phys-mem's number */
+    size_t len_dump_bitmap;     /* the size of the place used to store
+                                   dump_bitmap in vmcore */
+    off_t offset_dump_bitmap;   /* offset of dump_bitmap part in vmcore */
+    off_t offset_page;          /* offset of page part in vmcore */
+    size_t num_dumpable;        /* number of page that can be dumped */
+    uint32_t flag_compress;     /* indicate the compression format */
+} DumpState;
+
+uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
+uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
+uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
 #endif
diff --git a/stubs/dump.c b/stubs/dump.c
index 370cd96..fac7019 100644
--- a/stubs/dump.c
+++ b/stubs/dump.c
@@ -12,7 +12,7 @@
  */
 
 #include "qemu-common.h"
-#include "sysemu/dump.h"
+#include "sysemu/dump-arch.h"
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
 

[Qemu-devel] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Greg Kurz]

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

Fix ppc64 arch specific dump code to work correctly for little endian
guests.

We introduce a NoteFuncArg type to avoid adding extra arguments to all note
functions.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
[ rebased on top of current master branch,
  introduced NoteFuncArg,
  use new cpu_to_dump{16,32,64} endian helpers,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Reviewed-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

Changes in v3:
- better taste with the endian helpers naming

 target-ppc/arch_dump.c |   82 +++++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/target-ppc/arch_dump.c b/target-ppc/arch_dump.c
index 9dccf1a..5487b61 100644
--- a/target-ppc/arch_dump.c
+++ b/target-ppc/arch_dump.c
@@ -79,94 +79,109 @@ typedef struct noteStruct {
     } contents;
 } QEMU_PACKED Note;
 
+typedef struct NoteFuncArg {
+    Note note;
+    DumpState *state;
+} NoteFuncArg;
 
-static void ppc64_write_elf64_prstatus(Note *note, PowerPCCPU *cpu)
+static void ppc64_write_elf64_prstatus(NoteFuncArg *arg, PowerPCCPU *cpu)
 {
     int i;
     uint64_t cr;
     struct PPC64ElfPrstatus *prstatus;
     struct PPC64UserRegStruct *reg;
+    Note *note = &arg->note;
+    DumpState *s = arg->state;
 
-    note->hdr.n_type = cpu_to_be32(NT_PRSTATUS);
+    note->hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
 
     prstatus = &note->contents.prstatus;
     memset(prstatus, 0, sizeof(*prstatus));
     reg = &prstatus->pr_reg;
 
     for (i = 0; i < 32; i++) {
-        reg->gpr[i] = cpu_to_be64(cpu->env.gpr[i]);
+        reg->gpr[i] = cpu_to_dump64(s, cpu->env.gpr[i]);
     }
-    reg->nip = cpu_to_be64(cpu->env.nip);
-    reg->msr = cpu_to_be64(cpu->env.msr);
-    reg->ctr = cpu_to_be64(cpu->env.ctr);
-    reg->link = cpu_to_be64(cpu->env.lr);
-    reg->xer = cpu_to_be64(cpu_read_xer(&cpu->env));
+    reg->nip = cpu_to_dump64(s, cpu->env.nip);
+    reg->msr = cpu_to_dump64(s, cpu->env.msr);
+    reg->ctr = cpu_to_dump64(s, cpu->env.ctr);
+    reg->link = cpu_to_dump64(s, cpu->env.lr);
+    reg->xer = cpu_to_dump64(s, cpu_read_xer(&cpu->env));
 
     cr = 0;
     for (i = 0; i < 8; i++) {
         cr |= (cpu->env.crf[i] & 15) << (4 * (7 - i));
     }
-    reg->ccr = cpu_to_be64(cr);
+    reg->ccr = cpu_to_dump64(s, cr);
 }
 
-static void ppc64_write_elf64_fpregset(Note *note, PowerPCCPU *cpu)
+static void ppc64_write_elf64_fpregset(NoteFuncArg *arg, PowerPCCPU *cpu)
 {
     int i;
     struct PPC64ElfFpregset  *fpregset;
+    Note *note = &arg->note;
+    DumpState *s = arg->state;
 
-    note->hdr.n_type = cpu_to_be32(NT_PRFPREG);
+    note->hdr.n_type = cpu_to_dump32(s, NT_PRFPREG);
 
     fpregset = &note->contents.fpregset;
     memset(fpregset, 0, sizeof(*fpregset));
 
     for (i = 0; i < 32; i++) {
-        fpregset->fpr[i] = cpu_to_be64(cpu->env.fpr[i]);
+        fpregset->fpr[i] = cpu_to_dump64(s, cpu->env.fpr[i]);
     }
-    fpregset->fpscr = cpu_to_be64(cpu->env.fpscr);
+    fpregset->fpscr = cpu_to_dump64(s, cpu->env.fpscr);
 }
 
-static void ppc64_write_elf64_vmxregset(Note *note, PowerPCCPU *cpu)
+static void ppc64_write_elf64_vmxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
 {
     int i;
     struct PPC64ElfVmxregset *vmxregset;
+    Note *note = &arg->note;
+    DumpState *s = arg->state;
 
-    note->hdr.n_type = cpu_to_be32(NT_PPC_VMX);
+    note->hdr.n_type = cpu_to_dump32(s, NT_PPC_VMX);
     vmxregset = &note->contents.vmxregset;
     memset(vmxregset, 0, sizeof(*vmxregset));
 
     for (i = 0; i < 32; i++) {
-        vmxregset->avr[i].u64[0] = cpu_to_be64(cpu->env.avr[i].u64[0]);
-        vmxregset->avr[i].u64[1] = cpu_to_be64(cpu->env.avr[i].u64[1]);
+        vmxregset->avr[i].u64[0] = cpu_to_dump64(s, cpu->env.avr[i].u64[0]);
+        vmxregset->avr[i].u64[1] = cpu_to_dump64(s, cpu->env.avr[i].u64[1]);
     }
-    vmxregset->vscr.u32[3] = cpu_to_be32(cpu->env.vscr);
+    vmxregset->vscr.u32[3] = cpu_to_dump32(s, cpu->env.vscr);
 }
-static void ppc64_write_elf64_vsxregset(Note *note, PowerPCCPU *cpu)
+static void ppc64_write_elf64_vsxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
 {
     int i;
     struct PPC64ElfVsxregset *vsxregset;
+    Note *note = &arg->note;
+    DumpState *s = arg->state;
 
-    note->hdr.n_type = cpu_to_be32(NT_PPC_VSX);
+    note->hdr.n_type = cpu_to_dump32(s, NT_PPC_VSX);
     vsxregset = &note->contents.vsxregset;
     memset(vsxregset, 0, sizeof(*vsxregset));
 
     for (i = 0; i < 32; i++) {
-        vsxregset->vsr[i] = cpu_to_be64(cpu->env.vsr[i]);
+        vsxregset->vsr[i] = cpu_to_dump64(s, cpu->env.vsr[i]);
     }
 }
-static void ppc64_write_elf64_speregset(Note *note, PowerPCCPU *cpu)
+static void ppc64_write_elf64_speregset(NoteFuncArg *arg, PowerPCCPU *cpu)
 {
     struct PPC64ElfSperegset *speregset;
-    note->hdr.n_type = cpu_to_be32(NT_PPC_SPE);
+    Note *note = &arg->note;
+    DumpState *s = arg->state;
+
+    note->hdr.n_type = cpu_to_dump32(s, NT_PPC_SPE);
     speregset = &note->contents.speregset;
     memset(speregset, 0, sizeof(*speregset));
 
-    speregset->spe_acc = cpu_to_be64(cpu->env.spe_acc);
-    speregset->spe_fscr = cpu_to_be32(cpu->env.spe_fscr);
+    speregset->spe_acc = cpu_to_dump64(s, cpu->env.spe_acc);
+    speregset->spe_fscr = cpu_to_dump32(s, cpu->env.spe_fscr);
 }
 
 static const struct NoteFuncDescStruct {
     int contents_size;
-    void (*note_contents_func)(Note *note, PowerPCCPU *cpu);
+    void (*note_contents_func)(NoteFuncArg *arg, PowerPCCPU *cpu);
 } note_func[] = {
     {sizeof(((Note *)0)->contents.prstatus),  ppc64_write_elf64_prstatus},
     {sizeof(((Note *)0)->contents.fpregset),  ppc64_write_elf64_fpregset},
@@ -218,20 +233,21 @@ static int ppc64_write_all_elf64_notes(const char *note_name,
                                        PowerPCCPU *cpu, int id,
                                        void *opaque)
 {
-    Note note;
+    NoteFuncArg arg = { .state = opaque };
     int ret = -1;
     int note_size;
     const NoteFuncDesc *nf;
 
     for (nf = note_func; nf->note_contents_func; nf++) {
-        note.hdr.n_namesz = cpu_to_be32(sizeof(note.name));
-        note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
-        strncpy(note.name, note_name, sizeof(note.name));
+        arg.note.hdr.n_namesz = cpu_to_dump32(opaque, sizeof(arg.note.name));
+        arg.note.hdr.n_descsz = cpu_to_dump32(opaque, nf->contents_size);
+        strncpy(arg.note.name, note_name, sizeof(arg.note.name));
 
-        (*nf->note_contents_func)(&note, cpu);
+        (*nf->note_contents_func)(&arg, cpu);
 
-        note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-        ret = f(&note, note_size, opaque);
+        note_size =
+            sizeof(arg.note) - sizeof(arg.note.contents) + nf->contents_size;
+        ret = f(&arg.note, note_size, opaque);
         if (ret < 0) {
             return -1;
         }

[Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Greg Kurz]

POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR
special purpose register to decide the endianness to use when
entering interrupt handlers. When running a Linux guest, this
provides a hint on the endianness used by the kernel. From a
QEMU point of view, the information is needed for legacy virtio
support and crash dump support as well.

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

 No changes, resent in case it helps

 target-ppc/cpu-qom.h        |    1 +
 target-ppc/translate_init.c |   16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 47dc8e6..fdce638 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -76,6 +76,7 @@ typedef struct PowerPCCPUClass {
     int (*handle_mmu_fault)(PowerPCCPU *cpu, target_ulong eaddr, int rwx,
                             int mmu_idx);
 #endif
+    bool (*interrupts_big_endian)(PowerPCCPU *cpu);
 } PowerPCCPUClass;
 
 /**
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 7f53c33..a27e5a7 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -3102,6 +3102,18 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
     return 0;
 }
 
+static bool ppc_cpu_interrupts_big_endian_always(PowerPCCPU *cpu)
+{
+    return true;
+}
+
+#ifdef TARGET_PPC64
+static bool ppc_cpu_interrupts_big_endian_lpcr(PowerPCCPU *cpu)
+{
+    return !(cpu->env.spr[SPR_LPCR] & LPCR_ILE);
+}
+#endif
+
 /*****************************************************************************/
 /* PowerPC implementations definitions                                       */
 
@@ -7089,6 +7101,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
                  POWERPC_FLAG_VSX;
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
+    pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
 }
 
 POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
@@ -7132,6 +7145,7 @@ POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
                  POWERPC_FLAG_VSX;
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
+    pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
 }
 
 static void init_proc_POWER8(CPUPPCState *env)
@@ -7189,6 +7203,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
                  POWERPC_FLAG_VSX;
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
+    pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
 }
 #endif /* defined (TARGET_PPC64) */
 
@@ -8511,6 +8526,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     pcc->parent_realize = dc->realize;
     pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
     pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK;
+    pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
     dc->realize = ppc_cpu_realizefn;
     dc->unrealize = ppc_cpu_unrealizefn;
 

[Qemu-devel] [PATCH v2 4/4] ppc64 dump: Set the correct endianness in ELF dump header

[Greg Kurz]

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
[ use PowerPCCPUClass::interrupts_big_endian(),
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Reviewed-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

No changes, resent in case it helps

 target-ppc/arch_dump.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target-ppc/arch_dump.c b/target-ppc/arch_dump.c
index 5487b61..53e2fe0 100644
--- a/target-ppc/arch_dump.c
+++ b/target-ppc/arch_dump.c
@@ -196,12 +196,16 @@ typedef struct NoteFuncDescStruct NoteFuncDesc;
 int cpu_get_dump_info(ArchDumpInfo *info,
                       const struct GuestPhysBlockList *guest_phys_blocks)
 {
-    /*
-     * Currently only handling PPC64 big endian.
-     */
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
     info->d_machine = EM_PPC64;
-    info->d_endian = ELFDATA2MSB;
     info->d_class = ELFCLASS64;
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        info->d_endian = ELFDATA2MSB;
+    } else {
+        info->d_endian = ELFDATA2LSB;
+    }
 
     return 0;
 }

Re: [Qemu-devel] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Alexander Graf]

On 05/05/2014 10:05 AM, Greg Kurz wrote:
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>
> Fix ppc64 arch specific dump code to work correctly for little endian
> guests.
>
> We introduce a NoteFuncArg type to avoid adding extra arguments to all note
> functions.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> [ rebased on top of current master branch,
>    introduced NoteFuncArg,
>    use new cpu_to_dump{16,32,64} endian helpers,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Reviewed-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>
> Changes in v3:
> - better taste with the endian helpers naming
>
>   target-ppc/arch_dump.c |   82 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 49 insertions(+), 33 deletions(-)
>
> diff --git a/target-ppc/arch_dump.c b/target-ppc/arch_dump.c
> index 9dccf1a..5487b61 100644
> --- a/target-ppc/arch_dump.c
> +++ b/target-ppc/arch_dump.c
> @@ -79,94 +79,109 @@ typedef struct noteStruct {
>       } contents;
>   } QEMU_PACKED Note;
>   
> +typedef struct NoteFuncArg {
> +    Note note;
> +    DumpState *state;
> +} NoteFuncArg;
>   
> -static void ppc64_write_elf64_prstatus(Note *note, PowerPCCPU *cpu)
> +static void ppc64_write_elf64_prstatus(NoteFuncArg *arg, PowerPCCPU *cpu)
>   {
>       int i;
>       uint64_t cr;
>       struct PPC64ElfPrstatus *prstatus;
>       struct PPC64UserRegStruct *reg;
> +    Note *note = &arg->note;
> +    DumpState *s = arg->state;
>   
> -    note->hdr.n_type = cpu_to_be32(NT_PRSTATUS);
> +    note->hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
>   
>       prstatus = &note->contents.prstatus;
>       memset(prstatus, 0, sizeof(*prstatus));
>       reg = &prstatus->pr_reg;
>   
>       for (i = 0; i < 32; i++) {
> -        reg->gpr[i] = cpu_to_be64(cpu->env.gpr[i]);
> +        reg->gpr[i] = cpu_to_dump64(s, cpu->env.gpr[i]);
>       }
> -    reg->nip = cpu_to_be64(cpu->env.nip);
> -    reg->msr = cpu_to_be64(cpu->env.msr);
> -    reg->ctr = cpu_to_be64(cpu->env.ctr);
> -    reg->link = cpu_to_be64(cpu->env.lr);
> -    reg->xer = cpu_to_be64(cpu_read_xer(&cpu->env));
> +    reg->nip = cpu_to_dump64(s, cpu->env.nip);
> +    reg->msr = cpu_to_dump64(s, cpu->env.msr);
> +    reg->ctr = cpu_to_dump64(s, cpu->env.ctr);
> +    reg->link = cpu_to_dump64(s, cpu->env.lr);
> +    reg->xer = cpu_to_dump64(s, cpu_read_xer(&cpu->env));
>   
>       cr = 0;
>       for (i = 0; i < 8; i++) {
>           cr |= (cpu->env.crf[i] & 15) << (4 * (7 - i));
>       }
> -    reg->ccr = cpu_to_be64(cr);
> +    reg->ccr = cpu_to_dump64(s, cr);
>   }
>   
> -static void ppc64_write_elf64_fpregset(Note *note, PowerPCCPU *cpu)
> +static void ppc64_write_elf64_fpregset(NoteFuncArg *arg, PowerPCCPU *cpu)
>   {
>       int i;
>       struct PPC64ElfFpregset  *fpregset;
> +    Note *note = &arg->note;
> +    DumpState *s = arg->state;
>   
> -    note->hdr.n_type = cpu_to_be32(NT_PRFPREG);
> +    note->hdr.n_type = cpu_to_dump32(s, NT_PRFPREG);
>   
>       fpregset = &note->contents.fpregset;
>       memset(fpregset, 0, sizeof(*fpregset));
>   
>       for (i = 0; i < 32; i++) {
> -        fpregset->fpr[i] = cpu_to_be64(cpu->env.fpr[i]);
> +        fpregset->fpr[i] = cpu_to_dump64(s, cpu->env.fpr[i]);
>       }
> -    fpregset->fpscr = cpu_to_be64(cpu->env.fpscr);
> +    fpregset->fpscr = cpu_to_dump64(s, cpu->env.fpscr);
>   }
>   
> -static void ppc64_write_elf64_vmxregset(Note *note, PowerPCCPU *cpu)
> +static void ppc64_write_elf64_vmxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
>   {
>       int i;
>       struct PPC64ElfVmxregset *vmxregset;
> +    Note *note = &arg->note;
> +    DumpState *s = arg->state;
>   
> -    note->hdr.n_type = cpu_to_be32(NT_PPC_VMX);
> +    note->hdr.n_type = cpu_to_dump32(s, NT_PPC_VMX);
>       vmxregset = &note->contents.vmxregset;
>       memset(vmxregset, 0, sizeof(*vmxregset));
>   
>       for (i = 0; i < 32; i++) {
> -        vmxregset->avr[i].u64[0] = cpu_to_be64(cpu->env.avr[i].u64[0]);
> -        vmxregset->avr[i].u64[1] = cpu_to_be64(cpu->env.avr[i].u64[1]);
> +        vmxregset->avr[i].u64[0] = cpu_to_dump64(s, cpu->env.avr[i].u64[0]);
> +        vmxregset->avr[i].u64[1] = cpu_to_dump64(s, cpu->env.avr[i].u64[1]);

Is this correct? Tom, could you please ack if it is?


Alex

Re: [Qemu-devel] [PATCH v3 0/4] little-endian dump for ppc64

[Alexander Graf]

On 05/05/2014 10:02 AM, Greg Kurz wrote:
> Hi,
>
> Please find the latest patch set for dump-guest-memory to support
> little-endian ppc64 guest kernels. The only change since the last
> post is a better naming of the dump endian helpers. Since it
> affects patches 1 & 2, I resend the full serie, with Alex's
> Reviewed-by tags from the last review.

I like the patch set. If I also get an ack from Tom on the data layout 
of AVR registers I'd be more than happy to apply it to my tree. I'd also 
welcome someone non-ppc to review the dump patches.


Alex

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Peter Maydell]

On 5 May 2014 09:07, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR
> special purpose register to decide the endianness to use when
> entering interrupt handlers. When running a Linux guest, this
> provides a hint on the endianness used by the kernel. From a
> QEMU point of view, the information is needed for legacy virtio
> support and crash dump support as well.

Do you care about the case of:
 * kernel bigendian
 * userspace littleendian (or vice-versa)
 * guest kernel passes virtio device through to guest userspace
 * guest userspace is doing the manipulation of the device

?

(Will Deacon just suggested this as a possibility on the
kvm-arm mailing list...)

Also, are we documenting what the process should be for a
virtio implementation to decide the endianness for a particular
architecture? I assume we'd like kvmtool and QEMU to do
the same thing rather than subtly different things...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Greg Kurz]

On Tue, 6 May 2014 19:37:22 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 May 2014 09:07, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR
> > special purpose register to decide the endianness to use when
> > entering interrupt handlers. When running a Linux guest, this
> > provides a hint on the endianness used by the kernel. From a
> > QEMU point of view, the information is needed for legacy virtio
> > support and crash dump support as well.
> 
> Do you care about the case of:
>  * kernel bigendian

Yes. FWIW, ppc64 is still widely used in big endian mode we don't
want to break.

>  * userspace littleendian (or vice-versa)

We don't care about userspace here. We assume that virtio structures are
owned by the guest kernel.

>  * guest kernel passes virtio device through to guest userspace

Not sure to understand... could you please point me to an example ?

>  * guest userspace is doing the manipulation of the device
> 

Hmm... you mean we would have virtio drivers implemented in the guest
userspace ? Does that exist ? Please elaborate.

> ?
> 
> (Will Deacon just suggested this as a possibility on the
> kvm-arm mailing list...)
> 

Just discovered some virtio endian threads in the kvm-arm@ archives...
I'll take some time to read.

> Also, are we documenting what the process should be for a
> virtio implementation to decide the endianness for a particular
> architecture? I assume we'd like kvmtool and QEMU to do
> the same thing rather than subtly different things...
> 

Sure !

> thanks
> -- PMM
> 

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Greg Kurz]

On Mon, 05 May 2014 13:04:35 +0200
Alexander Graf <agraf@suse.de> wrote:
> On 05/05/2014 10:05 AM, Greg Kurz wrote:
> > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >
> > Fix ppc64 arch specific dump code to work correctly for little endian
> > guests.
> >
> > We introduce a NoteFuncArg type to avoid adding extra arguments to all note
> > functions.
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > [ rebased on top of current master branch,
> >    introduced NoteFuncArg,
> >    use new cpu_to_dump{16,32,64} endian helpers,
> >    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Reviewed-by: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >
> > Changes in v3:
> > - better taste with the endian helpers naming
> >
> >   target-ppc/arch_dump.c |   82 +++++++++++++++++++++++++++++-------------------
> >   1 file changed, 49 insertions(+), 33 deletions(-)
> >
> > diff --git a/target-ppc/arch_dump.c b/target-ppc/arch_dump.c
> > index 9dccf1a..5487b61 100644
> > --- a/target-ppc/arch_dump.c
> > +++ b/target-ppc/arch_dump.c
> > @@ -79,94 +79,109 @@ typedef struct noteStruct {
> >       } contents;
> >   } QEMU_PACKED Note;
> >   
> > +typedef struct NoteFuncArg {
> > +    Note note;
> > +    DumpState *state;
> > +} NoteFuncArg;
> >   
> > -static void ppc64_write_elf64_prstatus(Note *note, PowerPCCPU *cpu)
> > +static void ppc64_write_elf64_prstatus(NoteFuncArg *arg, PowerPCCPU *cpu)
> >   {
> >       int i;
> >       uint64_t cr;
> >       struct PPC64ElfPrstatus *prstatus;
> >       struct PPC64UserRegStruct *reg;
> > +    Note *note = &arg->note;
> > +    DumpState *s = arg->state;
> >   
> > -    note->hdr.n_type = cpu_to_be32(NT_PRSTATUS);
> > +    note->hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
> >   
> >       prstatus = &note->contents.prstatus;
> >       memset(prstatus, 0, sizeof(*prstatus));
> >       reg = &prstatus->pr_reg;
> >   
> >       for (i = 0; i < 32; i++) {
> > -        reg->gpr[i] = cpu_to_be64(cpu->env.gpr[i]);
> > +        reg->gpr[i] = cpu_to_dump64(s, cpu->env.gpr[i]);
> >       }
> > -    reg->nip = cpu_to_be64(cpu->env.nip);
> > -    reg->msr = cpu_to_be64(cpu->env.msr);
> > -    reg->ctr = cpu_to_be64(cpu->env.ctr);
> > -    reg->link = cpu_to_be64(cpu->env.lr);
> > -    reg->xer = cpu_to_be64(cpu_read_xer(&cpu->env));
> > +    reg->nip = cpu_to_dump64(s, cpu->env.nip);
> > +    reg->msr = cpu_to_dump64(s, cpu->env.msr);
> > +    reg->ctr = cpu_to_dump64(s, cpu->env.ctr);
> > +    reg->link = cpu_to_dump64(s, cpu->env.lr);
> > +    reg->xer = cpu_to_dump64(s, cpu_read_xer(&cpu->env));
> >   
> >       cr = 0;
> >       for (i = 0; i < 8; i++) {
> >           cr |= (cpu->env.crf[i] & 15) << (4 * (7 - i));
> >       }
> > -    reg->ccr = cpu_to_be64(cr);
> > +    reg->ccr = cpu_to_dump64(s, cr);
> >   }
> >   
> > -static void ppc64_write_elf64_fpregset(Note *note, PowerPCCPU *cpu)
> > +static void ppc64_write_elf64_fpregset(NoteFuncArg *arg, PowerPCCPU *cpu)
> >   {
> >       int i;
> >       struct PPC64ElfFpregset  *fpregset;
> > +    Note *note = &arg->note;
> > +    DumpState *s = arg->state;
> >   
> > -    note->hdr.n_type = cpu_to_be32(NT_PRFPREG);
> > +    note->hdr.n_type = cpu_to_dump32(s, NT_PRFPREG);
> >   
> >       fpregset = &note->contents.fpregset;
> >       memset(fpregset, 0, sizeof(*fpregset));
> >   
> >       for (i = 0; i < 32; i++) {
> > -        fpregset->fpr[i] = cpu_to_be64(cpu->env.fpr[i]);
> > +        fpregset->fpr[i] = cpu_to_dump64(s, cpu->env.fpr[i]);
> >       }
> > -    fpregset->fpscr = cpu_to_be64(cpu->env.fpscr);
> > +    fpregset->fpscr = cpu_to_dump64(s, cpu->env.fpscr);
> >   }
> >   
> > -static void ppc64_write_elf64_vmxregset(Note *note, PowerPCCPU *cpu)
> > +static void ppc64_write_elf64_vmxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
> >   {
> >       int i;
> >       struct PPC64ElfVmxregset *vmxregset;
> > +    Note *note = &arg->note;
> > +    DumpState *s = arg->state;
> >   
> > -    note->hdr.n_type = cpu_to_be32(NT_PPC_VMX);
> > +    note->hdr.n_type = cpu_to_dump32(s, NT_PPC_VMX);
> >       vmxregset = &note->contents.vmxregset;
> >       memset(vmxregset, 0, sizeof(*vmxregset));
> >   
> >       for (i = 0; i < 32; i++) {
> > -        vmxregset->avr[i].u64[0] = cpu_to_be64(cpu->env.avr[i].u64[0]);
> > -        vmxregset->avr[i].u64[1] = cpu_to_be64(cpu->env.avr[i].u64[1]);
> > +        vmxregset->avr[i].u64[0] = cpu_to_dump64(s, cpu->env.avr[i].u64[0]);
> > +        vmxregset->avr[i].u64[1] = cpu_to_dump64(s, cpu->env.avr[i].u64[1]);
> 
> Is this correct? Tom, could you please ack if it is?
> 
> 
> Alex
> 
> 

Tom,

Can you comment plz ?

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Peter Maydell]

On 7 May 2014 09:14, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Tue, 6 May 2014 19:37:22 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 May 2014 09:07, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> > POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR
>> > special purpose register to decide the endianness to use when
>> > entering interrupt handlers. When running a Linux guest, this
>> > provides a hint on the endianness used by the kernel. From a
>> > QEMU point of view, the information is needed for legacy virtio
>> > support and crash dump support as well.
>>
>> Do you care about the case of:
>>  * kernel bigendian
>
> Yes. FWIW, ppc64 is still widely used in big endian mode we don't
> want to break.
>
>>  * userspace littleendian (or vice-versa)
>
> We don't care about userspace here. We assume that virtio structures are
> owned by the guest kernel.
>
>>  * guest kernel passes virtio device through to guest userspace
>
> Not sure to understand... could you please point me to an example ?

Consider PCI passthrough of a virtio device to userspace Linux
or to a nested KVM/QEMU instance running inside the outermost
KVM. It's a bit of an odd corner case, but we should either accommodate
it or definitely say it's not expected to work consistently across
architectures I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Alexander Graf]

On 07.05.14 10:14, Greg Kurz wrote:
> On Tue, 6 May 2014 19:37:22 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 May 2014 09:07, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>>> POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR
>>> special purpose register to decide the endianness to use when
>>> entering interrupt handlers. When running a Linux guest, this
>>> provides a hint on the endianness used by the kernel. From a
>>> QEMU point of view, the information is needed for legacy virtio
>>> support and crash dump support as well.
>> Do you care about the case of:
>>   * kernel bigendian
> Yes. FWIW, ppc64 is still widely used in big endian mode we don't
> want to break.
>
>>   * userspace littleendian (or vice-versa)
> We don't care about userspace here. We assume that virtio structures are
> owned by the guest kernel.
>
>>   * guest kernel passes virtio device through to guest userspace
> Not sure to understand... could you please point me to an example ?
>
>>   * guest userspace is doing the manipulation of the device
>>
> Hmm... you mean we would have virtio drivers implemented in the guest
> userspace ? Does that exist ? Please elaborate.

Virtio bypasses the IOMMU by design, so user space drivers don't make 
sense here :).

I don't think we should overengineer hacks for legacy virtio.


Alex

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Peter Maydell]

On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
> I don't think we should overengineer hacks for legacy virtio.

Agreed. So what's our final conclusion: virtio endianness
is the endianness of the guest kernel at the point where
it triggers a reset of the virtio device, yes?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Alexander Graf]

> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
> 
>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
>> I don't think we should overengineer hacks for legacy virtio.
> 
> Agreed. So what's our final conclusion: virtio endianness
> is the endianness of the guest kernel at the point where
> it triggers a reset of the virtio device, yes?

The interrupt endianness for book3s PPC. Since that's an arch specific thing I think we should just make the determination mechanism arch dependent and list it in the spec.

Booke for example would be vastly different since there is no global LE flag - it's a bit in the TLB entries.


Alex

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Peter Maydell]

On 7 May 2014 10:37, Alexander Graf <agraf@suse.de> wrote:
>
>
>> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>
>>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
>>> I don't think we should overengineer hacks for legacy virtio.
>>
>> Agreed. So what's our final conclusion: virtio endianness
>> is the endianness of the guest kernel at the point where
>> it triggers a reset of the virtio device, yes?
>
> The interrupt endianness for book3s PPC. Since that's an arch
> specific thing I think we should just make the determination
> mechanism arch dependent and list it in the spec.
>
> Booke for example would be vastly different since there is no
> global LE flag - it's a bit in the TLB entries.

Sure, but I think we should also state the general principle
we're aiming to implement with the arch specific detail,
so that people figuring out what a new arch should do
have a guide to follow.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Alexander Graf]

> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
> 
>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
>> I don't think we should overengineer hacks for legacy virtio.
> 
> Agreed. So what's our final conclusion: virtio endianness
> is the endianness of the guest kernel at the point where
> it triggers a reset of the virtio device, yes?

I just realized we're talking about virtio in a non-virtio thread. This patch set is about core dump support which is different from virtio bi-endian support. While both may end up at the same logic, I don't like the idea to mix them. This function is PPC internal.

Alex

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Alexander Graf]

> Am 07.05.2014 um 11:40 schrieb Peter Maydell <peter.maydell@linaro.org>:
> 
>> On 7 May 2014 10:37, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>>>> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>> 
>>>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
>>>> I don't think we should overengineer hacks for legacy virtio.
>>> 
>>> Agreed. So what's our final conclusion: virtio endianness
>>> is the endianness of the guest kernel at the point where
>>> it triggers a reset of the virtio device, yes?
>> 
>> The interrupt endianness for book3s PPC. Since that's an arch
>> specific thing I think we should just make the determination
>> mechanism arch dependent and list it in the spec.
>> 
>> Booke for example would be vastly different since there is no
>> global LE flag - it's a bit in the TLB entries.
> 
> Sure, but I think we should also state the general principle
> we're aiming to implement with the arch specific detail,
> so that people figuring out what a new arch should do
> have a guide to follow.

The general principle is "Try to find the Linux guest compile time endianness" :). 


Alex

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Greg Kurz]

On Wed, 7 May 2014 11:41:10 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> 
> > Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
> > 
> >> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
> >> I don't think we should overengineer hacks for legacy virtio.
> > 
> > Agreed. So what's our final conclusion: virtio endianness
> > is the endianness of the guest kernel at the point where
> > it triggers a reset of the virtio device, yes?
> 
> I just realized we're talking about virtio in a non-virtio thread. This patch set is about core dump support which is different from virtio bi-endian support. While both may end up at the same logic, I don't like the idea to mix them. This function is PPC internal.
> 
> Alex
> 

Correct and now I have this feeling about using LPCR_ILE versus MSR_LE...

LPCR_ILE reflects the interrupt vector endianness. It is set during early boot
by the guest kernel according to the desired endianness. MSR_LE gives the
current endian mode for the cpu.

The idea is that you need to rely on LPCR_ILE when you peek from the host
because you lack context and MSR_LE may be have been temporarily changed.
This is clearly the case for dump support.

Now when it comes to virtio, we cache the endianness at device reset time: MSR_LE from
current_cpu should reflect the guest kernel endianness, no ?

In this case we could end up like what's being currently discussed with ARM:

http://www.spinics.net/lists/kvm-arm/msg09099.html
http://www.spinics.net/lists/kvm-arm/msg09091.html

Alex,

If we agree that current_cpu->MSR_LE does the job when the guest kernel resets
the device, then I guess we don't even need this patch...

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Alexander Graf]

On 05/07/2014 12:19 PM, Greg Kurz wrote:
> On Wed, 7 May 2014 11:41:10 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>>
>>> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>
>>>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
>>>> I don't think we should overengineer hacks for legacy virtio.
>>> Agreed. So what's our final conclusion: virtio endianness
>>> is the endianness of the guest kernel at the point where
>>> it triggers a reset of the virtio device, yes?
>> I just realized we're talking about virtio in a non-virtio thread. This patch set is about core dump support which is different from virtio bi-endian support. While both may end up at the same logic, I don't like the idea to mix them. This function is PPC internal.
>>
>> Alex
>>
> Correct and now I have this feeling about using LPCR_ILE versus MSR_LE...
>
> LPCR_ILE reflects the interrupt vector endianness. It is set during early boot
> by the guest kernel according to the desired endianness. MSR_LE gives the
> current endian mode for the cpu.
>
> The idea is that you need to rely on LPCR_ILE when you peek from the host
> because you lack context and MSR_LE may be have been temporarily changed.
> This is clearly the case for dump support.
>
> Now when it comes to virtio, we cache the endianness at device reset time: MSR_LE from
> current_cpu should reflect the guest kernel endianness, no ?
>
> In this case we could end up like what's being currently discussed with ARM:
>
> http://www.spinics.net/lists/kvm-arm/msg09099.html
> http://www.spinics.net/lists/kvm-arm/msg09091.html
>
> Alex,
>
> If we agree that current_cpu->MSR_LE does the job when the guest kernel resets
> the device, then I guess we don't even need this patch...

Either one works for me as long as we put it into a spec. No solution 
will be able to fulfill all cases.

The uglyness about the current_cpu bit is that devices are usually not 
supposed to know about the cpu accesses come from usually. But then 
again devices shouldn't know about the endianness of a cpu either so I 
guess it's ok to breach layers here.

Rusty, do you have strong feelings either way?


Alex

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Greg Kurz]

On Wed, 07 May 2014 13:54:36 +0200
Alexander Graf <agraf@suse.de> wrote:
> On 05/07/2014 12:19 PM, Greg Kurz wrote:
> > On Wed, 7 May 2014 11:41:10 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >>
> >>> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
> >>>
> >>>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
> >>>> I don't think we should overengineer hacks for legacy virtio.
> >>> Agreed. So what's our final conclusion: virtio endianness
> >>> is the endianness of the guest kernel at the point where
> >>> it triggers a reset of the virtio device, yes?
> >> I just realized we're talking about virtio in a non-virtio thread. This patch set is about core dump support which is different from virtio bi-endian support. While both may end up at the same logic, I don't like the idea to mix them. This function is PPC internal.
> >>
> >> Alex
> >>
> > Correct and now I have this feeling about using LPCR_ILE versus MSR_LE...
> >
> > LPCR_ILE reflects the interrupt vector endianness. It is set during early boot
> > by the guest kernel according to the desired endianness. MSR_LE gives the
> > current endian mode for the cpu.
> >
> > The idea is that you need to rely on LPCR_ILE when you peek from the host
> > because you lack context and MSR_LE may be have been temporarily changed.
> > This is clearly the case for dump support.
> >
> > Now when it comes to virtio, we cache the endianness at device reset time: MSR_LE from
> > current_cpu should reflect the guest kernel endianness, no ?
> >
> > In this case we could end up like what's being currently discussed with ARM:
> >
> > http://www.spinics.net/lists/kvm-arm/msg09099.html
> > http://www.spinics.net/lists/kvm-arm/msg09091.html
> >
> > Alex,
> >
> > If we agree that current_cpu->MSR_LE does the job when the guest kernel resets
> > the device, then I guess we don't even need this patch...
> 
> Either one works for me as long as we put it into a spec. No solution 
> will be able to fulfill all cases.
> 
> The uglyness about the current_cpu bit is that devices are usually not 
> supposed to know about the cpu accesses come from usually. But then 
> again devices shouldn't know about the endianness of a cpu either so I 
> guess it's ok to breach layers here.
> 
> Rusty, do you have strong feelings either way?
> 
> 
> Alex
> 

Coming back to the initial subject. What about changing the last sentence
in the commit message to the following ?

"And when it comes to dumping a guest, the information is needed to write
 ELF headers using the kernel endianness."

This would make the patch a PPC64 dump only business and end the debate. :)

Tell me if you want me to repost.

Cheers.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Alexander Graf]

On 05/07/2014 02:40 PM, Greg Kurz wrote:
> On Wed, 07 May 2014 13:54:36 +0200
> Alexander Graf <agraf@suse.de> wrote:
>> On 05/07/2014 12:19 PM, Greg Kurz wrote:
>>> On Wed, 7 May 2014 11:41:10 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>>
>>>>> Am 07.05.2014 um 11:26 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>>>
>>>>>> On 7 May 2014 10:09, Alexander Graf <agraf@suse.de> wrote:
>>>>>> I don't think we should overengineer hacks for legacy virtio.
>>>>> Agreed. So what's our final conclusion: virtio endianness
>>>>> is the endianness of the guest kernel at the point where
>>>>> it triggers a reset of the virtio device, yes?
>>>> I just realized we're talking about virtio in a non-virtio thread. This patch set is about core dump support which is different from virtio bi-endian support. While both may end up at the same logic, I don't like the idea to mix them. This function is PPC internal.
>>>>
>>>> Alex
>>>>
>>> Correct and now I have this feeling about using LPCR_ILE versus MSR_LE...
>>>
>>> LPCR_ILE reflects the interrupt vector endianness. It is set during early boot
>>> by the guest kernel according to the desired endianness. MSR_LE gives the
>>> current endian mode for the cpu.
>>>
>>> The idea is that you need to rely on LPCR_ILE when you peek from the host
>>> because you lack context and MSR_LE may be have been temporarily changed.
>>> This is clearly the case for dump support.
>>>
>>> Now when it comes to virtio, we cache the endianness at device reset time: MSR_LE from
>>> current_cpu should reflect the guest kernel endianness, no ?
>>>
>>> In this case we could end up like what's being currently discussed with ARM:
>>>
>>> http://www.spinics.net/lists/kvm-arm/msg09099.html
>>> http://www.spinics.net/lists/kvm-arm/msg09091.html
>>>
>>> Alex,
>>>
>>> If we agree that current_cpu->MSR_LE does the job when the guest kernel resets
>>> the device, then I guess we don't even need this patch...
>> Either one works for me as long as we put it into a spec. No solution
>> will be able to fulfill all cases.
>>
>> The uglyness about the current_cpu bit is that devices are usually not
>> supposed to know about the cpu accesses come from usually. But then
>> again devices shouldn't know about the endianness of a cpu either so I
>> guess it's ok to breach layers here.
>>
>> Rusty, do you have strong feelings either way?
>>
>>
>> Alex
>>
> Coming back to the initial subject. What about changing the last sentence
> in the commit message to the following ?
>
> "And when it comes to dumping a guest, the information is needed to write
>   ELF headers using the kernel endianness."
>
> This would make the patch a PPC64 dump only business and end the debate. :)
>
> Tell me if you want me to repost.

Let's wait for Tom's ack first - or if you can show me that you fully 
grasped that we're doing the "right thing" in all host/guest big/little 
combinations with AVX registers I'm happy too :)


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Tom Musta]

On 5/7/2014 3:20 AM, Greg Kurz wrote:
> On Mon, 05 May 2014 13:04:35 +0200
> Alexander Graf <agraf@suse.de> wrote:
>> On 05/05/2014 10:05 AM, Greg Kurz wrote:
>>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>
>>> Fix ppc64 arch specific dump code to work correctly for little endian
>>> guests.
>>>
>>> We introduce a NoteFuncArg type to avoid adding extra arguments to all note
>>> functions.
>>>
>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
[snip]

>>> -static void ppc64_write_elf64_vmxregset(Note *note, PowerPCCPU *cpu)
>>> +static void ppc64_write_elf64_vmxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
>>>   {
>>>       int i;
>>>       struct PPC64ElfVmxregset *vmxregset;
>>> +    Note *note = &arg->note;
>>> +    DumpState *s = arg->state;
>>>   
>>> -    note->hdr.n_type = cpu_to_be32(NT_PPC_VMX);
>>> +    note->hdr.n_type = cpu_to_dump32(s, NT_PPC_VMX);
>>>       vmxregset = &note->contents.vmxregset;
>>>       memset(vmxregset, 0, sizeof(*vmxregset));
>>>   
>>>       for (i = 0; i < 32; i++) {
>>> -        vmxregset->avr[i].u64[0] = cpu_to_be64(cpu->env.avr[i].u64[0]);
>>> -        vmxregset->avr[i].u64[1] = cpu_to_be64(cpu->env.avr[i].u64[1]);
>>> +        vmxregset->avr[i].u64[0] = cpu_to_dump64(s, cpu->env.avr[i].u64[0]);
>>> +        vmxregset->avr[i].u64[1] = cpu_to_dump64(s, cpu->env.avr[i].u64[1]);
>>
>> Is this correct? Tom, could you please ack if it is?
>>
>>
>> Alex
>>
>>
> 
> Tom,
> 
> Can you comment plz ?
> 
> Thanks.
> 

Is the avr element in the dump supposed to represent a 128-bit value?

Or maybe I should ask a more fundamental question:  should a dump of a BE guest
be identical regardless of whether it is a BE host or LE host.  Ditto for an
LE guest.

It feels like there is an endianness issue here but I have not yet been able
to put my finger on it.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Tom Musta]

On 5/7/2014 2:02 PM, Tom Musta wrote:

> It feels like there is an endianness issue here but I have not yet been able
> to put my finger on it.

OK ... after more thought and scribbling ... here is what I mean ....

Suppose I have a 64-bit value 0xa0a1a2a3a4a5a6a7 stored in guest memory and it
gets loaded into a GPR.  If I follow the dump code and view all four combinations
of guest/host big/little endian, I convince myself that the big endian guest
code writes the byte sequence 0xa0, 0xa1, 0xa2, ..., 0xa7 into the file.  And
the little endian guest dumps contain the sequence 0xa7, 0xa6, ..., 0xa0.

This make sense ... the endianness indicated in the dump header and the endianness of
the dump file layout are consistent, irrespective of the host endianness.

If I take this a step further and consider a 128-bit value 0xa0a1a2a3a4a5a6a7a8a9aaabacadaeaf
stored in guest memory and look at the AVR structure (via printf or debugger) after doing
a 128-bit lvx load, I get the following:

  +------+-------+------------------+------------------+---------------------+
  | Host | Guest | avr.u64[0]       | avr.u64[1]       | file sequence       |
  +------+-------+------------------+------------------+---------------------+
  | BE   | BE    | a0a1a2a3a4a5a6a7 | a8a9aaabacadaeaf | a0,...,a7,a8,...,af |
  | LE   | BE    | a8a9aaabacadaeaf | a0a1a2a3a4a5a6a7 | a8,...,af,a0,...,a7 |
  | BE   | LE    | a0a1a2a3a4a5a6a7 | a8a9aaabacadaeaf | a7,...,a0,af,...,a8 |
  | LE   | LE    | a8a9aaabacadaeaf | a0a1a2a3a4a5a6a7 | af,...,a8,a7,...,a0 |
  +------+-------+------------------+------------------+---------------------+

The last column represents how I think the proposed dump code will write bytes
to disk.  Notice that if you invert the (64-bit) array elements, then the two
BE dumps look alike and the two LE dumps look alike.  If you swap array u64
elements on LE hosts, and also swap on LE guests, then you get a byte sequence
that looks like a 128-bit integer in all cases.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Alexander Graf]

On 05/07/2014 10:54 PM, Tom Musta wrote:
> On 5/7/2014 2:02 PM, Tom Musta wrote:
>
>> It feels like there is an endianness issue here but I have not yet been able
>> to put my finger on it.
> OK ... after more thought and scribbling ... here is what I mean ....
>
> Suppose I have a 64-bit value 0xa0a1a2a3a4a5a6a7 stored in guest memory and it
> gets loaded into a GPR.  If I follow the dump code and view all four combinations
> of guest/host big/little endian, I convince myself that the big endian guest
> code writes the byte sequence 0xa0, 0xa1, 0xa2, ..., 0xa7 into the file.  And
> the little endian guest dumps contain the sequence 0xa7, 0xa6, ..., 0xa0.
>
> This make sense ... the endianness indicated in the dump header and the endianness of
> the dump file layout are consistent, irrespective of the host endianness.
>
> If I take this a step further and consider a 128-bit value 0xa0a1a2a3a4a5a6a7a8a9aaabacadaeaf
> stored in guest memory and look at the AVR structure (via printf or debugger) after doing
> a 128-bit lvx load, I get the following:
>
>    +------+-------+------------------+------------------+---------------------+
>    | Host | Guest | avr.u64[0]       | avr.u64[1]       | file sequence       |
>    +------+-------+------------------+------------------+---------------------+
>    | BE   | BE    | a0a1a2a3a4a5a6a7 | a8a9aaabacadaeaf | a0,...,a7,a8,...,af |
>    | LE   | BE    | a8a9aaabacadaeaf | a0a1a2a3a4a5a6a7 | a8,...,af,a0,...,a7 |
>    | BE   | LE    | a0a1a2a3a4a5a6a7 | a8a9aaabacadaeaf | a7,...,a0,af,...,a8 |
>    | LE   | LE    | a8a9aaabacadaeaf | a0a1a2a3a4a5a6a7 | af,...,a8,a7,...,a0 |
>    +------+-------+------------------+------------------+---------------------+
>
> The last column represents how I think the proposed dump code will write bytes
> to disk.  Notice that if you invert the (64-bit) array elements, then the two
> BE dumps look alike and the two LE dumps look alike.  If you swap array u64
> elements on LE hosts, and also swap on LE guests, then you get a byte sequence
> that looks like a 128-bit integer in all cases.

Since we're dumping an ELF core dump the big question is what gdb wants 
to load :)


Alex

Re: [Qemu-devel] [PATCH v3 0/4] little-endian dump for ppc64

[Andreas Färber]

Am 05.05.2014 10:02, schrieb Greg Kurz:
> Bharata B Rao (3):
>       dump: Make DumpState and endian conversion routines available for arch-specific dump code
>       ppc64-dump: Support dump for little endian ppc64
>       ppc64 dump: Set the correct endianness in ELF dump header

Both 2/4 and 4/4 touch target-ppc/arch_dump.c exclusively.
Please don't invent random topics, in particular not inconsistently. Use
target-ppc (QEMU scheme) or PPC (Alex' Linux'ish scheme), cf.
http://git.qemu-project.org/?p=qemu.git;a=history;f=target-ppc;hb=HEAD

I'm sure Alex can edit it if you're finally reaching consensus.

Regards,
Andreas

> 
> Greg Kurz (1):
>       target-ppc: ppc can be either endian

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

Re: [Qemu-devel] [PATCH v3 3/4] target-ppc: ppc can be either endian

[Rusty Russell]

Alexander Graf <agraf@suse.de> writes:
> On 05/07/2014 12:19 PM, Greg Kurz wrote:
> The uglyness about the current_cpu bit is that devices are usually not 
> supposed to know about the cpu accesses come from usually. But then 
> again devices shouldn't know about the endianness of a cpu either so I 
> guess it's ok to breach layers here.
>
> Rusty, do you have strong feelings either way?

My only strong feeling is that qemu is horrible to get patches into.

At this point, I really don't care :(

Rusty.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 2/4] ppc64-dump: Support dump for little endian ppc64

[Greg Kurz]

On Wed, 07 May 2014 15:54:10 -0500
Tom Musta <tommusta@gmail.com> wrote:

> On 5/7/2014 2:02 PM, Tom Musta wrote:
> 
> > It feels like there is an endianness issue here but I have not yet been able
> > to put my finger on it.
> 
> OK ... after more thought and scribbling ... here is what I mean ....
> 
> Suppose I have a 64-bit value 0xa0a1a2a3a4a5a6a7 stored in guest memory and it
> gets loaded into a GPR.  If I follow the dump code and view all four combinations
> of guest/host big/little endian, I convince myself that the big endian guest
> code writes the byte sequence 0xa0, 0xa1, 0xa2, ..., 0xa7 into the file.  And
> the little endian guest dumps contain the sequence 0xa7, 0xa6, ..., 0xa0.
> 
> This make sense ... the endianness indicated in the dump header and the endianness of
> the dump file layout are consistent, irrespective of the host endianness.
> 
> If I take this a step further and consider a 128-bit value 0xa0a1a2a3a4a5a6a7a8a9aaabacadaeaf
> stored in guest memory and look at the AVR structure (via printf or debugger) after doing
> a 128-bit lvx load, I get the following:
> 
>   +------+-------+------------------+------------------+---------------------+
>   | Host | Guest | avr.u64[0]       | avr.u64[1]       | file sequence       |
>   +------+-------+------------------+------------------+---------------------+
>   | BE   | BE    | a0a1a2a3a4a5a6a7 | a8a9aaabacadaeaf | a0,...,a7,a8,...,af |
>   | LE   | BE    | a8a9aaabacadaeaf | a0a1a2a3a4a5a6a7 | a8,...,af,a0,...,a7 |
>   | BE   | LE    | a0a1a2a3a4a5a6a7 | a8a9aaabacadaeaf | a7,...,a0,af,...,a8 |
>   | LE   | LE    | a8a9aaabacadaeaf | a0a1a2a3a4a5a6a7 | af,...,a8,a7,...,a0 |
>   +------+-------+------------------+------------------+---------------------+
> 
> The last column represents how I think the proposed dump code will write bytes
> to disk.  Notice that if you invert the (64-bit) array elements, then the two
> BE dumps look alike and the two LE dumps look alike.  If you swap array u64
> elements on LE hosts, and also swap on LE guests, then you get a byte sequence
> that looks like a 128-bit integer in all cases.
> 
> 

Then we already have an issue with the current code...

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.