[PATCH v5 00/18] dump: Add arch section and s390x PV dump

[PATCH v5 00/18] dump: Add arch section and s390x PV dump

[Janosch Frank]

Previously this series was two separate series:
 * Arch section support
   Adds the possibility for arch code to add custom section data.

 * s390 PV dump support
   Adds PV dump data to the custom arch sections.

I've chosen to merge them so it's easier to understand why the arch
section support has been implement the way it is.

Additionally I've added cleanup patches beforehand which clean up the
GuestPhysBlock usage.

v5:
	* Added a patch that moves the DumpState typedef and replaces
          the opaque pointers with properly typed ones
	* Removed the ELF header allocation since the codes has
          changed in a way that it's not needed anymore
	* Changed naming of dump_get_memblock_*() to dump_filter_memblock_*()
	* Removed various inline functions
	* Added a re-work of the filter variables

v4:
	* Moved the ELF note type introduction to the header sync
	* Split the iteration re-work into more patches
	* Added missing Rev-bys
	* Moved the introduction of section_offset to the patch where it's first used
	* Removed the buffer from prepare_elf_section_hdr_zero()
	* Removed buff argument from prepare_elf_section_hdr_zero()
	* Renamed some of the pv functions


Janosch Frank (18):
  dump: Replace opaque DumpState pointer with a typed one
  dump: Rename write_elf_loads to write_elf_phdr_loads
  dump: Refactor dump_iterate and introduce dump_filter_memblock_*()
  dump: Rework get_start_block
  dump: Rework filter area variables
  dump: Rework dump_calculate_size function
  dump: Split elf header functions into prepare and write
  dump: Rename write_elf*_phdr_note to prepare_elf*_phdr_note
  dump: Use a buffer for ELF section data and headers
  dump: Reorder struct DumpState
  dump: Swap segment and section header locations
  dump/dump: Add section string table support
  dump/dump: Add arch section support
  DRAFT: linux header sync
  s390x: Add protected dump cap
  s390x: Introduce PV query interface
  s390x: Add KVM PV dump interface
  s390x: pv: Add dump support

 dump/dump.c                      | 541 +++++++++++++++++++++----------
 hw/s390x/pv.c                    | 112 +++++++
 hw/s390x/s390-virtio-ccw.c       |   6 +
 include/elf.h                    |   2 +
 include/hw/core/sysemu-cpu-ops.h |   8 +-
 include/hw/s390x/pv.h            |  18 +
 include/qemu/typedefs.h          |   1 +
 include/sysemu/dump-arch.h       |   3 +
 include/sysemu/dump.h            |  36 +-
 linux-headers/linux/kvm.h        |  54 +++
 target/arm/arch_dump.c           |   6 +-
 target/arm/cpu.h                 |   4 +-
 target/i386/arch_dump.c          |  30 +-
 target/i386/cpu.h                |   8 +-
 target/ppc/arch_dump.c           |  18 +-
 target/ppc/cpu.h                 |   4 +-
 target/riscv/arch_dump.c         |   6 +-
 target/riscv/cpu.h               |   4 +-
 target/s390x/arch_dump.c         | 250 ++++++++++++--
 target/s390x/kvm/kvm.c           |   7 +
 target/s390x/kvm/kvm_s390x.h     |   1 +
 target/s390x/s390x-internal.h    |   2 +-
 22 files changed, 858 insertions(+), 263 deletions(-)

-- 
2.34.1

[PATCH v5 01/18] dump: Replace opaque DumpState pointer with a typed one

[Janosch Frank]

It's always better to convey the type of a pointer if at all
possible. So let's add the DumpState typedef to typedefs.h and move
the dump note functions from the opaque pointers to DumpState
pointers.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Cédric Le Goater <clg@kaod.org>
CC: Daniel Henrique Barboza <danielhb413@gmail.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Greg Kurz <groug@kaod.org>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Alistair Francis <alistair.francis@wdc.com>
CC: Bin Meng <bin.meng@windriver.com>
CC: Cornelia Huck <cohuck@redhat.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Richard Henderson <richard.henderson@linaro.org>
CC: David Hildenbrand <david@redhat.com>
---
 include/hw/core/sysemu-cpu-ops.h |  8 ++++----
 include/qemu/typedefs.h          |  1 +
 target/arm/arch_dump.c           |  6 ++----
 target/arm/cpu.h                 |  4 ++--
 target/i386/arch_dump.c          | 30 +++++++++++++++---------------
 target/i386/cpu.h                |  8 ++++----
 target/ppc/arch_dump.c           | 18 +++++++++---------
 target/ppc/cpu.h                 |  4 ++--
 target/riscv/arch_dump.c         |  6 ++----
 target/riscv/cpu.h               |  4 ++--
 target/s390x/arch_dump.c         | 10 +++++-----
 target/s390x/s390x-internal.h    |  2 +-
 12 files changed, 49 insertions(+), 52 deletions(-)

diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index a9ba39e5f2..ee169b872c 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -53,25 +53,25 @@ typedef struct SysemuCPUOps {
      * 32-bit VM coredump.
      */
     int (*write_elf32_note)(WriteCoreDumpFunction f, CPUState *cpu,
-                            int cpuid, void *opaque);
+                            int cpuid, DumpState *s);
     /**
      * @write_elf64_note: Callback for writing a CPU-specific ELF note to a
      * 64-bit VM coredump.
      */
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
-                            int cpuid, void *opaque);
+                            int cpuid, DumpState *s);
     /**
      * @write_elf32_qemunote: Callback for writing a CPU- and QEMU-specific ELF
      * note to a 32-bit VM coredump.
      */
     int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
-                                void *opaque);
+                                DumpState *s);
     /**
      * @write_elf64_qemunote: Callback for writing a CPU- and QEMU-specific ELF
      * note to a 64-bit VM coredump.
      */
     int (*write_elf64_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
-                                void *opaque);
+                                DumpState *s);
     /**
      * @virtio_is_big_endian: Callback to return %true if a CPU which supports
      * runtime configurable endianness is currently big-endian.
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 42f4ceb701..054fd46fa6 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -129,6 +129,7 @@ typedef struct VirtIODevice VirtIODevice;
 typedef struct Visitor Visitor;
 typedef struct VMChangeStateEntry VMChangeStateEntry;
 typedef struct VMStateDescription VMStateDescription;
+typedef struct DumpState DumpState;
 
 /*
  * Pointer types
diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c
index b1f040e69f..2d8e41ab8a 100644
--- a/target/arm/arch_dump.c
+++ b/target/arm/arch_dump.c
@@ -232,12 +232,11 @@ static int aarch64_write_elf64_sve(WriteCoreDumpFunction f,
 #endif
 
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                             int cpuid, void *opaque)
+                             int cpuid, DumpState *s)
 {
     struct aarch64_note note;
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    DumpState *s = opaque;
     uint64_t pstate, sp;
     int ret, i;
 
@@ -360,12 +359,11 @@ static int arm_write_elf32_vfp(WriteCoreDumpFunction f, CPUARMState *env,
 }
 
 int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                             int cpuid, void *opaque)
+                             int cpuid, DumpState *s)
 {
     struct arm_note note;
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    DumpState *s = opaque;
     int ret, i;
     bool fpvalid = cpu_isar_feature(aa32_vfp_simd, cpu);
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5168e3d837..fc8b358779 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1100,9 +1100,9 @@ int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
 const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname);
 
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                             int cpuid, void *opaque);
+                             int cpuid, DumpState *s);
 int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                             int cpuid, void *opaque);
+                             int cpuid, DumpState *s);
 
 #ifdef TARGET_AARCH64
 int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
index 004141fc04..c290910a04 100644
--- a/target/i386/arch_dump.c
+++ b/target/i386/arch_dump.c
@@ -42,7 +42,7 @@ typedef struct {
 
 static int x86_64_write_elf64_note(WriteCoreDumpFunction f,
                                    CPUX86State *env, int id,
-                                   void *opaque)
+                                   DumpState *s)
 {
     x86_64_user_regs_struct regs;
     Elf64_Nhdr *note;
@@ -94,7 +94,7 @@ static int x86_64_write_elf64_note(WriteCoreDumpFunction f,
     buf += descsz - sizeof(x86_64_user_regs_struct)-sizeof(target_ulong);
     memcpy(buf, &regs, sizeof(x86_64_user_regs_struct));
 
-    ret = f(note, note_size, opaque);
+    ret = f(note, note_size, s);
     g_free(note);
     if (ret < 0) {
         return -1;
@@ -148,7 +148,7 @@ static void x86_fill_elf_prstatus(x86_elf_prstatus *prstatus, CPUX86State *env,
 }
 
 static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
-                                int id, void *opaque)
+                                int id, DumpState *s)
 {
     x86_elf_prstatus prstatus;
     Elf64_Nhdr *note;
@@ -170,7 +170,7 @@ static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
     buf += ROUND_UP(name_size, 4);
     memcpy(buf, &prstatus, sizeof(prstatus));
 
-    ret = f(note, note_size, opaque);
+    ret = f(note, note_size, s);
     g_free(note);
     if (ret < 0) {
         return -1;
@@ -180,7 +180,7 @@ static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
 }
 
 int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                             int cpuid, void *opaque)
+                             int cpuid, DumpState *s)
 {
     X86CPU *cpu = X86_CPU(cs);
     int ret;
@@ -189,10 +189,10 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
     bool lma = !!(first_x86_cpu->env.hflags & HF_LMA_MASK);
 
     if (lma) {
-        ret = x86_64_write_elf64_note(f, &cpu->env, cpuid, opaque);
+        ret = x86_64_write_elf64_note(f, &cpu->env, cpuid, s);
     } else {
 #endif
-        ret = x86_write_elf64_note(f, &cpu->env, cpuid, opaque);
+        ret = x86_write_elf64_note(f, &cpu->env, cpuid, s);
 #ifdef TARGET_X86_64
     }
 #endif
@@ -201,7 +201,7 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
 }
 
 int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                             int cpuid, void *opaque)
+                             int cpuid, DumpState *s)
 {
     X86CPU *cpu = X86_CPU(cs);
     x86_elf_prstatus prstatus;
@@ -224,7 +224,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
     buf += ROUND_UP(name_size, 4);
     memcpy(buf, &prstatus, sizeof(prstatus));
 
-    ret = f(note, note_size, opaque);
+    ret = f(note, note_size, s);
     g_free(note);
     if (ret < 0) {
         return -1;
@@ -329,7 +329,7 @@ static void qemu_get_cpustate(QEMUCPUState *s, CPUX86State *env)
 
 static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
                                       CPUX86State *env,
-                                      void *opaque,
+                                      DumpState *s,
                                       int type)
 {
     QEMUCPUState state;
@@ -369,7 +369,7 @@ static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
     buf += ROUND_UP(name_size, 4);
     memcpy(buf, &state, sizeof(state));
 
-    ret = f(note, note_size, opaque);
+    ret = f(note, note_size, s);
     g_free(note);
     if (ret < 0) {
         return -1;
@@ -379,19 +379,19 @@ static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
 }
 
 int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cs,
-                                 void *opaque)
+                                 DumpState *s)
 {
     X86CPU *cpu = X86_CPU(cs);
 
-    return cpu_write_qemu_note(f, &cpu->env, opaque, 1);
+    return cpu_write_qemu_note(f, &cpu->env, s, 1);
 }
 
 int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cs,
-                                 void *opaque)
+                                 DumpState *s)
 {
     X86CPU *cpu = X86_CPU(cs);
 
-    return cpu_write_qemu_note(f, &cpu->env, opaque, 0);
+    return cpu_write_qemu_note(f, &cpu->env, s, 0);
 }
 
 int cpu_get_dump_info(ArchDumpInfo *info,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 82004b65b9..b75108d6a3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1938,13 +1938,13 @@ extern const VMStateDescription vmstate_x86_cpu;
 int x86_cpu_pending_interrupt(CPUState *cs, int interrupt_request);
 
 int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cpu,
-                             int cpuid, void *opaque);
+                             int cpuid, DumpState *s);
 int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cpu,
-                             int cpuid, void *opaque);
+                             int cpuid, DumpState *s);
 int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
-                                 void *opaque);
+                                 DumpState *s);
 int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
-                                 void *opaque);
+                                 DumpState *s);
 
 void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
                                 Error **errp);
diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
index 1139cead9f..f58e6359d5 100644
--- a/target/ppc/arch_dump.c
+++ b/target/ppc/arch_dump.c
@@ -270,23 +270,23 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
 static int ppc_write_all_elf_notes(const char *note_name,
                                    WriteCoreDumpFunction f,
                                    PowerPCCPU *cpu, int id,
-                                   void *opaque)
+                                   DumpState *s)
 {
-    NoteFuncArg arg = { .state = opaque };
+    NoteFuncArg arg = { .state = s };
     int ret = -1;
     int note_size;
     const NoteFuncDesc *nf;
 
     for (nf = note_func; nf->note_contents_func; nf++) {
-        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);
+        arg.note.hdr.n_namesz = cpu_to_dump32(s, sizeof(arg.note.name));
+        arg.note.hdr.n_descsz = cpu_to_dump32(s, nf->contents_size);
         strncpy(arg.note.name, note_name, sizeof(arg.note.name));
 
         (*nf->note_contents_func)(&arg, cpu);
 
         note_size =
             sizeof(arg.note) - sizeof(arg.note.contents) + nf->contents_size;
-        ret = f(&arg.note, note_size, opaque);
+        ret = f(&arg.note, note_size, s);
         if (ret < 0) {
             return -1;
         }
@@ -295,15 +295,15 @@ static int ppc_write_all_elf_notes(const char *note_name,
 }
 
 int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque)
+                               int cpuid, DumpState *s)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, opaque);
+    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, s);
 }
 
 int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque)
+                               int cpuid, DumpState *s)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, opaque);
+    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, s);
 }
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index a4c893cfad..f7d28fe093 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1354,9 +1354,9 @@ void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
 const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
 #endif
 int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque);
+                               int cpuid, DumpState *s);
 int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque);
+                               int cpuid, DumpState *s);
 #ifndef CONFIG_USER_ONLY
 void ppc_cpu_do_interrupt(CPUState *cpu);
 bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
diff --git a/target/riscv/arch_dump.c b/target/riscv/arch_dump.c
index 709f621d82..736a232956 100644
--- a/target/riscv/arch_dump.c
+++ b/target/riscv/arch_dump.c
@@ -64,12 +64,11 @@ static void riscv64_note_init(struct riscv64_note *note, DumpState *s,
 }
 
 int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque)
+                               int cpuid, DumpState *s)
 {
     struct riscv64_note note;
     RISCVCPU *cpu = RISCV_CPU(cs);
     CPURISCVState *env = &cpu->env;
-    DumpState *s = opaque;
     int ret, i = 0;
     const char name[] = "CORE";
 
@@ -134,12 +133,11 @@ static void riscv32_note_init(struct riscv32_note *note, DumpState *s,
 }
 
 int riscv_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque)
+                               int cpuid, DumpState *s)
 {
     struct riscv32_note note;
     RISCVCPU *cpu = RISCV_CPU(cs);
     CPURISCVState *env = &cpu->env;
-    DumpState *s = opaque;
     int ret, i;
     const char name[] = "CORE";
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5c7acc055a..9699504721 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -505,9 +505,9 @@ extern const char * const riscv_fpr_regnames[];
 const char *riscv_cpu_get_trap_name(target_ulong cause, bool async);
 void riscv_cpu_do_interrupt(CPUState *cpu);
 int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque);
+                               int cpuid, DumpState *s);
 int riscv_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
-                               int cpuid, void *opaque);
+                               int cpuid, DumpState *s);
 int riscv_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 int riscv_cpu_hviprio_index2irq(int index, int *out_irq, int *out_rdzero);
diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 08daf93ae1..f60a14920d 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -204,7 +204,7 @@ static const NoteFuncDesc note_linux[] = {
 static int s390x_write_elf64_notes(const char *note_name,
                                        WriteCoreDumpFunction f,
                                        S390CPU *cpu, int id,
-                                       void *opaque,
+                                       DumpState *s,
                                        const NoteFuncDesc *funcs)
 {
     Note note;
@@ -222,7 +222,7 @@ static int s390x_write_elf64_notes(const char *note_name,
         (*nf->note_contents_func)(&note, cpu, id);
 
         note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-        ret = f(&note, note_size, opaque);
+        ret = f(&note, note_size, s);
 
         if (ret < 0) {
             return -1;
@@ -235,16 +235,16 @@ static int s390x_write_elf64_notes(const char *note_name,
 
 
 int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                              int cpuid, void *opaque)
+                              int cpuid, DumpState *s)
 {
     S390CPU *cpu = S390_CPU(cs);
     int r;
 
-    r = s390x_write_elf64_notes("CORE", f, cpu, cpuid, opaque, note_core);
+    r = s390x_write_elf64_notes("CORE", f, cpu, cpuid, s, note_core);
     if (r) {
         return r;
     }
-    return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, opaque, note_linux);
+    return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
 }
 
 int cpu_get_dump_info(ArchDumpInfo *info,
diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 6aba7fd0ca..b5ae0ae364 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -227,7 +227,7 @@ static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
 
 /* arch_dump.c */
 int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
-                              int cpuid, void *opaque);
+                              int cpuid, DumpState *s);
 
 
 /* cc_helper.c */
-- 
2.34.1

[PATCH v5 02/18] dump: Rename write_elf_loads to write_elf_phdr_loads

[Janosch Frank]

Let's make it a bit clearer that we write the program headers of the
PT_LOAD type.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@ibm.linux.com>
---
 dump/dump.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 4d9658ffa2..0ed7cf9c7b 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -490,7 +490,7 @@ static void get_offset_range(hwaddr phys_addr,
     }
 }
 
-static void write_elf_loads(DumpState *s, Error **errp)
+static void write_elf_phdr_loads(DumpState *s, Error **errp)
 {
     ERRP_GUARD();
     hwaddr offset, filesz;
@@ -573,8 +573,8 @@ static void dump_begin(DumpState *s, Error **errp)
         return;
     }
 
-    /* write all PT_LOAD to vmcore */
-    write_elf_loads(s, errp);
+    /* write all PT_LOADs to vmcore */
+    write_elf_phdr_loads(s, errp);
     if (*errp) {
         return;
     }
-- 
2.34.1

[PATCH v5 03/18] dump: Refactor dump_iterate and introduce dump_filter_memblock_*()

[Janosch Frank]

The iteration over the memblocks in dump_iterate() is hard to
understand so it's about time to clean it up. Instead of manually
grabbing the next memblock we can use QTAILQ_FOREACH to iterate over
all memblocks.

Additionally we move the calculation of the offset and length out by
introducing and using the dump_filter_memblock_*() functions. These
functions will later be used to cleanup other parts of dump.c.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 dump/dump.c | 80 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 35 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 0ed7cf9c7b..340de5a1e7 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -591,31 +591,43 @@ static void dump_begin(DumpState *s, Error **errp)
     write_elf_notes(s, errp);
 }
 
-static int get_next_block(DumpState *s, GuestPhysBlock *block)
+static int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
+                                           int64_t filter_area_start,
+                                           int64_t filter_area_length)
 {
-    while (1) {
-        block = QTAILQ_NEXT(block, next);
-        if (!block) {
-            /* no more block */
-            return 1;
-        }
+    int64_t size, left, right;
 
-        s->start = 0;
-        s->next_block = block;
-        if (s->has_filter) {
-            if (block->target_start >= s->begin + s->length ||
-                block->target_end <= s->begin) {
-                /* This block is out of the range */
-                continue;
-            }
-
-            if (s->begin > block->target_start) {
-                s->start = s->begin - block->target_start;
-            }
-        }
-
-        return 0;
+    /* No filter, return full size */
+    if (!filter_area_length) {
+        return block->target_end - block->target_start;
     }
+
+    /* calculate the overlapped region. */
+    left = MAX(filter_area_start, block->target_start);
+    right = MIN(filter_area_start + filter_area_length, block->target_end);
+    size = right - left;
+    size = size > 0 ? size : 0;
+
+    return size;
+}
+
+static int64_t dump_filtered_memblock_start(GuestPhysBlock *block,
+                                            int64_t filter_area_start,
+                                            int64_t filter_area_length)
+{
+    if (filter_area_length) {
+        /* return -1 if the block is not within filter area */
+        if (block->target_start >= filter_area_start + filter_area_length ||
+            block->target_end <= filter_area_start) {
+            return -1;
+        }
+
+        if (filter_area_start > block->target_start) {
+            return filter_area_start - block->target_start;
+        }
+    }
+
+    return 0;
 }
 
 /* write all memory to vmcore */
@@ -623,24 +635,22 @@ static void dump_iterate(DumpState *s, Error **errp)
 {
     ERRP_GUARD();
     GuestPhysBlock *block;
-    int64_t size;
+    int64_t memblock_size, memblock_start;
 
-    do {
-        block = s->next_block;
-
-        size = block->target_end - block->target_start;
-        if (s->has_filter) {
-            size -= s->start;
-            if (s->begin + s->length < block->target_end) {
-                size -= block->target_end - (s->begin + s->length);
-            }
+    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
+        memblock_start = dump_filtered_memblock_start(block, s->begin, s->length);
+        if (memblock_start == -1) {
+            continue;
         }
-        write_memory(s, block, s->start, size, errp);
+
+        memblock_size = dump_filtered_memblock_size(block, s->begin, s->length);
+
+        /* Write the memory to file */
+        write_memory(s, block, memblock_start, memblock_size, errp);
         if (*errp) {
             return;
         }
-
-    } while (!get_next_block(s, block));
+    }
 }
 
 static void create_vmcore(DumpState *s, Error **errp)
-- 
2.34.1

[PATCH v5 04/18] dump: Rework get_start_block

[Janosch Frank]

get_start_block() returns the start address of the first memory block
or -1.

With the GuestPhysBlock iterator conversion we don't need to set the
start address and can therefore remove that code and the "start"
DumpState struct member. The only functionality left is the validation
of the start block so it only makes sense to re-name the function to
validate_start_block()

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 dump/dump.c           | 20 ++++++--------------
 include/sysemu/dump.h |  2 --
 2 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 340de5a1e7..e204912a89 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1500,30 +1500,22 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
     }
 }
 
-static ram_addr_t get_start_block(DumpState *s)
+static int validate_start_block(DumpState *s)
 {
     GuestPhysBlock *block;
 
     if (!s->has_filter) {
-        s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         return 0;
     }
 
     QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
+        /* This block is out of the range */
         if (block->target_start >= s->begin + s->length ||
             block->target_end <= s->begin) {
-            /* This block is out of the range */
             continue;
         }
-
-        s->next_block = block;
-        if (s->begin > block->target_start) {
-            s->start = s->begin - block->target_start;
-        } else {
-            s->start = 0;
-        }
-        return s->start;
-    }
+        return 0;
+   }
 
     return -1;
 }
@@ -1670,8 +1662,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         goto cleanup;
     }
 
-    s->start = get_start_block(s);
-    if (s->start == -1) {
+    /* Is the filter filtering everything? */
+    if (validate_start_block(s) == -1) {
         error_setg(errp, QERR_INVALID_PARAMETER, "begin");
         goto cleanup;
     }
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index ffc2ea1072..7fce1d4af6 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -166,8 +166,6 @@ typedef struct DumpState {
     hwaddr memory_offset;
     int fd;
 
-    GuestPhysBlock *next_block;
-    ram_addr_t start;
     bool has_filter;
     int64_t begin;
     int64_t length;
-- 
2.34.1

[PATCH v5 05/18] dump: Rework filter area variables

[Janosch Frank]

While the DumpState begin and length variables directly mirror the API
variable names they are not very descriptive. So let's add a
"filter_area_" prefix and make has_filter a function checking length > 0.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 53 +++++++++++++++++++++++++------------------
 include/sysemu/dump.h | 13 ++++++++---
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index e204912a89..b043337bc7 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -59,6 +59,11 @@ static inline bool dump_is_64bit(DumpState *s)
     return s->dump_info.d_class == ELFCLASS64;
 }
 
+static inline bool dump_has_filter(DumpState *s)
+{
+    return s->filter_area_length > 0;
+}
+
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
 {
     if (s->dump_info.d_endian == ELFDATA2LSB) {
@@ -443,29 +448,30 @@ static void get_offset_range(hwaddr phys_addr,
     *p_offset = -1;
     *p_filesz = 0;
 
-    if (s->has_filter) {
-        if (phys_addr < s->begin || phys_addr >= s->begin + s->length) {
+    if (dump_has_filter(s)) {
+        if (phys_addr < s->filter_area_begin ||
+            phys_addr >= s->filter_area_begin + s->filter_area_length) {
             return;
         }
     }
 
     QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
-        if (s->has_filter) {
-            if (block->target_start >= s->begin + s->length ||
-                block->target_end <= s->begin) {
+        if (dump_has_filter(s)) {
+            if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
+                block->target_end <= s->filter_area_begin) {
                 /* This block is out of the range */
                 continue;
             }
 
-            if (s->begin <= block->target_start) {
+            if (s->filter_area_begin <= block->target_start) {
                 start = block->target_start;
             } else {
-                start = s->begin;
+                start = s->filter_area_begin;
             }
 
             size_in_block = block->target_end - start;
-            if (s->begin + s->length < block->target_end) {
-                size_in_block -= block->target_end - (s->begin + s->length);
+            if (s->filter_area_begin + s->filter_area_length < block->target_end) {
+                size_in_block -= block->target_end - (s->filter_area_begin + s->filter_area_length);
             }
         } else {
             start = block->target_start;
@@ -638,12 +644,12 @@ static void dump_iterate(DumpState *s, Error **errp)
     int64_t memblock_size, memblock_start;
 
     QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
-        memblock_start = dump_filtered_memblock_start(block, s->begin, s->length);
+        memblock_start = dump_filtered_memblock_start(block, s->filter_area_begin, s->filter_area_length);
         if (memblock_start == -1) {
             continue;
         }
 
-        memblock_size = dump_filtered_memblock_size(block, s->begin, s->length);
+        memblock_size = dump_filtered_memblock_size(block, s->filter_area_begin, s->filter_area_length);
 
         /* Write the memory to file */
         write_memory(s, block, memblock_start, memblock_size, errp);
@@ -1504,14 +1510,14 @@ static int validate_start_block(DumpState *s)
 {
     GuestPhysBlock *block;
 
-    if (!s->has_filter) {
+    if (!dump_has_filter(s)) {
         return 0;
     }
 
     QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
         /* This block is out of the range */
-        if (block->target_start >= s->begin + s->length ||
-            block->target_end <= s->begin) {
+        if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
+            block->target_end <= s->filter_area_begin) {
             continue;
         }
         return 0;
@@ -1550,10 +1556,10 @@ static int64_t dump_calculate_size(DumpState *s)
     int64_t size = 0, total = 0, left = 0, right = 0;
 
     QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
-        if (s->has_filter) {
+        if (dump_has_filter(s)) {
             /* calculate the overlapped region. */
-            left = MAX(s->begin, block->target_start);
-            right = MIN(s->begin + s->length, block->target_end);
+            left = MAX(s->filter_area_begin, block->target_start);
+            right = MIN(s->filter_area_begin + s->filter_area_length, block->target_end);
             size = right - left;
             size = size > 0 ? size : 0;
         } else {
@@ -1643,9 +1649,12 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     }
 
     s->fd = fd;
-    s->has_filter = has_filter;
-    s->begin = begin;
-    s->length = length;
+    if (has_filter && !length) {
+        error_setg(errp, QERR_INVALID_PARAMETER, "length");
+        goto cleanup;
+    }
+    s->filter_area_begin = begin;
+    s->filter_area_length = length;
 
     memory_mapping_list_init(&s->list);
 
@@ -1778,8 +1787,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         return;
     }
 
-    if (s->has_filter) {
-        memory_mapping_filter(&s->list, s->begin, s->length);
+    if (dump_has_filter(s)) {
+        memory_mapping_filter(&s->list, s->filter_area_begin, s->filter_area_length);
     }
 
     /*
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7fce1d4af6..b62513d87d 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -166,9 +166,16 @@ typedef struct DumpState {
     hwaddr memory_offset;
     int fd;
 
-    bool has_filter;
-    int64_t begin;
-    int64_t length;
+    /*
+     * Dump filter area variables
+     *
+     * A filtered dump only contains the guest memory designated by
+     * the start address and length variables defined below.
+     *
+     * If length is 0, no filtering is applied.
+     */
+    int64_t filter_area_begin;  /* Start address of partial guest memory area */
+    int64_t filter_area_length; /* Length of partial guest memory area */
 
     uint8_t *note_buf;          /* buffer for notes */
     size_t note_buf_offset;     /* the writing place in note_buf */
-- 
2.34.1

[PATCH v5 06/18] dump: Rework dump_calculate_size function

[Janosch Frank]

dump_calculate_size() sums up all the sizes of the guest memory
blocks. Since we already have a function that calculates the size of a
single memory block (dump_get_memblock_size()) we can simply iterate
over the blocks and use the function instead of calculating the size
ourselves.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 dump/dump.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index b043337bc7..d82cc46d7d 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1548,25 +1548,19 @@ bool qemu_system_dump_in_progress(void)
     return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE);
 }
 
-/* calculate total size of memory to be dumped (taking filter into
- * acoount.) */
+/*
+ * calculate total size of memory to be dumped (taking filter into
+ * account.)
+ */
 static int64_t dump_calculate_size(DumpState *s)
 {
     GuestPhysBlock *block;
-    int64_t size = 0, total = 0, left = 0, right = 0;
+    int64_t total = 0;
 
     QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
-        if (dump_has_filter(s)) {
-            /* calculate the overlapped region. */
-            left = MAX(s->filter_area_begin, block->target_start);
-            right = MIN(s->filter_area_begin + s->filter_area_length, block->target_end);
-            size = right - left;
-            size = size > 0 ? size : 0;
-        } else {
-            /* count the whole region in */
-            size = (block->target_end - block->target_start);
-        }
-        total += size;
+        total += dump_filtered_memblock_size(block,
+                                             s->filter_area_begin,
+                                             s->filter_area_length);
     }
 
     return total;
-- 
2.34.1

[PATCH v5 07/18] dump: Split elf header functions into prepare and write

[Janosch Frank]

Let's split the write from the modification of the elf header so we
can consolidate the write of the data in one function.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c | 100 ++++++++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 47 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index d82cc46d7d..8a2a97a85e 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -131,7 +131,7 @@ static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
     return 0;
 }
 
-static void write_elf64_header(DumpState *s, Error **errp)
+static void prepare_elf64_header(DumpState *s, Elf64_Ehdr *elf_header)
 {
     /*
      * phnum in the elf header is 16 bit, if we have more segments we
@@ -139,34 +139,27 @@ static void write_elf64_header(DumpState *s, Error **errp)
      * special section.
      */
     uint16_t phnum = MIN(s->phdr_num, PN_XNUM);
-    Elf64_Ehdr elf_header;
-    int ret;
 
-    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_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, s->phdr_offset);
-    elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
-    elf_header.e_phnum = cpu_to_dump16(s, phnum);
+    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_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, s->phdr_offset);
+    elf_header->e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
+    elf_header->e_phnum = cpu_to_dump16(s, phnum);
     if (s->shdr_num) {
-        elf_header.e_shoff = cpu_to_dump64(s, s->shdr_offset);
-        elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
-        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
-    }
-
-    ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "dump: failed to write elf header");
+        elf_header->e_shoff = cpu_to_dump64(s, s->shdr_offset);
+        elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
+        elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num);
     }
 }
 
-static void write_elf32_header(DumpState *s, Error **errp)
+static void prepare_elf32_header(DumpState *s, Elf32_Ehdr *elf_header)
 {
     /*
      * phnum in the elf header is 16 bit, if we have more segments we
@@ -174,28 +167,45 @@ static void write_elf32_header(DumpState *s, Error **errp)
      * special section.
      */
     uint16_t phnum = MIN(s->phdr_num, PN_XNUM);
-    Elf32_Ehdr elf_header;
+
+    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] = s->dump_info.d_endian;
+    elf_header->e_ident[EI_VERSION] = EV_CURRENT;
+    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, s->phdr_offset);
+    elf_header->e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
+    elf_header->e_phnum = cpu_to_dump16(s, phnum);
+    if (s->shdr_num) {
+        elf_header->e_shoff = cpu_to_dump32(s, s->shdr_offset);
+        elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
+        elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num);
+    }
+}
+
+static void write_elf_header(DumpState *s, Error **errp)
+{
+    Elf32_Ehdr elf32_header;
+    Elf64_Ehdr elf64_header;
+    size_t header_size;
+    void *header_ptr;
     int ret;
 
-    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] = s->dump_info.d_endian;
-    elf_header.e_ident[EI_VERSION] = EV_CURRENT;
-    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, s->phdr_offset);
-    elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
-    elf_header.e_phnum = cpu_to_dump16(s, phnum);
-    if (s->shdr_num) {
-        elf_header.e_shoff = cpu_to_dump32(s, s->shdr_offset);
-        elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
-        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
+    if (dump_is_64bit(s)) {
+        prepare_elf64_header(s, &elf64_header);
+        header_size = sizeof(elf64_header);
+        header_ptr = &elf64_header;
+    } else {
+        prepare_elf32_header(s, &elf32_header);
+        header_size = sizeof(elf32_header);
+        header_ptr = &elf32_header;
     }
 
-    ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
+    ret = fd_write_vmcore(header_ptr, header_size, s);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "dump: failed to write elf header");
     }
@@ -564,11 +574,7 @@ static void dump_begin(DumpState *s, Error **errp)
      */
 
     /* write elf header to vmcore */
-    if (dump_is_64bit(s)) {
-        write_elf64_header(s, errp);
-    } else {
-        write_elf32_header(s, errp);
-    }
+    write_elf_header(s, errp);
     if (*errp) {
         return;
     }
-- 
2.34.1

[PATCH v5 08/18] dump: Rename write_elf*_phdr_note to prepare_elf*_phdr_note

[Janosch Frank]

The functions in question do not actually write to the file descriptor
they set up a buffer which is later written to the fd.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 8a2a97a85e..a905316fe5 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -260,7 +260,7 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
     }
 }
 
-static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
+static void prepare_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
 {
     memset(phdr, 0, sizeof(*phdr));
     phdr->p_type = cpu_to_dump32(s, PT_NOTE);
@@ -316,7 +316,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
     write_guest_note(f, s, errp);
 }
 
-static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
+static void prepare_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
 {
     memset(phdr, 0, sizeof(*phdr));
     phdr->p_type = cpu_to_dump32(s, PT_NOTE);
@@ -364,11 +364,11 @@ static void write_elf_phdr_note(DumpState *s, Error **errp)
     int ret;
 
     if (dump_is_64bit(s)) {
-        write_elf64_phdr_note(s, &phdr64);
+        prepare_elf64_phdr_note(s, &phdr64);
         size = sizeof(phdr64);
         phdr = &phdr64;
     } else {
-        write_elf32_phdr_note(s, &phdr32);
+        prepare_elf32_phdr_note(s, &phdr32);
         size = sizeof(phdr32);
         phdr = &phdr32;
     }
-- 
2.34.1

[PATCH v5 09/18] dump: Use a buffer for ELF section data and headers

[Janosch Frank]

Currently we're writing the NULL section header if we overflow the
physical header number in the ELF header. But in the future we'll add
custom section headers AND section data.

To facilitate this we need to rearange section handling a bit. As with
the other ELF headers we split the code into a prepare and a write
step.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 83 +++++++++++++++++++++++++++++--------------
 include/sysemu/dump.h |  2 ++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index a905316fe5..0051c71d08 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error **errp)
     }
 }
 
-static void write_elf_section(DumpState *s, int type, Error **errp)
+static void prepare_elf_section_hdr_zero(DumpState *s)
 {
-    Elf32_Shdr shdr32;
-    Elf64_Shdr shdr64;
-    int shdr_size;
-    void *shdr;
+    if (dump_is_64bit(s)) {
+        Elf64_Shdr *shdr64 = s->elf_section_hdrs;
+
+        shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
+    } else {
+        Elf32_Shdr *shdr32 = s->elf_section_hdrs;
+
+        shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
+    }
+}
+
+static void prepare_elf_section_hdrs(DumpState *s)
+{
+    size_t len, sizeof_shdr;
+
+    /*
+     * Section ordering:
+     * - HDR zero
+     */
+    sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
+    len = sizeof_shdr * s->shdr_num;
+    s->elf_section_hdrs = g_malloc0(len);
+
+    /*
+     * The first section header is ALWAYS a special initial section
+     * header.
+     *
+     * The header should be 0 with one exception being that if
+     * phdr_num is PN_XNUM then the sh_info field contains the real
+     * number of segment entries.
+     *
+     * As we zero allocate the buffer we will only need to modify
+     * sh_info for the PN_XNUM case.
+     */
+    if (s->phdr_num >= PN_XNUM) {
+        prepare_elf_section_hdr_zero(s);
+    }
+}
+
+static void write_elf_section_headers(DumpState *s, Error **errp)
+{
+    size_t sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
     int ret;
 
-    if (type == 0) {
-        shdr_size = sizeof(Elf32_Shdr);
-        memset(&shdr32, 0, shdr_size);
-        shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
-        shdr = &shdr32;
-    } else {
-        shdr_size = sizeof(Elf64_Shdr);
-        memset(&shdr64, 0, shdr_size);
-        shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
-        shdr = &shdr64;
-    }
+    prepare_elf_section_hdrs(s);
 
-    ret = fd_write_vmcore(shdr, shdr_size, s);
+    ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s);
     if (ret < 0) {
-        error_setg_errno(errp, -ret,
-                         "dump: failed to write section header table");
+        error_setg_errno(errp, -ret, "dump: failed to write section headers");
     }
 }
 
@@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp)
         return;
     }
 
+    /* write section headers to vmcore */
+    write_elf_section_headers(s, errp);
+    if (*errp) {
+        return;
+    }
+
     /* write PT_NOTE to vmcore */
     write_elf_phdr_note(s, errp);
     if (*errp) {
@@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp)
         return;
     }
 
-    /* write section to vmcore */
-    if (s->shdr_num) {
-        write_elf_section(s, 1, errp);
-        if (*errp) {
-            return;
-        }
-    }
-
     /* write notes to vmcore */
     write_elf_notes(s, errp);
 }
@@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp)
         return;
     }
 
+    /* Iterate over memory and dump it to file */
     dump_iterate(s, errp);
+    if (*errp) {
+        return;
+    }
 }
 
 static int write_start_flat_header(int fd)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index b62513d87d..9995f65dc8 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -177,6 +177,8 @@ typedef struct DumpState {
     int64_t filter_area_begin;  /* Start address of partial guest memory area */
     int64_t filter_area_length; /* Length of partial guest memory area */
 
+    void *elf_section_hdrs;     /* Pointer to section header buffer */
+
     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 */
-- 
2.34.1

[PATCH v5 10/18] dump: Reorder struct DumpState

[Janosch Frank]

Let's move ELF related members into one block and guest memory related
ones into another to improve readability.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/dump.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 9995f65dc8..9ed811b313 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -154,15 +154,8 @@ typedef struct DumpState {
     GuestPhysBlockList guest_phys_blocks;
     ArchDumpInfo dump_info;
     MemoryMappingList list;
-    uint32_t phdr_num;
-    uint32_t shdr_num;
     bool resume;
     bool detached;
-    ssize_t note_size;
-    hwaddr shdr_offset;
-    hwaddr phdr_offset;
-    hwaddr section_offset;
-    hwaddr note_offset;
     hwaddr memory_offset;
     int fd;
 
@@ -177,6 +170,15 @@ typedef struct DumpState {
     int64_t filter_area_begin;  /* Start address of partial guest memory area */
     int64_t filter_area_length; /* Length of partial guest memory area */
 
+    /* Elf dump related data */
+    uint32_t phdr_num;
+    uint32_t shdr_num;
+    ssize_t note_size;
+    hwaddr shdr_offset;
+    hwaddr phdr_offset;
+    hwaddr section_offset;
+    hwaddr note_offset;
+
     void *elf_section_hdrs;     /* Pointer to section header buffer */
 
     uint8_t *note_buf;          /* buffer for notes */
-- 
2.34.1

[PATCH v5 11/18] dump: Swap segment and section header locations

[Janosch Frank]

For the upcoming string table and arch section support we need to
modify the elf layout a bit. Instead of the segments, i.e. the guest's
memory contents, being the last area the section data will live at
the end of the file. This will allow us to write the section data
after all guest memory has been dumped which is important for the s390
PV dump support.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 dump/dump.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 0051c71d08..31eb20108c 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -595,6 +595,9 @@ static void dump_begin(DumpState *s, Error **errp)
      *   --------------
      *   |  memory     |
      *   --------------
+     *   |  sectn data |
+     *   --------------
+
      *
      * we only know where the memory is saved after we write elf note into
      * vmcore.
@@ -1842,17 +1845,16 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     }
 
     if (dump_is_64bit(s)) {
-        s->phdr_offset = sizeof(Elf64_Ehdr);
-        s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
-        s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
-        s->memory_offset = s->note_offset + s->note_size;
+        s->shdr_offset = sizeof(Elf64_Ehdr);
+        s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
+        s->note_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
     } else {
-
-        s->phdr_offset = sizeof(Elf32_Ehdr);
-        s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
-        s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
-        s->memory_offset = s->note_offset + s->note_size;
+        s->shdr_offset = sizeof(Elf32_Ehdr);
+        s->phdr_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
+        s->note_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
     }
+    s->memory_offset = s->note_offset + s->note_size;
+    s->section_offset = s->memory_offset + s->total_size;
 
     return;
 
-- 
2.34.1

[PATCH v5 12/18] dump/dump: Add section string table support

[Janosch Frank]

As sections don't have a type like the notes do we need another way to
determine their contents. The string table allows us to assign each
section an identification string which architectures can then use to
tag their sections with.

There will be no string table if the architecture doesn't add custom
sections which are introduced in a following patch.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 71 +++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/dump.h |  4 +++
 2 files changed, 75 insertions(+)

diff --git a/dump/dump.c b/dump/dump.c
index 31eb20108c..0d6dbf453a 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -103,6 +103,7 @@ static int dump_cleanup(DumpState *s)
     memory_mapping_list_free(&s->list);
     close(s->fd);
     g_free(s->guest_note);
+    g_array_unref(s->string_table_buf);
     s->guest_note = NULL;
     if (s->resume) {
         if (s->detached) {
@@ -156,6 +157,9 @@ static void prepare_elf64_header(DumpState *s, Elf64_Ehdr *elf_header)
         elf_header->e_shoff = cpu_to_dump64(s, s->shdr_offset);
         elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
         elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num);
+        if (s->string_table_usage) {
+            elf_header->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
+        }
     }
 }
 
@@ -184,6 +188,9 @@ static void prepare_elf32_header(DumpState *s, Elf32_Ehdr *elf_header)
         elf_header->e_shoff = cpu_to_dump32(s, s->shdr_offset);
         elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
         elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num);
+        if (s->string_table_usage) {
+            elf_header->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
+        }
     }
 }
 
@@ -393,17 +400,50 @@ static void prepare_elf_section_hdr_zero(DumpState *s)
     }
 }
 
+static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
+{
+    Elf32_Shdr shdr32;
+    Elf64_Shdr shdr64;
+    int shdr_size;
+    void *shdr;
+
+    if (dump_is_64bit(s)) {
+        shdr_size = sizeof(Elf64_Shdr);
+        memset(&shdr64, 0, shdr_size);
+        shdr64.sh_type = SHT_STRTAB;
+        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
+        shdr64.sh_name = s->string_table_buf->len;
+        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
+        shdr64.sh_size = s->string_table_buf->len;
+        shdr = &shdr64;
+    } else {
+        shdr_size = sizeof(Elf32_Shdr);
+        memset(&shdr32, 0, shdr_size);
+        shdr32.sh_type = SHT_STRTAB;
+        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
+        shdr32.sh_name = s->string_table_buf->len;
+        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
+        shdr32.sh_size = s->string_table_buf->len;
+        shdr = &shdr32;
+    }
+
+    memcpy(buff, shdr, shdr_size);
+}
+
 static void prepare_elf_section_hdrs(DumpState *s)
 {
     size_t len, sizeof_shdr;
+    void *buff_hdr;
 
     /*
      * Section ordering:
      * - HDR zero
+     * - String table hdr
      */
     sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
     len = sizeof_shdr * s->shdr_num;
     s->elf_section_hdrs = g_malloc0(len);
+    buff_hdr = s->elf_section_hdrs;
 
     /*
      * The first section header is ALWAYS a special initial section
@@ -419,6 +459,17 @@ static void prepare_elf_section_hdrs(DumpState *s)
     if (s->phdr_num >= PN_XNUM) {
         prepare_elf_section_hdr_zero(s);
     }
+    buff_hdr += sizeof_shdr;
+
+    if (s->shdr_num < 2) {
+        return;
+    }
+
+    /*
+     * String table is the last section since strings are added via
+     * arch_sections_write_hdr().
+     */
+    prepare_elf_section_hdr_string(s, buff_hdr);
 }
 
 static void write_elf_section_headers(DumpState *s, Error **errp)
@@ -1688,6 +1739,14 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     s->filter_area_begin = begin;
     s->filter_area_length = length;
 
+    /* First index is 0, it's the special null name */
+    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
+    /*
+     * Allocate the null name, due to the clearing option set to true
+     * it will be 0.
+     */
+    g_array_set_size(s->string_table_buf, 1);
+
     memory_mapping_list_init(&s->list);
 
     guest_phys_blocks_init(&s->guest_phys_blocks);
@@ -1844,6 +1903,18 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         }
     }
 
+    /*
+     * calculate shdr_num and elf_section_data_size so we know the offsets and
+     * sizes of all parts.
+     *
+     * If phdr_num overflowed we have at least one section header
+     * More sections/hdrs can be added by the architectures
+     */
+    if (s->shdr_num > 1) {
+        /* Reserve the string table */
+        s->shdr_num += 1;
+    }
+
     if (dump_is_64bit(s)) {
         s->shdr_offset = sizeof(Elf64_Ehdr);
         s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 9ed811b313..358b038d47 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -180,6 +180,10 @@ typedef struct DumpState {
     hwaddr note_offset;
 
     void *elf_section_hdrs;     /* Pointer to section header buffer */
+    void *elf_section_data;     /* Pointer to section data buffer */
+    uint64_t elf_section_data_size; /* Size of section data */
+    GArray *string_table_buf;   /* String table data buffer */
+    bool string_table_usage;    /* Indicates if string table is used */
 
     uint8_t *note_buf;          /* buffer for notes */
     size_t note_buf_offset;     /* the writing place in note_buf */
-- 
2.34.1

[PATCH v5 13/18] dump/dump: Add arch section support

[Janosch Frank]

Add hooks which architectures can use to add arbitrary data to custom
sections.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c                | 120 ++++++++++++++++++++++++++++++-------
 include/sysemu/dump-arch.h |   3 +
 2 files changed, 100 insertions(+), 23 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 0d6dbf453a..65b18fc602 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -430,7 +430,7 @@ static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
     memcpy(buff, shdr, shdr_size);
 }
 
-static void prepare_elf_section_hdrs(DumpState *s)
+static void prepare_elf_section_hdrs(DumpState *s, Error **errp)
 {
     size_t len, sizeof_shdr;
     void *buff_hdr;
@@ -438,6 +438,7 @@ static void prepare_elf_section_hdrs(DumpState *s)
     /*
      * Section ordering:
      * - HDR zero
+     * - Arch section hdrs
      * - String table hdr
      */
     sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
@@ -465,6 +466,16 @@ static void prepare_elf_section_hdrs(DumpState *s)
         return;
     }
 
+    /* Add architecture defined section headers */
+    if (s->dump_info.arch_sections_write_hdr_fn) {
+        buff_hdr += s->dump_info.arch_sections_write_hdr_fn(s, buff_hdr);
+
+        if (s->shdr_num >= SHN_LORESERVE) {
+            error_setg_errno(errp, EINVAL, "dump: too many architecture defined sections");
+            return;
+        }
+    }
+
     /*
      * String table is the last section since strings are added via
      * arch_sections_write_hdr().
@@ -477,7 +488,10 @@ static void write_elf_section_headers(DumpState *s, Error **errp)
     size_t sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
     int ret;
 
-    prepare_elf_section_hdrs(s);
+    prepare_elf_section_hdrs(s, errp);
+    if (*errp) {
+        return;
+    }
 
     ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s);
     if (ret < 0) {
@@ -485,6 +499,30 @@ static void write_elf_section_headers(DumpState *s, Error **errp)
     }
 }
 
+static void write_elf_sections(DumpState *s, Error **errp)
+{
+    int ret;
+
+    if (!s->elf_section_data_size) {
+        return;
+    }
+
+    /* Write architecture section data */
+    ret = fd_write_vmcore(s->elf_section_data,
+                          s->elf_section_data_size, s);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "dump: failed to write architecture section  data");
+        return;
+    }
+
+    /* Write string table */
+    ret = fd_write_vmcore(s->string_table_buf->data,
+                          s->string_table_buf->len, s);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "dump: failed to write string table data");
+    }
+}
+
 static void write_data(DumpState *s, void *buf, int length, Error **errp)
 {
     int ret;
@@ -744,6 +782,24 @@ static void dump_iterate(DumpState *s, Error **errp)
     }
 }
 
+static void dump_end(DumpState *s, Error **errp)
+{
+    ERRP_GUARD();
+
+    if (!s->elf_section_data_size) {
+        return;
+    }
+    s->elf_section_data = g_malloc0(s->elf_section_data_size);
+
+    /* Adds the architecture defined section data to s->elf_section_data  */
+    if (s->dump_info.arch_sections_write_fn) {
+        s->dump_info.arch_sections_write_fn(s, s->elf_section_data);
+    }
+
+    /* write sections to vmcore */
+    write_elf_sections(s, errp);
+}
+
 static void create_vmcore(DumpState *s, Error **errp)
 {
     ERRP_GUARD();
@@ -758,6 +814,9 @@ static void create_vmcore(DumpState *s, Error **errp)
     if (*errp) {
         return;
     }
+
+    /* Write the section data */
+    dump_end(s, errp);
 }
 
 static int write_start_flat_header(int fd)
@@ -1883,38 +1942,53 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     }
 
     /*
-     * calculate phdr_num
-     *
-     * the type of ehdr->e_phnum is uint16_t, so we should avoid overflow
+     * Adds the number of architecture sections to shdr_num, sets
+     * string_table_usage and sets elf_section_data_size so we know
+     * the offsets and sizes of all parts.
      */
-    s->phdr_num = 1; /* PT_NOTE */
-    if (s->list.num < UINT16_MAX - 2) {
-        s->shdr_num = 0;
-        s->phdr_num += s->list.num;
-    } else {
-        /* sh_info of section 0 holds the real number of phdrs */
-        s->shdr_num = 1;
+    if (s->dump_info.arch_sections_add_fn) {
+        s->dump_info.arch_sections_add_fn(s);
 
-        /* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
-        if (s->list.num <= UINT32_MAX - 1) {
-            s->phdr_num += s->list.num;
-        } else {
-            s->phdr_num = UINT32_MAX;
+        if (s->shdr_num) {
+            s->string_table_usage = true;
         }
     }
 
     /*
-     * calculate shdr_num and elf_section_data_size so we know the offsets and
-     * sizes of all parts.
+     * Calculate phdr_num
      *
-     * If phdr_num overflowed we have at least one section header
-     * More sections/hdrs can be added by the architectures
+     * The absolute maximum amount of phdrs is UINT32_MAX - 1 as
+     * sh_info is 32 bit. There's special handling once we go over
+     * UINT16_MAX - 1 but that is handled in the ehdr and section
+     * code.
      */
-    if (s->shdr_num > 1) {
-        /* Reserve the string table */
+    s->phdr_num = 1; /* Reserve PT_NOTE */
+    if (s->list.num <= UINT32_MAX - 1) {
+        s->phdr_num += s->list.num;
+    } else {
+        s->phdr_num = UINT32_MAX;
+    }
+
+    /*
+     * The first section header is always a special one in which most
+     * fields are 0.
+     *
+     * We need it if we have custom sections and if phdr_num >=
+     * PN_XNUM so we can write the real phdr_num into sh_info.
+     */
+    if (s->shdr_num || s->phdr_num >= PN_XNUM) {
         s->shdr_num += 1;
     }
 
+    /* Reserve the string table for the arch sections if needed */
+    if (s->string_table_usage) {
+        s->shdr_num += 1;
+    }
+
+    /*
+     * Now that the number of section and program headers is known we
+     * can calculate the offsets of the headers and data.
+     */
     if (dump_is_64bit(s)) {
         s->shdr_offset = sizeof(Elf64_Ehdr);
         s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
index e25b02e990..824c0c1152 100644
--- a/include/sysemu/dump-arch.h
+++ b/include/sysemu/dump-arch.h
@@ -21,6 +21,9 @@ typedef struct ArchDumpInfo {
     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. */
+    void (*arch_sections_add_fn)(DumpState *s);
+    uint64_t (*arch_sections_write_hdr_fn)(DumpState *s, uint8_t *buff);
+    void (*arch_sections_write_fn)(DumpState *s, uint8_t *buff);
 } ArchDumpInfo;
 
 struct GuestPhysBlockList; /* memory_mapping.h */
-- 
2.34.1

[PATCH v5 14/18] DRAFT: linux header sync

[Janosch Frank]

Add the uapi data for KVM_CAP_S390_PROTECTED_DUMP which I expect to be
added with 5.20.

Also add the missing NT_S390_RI_CB and the new NT_S390_PV_CPU_DATA elf
note types.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 include/elf.h             |  2 ++
 linux-headers/linux/kvm.h | 54 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/elf.h b/include/elf.h
index 3a4bcb646a..94fdcfd8dc 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -1649,6 +1649,8 @@ typedef struct elf64_shdr {
 #define NT_TASKSTRUCT	4
 #define NT_AUXV		6
 #define NT_PRXFPREG     0x46e62b7f      /* copied from gdb5.1/include/elf/common.h */
+#define NT_S390_PV_CPU_DATA	0x30e	/* s390 protvirt cpu dump data */
+#define NT_S390_RI_CB	0x30d		/* s390 runtime instrumentation */
 #define NT_S390_GS_CB   0x30b           /* s390 guarded storage registers */
 #define NT_S390_VXRS_HIGH 0x30a         /* s390 vector registers 16-31 */
 #define NT_S390_VXRS_LOW  0x309         /* s390 vector registers 0-15 (lower half) */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index f089349149..46133ef36c 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1150,6 +1150,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_DISABLE_QUIRKS2 213
 /* #define KVM_CAP_VM_TSC_CONTROL 214 */
 #define KVM_CAP_SYSTEM_EVENT_DATA 215
+#define KVM_CAP_S390_PROTECTED_DUMP 217
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1651,6 +1652,55 @@ struct kvm_s390_pv_unp {
 	__u64 tweak;
 };
 
+enum pv_cmd_info_id {
+	KVM_PV_INFO_VM,
+	KVM_PV_INFO_DUMP,
+};
+
+struct kvm_s390_pv_info_dump {
+	__u64 dump_cpu_buffer_len;
+	__u64 dump_config_mem_buffer_per_1m;
+	__u64 dump_config_finalize_len;
+};
+
+struct kvm_s390_pv_info_vm {
+	__u64 inst_calls_list[4];
+	__u64 max_cpus;
+	__u64 max_guests;
+	__u64 max_guest_addr;
+	__u64 feature_indication;
+};
+
+struct kvm_s390_pv_info_header {
+	__u32 id;
+	__u32 len_max;
+	__u32 len_written;
+	__u32 reserved;
+};
+
+struct kvm_s390_pv_info {
+	struct kvm_s390_pv_info_header header;
+	union {
+		struct kvm_s390_pv_info_dump dump;
+		struct kvm_s390_pv_info_vm vm;
+	};
+};
+
+enum pv_cmd_dmp_id {
+        KVM_PV_DUMP_INIT,
+        KVM_PV_DUMP_CONFIG_STATE,
+        KVM_PV_DUMP_COMPLETE,
+        KVM_PV_DUMP_CPU,
+};
+
+struct kvm_s390_pv_dmp {
+        __u64 subcmd;
+        __u64 buff_addr;
+        __u64 buff_len;
+        __u64 gaddr;
+        __u64 reserved[4];
+};
+
 enum pv_cmd_id {
 	KVM_PV_ENABLE,
 	KVM_PV_DISABLE,
@@ -1659,6 +1709,8 @@ enum pv_cmd_id {
 	KVM_PV_VERIFY,
 	KVM_PV_PREP_RESET,
 	KVM_PV_UNSHARE_ALL,
+        KVM_PV_INFO,
+        KVM_PV_DUMP,
 };
 
 struct kvm_pv_cmd {
@@ -2067,4 +2119,6 @@ struct kvm_stats_desc {
 /* Available with KVM_CAP_XSAVE2 */
 #define KVM_GET_XSAVE2		  _IOR(KVMIO,  0xcf, struct kvm_xsave)
 
+#define KVM_S390_PV_CPU_COMMAND _IOWR(KVMIO, 0xd0, struct kvm_pv_cmd)
+
 #endif /* __LINUX_KVM_H */
-- 
2.34.1

[PATCH v5 15/18] s390x: Add protected dump cap

[Janosch Frank]

Add a protected dump capability for later feature checking.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
---
 target/s390x/kvm/kvm.c       | 7 +++++++
 target/s390x/kvm/kvm_s390x.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 7bd8db0e7b..cbd8c91424 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -157,6 +157,7 @@ static int cap_ri;
 static int cap_hpage_1m;
 static int cap_vcpu_resets;
 static int cap_protected;
+static int cap_protected_dump;
 
 static bool mem_op_storage_key_support;
 
@@ -362,6 +363,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
     cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
     cap_protected = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
+    cap_protected_dump = kvm_check_extension(s, KVM_CAP_S390_PROTECTED_DUMP);
 
     kvm_vm_enable_cap(s, KVM_CAP_S390_USER_SIGP, 0);
     kvm_vm_enable_cap(s, KVM_CAP_S390_VECTOR_REGISTERS, 0);
@@ -2043,6 +2045,11 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
     return kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick);
 }
 
+int kvm_s390_get_protected_dump(void)
+{
+    return cap_protected_dump;
+}
+
 int kvm_s390_get_ri(void)
 {
     return cap_ri;
diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h
index 05a5e1e6f4..31a69f9ce2 100644
--- a/target/s390x/kvm/kvm_s390x.h
+++ b/target/s390x/kvm/kvm_s390x.h
@@ -26,6 +26,7 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state);
 void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu);
 int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu);
 int kvm_s390_get_hpage_1m(void);
+int kvm_s390_get_protected_dump(void);
 int kvm_s390_get_ri(void);
 int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
-- 
2.34.1

[PATCH v5 16/18] s390x: Introduce PV query interface

[Janosch Frank]

Introduce an interface over which we can get information about UV data.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
---
 hw/s390x/pv.c              | 61 ++++++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c |  6 ++++
 include/hw/s390x/pv.h      | 10 +++++++
 3 files changed, 77 insertions(+)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 401b63d6cb..2b892b45e8 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -20,6 +20,11 @@
 #include "exec/confidential-guest-support.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/pv.h"
+#include "target/s390x/kvm/kvm_s390x.h"
+
+static bool info_valid;
+static struct kvm_s390_pv_info_vm info_vm;
+static struct kvm_s390_pv_info_dump info_dump;
 
 static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
 {
@@ -56,6 +61,42 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
     }                                  \
 }
 
+int s390_pv_query_info(void)
+{
+    struct kvm_s390_pv_info info = {
+        .header.id = KVM_PV_INFO_VM,
+        .header.len_max = sizeof(info.header) + sizeof(info.vm),
+    };
+    int rc;
+
+    /* Info API's first user is dump so they are bundled */
+    if (!kvm_s390_get_protected_dump()) {
+        return 0;
+    }
+
+    rc = s390_pv_cmd(KVM_PV_INFO, &info);
+    if (rc) {
+        error_report("KVM PV INFO cmd %x failed: %s",
+                     info.header.id, strerror(rc));
+        return rc;
+    }
+    memcpy(&info_vm, &info.vm, sizeof(info.vm));
+
+    info.header.id = KVM_PV_INFO_DUMP;
+    info.header.len_max = sizeof(info.header) + sizeof(info.dump);
+    rc = s390_pv_cmd(KVM_PV_INFO, &info);
+    if (rc) {
+        error_report("KVM PV INFO cmd %x failed: %s",
+                     info.header.id, strerror(rc));
+        return rc;
+    }
+
+    memcpy(&info_dump, &info.dump, sizeof(info.dump));
+    info_valid = true;
+
+    return rc;
+}
+
 int s390_pv_vm_enable(void)
 {
     return s390_pv_cmd(KVM_PV_ENABLE, NULL);
@@ -114,6 +155,26 @@ void s390_pv_inject_reset_error(CPUState *cs)
     env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
 }
 
+uint64_t kvm_s390_pv_dmp_get_size_cpu(void)
+{
+    return info_dump.dump_cpu_buffer_len;
+}
+
+uint64_t kvm_s390_pv_dmp_get_size_complete(void)
+{
+    return info_dump.dump_config_finalize_len;
+}
+
+uint64_t kvm_s390_pv_dmp_get_size_stor_state(void)
+{
+    return info_dump.dump_config_mem_buffer_per_1m;
+}
+
+bool kvm_s390_pv_info_basic_valid(void)
+{
+    return info_valid;
+}
+
 #define TYPE_S390_PV_GUEST "s390-pv-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(S390PVGuest, S390_PV_GUEST)
 
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index cc3097bfee..7982f7973f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -366,6 +366,12 @@ static int s390_machine_protect(S390CcwMachineState *ms)
 
     ms->pv = true;
 
+    /* Will return 0 if API is not available since it's not vital */
+    rc = s390_pv_query_info();
+    if (rc) {
+        goto out_err;
+    }
+
     /* Set SE header and unpack */
     rc = s390_ipl_prepare_pv_header();
     if (rc) {
diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 1f1f545bfc..573259cf2b 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -38,6 +38,7 @@ static inline bool s390_is_pv(void)
     return ccw->pv;
 }
 
+int s390_pv_query_info(void);
 int s390_pv_vm_enable(void);
 void s390_pv_vm_disable(void);
 int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
@@ -46,8 +47,13 @@ void s390_pv_prep_reset(void);
 int s390_pv_verify(void);
 void s390_pv_unshare(void);
 void s390_pv_inject_reset_error(CPUState *cs);
+uint64_t kvm_s390_pv_dmp_get_size_cpu(void);
+uint64_t kvm_s390_pv_dmp_get_size_stor_state(void);
+uint64_t kvm_s390_pv_dmp_get_size_complete(void);
+bool kvm_s390_pv_info_basic_valid(void);
 #else /* CONFIG_KVM */
 static inline bool s390_is_pv(void) { return false; }
+static inline int s390_pv_query_info(void) { return 0; }
 static inline int s390_pv_vm_enable(void) { return 0; }
 static inline void s390_pv_vm_disable(void) {}
 static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
@@ -56,6 +62,10 @@ static inline void s390_pv_prep_reset(void) {}
 static inline int s390_pv_verify(void) { return 0; }
 static inline void s390_pv_unshare(void) {}
 static inline void s390_pv_inject_reset_error(CPUState *cs) {};
+static inline uint64_t kvm_s390_pv_dmp_get_size_cpu(void) { return 0; }
+static inline uint64_t kvm_s390_pv_dmp_get_size_stor_state(void) { return 0; }
+static inline uint64_t kvm_s390_pv_dmp_get_size_complete(void) { return 0; }
+static inline bool kvm_s390_pv_info_basic_valid(void) { return false; }
 #endif /* CONFIG_KVM */
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-- 
2.34.1

[PATCH v5 17/18] s390x: Add KVM PV dump interface

[Janosch Frank]

Let's add a few bits of code which hide the new KVM PV dump API from
us via new functions.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/pv.c         | 51 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/pv.h |  8 +++++++
 2 files changed, 59 insertions(+)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 2b892b45e8..ce3b6ad3e9 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -175,6 +175,57 @@ bool kvm_s390_pv_info_basic_valid(void)
     return info_valid;
 }
 
+static int s390_pv_dump_cmd(uint64_t subcmd, uint64_t uaddr, uint64_t gaddr,
+                            uint64_t len)
+{
+    struct kvm_s390_pv_dmp dmp = {
+        .subcmd = subcmd,
+        .buff_addr = uaddr,
+        .buff_len = len,
+        .gaddr = gaddr,
+    };
+    int ret;
+
+    ret = s390_pv_cmd(KVM_PV_DUMP, (void *)&dmp);
+    if (ret) {
+        error_report("KVM DUMP command %ld failed", subcmd);
+    }
+    return ret;
+}
+
+int kvm_s390_dump_cpu(S390CPU *cpu, void *buff)
+{
+    struct kvm_s390_pv_dmp dmp = {
+        .subcmd = KVM_PV_DUMP_CPU,
+        .buff_addr = (uint64_t)buff,
+        .gaddr = 0,
+        .buff_len = info_dump.dump_cpu_buffer_len,
+    };
+    struct kvm_pv_cmd pv = {
+        .cmd = KVM_PV_DUMP,
+        .data = (uint64_t)&dmp,
+    };
+
+    return kvm_vcpu_ioctl(CPU(cpu), KVM_S390_PV_CPU_COMMAND, &pv);
+}
+
+int kvm_s390_dump_init(void)
+{
+    return s390_pv_dump_cmd(KVM_PV_DUMP_INIT, 0, 0, 0);
+}
+
+int kvm_s390_dump_mem(uint64_t gaddr, size_t len, void *dest)
+{
+    return s390_pv_dump_cmd(KVM_PV_DUMP_CONFIG_STATE, (uint64_t)dest,
+                            gaddr, len);
+}
+
+int kvm_s390_dump_complete(void *buff)
+{
+    return s390_pv_dump_cmd(KVM_PV_DUMP_COMPLETE, (uint64_t)buff, 0,
+                            info_dump.dump_config_finalize_len);
+}
+
 #define TYPE_S390_PV_GUEST "s390-pv-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(S390PVGuest, S390_PV_GUEST)
 
diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 573259cf2b..02a6c06b9f 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -51,6 +51,10 @@ uint64_t kvm_s390_pv_dmp_get_size_cpu(void);
 uint64_t kvm_s390_pv_dmp_get_size_stor_state(void);
 uint64_t kvm_s390_pv_dmp_get_size_complete(void);
 bool kvm_s390_pv_info_basic_valid(void);
+int kvm_s390_dump_init(void);
+int kvm_s390_dump_cpu(S390CPU *cpu, void *buff);
+int kvm_s390_dump_mem(uint64_t addr, size_t len, void *dest);
+int kvm_s390_dump_complete(void *buff);
 #else /* CONFIG_KVM */
 static inline bool s390_is_pv(void) { return false; }
 static inline int s390_pv_query_info(void) { return 0; }
@@ -66,6 +70,10 @@ static inline uint64_t kvm_s390_pv_dmp_get_size_cpu(void) { return 0; }
 static inline uint64_t kvm_s390_pv_dmp_get_size_stor_state(void) { return 0; }
 static inline uint64_t kvm_s390_pv_dmp_get_size_complete(void) { return 0; }
 static inline bool kvm_s390_pv_info_basic_valid(void) { return false; }
+static inline int kvm_s390_dump_init(void) { return 0; }
+static inline int kvm_s390_dump_cpu(S390CPU *cpu, void *buff, size_t len) { return 0; }
+static inline int kvm_s390_dump_mem(uint64_t addr, size_t len, void *dest) { return 0; }
+static inline int kvm_s390_dump_complete(void *buff) { return 0; }
 #endif /* CONFIG_KVM */
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-- 
2.34.1

[PATCH v5 18/18] s390x: pv: Add dump support

[Janosch Frank]

Sometimes dumping a guest from the outside is the only way to get the
data that is needed. This can be the case if a dumping mechanism like
KDUMP hasn't been configured or data needs to be fetched at a specific
point. Dumping a protected guest from the outside without help from
fw/hw doesn't yield sufficient data to be useful. Hence we now
introduce PV dump support.

The PV dump support works by integrating the firmware into the dump
process. New Ultravisor calls are used to initiate the dump process,
dump cpu data, dump memory state and lastly complete the dump process.
The UV calls are exposed by KVM via the new KVM_PV_DUMP command and
its subcommands. The guest's data is fully encrypted and can only be
decrypted by the entity that owns the customer communication key for
the dumped guest. Also dumping needs to be allowed via a flag in the
SE header.

On the QEMU side of things we store the PV dump data in the newly
introduced architecture ELF sections (storage state and completion
data) and the cpu notes (for cpu dump data).

Users can use the zgetdump tool to convert the encrypted QEMU dump to an
unencrypted one.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c              |  12 +-
 include/sysemu/dump.h    |   5 +
 target/s390x/arch_dump.c | 242 ++++++++++++++++++++++++++++++++++-----
 3 files changed, 227 insertions(+), 32 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 65b18fc602..7cf5eb7c8b 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -720,9 +720,9 @@ static void dump_begin(DumpState *s, Error **errp)
     write_elf_notes(s, errp);
 }
 
-static int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
-                                           int64_t filter_area_start,
-                                           int64_t filter_area_length)
+int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
+                                    int64_t filter_area_start,
+                                    int64_t filter_area_length)
 {
     int64_t size, left, right;
 
@@ -740,9 +740,9 @@ static int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
     return size;
 }
 
-static int64_t dump_filtered_memblock_start(GuestPhysBlock *block,
-                                            int64_t filter_area_start,
-                                            int64_t filter_area_length)
+int64_t dump_filtered_memblock_start(GuestPhysBlock *block,
+                                     int64_t filter_area_start,
+                                     int64_t filter_area_length)
 {
     if (filter_area_length) {
         /* return -1 if the block is not within filter area */
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 358b038d47..245c26fbca 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -216,4 +216,9 @@ typedef struct 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);
+
+int64_t dump_filtered_memblock_size(GuestPhysBlock *block, int64_t filter_area_start,
+                                    int64_t filter_area_length);
+int64_t dump_filtered_memblock_start(GuestPhysBlock *block, int64_t filter_area_start,
+                                     int64_t filter_area_length);
 #endif
diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index f60a14920d..5e8e03d536 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -16,7 +16,8 @@
 #include "s390x-internal.h"
 #include "elf.h"
 #include "sysemu/dump.h"
-
+#include "hw/s390x/pv.h"
+#include "kvm/kvm_s390x.h"
 
 struct S390xUserRegsStruct {
     uint64_t psw[2];
@@ -76,9 +77,16 @@ typedef struct noteStruct {
         uint64_t todcmp;
         uint32_t todpreg;
         uint64_t ctrs[16];
+        uint8_t dynamic[1];  /*
+                              * Would be a flexible array member, if
+                              * that was legal inside a union. Real
+                              * size comes from PV info interface.
+                              */
     } contents;
 } QEMU_PACKED Note;
 
+static bool pv_dump_initialized;
+
 static void s390x_write_elf64_prstatus(Note *note, S390CPU *cpu, int id)
 {
     int i;
@@ -177,28 +185,39 @@ static void s390x_write_elf64_prefix(Note *note, S390CPU *cpu, int id)
     note->contents.prefix = cpu_to_be32((uint32_t)(cpu->env.psa));
 }
 
+static void s390x_write_elf64_pv(Note *note, S390CPU *cpu, int id)
+{
+    note->hdr.n_type = cpu_to_be32(NT_S390_PV_CPU_DATA);
+    if (!pv_dump_initialized) {
+        return;
+    }
+    kvm_s390_dump_cpu(cpu, &note->contents.dynamic);
+}
 
 typedef struct NoteFuncDescStruct {
     int contents_size;
+    uint64_t (*note_size_func)(void); /* NULL for non-dynamic sized contents */
     void (*note_contents_func)(Note *note, S390CPU *cpu, int id);
+    bool pvonly;
 } NoteFuncDesc;
 
 static const NoteFuncDesc note_core[] = {
-    {sizeof_field(Note, contents.prstatus), s390x_write_elf64_prstatus},
-    {sizeof_field(Note, contents.fpregset), s390x_write_elf64_fpregset},
-    { 0, NULL}
+    {sizeof_field(Note, contents.prstatus), NULL, s390x_write_elf64_prstatus, false},
+    {sizeof_field(Note, contents.fpregset), NULL, s390x_write_elf64_fpregset, false},
+    { 0, NULL, NULL}
 };
 
 static const NoteFuncDesc note_linux[] = {
-    {sizeof_field(Note, contents.prefix),   s390x_write_elf64_prefix},
-    {sizeof_field(Note, contents.ctrs),     s390x_write_elf64_ctrs},
-    {sizeof_field(Note, contents.timer),    s390x_write_elf64_timer},
-    {sizeof_field(Note, contents.todcmp),   s390x_write_elf64_todcmp},
-    {sizeof_field(Note, contents.todpreg),  s390x_write_elf64_todpreg},
-    {sizeof_field(Note, contents.vregslo),  s390x_write_elf64_vregslo},
-    {sizeof_field(Note, contents.vregshi),  s390x_write_elf64_vregshi},
-    {sizeof_field(Note, contents.gscb),     s390x_write_elf64_gscb},
-    { 0, NULL}
+    {sizeof_field(Note, contents.prefix),   NULL, s390x_write_elf64_prefix,  false},
+    {sizeof_field(Note, contents.ctrs),     NULL, s390x_write_elf64_ctrs,    false},
+    {sizeof_field(Note, contents.timer),    NULL, s390x_write_elf64_timer,   false},
+    {sizeof_field(Note, contents.todcmp),   NULL, s390x_write_elf64_todcmp,  false},
+    {sizeof_field(Note, contents.todpreg),  NULL, s390x_write_elf64_todpreg, false},
+    {sizeof_field(Note, contents.vregslo),  NULL, s390x_write_elf64_vregslo, false},
+    {sizeof_field(Note, contents.vregshi),  NULL, s390x_write_elf64_vregshi, false},
+    {sizeof_field(Note, contents.gscb),     NULL, s390x_write_elf64_gscb,    false},
+    {0, kvm_s390_pv_dmp_get_size_cpu,       s390x_write_elf64_pv, true},
+    { 0, NULL, NULL}
 };
 
 static int s390x_write_elf64_notes(const char *note_name,
@@ -207,22 +226,41 @@ static int s390x_write_elf64_notes(const char *note_name,
                                        DumpState *s,
                                        const NoteFuncDesc *funcs)
 {
-    Note note;
+    Note note, *notep;
     const NoteFuncDesc *nf;
-    int note_size;
+    int note_size, content_size;
     int ret = -1;
 
     assert(strlen(note_name) < sizeof(note.name));
 
     for (nf = funcs; nf->note_contents_func; nf++) {
-        memset(&note, 0, sizeof(note));
-        note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
-        note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
-        g_strlcpy(note.name, note_name, sizeof(note.name));
-        (*nf->note_contents_func)(&note, cpu, id);
+        notep = &note;
+        if (nf->pvonly && !s390_is_pv()) {
+            continue;
+        }
 
-        note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-        ret = f(&note, note_size, s);
+        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();
+        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
+
+        /* Notes with dynamic sizes need to allocate a note */
+        if (nf->note_size_func) {
+            notep = g_malloc0(note_size);
+        }
+
+        memset(notep, 0, sizeof(note));
+
+        /* Setup note header data */
+        notep->hdr.n_descsz = cpu_to_be32(content_size);
+        notep->hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
+        g_strlcpy(notep->name, note_name, sizeof(notep->name));
+
+        /* Get contents and write them out */
+        (*nf->note_contents_func)(notep, cpu, id);
+        ret = f(notep, note_size, s);
+
+        if (nf->note_size_func) {
+            g_free(notep);
+        }
 
         if (ret < 0) {
             return -1;
@@ -247,12 +285,161 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
     return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
 }
 
+/* PV dump section size functions */
+static uint64_t get_dump_stor_state_size_from_len(uint64_t len)
+{
+    return (len / (1 << 20)) * kvm_s390_pv_dmp_get_size_stor_state();
+}
+
+static uint64_t get_size_stor_state(DumpState *s)
+{
+    return get_dump_stor_state_size_from_len(s->total_size);
+}
+
+static uint64_t get_size_complete(DumpState *s)
+{
+    return kvm_s390_pv_dmp_get_size_complete();
+}
+
+/* PV dump section data functions*/
+static int get_data_complete(DumpState *s, uint8_t *buff)
+{
+    int rc;
+
+    if (!pv_dump_initialized) {
+        return 0;
+    }
+    rc = kvm_s390_dump_complete(buff);
+    if (!rc) {
+            pv_dump_initialized = false;
+    }
+    return rc;
+}
+
+static int dump_mem(DumpState *s, uint64_t gaddr, uint8_t *buff, uint64_t buff_len)
+{
+    /* We need the gaddr + len and something to write to */
+    if (!pv_dump_initialized) {
+        return 0;
+    }
+    return kvm_s390_dump_mem(gaddr, buff_len, buff);
+}
+
+static int get_data_mem(DumpState *s, uint8_t *buff)
+{
+    int64_t memblock_size, memblock_start;
+    GuestPhysBlock *block;
+    uint64_t off;
+
+    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
+        memblock_start = dump_filtered_memblock_start(block, s->filter_area_begin,
+                                                      s->filter_area_length);
+        if (memblock_start == -1) {
+            continue;
+        }
+
+        memblock_size = dump_filtered_memblock_size(block, s->filter_area_begin,
+                                                    s->filter_area_length);
+
+        off = get_dump_stor_state_size_from_len(block->target_start);
+        dump_mem(s, block->target_start, buff + off,
+                 get_dump_stor_state_size_from_len(memblock_size));
+    }
+
+    return 0;
+}
+
+struct sections {
+    uint64_t (*sections_size_func)(DumpState *s);
+    int (*sections_contents_func)(DumpState *s, uint8_t *buff);
+    char sctn_str[12];
+} sections[] = {
+    { get_size_stor_state, get_data_mem, "pv_mem_meta"},
+    { get_size_complete, get_data_complete, "pv_compl"},
+    {NULL , NULL, ""}
+};
+
+static uint64_t arch_sections_write_hdr(DumpState *s, uint8_t *buff)
+{
+    Elf64_Shdr *shdr = (void *)buff;
+    struct sections *sctn = sections;
+    uint64_t off = s->section_offset;
+
+    if (!s390_is_pv()) {
+        return 0;
+    }
+
+    for (; sctn->sections_size_func; off += shdr->sh_size, sctn++, shdr++) {
+        memset(shdr, 0, sizeof(*shdr));
+        shdr->sh_type = SHT_PROGBITS;
+        shdr->sh_offset = off;
+        shdr->sh_size = sctn->sections_size_func(s);
+        shdr->sh_name = s->string_table_buf->len;
+        g_array_append_vals(s->string_table_buf, sctn->sctn_str, sizeof(sctn->sctn_str));
+    }
+
+    return (uintptr_t)shdr - (uintptr_t)buff;
+}
+
+
+/* Add arch specific number of sections and their respective sizes */
+static void arch_sections_add(DumpState *s)
+{
+    struct sections *sctn = sections;
+
+    /*
+     * We only do a PV dump if we are running a PV guest, KVM supports
+     * the dump API and we got valid dump length information.
+     */
+    if (!s390_is_pv() || !kvm_s390_get_protected_dump() ||
+        !kvm_s390_pv_info_basic_valid()) {
+        return;
+    }
+
+    /*
+     * Start the UV dump process by doing the initialize dump call via
+     * KVM as the proxy.
+     */
+    if (!kvm_s390_dump_init()) {
+            pv_dump_initialized = true;
+    }
+
+    for (; sctn->sections_size_func; sctn++) {
+        s->shdr_num += 1;
+        s->elf_section_data_size += sctn->sections_size_func(s);
+    }
+
+    /* We use the string table to identify the sections */
+    s->string_table_usage = true;
+}
+
+/*
+ * After the PV dump has been initialized, the CPU data has been
+ * fetched and memory has been dumped, we need to grab the tweak data
+ * and the completion data.
+ */
+static void arch_sections_write(DumpState *s, uint8_t *buff)
+{
+    struct sections *sctn = sections;
+
+    /* shdr_num should only have been set > 1 if we are protected */
+    assert(s390_is_pv());
+
+    for (; sctn->sections_size_func; sctn++) {
+        sctn->sections_contents_func(s, buff);
+        buff += sctn->sections_size_func(s);
+    }
+}
+
 int cpu_get_dump_info(ArchDumpInfo *info,
                       const struct GuestPhysBlockList *guest_phys_blocks)
 {
     info->d_machine = EM_S390;
     info->d_endian = ELFDATA2MSB;
     info->d_class = ELFCLASS64;
+    info->arch_sections_add_fn = *arch_sections_add;
+    info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
+    info->arch_sections_write_fn = *arch_sections_write;
 
     return 0;
 }
@@ -261,7 +448,7 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
 {
     int name_size = 8; /* "LINUX" or "CORE" + pad */
     size_t elf_note_size = 0;
-    int note_head_size;
+    int note_head_size, content_size;
     const NoteFuncDesc *nf;
 
     assert(class == ELFCLASS64);
@@ -270,12 +457,15 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
     note_head_size = sizeof(Elf64_Nhdr);
 
     for (nf = note_core; nf->note_contents_func; nf++) {
-        elf_note_size = elf_note_size + note_head_size + name_size +
-                        nf->contents_size;
+        elf_note_size = elf_note_size + note_head_size + name_size + nf->contents_size;
     }
     for (nf = note_linux; nf->note_contents_func; nf++) {
+        if (nf->pvonly && !s390_is_pv()) {
+            continue;
+        }
+        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();
         elf_note_size = elf_note_size + note_head_size + name_size +
-                        nf->contents_size;
+                        content_size;
     }
 
     return (elf_note_size) * nr_cpus;
-- 
2.34.1

Re: [PATCH v5 18/18] s390x: pv: Add dump support

[Janosch Frank]

On 8/11/22 14:11, Janosch Frank wrote:
> Sometimes dumping a guest from the outside is the only way to get the
> data that is needed. This can be the case if a dumping mechanism like
> KDUMP hasn't been configured or data needs to be fetched at a specific
> point. Dumping a protected guest from the outside without help from
> fw/hw doesn't yield sufficient data to be useful. Hence we now
> introduce PV dump support.
> 
> The PV dump support works by integrating the firmware into the dump
> process. New Ultravisor calls are used to initiate the dump process,
> dump cpu data, dump memory state and lastly complete the dump process.
> The UV calls are exposed by KVM via the new KVM_PV_DUMP command and
> its subcommands. The guest's data is fully encrypted and can only be
> decrypted by the entity that owns the customer communication key for
> the dumped guest. Also dumping needs to be allowed via a flag in the
> SE header.
> 
> On the QEMU side of things we store the PV dump data in the newly
> introduced architecture ELF sections (storage state and completion
> data) and the cpu notes (for cpu dump data).
> 
> Users can use the zgetdump tool to convert the encrypted QEMU dump to an
> unencrypted one.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>


Seems like I forgot to amend this commit with the naming changes before 
sending:

diff --git i/target/s390x/arch_dump.c w/target/s390x/arch_dump.c
index 5e8e03d536..233f23c071 100644
--- i/target/s390x/arch_dump.c
+++ w/target/s390x/arch_dump.c
@@ -286,14 +286,14 @@ int 
s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
  }

  /* PV dump section size functions */
-static uint64_t get_dump_stor_state_size_from_len(uint64_t len)
+static uint64_t get_stor_state_size_from_len(uint64_t len)
  {
      return (len / (1 << 20)) * kvm_s390_pv_dmp_get_size_stor_state();
  }

  static uint64_t get_size_stor_state(DumpState *s)
  {
-    return get_dump_stor_state_size_from_len(s->total_size);
+    return get_stor_state_size_from_len(s->total_size);
  }

  static uint64_t get_size_complete(DumpState *s)
@@ -316,7 +316,8 @@ static int get_data_complete(DumpState *s, uint8_t 
*buff)
      return rc;
  }

-static int dump_mem(DumpState *s, uint64_t gaddr, uint8_t *buff, 
uint64_t buff_len)
+static int get_stor_state_block(DumpState *s, uint64_t gaddr, uint8_t 
*buff,
+                                uint64_t buff_len)
  {
      /* We need the gaddr + len and something to write to */
      if (!pv_dump_initialized) {
@@ -325,7 +326,7 @@ static int dump_mem(DumpState *s, uint64_t gaddr, 
uint8_t *buff, uint64_t buff_l
      return kvm_s390_dump_mem(gaddr, buff_len, buff);
  }

-static int get_data_mem(DumpState *s, uint8_t *buff)
+static int get_store_state(DumpState *s, uint8_t *buff)
  {
      int64_t memblock_size, memblock_start;
      GuestPhysBlock *block;
@@ -341,9 +342,9 @@ static int get_data_mem(DumpState *s, uint8_t *buff)
          memblock_size = dump_filtered_memblock_size(block, 
s->filter_area_begin,
 
s->filter_area_length);

-        off = get_dump_stor_state_size_from_len(block->target_start);
-        dump_mem(s, block->target_start, buff + off,
-                 get_dump_stor_state_size_from_len(memblock_size));
+        off = get_stor_state_size_from_len(block->target_start);
+        get_stor_state_block(s, block->target_start, buff + off,
+                             get_stor_state_size_from_len(memblock_size));
      }

      return 0;
@@ -354,7 +355,7 @@ struct sections {
      int (*sections_contents_func)(DumpState *s, uint8_t *buff);
      char sctn_str[12];
  } sections[] = {
-    { get_size_stor_state, get_data_mem, "pv_mem_meta"},
+    { get_size_stor_state, get_store_state, "pv_mem_meta"},
      { get_size_complete, get_data_complete, "pv_compl"},
      {NULL , NULL, ""}
  };

Re: [PATCH v5 01/18] dump: Replace opaque DumpState pointer with a typed one

[Daniel Henrique Barboza]

On 8/11/22 09:10, Janosch Frank wrote:
> It's always better to convey the type of a pointer if at all
> possible. So let's add the DumpState typedef to typedefs.h and move
> the dump note functions from the opaque pointers to DumpState
> pointers.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Cédric Le Goater <clg@kaod.org>
> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Greg Kurz <groug@kaod.org>
> CC: Palmer Dabbelt <palmer@dabbelt.com>
> CC: Alistair Francis <alistair.francis@wdc.com>
> CC: Bin Meng <bin.meng@windriver.com>
> CC: Cornelia Huck <cohuck@redhat.com>
> CC: Thomas Huth <thuth@redhat.com>
> CC: Richard Henderson <richard.henderson@linaro.org>
> CC: David Hildenbrand <david@redhat.com>
> ---

For target/ppc changes:

Acked-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/core/sysemu-cpu-ops.h |  8 ++++----
>   include/qemu/typedefs.h          |  1 +
>   target/arm/arch_dump.c           |  6 ++----
>   target/arm/cpu.h                 |  4 ++--
>   target/i386/arch_dump.c          | 30 +++++++++++++++---------------
>   target/i386/cpu.h                |  8 ++++----
>   target/ppc/arch_dump.c           | 18 +++++++++---------
>   target/ppc/cpu.h                 |  4 ++--
>   target/riscv/arch_dump.c         |  6 ++----
>   target/riscv/cpu.h               |  4 ++--
>   target/s390x/arch_dump.c         | 10 +++++-----
>   target/s390x/s390x-internal.h    |  2 +-
>   12 files changed, 49 insertions(+), 52 deletions(-)
> 
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index a9ba39e5f2..ee169b872c 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -53,25 +53,25 @@ typedef struct SysemuCPUOps {
>        * 32-bit VM coredump.
>        */
>       int (*write_elf32_note)(WriteCoreDumpFunction f, CPUState *cpu,
> -                            int cpuid, void *opaque);
> +                            int cpuid, DumpState *s);
>       /**
>        * @write_elf64_note: Callback for writing a CPU-specific ELF note to a
>        * 64-bit VM coredump.
>        */
>       int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
> -                            int cpuid, void *opaque);
> +                            int cpuid, DumpState *s);
>       /**
>        * @write_elf32_qemunote: Callback for writing a CPU- and QEMU-specific ELF
>        * note to a 32-bit VM coredump.
>        */
>       int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
> -                                void *opaque);
> +                                DumpState *s);
>       /**
>        * @write_elf64_qemunote: Callback for writing a CPU- and QEMU-specific ELF
>        * note to a 64-bit VM coredump.
>        */
>       int (*write_elf64_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
> -                                void *opaque);
> +                                DumpState *s);
>       /**
>        * @virtio_is_big_endian: Callback to return %true if a CPU which supports
>        * runtime configurable endianness is currently big-endian.
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 42f4ceb701..054fd46fa6 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -129,6 +129,7 @@ typedef struct VirtIODevice VirtIODevice;
>   typedef struct Visitor Visitor;
>   typedef struct VMChangeStateEntry VMChangeStateEntry;
>   typedef struct VMStateDescription VMStateDescription;
> +typedef struct DumpState DumpState;
>   
>   /*
>    * Pointer types
> diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c
> index b1f040e69f..2d8e41ab8a 100644
> --- a/target/arm/arch_dump.c
> +++ b/target/arm/arch_dump.c
> @@ -232,12 +232,11 @@ static int aarch64_write_elf64_sve(WriteCoreDumpFunction f,
>   #endif
>   
>   int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>   {
>       struct aarch64_note note;
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> -    DumpState *s = opaque;
>       uint64_t pstate, sp;
>       int ret, i;
>   
> @@ -360,12 +359,11 @@ static int arm_write_elf32_vfp(WriteCoreDumpFunction f, CPUARMState *env,
>   }
>   
>   int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>   {
>       struct arm_note note;
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> -    DumpState *s = opaque;
>       int ret, i;
>       bool fpvalid = cpu_isar_feature(aa32_vfp_simd, cpu);
>   
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5168e3d837..fc8b358779 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1100,9 +1100,9 @@ int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
>   const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname);
>   
>   int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>   int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>   
>   #ifdef TARGET_AARCH64
>   int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index 004141fc04..c290910a04 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -42,7 +42,7 @@ typedef struct {
>   
>   static int x86_64_write_elf64_note(WriteCoreDumpFunction f,
>                                      CPUX86State *env, int id,
> -                                   void *opaque)
> +                                   DumpState *s)
>   {
>       x86_64_user_regs_struct regs;
>       Elf64_Nhdr *note;
> @@ -94,7 +94,7 @@ static int x86_64_write_elf64_note(WriteCoreDumpFunction f,
>       buf += descsz - sizeof(x86_64_user_regs_struct)-sizeof(target_ulong);
>       memcpy(buf, &regs, sizeof(x86_64_user_regs_struct));
>   
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>       g_free(note);
>       if (ret < 0) {
>           return -1;
> @@ -148,7 +148,7 @@ static void x86_fill_elf_prstatus(x86_elf_prstatus *prstatus, CPUX86State *env,
>   }
>   
>   static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
> -                                int id, void *opaque)
> +                                int id, DumpState *s)
>   {
>       x86_elf_prstatus prstatus;
>       Elf64_Nhdr *note;
> @@ -170,7 +170,7 @@ static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
>       buf += ROUND_UP(name_size, 4);
>       memcpy(buf, &prstatus, sizeof(prstatus));
>   
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>       g_free(note);
>       if (ret < 0) {
>           return -1;
> @@ -180,7 +180,7 @@ static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
>   }
>   
>   int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>   {
>       X86CPU *cpu = X86_CPU(cs);
>       int ret;
> @@ -189,10 +189,10 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>       bool lma = !!(first_x86_cpu->env.hflags & HF_LMA_MASK);
>   
>       if (lma) {
> -        ret = x86_64_write_elf64_note(f, &cpu->env, cpuid, opaque);
> +        ret = x86_64_write_elf64_note(f, &cpu->env, cpuid, s);
>       } else {
>   #endif
> -        ret = x86_write_elf64_note(f, &cpu->env, cpuid, opaque);
> +        ret = x86_write_elf64_note(f, &cpu->env, cpuid, s);
>   #ifdef TARGET_X86_64
>       }
>   #endif
> @@ -201,7 +201,7 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>   }
>   
>   int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>   {
>       X86CPU *cpu = X86_CPU(cs);
>       x86_elf_prstatus prstatus;
> @@ -224,7 +224,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>       buf += ROUND_UP(name_size, 4);
>       memcpy(buf, &prstatus, sizeof(prstatus));
>   
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>       g_free(note);
>       if (ret < 0) {
>           return -1;
> @@ -329,7 +329,7 @@ static void qemu_get_cpustate(QEMUCPUState *s, CPUX86State *env)
>   
>   static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
>                                         CPUX86State *env,
> -                                      void *opaque,
> +                                      DumpState *s,
>                                         int type)
>   {
>       QEMUCPUState state;
> @@ -369,7 +369,7 @@ static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
>       buf += ROUND_UP(name_size, 4);
>       memcpy(buf, &state, sizeof(state));
>   
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>       g_free(note);
>       if (ret < 0) {
>           return -1;
> @@ -379,19 +379,19 @@ static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
>   }
>   
>   int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cs,
> -                                 void *opaque)
> +                                 DumpState *s)
>   {
>       X86CPU *cpu = X86_CPU(cs);
>   
> -    return cpu_write_qemu_note(f, &cpu->env, opaque, 1);
> +    return cpu_write_qemu_note(f, &cpu->env, s, 1);
>   }
>   
>   int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cs,
> -                                 void *opaque)
> +                                 DumpState *s)
>   {
>       X86CPU *cpu = X86_CPU(cs);
>   
> -    return cpu_write_qemu_note(f, &cpu->env, opaque, 0);
> +    return cpu_write_qemu_note(f, &cpu->env, s, 0);
>   }
>   
>   int cpu_get_dump_info(ArchDumpInfo *info,
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 82004b65b9..b75108d6a3 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1938,13 +1938,13 @@ extern const VMStateDescription vmstate_x86_cpu;
>   int x86_cpu_pending_interrupt(CPUState *cs, int interrupt_request);
>   
>   int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cpu,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>   int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cpu,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>   int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
> -                                 void *opaque);
> +                                 DumpState *s);
>   int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
> -                                 void *opaque);
> +                                 DumpState *s);
>   
>   void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
>                                   Error **errp);
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 1139cead9f..f58e6359d5 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -270,23 +270,23 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>   static int ppc_write_all_elf_notes(const char *note_name,
>                                      WriteCoreDumpFunction f,
>                                      PowerPCCPU *cpu, int id,
> -                                   void *opaque)
> +                                   DumpState *s)
>   {
> -    NoteFuncArg arg = { .state = opaque };
> +    NoteFuncArg arg = { .state = s };
>       int ret = -1;
>       int note_size;
>       const NoteFuncDesc *nf;
>   
>       for (nf = note_func; nf->note_contents_func; nf++) {
> -        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);
> +        arg.note.hdr.n_namesz = cpu_to_dump32(s, sizeof(arg.note.name));
> +        arg.note.hdr.n_descsz = cpu_to_dump32(s, nf->contents_size);
>           strncpy(arg.note.name, note_name, sizeof(arg.note.name));
>   
>           (*nf->note_contents_func)(&arg, cpu);
>   
>           note_size =
>               sizeof(arg.note) - sizeof(arg.note.contents) + nf->contents_size;
> -        ret = f(&arg.note, note_size, opaque);
> +        ret = f(&arg.note, note_size, s);
>           if (ret < 0) {
>               return -1;
>           }
> @@ -295,15 +295,15 @@ static int ppc_write_all_elf_notes(const char *note_name,
>   }
>   
>   int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>   {
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, opaque);
> +    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, s);
>   }
>   
>   int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>   {
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, opaque);
> +    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, s);
>   }
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a4c893cfad..f7d28fe093 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1354,9 +1354,9 @@ void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
>   const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
>   #endif
>   int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>   int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>   #ifndef CONFIG_USER_ONLY
>   void ppc_cpu_do_interrupt(CPUState *cpu);
>   bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> diff --git a/target/riscv/arch_dump.c b/target/riscv/arch_dump.c
> index 709f621d82..736a232956 100644
> --- a/target/riscv/arch_dump.c
> +++ b/target/riscv/arch_dump.c
> @@ -64,12 +64,11 @@ static void riscv64_note_init(struct riscv64_note *note, DumpState *s,
>   }
>   
>   int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>   {
>       struct riscv64_note note;
>       RISCVCPU *cpu = RISCV_CPU(cs);
>       CPURISCVState *env = &cpu->env;
> -    DumpState *s = opaque;
>       int ret, i = 0;
>       const char name[] = "CORE";
>   
> @@ -134,12 +133,11 @@ static void riscv32_note_init(struct riscv32_note *note, DumpState *s,
>   }
>   
>   int riscv_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>   {
>       struct riscv32_note note;
>       RISCVCPU *cpu = RISCV_CPU(cs);
>       CPURISCVState *env = &cpu->env;
> -    DumpState *s = opaque;
>       int ret, i;
>       const char name[] = "CORE";
>   
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5c7acc055a..9699504721 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -505,9 +505,9 @@ extern const char * const riscv_fpr_regnames[];
>   const char *riscv_cpu_get_trap_name(target_ulong cause, bool async);
>   void riscv_cpu_do_interrupt(CPUState *cpu);
>   int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>   int riscv_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>   int riscv_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>   int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>   int riscv_cpu_hviprio_index2irq(int index, int *out_irq, int *out_rdzero);
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index 08daf93ae1..f60a14920d 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -204,7 +204,7 @@ static const NoteFuncDesc note_linux[] = {
>   static int s390x_write_elf64_notes(const char *note_name,
>                                          WriteCoreDumpFunction f,
>                                          S390CPU *cpu, int id,
> -                                       void *opaque,
> +                                       DumpState *s,
>                                          const NoteFuncDesc *funcs)
>   {
>       Note note;
> @@ -222,7 +222,7 @@ static int s390x_write_elf64_notes(const char *note_name,
>           (*nf->note_contents_func)(&note, cpu, id);
>   
>           note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
> -        ret = f(&note, note_size, opaque);
> +        ret = f(&note, note_size, s);
>   
>           if (ret < 0) {
>               return -1;
> @@ -235,16 +235,16 @@ static int s390x_write_elf64_notes(const char *note_name,
>   
>   
>   int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                              int cpuid, void *opaque)
> +                              int cpuid, DumpState *s)
>   {
>       S390CPU *cpu = S390_CPU(cs);
>       int r;
>   
> -    r = s390x_write_elf64_notes("CORE", f, cpu, cpuid, opaque, note_core);
> +    r = s390x_write_elf64_notes("CORE", f, cpu, cpuid, s, note_core);
>       if (r) {
>           return r;
>       }
> -    return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, opaque, note_linux);
> +    return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
>   }
>   
>   int cpu_get_dump_info(ArchDumpInfo *info,
> diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
> index 6aba7fd0ca..b5ae0ae364 100644
> --- a/target/s390x/s390x-internal.h
> +++ b/target/s390x/s390x-internal.h
> @@ -227,7 +227,7 @@ static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
>   
>   /* arch_dump.c */
>   int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                              int cpuid, void *opaque);
> +                              int cpuid, DumpState *s);
>   
>   
>   /* cc_helper.c */

Re: [PATCH v5 01/18] dump: Replace opaque DumpState pointer with a typed one

[Marc-André Lureau]

On Thu, Aug 11, 2022 at 4:13 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> It's always better to convey the type of a pointer if at all
> possible. So let's add the DumpState typedef to typedefs.h and move
> the dump note functions from the opaque pointers to DumpState
> pointers.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Cédric Le Goater <clg@kaod.org>
> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Greg Kurz <groug@kaod.org>
> CC: Palmer Dabbelt <palmer@dabbelt.com>
> CC: Alistair Francis <alistair.francis@wdc.com>
> CC: Bin Meng <bin.meng@windriver.com>
> CC: Cornelia Huck <cohuck@redhat.com>
> CC: Thomas Huth <thuth@redhat.com>
> CC: Richard Henderson <richard.henderson@linaro.org>
> CC: David Hildenbrand <david@redhat.com>
> ---
>  include/hw/core/sysemu-cpu-ops.h |  8 ++++----
>  include/qemu/typedefs.h          |  1 +
>  target/arm/arch_dump.c           |  6 ++----
>  target/arm/cpu.h                 |  4 ++--
>  target/i386/arch_dump.c          | 30 +++++++++++++++---------------
>  target/i386/cpu.h                |  8 ++++----
>  target/ppc/arch_dump.c           | 18 +++++++++---------
>  target/ppc/cpu.h                 |  4 ++--
>  target/riscv/arch_dump.c         |  6 ++----
>  target/riscv/cpu.h               |  4 ++--
>  target/s390x/arch_dump.c         | 10 +++++-----
>  target/s390x/s390x-internal.h    |  2 +-
>  12 files changed, 49 insertions(+), 52 deletions(-)
>
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index a9ba39e5f2..ee169b872c 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -53,25 +53,25 @@ typedef struct SysemuCPUOps {
>       * 32-bit VM coredump.
>       */
>      int (*write_elf32_note)(WriteCoreDumpFunction f, CPUState *cpu,
> -                            int cpuid, void *opaque);
> +                            int cpuid, DumpState *s);
>      /**
>       * @write_elf64_note: Callback for writing a CPU-specific ELF note to a
>       * 64-bit VM coredump.
>       */
>      int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
> -                            int cpuid, void *opaque);
> +                            int cpuid, DumpState *s);
>      /**
>       * @write_elf32_qemunote: Callback for writing a CPU- and QEMU-specific ELF
>       * note to a 32-bit VM coredump.
>       */
>      int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
> -                                void *opaque);
> +                                DumpState *s);
>      /**
>       * @write_elf64_qemunote: Callback for writing a CPU- and QEMU-specific ELF
>       * note to a 64-bit VM coredump.
>       */
>      int (*write_elf64_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
> -                                void *opaque);
> +                                DumpState *s);
>      /**
>       * @virtio_is_big_endian: Callback to return %true if a CPU which supports
>       * runtime configurable endianness is currently big-endian.
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 42f4ceb701..054fd46fa6 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -129,6 +129,7 @@ typedef struct VirtIODevice VirtIODevice;
>  typedef struct Visitor Visitor;
>  typedef struct VMChangeStateEntry VMChangeStateEntry;
>  typedef struct VMStateDescription VMStateDescription;
> +typedef struct DumpState DumpState;
>
>  /*
>   * Pointer types
> diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c
> index b1f040e69f..2d8e41ab8a 100644
> --- a/target/arm/arch_dump.c
> +++ b/target/arm/arch_dump.c
> @@ -232,12 +232,11 @@ static int aarch64_write_elf64_sve(WriteCoreDumpFunction f,
>  #endif
>
>  int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>  {
>      struct aarch64_note note;
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> -    DumpState *s = opaque;
>      uint64_t pstate, sp;
>      int ret, i;
>
> @@ -360,12 +359,11 @@ static int arm_write_elf32_vfp(WriteCoreDumpFunction f, CPUARMState *env,
>  }
>
>  int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>  {
>      struct arm_note note;
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> -    DumpState *s = opaque;
>      int ret, i;
>      bool fpvalid = cpu_isar_feature(aa32_vfp_simd, cpu);
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5168e3d837..fc8b358779 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1100,9 +1100,9 @@ int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
>  const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname);
>
>  int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>  int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>
>  #ifdef TARGET_AARCH64
>  int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index 004141fc04..c290910a04 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -42,7 +42,7 @@ typedef struct {
>
>  static int x86_64_write_elf64_note(WriteCoreDumpFunction f,
>                                     CPUX86State *env, int id,
> -                                   void *opaque)
> +                                   DumpState *s)
>  {
>      x86_64_user_regs_struct regs;
>      Elf64_Nhdr *note;
> @@ -94,7 +94,7 @@ static int x86_64_write_elf64_note(WriteCoreDumpFunction f,
>      buf += descsz - sizeof(x86_64_user_regs_struct)-sizeof(target_ulong);
>      memcpy(buf, &regs, sizeof(x86_64_user_regs_struct));
>
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>      g_free(note);
>      if (ret < 0) {
>          return -1;
> @@ -148,7 +148,7 @@ static void x86_fill_elf_prstatus(x86_elf_prstatus *prstatus, CPUX86State *env,
>  }
>
>  static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
> -                                int id, void *opaque)
> +                                int id, DumpState *s)
>  {
>      x86_elf_prstatus prstatus;
>      Elf64_Nhdr *note;
> @@ -170,7 +170,7 @@ static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
>      buf += ROUND_UP(name_size, 4);
>      memcpy(buf, &prstatus, sizeof(prstatus));
>
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>      g_free(note);
>      if (ret < 0) {
>          return -1;
> @@ -180,7 +180,7 @@ static int x86_write_elf64_note(WriteCoreDumpFunction f, CPUX86State *env,
>  }
>
>  int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      int ret;
> @@ -189,10 +189,10 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>      bool lma = !!(first_x86_cpu->env.hflags & HF_LMA_MASK);
>
>      if (lma) {
> -        ret = x86_64_write_elf64_note(f, &cpu->env, cpuid, opaque);
> +        ret = x86_64_write_elf64_note(f, &cpu->env, cpuid, s);
>      } else {
>  #endif
> -        ret = x86_write_elf64_note(f, &cpu->env, cpuid, opaque);
> +        ret = x86_write_elf64_note(f, &cpu->env, cpuid, s);
>  #ifdef TARGET_X86_64
>      }
>  #endif
> @@ -201,7 +201,7 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>  }
>
>  int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                             int cpuid, void *opaque)
> +                             int cpuid, DumpState *s)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      x86_elf_prstatus prstatus;
> @@ -224,7 +224,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>      buf += ROUND_UP(name_size, 4);
>      memcpy(buf, &prstatus, sizeof(prstatus));
>
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>      g_free(note);
>      if (ret < 0) {
>          return -1;
> @@ -329,7 +329,7 @@ static void qemu_get_cpustate(QEMUCPUState *s, CPUX86State *env)
>
>  static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
>                                        CPUX86State *env,
> -                                      void *opaque,
> +                                      DumpState *s,
>                                        int type)
>  {
>      QEMUCPUState state;
> @@ -369,7 +369,7 @@ static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
>      buf += ROUND_UP(name_size, 4);
>      memcpy(buf, &state, sizeof(state));
>
> -    ret = f(note, note_size, opaque);
> +    ret = f(note, note_size, s);
>      g_free(note);
>      if (ret < 0) {
>          return -1;
> @@ -379,19 +379,19 @@ static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
>  }
>
>  int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cs,
> -                                 void *opaque)
> +                                 DumpState *s)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>
> -    return cpu_write_qemu_note(f, &cpu->env, opaque, 1);
> +    return cpu_write_qemu_note(f, &cpu->env, s, 1);
>  }
>
>  int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cs,
> -                                 void *opaque)
> +                                 DumpState *s)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>
> -    return cpu_write_qemu_note(f, &cpu->env, opaque, 0);
> +    return cpu_write_qemu_note(f, &cpu->env, s, 0);
>  }
>
>  int cpu_get_dump_info(ArchDumpInfo *info,
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 82004b65b9..b75108d6a3 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1938,13 +1938,13 @@ extern const VMStateDescription vmstate_x86_cpu;
>  int x86_cpu_pending_interrupt(CPUState *cs, int interrupt_request);
>
>  int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cpu,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>  int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cpu,
> -                             int cpuid, void *opaque);
> +                             int cpuid, DumpState *s);
>  int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
> -                                 void *opaque);
> +                                 DumpState *s);
>  int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
> -                                 void *opaque);
> +                                 DumpState *s);
>
>  void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
>                                  Error **errp);
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 1139cead9f..f58e6359d5 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -270,23 +270,23 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>  static int ppc_write_all_elf_notes(const char *note_name,
>                                     WriteCoreDumpFunction f,
>                                     PowerPCCPU *cpu, int id,
> -                                   void *opaque)
> +                                   DumpState *s)
>  {
> -    NoteFuncArg arg = { .state = opaque };
> +    NoteFuncArg arg = { .state = s };
>      int ret = -1;
>      int note_size;
>      const NoteFuncDesc *nf;
>
>      for (nf = note_func; nf->note_contents_func; nf++) {
> -        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);
> +        arg.note.hdr.n_namesz = cpu_to_dump32(s, sizeof(arg.note.name));
> +        arg.note.hdr.n_descsz = cpu_to_dump32(s, nf->contents_size);
>          strncpy(arg.note.name, note_name, sizeof(arg.note.name));
>
>          (*nf->note_contents_func)(&arg, cpu);
>
>          note_size =
>              sizeof(arg.note) - sizeof(arg.note.contents) + nf->contents_size;
> -        ret = f(&arg.note, note_size, opaque);
> +        ret = f(&arg.note, note_size, s);
>          if (ret < 0) {
>              return -1;
>          }
> @@ -295,15 +295,15 @@ static int ppc_write_all_elf_notes(const char *note_name,
>  }
>
>  int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, opaque);
> +    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, s);
>  }
>
>  int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, opaque);
> +    return ppc_write_all_elf_notes("CORE", f, cpu, cpuid, s);
>  }
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a4c893cfad..f7d28fe093 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1354,9 +1354,9 @@ void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
>  const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
>  #endif
>  int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>  int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>  #ifndef CONFIG_USER_ONLY
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> diff --git a/target/riscv/arch_dump.c b/target/riscv/arch_dump.c
> index 709f621d82..736a232956 100644
> --- a/target/riscv/arch_dump.c
> +++ b/target/riscv/arch_dump.c
> @@ -64,12 +64,11 @@ static void riscv64_note_init(struct riscv64_note *note, DumpState *s,
>  }
>
>  int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>  {
>      struct riscv64_note note;
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      CPURISCVState *env = &cpu->env;
> -    DumpState *s = opaque;
>      int ret, i = 0;
>      const char name[] = "CORE";
>
> @@ -134,12 +133,11 @@ static void riscv32_note_init(struct riscv32_note *note, DumpState *s,
>  }
>
>  int riscv_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque)
> +                               int cpuid, DumpState *s)
>  {
>      struct riscv32_note note;
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      CPURISCVState *env = &cpu->env;
> -    DumpState *s = opaque;
>      int ret, i;
>      const char name[] = "CORE";
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5c7acc055a..9699504721 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -505,9 +505,9 @@ extern const char * const riscv_fpr_regnames[];
>  const char *riscv_cpu_get_trap_name(target_ulong cause, bool async);
>  void riscv_cpu_do_interrupt(CPUState *cpu);
>  int riscv_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>  int riscv_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> -                               int cpuid, void *opaque);
> +                               int cpuid, DumpState *s);
>  int riscv_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>  int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  int riscv_cpu_hviprio_index2irq(int index, int *out_irq, int *out_rdzero);
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index 08daf93ae1..f60a14920d 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -204,7 +204,7 @@ static const NoteFuncDesc note_linux[] = {
>  static int s390x_write_elf64_notes(const char *note_name,
>                                         WriteCoreDumpFunction f,
>                                         S390CPU *cpu, int id,
> -                                       void *opaque,
> +                                       DumpState *s,
>                                         const NoteFuncDesc *funcs)
>  {
>      Note note;
> @@ -222,7 +222,7 @@ static int s390x_write_elf64_notes(const char *note_name,
>          (*nf->note_contents_func)(&note, cpu, id);
>
>          note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
> -        ret = f(&note, note_size, opaque);
> +        ret = f(&note, note_size, s);
>
>          if (ret < 0) {
>              return -1;
> @@ -235,16 +235,16 @@ static int s390x_write_elf64_notes(const char *note_name,
>
>
>  int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                              int cpuid, void *opaque)
> +                              int cpuid, DumpState *s)
>  {
>      S390CPU *cpu = S390_CPU(cs);
>      int r;
>
> -    r = s390x_write_elf64_notes("CORE", f, cpu, cpuid, opaque, note_core);
> +    r = s390x_write_elf64_notes("CORE", f, cpu, cpuid, s, note_core);
>      if (r) {
>          return r;
>      }
> -    return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, opaque, note_linux);
> +    return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
>  }
>
>  int cpu_get_dump_info(ArchDumpInfo *info,
> diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
> index 6aba7fd0ca..b5ae0ae364 100644
> --- a/target/s390x/s390x-internal.h
> +++ b/target/s390x/s390x-internal.h
> @@ -227,7 +227,7 @@ static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
>
>  /* arch_dump.c */
>  int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> -                              int cpuid, void *opaque);
> +                              int cpuid, DumpState *s);
>
>
>  /* cc_helper.c */
> --
> 2.34.1
>

Re: [PATCH v5 03/18] dump: Refactor dump_iterate and introduce dump_filter_memblock_*()

[Marc-André Lureau]

On Thu, Aug 11, 2022 at 4:12 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> The iteration over the memblocks in dump_iterate() is hard to
> understand so it's about time to clean it up. Instead of manually
> grabbing the next memblock we can use QTAILQ_FOREACH to iterate over
> all memblocks.
>
> Additionally we move the calculation of the offset and length out by
> introducing and using the dump_filter_memblock_*() functions. These
> functions will later be used to cleanup other parts of dump.c.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c | 80 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 45 insertions(+), 35 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 0ed7cf9c7b..340de5a1e7 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -591,31 +591,43 @@ static void dump_begin(DumpState *s, Error **errp)
>      write_elf_notes(s, errp);
>  }
>
> -static int get_next_block(DumpState *s, GuestPhysBlock *block)
> +static int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
> +                                           int64_t filter_area_start,
> +                                           int64_t filter_area_length)
>  {
> -    while (1) {
> -        block = QTAILQ_NEXT(block, next);
> -        if (!block) {
> -            /* no more block */
> -            return 1;
> -        }
> +    int64_t size, left, right;
>
> -        s->start = 0;
> -        s->next_block = block;
> -        if (s->has_filter) {
> -            if (block->target_start >= s->begin + s->length ||
> -                block->target_end <= s->begin) {
> -                /* This block is out of the range */
> -                continue;
> -            }
> -
> -            if (s->begin > block->target_start) {
> -                s->start = s->begin - block->target_start;
> -            }
> -        }
> -
> -        return 0;
> +    /* No filter, return full size */
> +    if (!filter_area_length) {
> +        return block->target_end - block->target_start;
>      }
> +
> +    /* calculate the overlapped region. */
> +    left = MAX(filter_area_start, block->target_start);
> +    right = MIN(filter_area_start + filter_area_length, block->target_end);
> +    size = right - left;
> +    size = size > 0 ? size : 0;
> +
> +    return size;
> +}
> +
> +static int64_t dump_filtered_memblock_start(GuestPhysBlock *block,
> +                                            int64_t filter_area_start,
> +                                            int64_t filter_area_length)
> +{
> +    if (filter_area_length) {
> +        /* return -1 if the block is not within filter area */
> +        if (block->target_start >= filter_area_start + filter_area_length ||
> +            block->target_end <= filter_area_start) {
> +            return -1;
> +        }
> +
> +        if (filter_area_start > block->target_start) {
> +            return filter_area_start - block->target_start;
> +        }
> +    }
> +
> +    return 0;
>  }
>
>  /* write all memory to vmcore */
> @@ -623,24 +635,22 @@ static void dump_iterate(DumpState *s, Error **errp)
>  {
>      ERRP_GUARD();
>      GuestPhysBlock *block;
> -    int64_t size;
> +    int64_t memblock_size, memblock_start;
>
> -    do {
> -        block = s->next_block;
> -
> -        size = block->target_end - block->target_start;
> -        if (s->has_filter) {
> -            size -= s->start;
> -            if (s->begin + s->length < block->target_end) {
> -                size -= block->target_end - (s->begin + s->length);
> -            }
> +    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> +        memblock_start = dump_filtered_memblock_start(block, s->begin, s->length);
> +        if (memblock_start == -1) {
> +            continue;
>          }
> -        write_memory(s, block, s->start, size, errp);
> +
> +        memblock_size = dump_filtered_memblock_size(block, s->begin, s->length);
> +
> +        /* Write the memory to file */
> +        write_memory(s, block, memblock_start, memblock_size, errp);
>          if (*errp) {
>              return;
>          }
> -
> -    } while (!get_next_block(s, block));
> +    }
>  }
>
>  static void create_vmcore(DumpState *s, Error **errp)
> --
> 2.34.1
>

Re: [PATCH v5 05/18] dump: Rework filter area variables

[Marc-André Lureau]

Hi

On Thu, Aug 11, 2022 at 4:12 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> While the DumpState begin and length variables directly mirror the API
> variable names they are not very descriptive. So let's add a
> "filter_area_" prefix and make has_filter a function checking length > 0.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 53 +++++++++++++++++++++++++------------------
>  include/sysemu/dump.h | 13 ++++++++---
>  2 files changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index e204912a89..b043337bc7 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -59,6 +59,11 @@ static inline bool dump_is_64bit(DumpState *s)
>      return s->dump_info.d_class == ELFCLASS64;
>  }
>
> +static inline bool dump_has_filter(DumpState *s)

I'd drop the inline, and let the compiler decide.


Otherwise:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +{
> +    return s->filter_area_length > 0;
> +}
> +
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
>  {
>      if (s->dump_info.d_endian == ELFDATA2LSB) {
> @@ -443,29 +448,30 @@ static void get_offset_range(hwaddr phys_addr,
>      *p_offset = -1;
>      *p_filesz = 0;
>
> -    if (s->has_filter) {
> -        if (phys_addr < s->begin || phys_addr >= s->begin + s->length) {
> +    if (dump_has_filter(s)) {
> +        if (phys_addr < s->filter_area_begin ||
> +            phys_addr >= s->filter_area_begin + s->filter_area_length) {
>              return;
>          }
>      }
>
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> -        if (s->has_filter) {
> -            if (block->target_start >= s->begin + s->length ||
> -                block->target_end <= s->begin) {
> +        if (dump_has_filter(s)) {
> +            if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
> +                block->target_end <= s->filter_area_begin) {
>                  /* This block is out of the range */
>                  continue;
>              }
>
> -            if (s->begin <= block->target_start) {
> +            if (s->filter_area_begin <= block->target_start) {
>                  start = block->target_start;
>              } else {
> -                start = s->begin;
> +                start = s->filter_area_begin;
>              }
>
>              size_in_block = block->target_end - start;
> -            if (s->begin + s->length < block->target_end) {
> -                size_in_block -= block->target_end - (s->begin + s->length);
> +            if (s->filter_area_begin + s->filter_area_length < block->target_end) {
> +                size_in_block -= block->target_end - (s->filter_area_begin + s->filter_area_length);
>              }
>          } else {
>              start = block->target_start;
> @@ -638,12 +644,12 @@ static void dump_iterate(DumpState *s, Error **errp)
>      int64_t memblock_size, memblock_start;
>
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> -        memblock_start = dump_filtered_memblock_start(block, s->begin, s->length);
> +        memblock_start = dump_filtered_memblock_start(block, s->filter_area_begin, s->filter_area_length);
>          if (memblock_start == -1) {
>              continue;
>          }
>
> -        memblock_size = dump_filtered_memblock_size(block, s->begin, s->length);
> +        memblock_size = dump_filtered_memblock_size(block, s->filter_area_begin, s->filter_area_length);
>
>          /* Write the memory to file */
>          write_memory(s, block, memblock_start, memblock_size, errp);
> @@ -1504,14 +1510,14 @@ static int validate_start_block(DumpState *s)
>  {
>      GuestPhysBlock *block;
>
> -    if (!s->has_filter) {
> +    if (!dump_has_filter(s)) {
>          return 0;
>      }
>
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
>          /* This block is out of the range */
> -        if (block->target_start >= s->begin + s->length ||
> -            block->target_end <= s->begin) {
> +        if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
> +            block->target_end <= s->filter_area_begin) {
>              continue;
>          }
>          return 0;
> @@ -1550,10 +1556,10 @@ static int64_t dump_calculate_size(DumpState *s)
>      int64_t size = 0, total = 0, left = 0, right = 0;
>
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> -        if (s->has_filter) {
> +        if (dump_has_filter(s)) {
>              /* calculate the overlapped region. */
> -            left = MAX(s->begin, block->target_start);
> -            right = MIN(s->begin + s->length, block->target_end);
> +            left = MAX(s->filter_area_begin, block->target_start);
> +            right = MIN(s->filter_area_begin + s->filter_area_length, block->target_end);
>              size = right - left;
>              size = size > 0 ? size : 0;
>          } else {
> @@ -1643,9 +1649,12 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      }
>
>      s->fd = fd;
> -    s->has_filter = has_filter;
> -    s->begin = begin;
> -    s->length = length;
> +    if (has_filter && !length) {
> +        error_setg(errp, QERR_INVALID_PARAMETER, "length");
> +        goto cleanup;
> +    }
> +    s->filter_area_begin = begin;
> +    s->filter_area_length = length;
>
>      memory_mapping_list_init(&s->list);
>
> @@ -1778,8 +1787,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          return;
>      }
>
> -    if (s->has_filter) {
> -        memory_mapping_filter(&s->list, s->begin, s->length);
> +    if (dump_has_filter(s)) {
> +        memory_mapping_filter(&s->list, s->filter_area_begin, s->filter_area_length);
>      }
>
>      /*
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 7fce1d4af6..b62513d87d 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -166,9 +166,16 @@ typedef struct DumpState {
>      hwaddr memory_offset;
>      int fd;
>
> -    bool has_filter;
> -    int64_t begin;
> -    int64_t length;
> +    /*
> +     * Dump filter area variables
> +     *
> +     * A filtered dump only contains the guest memory designated by
> +     * the start address and length variables defined below.
> +     *
> +     * If length is 0, no filtering is applied.
> +     */
> +    int64_t filter_area_begin;  /* Start address of partial guest memory area */
> +    int64_t filter_area_length; /* Length of partial guest memory area */
>
>      uint8_t *note_buf;          /* buffer for notes */
>      size_t note_buf_offset;     /* the writing place in note_buf */
> --
> 2.34.1
>

Re: [PATCH v5 07/18] dump: Split elf header functions into prepare and write

[Marc-André Lureau]

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

On Thu, Aug 11, 2022 at 4:29 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's split the write from the modification of the elf header so we
> can consolidate the write of the data in one function.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  dump/dump.c | 100 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 53 insertions(+), 47 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d82cc46d7d..8a2a97a85e 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -131,7 +131,7 @@ static int fd_write_vmcore(const void *buf, size_t
> size, void *opaque)
>      return 0;
>  }
>
> -static void write_elf64_header(DumpState *s, Error **errp)
> +static void prepare_elf64_header(DumpState *s, Elf64_Ehdr *elf_header)
>  {
>      /*
>       * phnum in the elf header is 16 bit, if we have more segments we
> @@ -139,34 +139,27 @@ static void write_elf64_header(DumpState *s, Error
> **errp)
>       * special section.
>       */
>      uint16_t phnum = MIN(s->phdr_num, PN_XNUM);
> -    Elf64_Ehdr elf_header;
> -    int ret;
>
> -    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_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, s->phdr_offset);
> -    elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
> -    elf_header.e_phnum = cpu_to_dump16(s, phnum);
> +    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_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, s->phdr_offset);
> +    elf_header->e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
> +    elf_header->e_phnum = cpu_to_dump16(s, phnum);
>      if (s->shdr_num) {
> -        elf_header.e_shoff = cpu_to_dump64(s, s->shdr_offset);
> -        elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> -        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
> -    }
> -
> -    ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret, "dump: failed to write elf header");
> +        elf_header->e_shoff = cpu_to_dump64(s, s->shdr_offset);
> +        elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> +        elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num);
>      }
>  }
>
> -static void write_elf32_header(DumpState *s, Error **errp)
> +static void prepare_elf32_header(DumpState *s, Elf32_Ehdr *elf_header)
>  {
>      /*
>       * phnum in the elf header is 16 bit, if we have more segments we
> @@ -174,28 +167,45 @@ static void write_elf32_header(DumpState *s, Error
> **errp)
>       * special section.
>       */
>      uint16_t phnum = MIN(s->phdr_num, PN_XNUM);
> -    Elf32_Ehdr elf_header;
> +
> +    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] = s->dump_info.d_endian;
> +    elf_header->e_ident[EI_VERSION] = EV_CURRENT;
> +    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, s->phdr_offset);
> +    elf_header->e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
> +    elf_header->e_phnum = cpu_to_dump16(s, phnum);
> +    if (s->shdr_num) {
> +        elf_header->e_shoff = cpu_to_dump32(s, s->shdr_offset);
> +        elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> +        elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num);
> +    }
> +}
> +
> +static void write_elf_header(DumpState *s, Error **errp)
> +{
> +    Elf32_Ehdr elf32_header;
> +    Elf64_Ehdr elf64_header;
> +    size_t header_size;
> +    void *header_ptr;
>      int ret;
>
> -    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] = s->dump_info.d_endian;
> -    elf_header.e_ident[EI_VERSION] = EV_CURRENT;
> -    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, s->phdr_offset);
> -    elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
> -    elf_header.e_phnum = cpu_to_dump16(s, phnum);
> -    if (s->shdr_num) {
> -        elf_header.e_shoff = cpu_to_dump32(s, s->shdr_offset);
> -        elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> -        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
> +    if (dump_is_64bit(s)) {
> +        prepare_elf64_header(s, &elf64_header);
> +        header_size = sizeof(elf64_header);
> +        header_ptr = &elf64_header;
> +    } else {
> +        prepare_elf32_header(s, &elf32_header);
> +        header_size = sizeof(elf32_header);
> +        header_ptr = &elf32_header;
>      }
>
> -    ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> +    ret = fd_write_vmcore(header_ptr, header_size, s);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "dump: failed to write elf header");
>      }
> @@ -564,11 +574,7 @@ static void dump_begin(DumpState *s, Error **errp)
>       */
>
>      /* write elf header to vmcore */
> -    if (dump_is_64bit(s)) {
> -        write_elf64_header(s, errp);
> -    } else {
> -        write_elf32_header(s, errp);
> -    }
> +    write_elf_header(s, errp);
>      if (*errp) {
>          return;
>      }
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 8295 bytes --]

Re: [PATCH v5 08/18] dump: Rename write_elf*_phdr_note to prepare_elf*_phdr_note

[Marc-André Lureau]

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

On Thu, Aug 11, 2022 at 4:43 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> The functions in question do not actually write to the file descriptor
> they set up a buffer which is later written to the fd.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  dump/dump.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 8a2a97a85e..a905316fe5 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -260,7 +260,7 @@ static void write_elf32_load(DumpState *s,
> MemoryMapping *memory_mapping,
>      }
>  }
>
> -static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
> +static void prepare_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
>  {
>      memset(phdr, 0, sizeof(*phdr));
>      phdr->p_type = cpu_to_dump32(s, PT_NOTE);
> @@ -316,7 +316,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f,
> DumpState *s,
>      write_guest_note(f, s, errp);
>  }
>
> -static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
> +static void prepare_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
>  {
>      memset(phdr, 0, sizeof(*phdr));
>      phdr->p_type = cpu_to_dump32(s, PT_NOTE);
> @@ -364,11 +364,11 @@ static void write_elf_phdr_note(DumpState *s, Error
> **errp)
>      int ret;
>
>      if (dump_is_64bit(s)) {
> -        write_elf64_phdr_note(s, &phdr64);
> +        prepare_elf64_phdr_note(s, &phdr64);
>          size = sizeof(phdr64);
>          phdr = &phdr64;
>      } else {
> -        write_elf32_phdr_note(s, &phdr32);
> +        prepare_elf32_phdr_note(s, &phdr32);
>          size = sizeof(phdr32);
>          phdr = &phdr32;
>      }
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2682 bytes --]

Re: [PATCH v5 09/18] dump: Use a buffer for ELF section data and headers

[Marc-André Lureau]

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

Hi

On Thu, Aug 11, 2022 at 4:16 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> Currently we're writing the NULL section header if we overflow the
> physical header number in the ELF header. But in the future we'll add
> custom section headers AND section data.
>
> To facilitate this we need to rearange section handling a bit. As with
> the other ELF headers we split the code into a prepare and a write
> step.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
---
>  dump/dump.c           | 83 +++++++++++++++++++++++++++++--------------
>  include/sysemu/dump.h |  2 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a905316fe5..0051c71d08 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error
> **errp)
>      }
>  }
>
> -static void write_elf_section(DumpState *s, int type, Error **errp)
> +static void prepare_elf_section_hdr_zero(DumpState *s)
>  {
> -    Elf32_Shdr shdr32;
> -    Elf64_Shdr shdr64;
> -    int shdr_size;
> -    void *shdr;
> +    if (dump_is_64bit(s)) {
> +        Elf64_Shdr *shdr64 = s->elf_section_hdrs;
> +
> +        shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
> +    } else {
> +        Elf32_Shdr *shdr32 = s->elf_section_hdrs;
> +
> +        shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
> +    }
> +}
> +
> +static void prepare_elf_section_hdrs(DumpState *s)
> +{
> +    size_t len, sizeof_shdr;
> +
> +    /*
> +     * Section ordering:
> +     * - HDR zero
> +     */
> +    sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) :
> sizeof(Elf32_Shdr);
> +    len = sizeof_shdr * s->shdr_num;
> +    s->elf_section_hdrs = g_malloc0(len);
> +
> +    /*
> +     * The first section header is ALWAYS a special initial section
> +     * header.
> +     *
> +     * The header should be 0 with one exception being that if
> +     * phdr_num is PN_XNUM then the sh_info field contains the real
> +     * number of segment entries.
> +     *
> +     * As we zero allocate the buffer we will only need to modify
> +     * sh_info for the PN_XNUM case.
> +     */
> +    if (s->phdr_num >= PN_XNUM) {
> +        prepare_elf_section_hdr_zero(s);
> +    }
> +}
> +
> +static void write_elf_section_headers(DumpState *s, Error **errp)
> +{
> +    size_t sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) :
> sizeof(Elf32_Shdr);
>      int ret;
>
> -    if (type == 0) {
> -        shdr_size = sizeof(Elf32_Shdr);
> -        memset(&shdr32, 0, shdr_size);
> -        shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
> -        shdr = &shdr32;
> -    } else {
> -        shdr_size = sizeof(Elf64_Shdr);
> -        memset(&shdr64, 0, shdr_size);
> -        shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> -        shdr = &shdr64;
> -    }
> +    prepare_elf_section_hdrs(s);
>
> -    ret = fd_write_vmcore(shdr, shdr_size, s);
> +    ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr,
> s);
>      if (ret < 0) {
> -        error_setg_errno(errp, -ret,
> -                         "dump: failed to write section header table");
> +        error_setg_errno(errp, -ret, "dump: failed to write section
> headers");
>      }
>  }
>
> @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>
> +    /* write section headers to vmcore */
> +    write_elf_section_headers(s, errp);
> +    if (*errp) {
> +        return;
> +    }
>

Can you make that move a separate commit? And please explain why this is
valid, and also update the table in the comment too.

Otherwise, changes look good to me.

+
>      /* write PT_NOTE to vmcore */
>      write_elf_phdr_note(s, errp);
>      if (*errp) {
> @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>
> -    /* write section to vmcore */
> -    if (s->shdr_num) {
> -        write_elf_section(s, 1, errp);
> -        if (*errp) {
> -            return;
> -        }
> -    }
> -
>      /* write notes to vmcore */
>      write_elf_notes(s, errp);
>  }
> @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp)
>          return;
>      }
>
> +    /* Iterate over memory and dump it to file */
>      dump_iterate(s, errp);
> +    if (*errp) {
> +        return;
> +    }
>  }
>
>  static int write_start_flat_header(int fd)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b62513d87d..9995f65dc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -177,6 +177,8 @@ typedef struct DumpState {
>      int64_t filter_area_begin;  /* Start address of partial guest memory
> area */
>      int64_t filter_area_length; /* Length of partial guest memory area */
>
> +    void *elf_section_hdrs;     /* Pointer to section header buffer */
> +
>      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 */
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6689 bytes --]

Re: [PATCH v5 17/18] s390x: Add KVM PV dump interface

[Steffen Eiden]

On 8/11/22 14:11, Janosch Frank wrote:
> Let's add a few bits of code which hide the new KVM PV dump API from
> us via new functions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   hw/s390x/pv.c         | 51 +++++++++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/pv.h |  8 +++++++
>   2 files changed, 59 insertions(+)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 2b892b45e8..ce3b6ad3e9 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -175,6 +175,57 @@ bool kvm_s390_pv_info_basic_valid(void)
>       return info_valid;
>   }
>   
> +static int s390_pv_dump_cmd(uint64_t subcmd, uint64_t uaddr, uint64_t gaddr,
> +                            uint64_t len)
> +{
> +    struct kvm_s390_pv_dmp dmp = {
> +        .subcmd = subcmd,
> +        .buff_addr = uaddr,
> +        .buff_len = len,
> +        .gaddr = gaddr,
> +    };
> +    int ret;
> +
> +    ret = s390_pv_cmd(KVM_PV_DUMP, (void *)&dmp);
> +    if (ret) {
> +        error_report("KVM DUMP command %ld failed", subcmd);
> +    }
> +    return ret;
> +}
> +
> +int kvm_s390_dump_cpu(S390CPU *cpu, void *buff)
> +{
> +    struct kvm_s390_pv_dmp dmp = {
> +        .subcmd = KVM_PV_DUMP_CPU,
> +        .buff_addr = (uint64_t)buff,
> +        .gaddr = 0,
> +        .buff_len = info_dump.dump_cpu_buffer_len,
> +    };
> +    struct kvm_pv_cmd pv = {
> +        .cmd = KVM_PV_DUMP,
> +        .data = (uint64_t)&dmp,
> +    };
> +
> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_S390_PV_CPU_COMMAND, &pv);
> +}
> +
> +int kvm_s390_dump_init(void)
> +{
> +    return s390_pv_dump_cmd(KVM_PV_DUMP_INIT, 0, 0, 0);
> +}
> +

I suggest changing that into something like
  * kvm_s390_dump_mem_state
  * kvm_s390_dump_config_state

This would make the effect of that function more clear.
It is not dumping the memory, but getting (part of) the
necessary metadata to process the dump.

Additionally, I suggest to reflect the name changes in the next patch.
I mark all functions I find.
> +int kvm_s390_dump_mem(uint64_t gaddr, size_t len, void *dest)
> +{
> +    return s390_pv_dump_cmd(KVM_PV_DUMP_CONFIG_STATE, (uint64_t)dest,
> +                            gaddr, len);
> +}
> +
> +int kvm_s390_dump_complete(void *buff)
> +{
> +    return s390_pv_dump_cmd(KVM_PV_DUMP_COMPLETE, (uint64_t)buff, 0,
> +                            info_dump.dump_config_finalize_len);
> +}
> +
>   #define TYPE_S390_PV_GUEST "s390-pv-guest"
>   OBJECT_DECLARE_SIMPLE_TYPE(S390PVGuest, S390_PV_GUEST)
>   
> diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
> index 573259cf2b..02a6c06b9f 100644
> --- a/include/hw/s390x/pv.h
> +++ b/include/hw/s390x/pv.h
> @@ -51,6 +51,10 @@ uint64_t kvm_s390_pv_dmp_get_size_cpu(void);
>   uint64_t kvm_s390_pv_dmp_get_size_stor_state(void);
>   uint64_t kvm_s390_pv_dmp_get_size_complete(void);
>   bool kvm_s390_pv_info_basic_valid(void);
> +int kvm_s390_dump_init(void);
> +int kvm_s390_dump_cpu(S390CPU *cpu, void *buff);
> +int kvm_s390_dump_mem(uint64_t addr, size_t len, void *dest);
> +int kvm_s390_dump_complete(void *buff);
>   #else /* CONFIG_KVM */
>   static inline bool s390_is_pv(void) { return false; }
>   static inline int s390_pv_query_info(void) { return 0; }
> @@ -66,6 +70,10 @@ static inline uint64_t kvm_s390_pv_dmp_get_size_cpu(void) { return 0; }
>   static inline uint64_t kvm_s390_pv_dmp_get_size_stor_state(void) { return 0; }
>   static inline uint64_t kvm_s390_pv_dmp_get_size_complete(void) { return 0; }
>   static inline bool kvm_s390_pv_info_basic_valid(void) { return false; }
> +static inline int kvm_s390_dump_init(void) { return 0; }
> +static inline int kvm_s390_dump_cpu(S390CPU *cpu, void *buff, size_t len) { return 0; }
> +static inline int kvm_s390_dump_mem(uint64_t addr, size_t len, void *dest) { return 0; }
> +static inline int kvm_s390_dump_complete(void *buff) { return 0; }
>   #endif /* CONFIG_KVM */
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);

Beside that nit, LGTM.

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [PATCH v5 18/18] s390x: pv: Add dump support

[Steffen Eiden]

On 8/11/22 14:11, Janosch Frank wrote:
> Sometimes dumping a guest from the outside is the only way to get the
> data that is needed. This can be the case if a dumping mechanism like
> KDUMP hasn't been configured or data needs to be fetched at a specific
> point. Dumping a protected guest from the outside without help from
> fw/hw doesn't yield sufficient data to be useful. Hence we now
> introduce PV dump support.
> 
> The PV dump support works by integrating the firmware into the dump
> process. New Ultravisor calls are used to initiate the dump process,
> dump cpu data, dump memory state and lastly complete the dump process.
> The UV calls are exposed by KVM via the new KVM_PV_DUMP command and
> its subcommands. The guest's data is fully encrypted and can only be
> decrypted by the entity that owns the customer communication key for
> the dumped guest. Also dumping needs to be allowed via a flag in the
> SE header.
> 
> On the QEMU side of things we store the PV dump data in the newly
> introduced architecture ELF sections (storage state and completion
> data) and the cpu notes (for cpu dump data).
> 
> Users can use the zgetdump tool to convert the encrypted QEMU dump to an
> unencrypted one.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   dump/dump.c              |  12 +-
>   include/sysemu/dump.h    |   5 +
>   target/s390x/arch_dump.c | 242 ++++++++++++++++++++++++++++++++++-----
>   3 files changed, 227 insertions(+), 32 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 65b18fc602..7cf5eb7c8b 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -720,9 +720,9 @@ static void dump_begin(DumpState *s, Error **errp)
>       write_elf_notes(s, errp);
>   }
>   
> -static int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
> -                                           int64_t filter_area_start,
> -                                           int64_t filter_area_length)
> +int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
> +                                    int64_t filter_area_start,
> +                                    int64_t filter_area_length)
>   {
>       int64_t size, left, right;
>   
> @@ -740,9 +740,9 @@ static int64_t dump_filtered_memblock_size(GuestPhysBlock *block,
>       return size;
>   }
>   
> -static int64_t dump_filtered_memblock_start(GuestPhysBlock *block,
> -                                            int64_t filter_area_start,
> -                                            int64_t filter_area_length)
> +int64_t dump_filtered_memblock_start(GuestPhysBlock *block,
> +                                     int64_t filter_area_start,
> +                                     int64_t filter_area_length)
>   {
>       if (filter_area_length) {
>           /* return -1 if the block is not within filter area */
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 358b038d47..245c26fbca 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -216,4 +216,9 @@ typedef struct 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);
> +
> +int64_t dump_filtered_memblock_size(GuestPhysBlock *block, int64_t filter_area_start,
> +                                    int64_t filter_area_length);
> +int64_t dump_filtered_memblock_start(GuestPhysBlock *block, int64_t filter_area_start,
> +                                     int64_t filter_area_length);
>   #endif
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index f60a14920d..5e8e03d536 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -16,7 +16,8 @@
>   #include "s390x-internal.h"
>   #include "elf.h"
>   #include "sysemu/dump.h"
> -
> +#include "hw/s390x/pv.h"
> +#include "kvm/kvm_s390x.h"
>   
>   struct S390xUserRegsStruct {
>       uint64_t psw[2];
> @@ -76,9 +77,16 @@ typedef struct noteStruct {
>           uint64_t todcmp;
>           uint32_t todpreg;
>           uint64_t ctrs[16];
> +        uint8_t dynamic[1];  /*
> +                              * Would be a flexible array member, if
> +                              * that was legal inside a union. Real
> +                              * size comes from PV info interface.
> +                              */
>       } contents;
>   } QEMU_PACKED Note;
>   
> +static bool pv_dump_initialized;
> +
>   static void s390x_write_elf64_prstatus(Note *note, S390CPU *cpu, int id)
>   {
>       int i;
> @@ -177,28 +185,39 @@ static void s390x_write_elf64_prefix(Note *note, S390CPU *cpu, int id)
>       note->contents.prefix = cpu_to_be32((uint32_t)(cpu->env.psa));
>   }
>   
> +static void s390x_write_elf64_pv(Note *note, S390CPU *cpu, int id)
> +{
> +    note->hdr.n_type = cpu_to_be32(NT_S390_PV_CPU_DATA);
> +    if (!pv_dump_initialized) {
> +        return;
> +    }
> +    kvm_s390_dump_cpu(cpu, &note->contents.dynamic);
> +}
>   
>   typedef struct NoteFuncDescStruct {
>       int contents_size;
> +    uint64_t (*note_size_func)(void); /* NULL for non-dynamic sized contents */
>       void (*note_contents_func)(Note *note, S390CPU *cpu, int id);
> +    bool pvonly;
>   } NoteFuncDesc;
>   
>   static const NoteFuncDesc note_core[] = {
> -    {sizeof_field(Note, contents.prstatus), s390x_write_elf64_prstatus},
> -    {sizeof_field(Note, contents.fpregset), s390x_write_elf64_fpregset},
> -    { 0, NULL}
> +    {sizeof_field(Note, contents.prstatus), NULL, s390x_write_elf64_prstatus, false},
> +    {sizeof_field(Note, contents.fpregset), NULL, s390x_write_elf64_fpregset, false}, > +    { 0, NULL, NULL}
You are missing a false here.
>   };
>   
>   static const NoteFuncDesc note_linux[] = {
> -    {sizeof_field(Note, contents.prefix),   s390x_write_elf64_prefix},
> -    {sizeof_field(Note, contents.ctrs),     s390x_write_elf64_ctrs},
> -    {sizeof_field(Note, contents.timer),    s390x_write_elf64_timer},
> -    {sizeof_field(Note, contents.todcmp),   s390x_write_elf64_todcmp},
> -    {sizeof_field(Note, contents.todpreg),  s390x_write_elf64_todpreg},
> -    {sizeof_field(Note, contents.vregslo),  s390x_write_elf64_vregslo},
> -    {sizeof_field(Note, contents.vregshi),  s390x_write_elf64_vregshi},
> -    {sizeof_field(Note, contents.gscb),     s390x_write_elf64_gscb},
> -    { 0, NULL}
> +    {sizeof_field(Note, contents.prefix),   NULL, s390x_write_elf64_prefix,  false},
> +    {sizeof_field(Note, contents.ctrs),     NULL, s390x_write_elf64_ctrs,    false},
> +    {sizeof_field(Note, contents.timer),    NULL, s390x_write_elf64_timer,   false},
> +    {sizeof_field(Note, contents.todcmp),   NULL, s390x_write_elf64_todcmp,  false},
> +    {sizeof_field(Note, contents.todpreg),  NULL, s390x_write_elf64_todpreg, false},
> +    {sizeof_field(Note, contents.vregslo),  NULL, s390x_write_elf64_vregslo, false},
> +    {sizeof_field(Note, contents.vregshi),  NULL, s390x_write_elf64_vregshi, false},
> +    {sizeof_field(Note, contents.gscb),     NULL, s390x_write_elf64_gscb,    false},
> +    {0, kvm_s390_pv_dmp_get_size_cpu,       s390x_write_elf64_pv, true} > +    { 0, NULL, NULL}
and here.
>   };
>   
>   static int s390x_write_elf64_notes(const char *note_name,
> @@ -207,22 +226,41 @@ static int s390x_write_elf64_notes(const char *note_name,
>                                          DumpState *s,
>                                          const NoteFuncDesc *funcs)
>   {
> -    Note note;
> +    Note note, *notep;
>       const NoteFuncDesc *nf;
> -    int note_size;
> +    int note_size, content_size;
>       int ret = -1;
>   
>       assert(strlen(note_name) < sizeof(note.name));
>   
>       for (nf = funcs; nf->note_contents_func; nf++) {
> -        memset(&note, 0, sizeof(note));
> -        note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> -        note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
> -        g_strlcpy(note.name, note_name, sizeof(note.name));
> -        (*nf->note_contents_func)(&note, cpu, id);
> +        notep = &note;
> +        if (nf->pvonly && !s390_is_pv()) {
> +            continue;
> +        }
>   
> -        note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
> -        ret = f(&note, note_size, s);
> +        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();
> +        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
> +
> +        /* Notes with dynamic sizes need to allocate a note */
> +        if (nf->note_size_func) {
> +            notep = g_malloc0(note_size);
> +        }
> +
> +        memset(notep, 0, sizeof(note));
> +
> +        /* Setup note header data */
> +        notep->hdr.n_descsz = cpu_to_be32(content_size);
> +        notep->hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> +        g_strlcpy(notep->name, note_name, sizeof(notep->name));
> +
> +        /* Get contents and write them out */
> +        (*nf->note_contents_func)(notep, cpu, id);
> +        ret = f(notep, note_size, s);
> +
> +        if (nf->note_size_func) {
> +            g_free(notep);
> +        }
>   
>           if (ret < 0) {
>               return -1;
> @@ -247,12 +285,161 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>       return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
>   }
>   
> +/* PV dump section size functions */
> +static uint64_t get_dump_stor_state_size_from_len(uint64_t len)
> +{
> +    return (len / (1 << 20)) * kvm_s390_pv_dmp_get_size_stor_state();
> +}
> +
> +static uint64_t get_size_stor_state(DumpState *s)
> +{
> +    return get_dump_stor_state_size_from_len(s->total_size);
> +}
> +
> +static uint64_t get_size_complete(DumpState *s)
> +{
> +    return kvm_s390_pv_dmp_get_size_complete();
> +}
> +
> +/* PV dump section data functions*/
> +static int get_data_complete(DumpState *s, uint8_t *buff)
> +{
> +    int rc;
> +
> +    if (!pv_dump_initialized) {
> +        return 0;
> +    }
> +    rc = kvm_s390_dump_complete(buff);
> +    if (!rc) {
> +            pv_dump_initialized = false;
> +    }
> +    return rc;
> +}
> +
Similar to my comments in the last patch:
dump_mem_state
> +static int dump_mem(DumpState *s, uint64_t gaddr, uint8_t *buff, uint64_t buff_len)
> +{
> +    /* We need the gaddr + len and something to write to */
> +    if (!pv_dump_initialized) {
> +        return 0;
> +    }
> +    return kvm_s390_dump_mem(gaddr, buff_len, buff);
> +}
> +
get_data_mem_state
> +static int get_data_mem(DumpState *s, uint8_t *buff)
> +{
> +    int64_t memblock_size, memblock_start;
> +    GuestPhysBlock *block;
> +    uint64_t off;
> +
> +    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> +        memblock_start = dump_filtered_memblock_start(block, s->filter_area_begin,
> +                                                      s->filter_area_length);
> +        if (memblock_start == -1) {
> +            continue;
> +        }
> +
> +        memblock_size = dump_filtered_memblock_size(block, s->filter_area_begin,
> +                                                    s->filter_area_length);
> +
> +        off = get_dump_stor_state_size_from_len(block->target_start);
> +        dump_mem(s, block->target_start, buff + off,
> +                 get_dump_stor_state_size_from_len(memblock_size));
> +    }
> +
> +    return 0;
> +}
> +
> +struct sections {
> +    uint64_t (*sections_size_func)(DumpState *s);
> +    int (*sections_contents_func)(DumpState *s, uint8_t *buff);
> +    char sctn_str[12];
> +} sections[] = {
> +    { get_size_stor_state, get_data_mem, "pv_mem_meta"},
> +    { get_size_complete, get_data_complete, "pv_compl"},
> +    {NULL , NULL, ""}
> +};
> +
> +static uint64_t arch_sections_write_hdr(DumpState *s, uint8_t *buff)
> +{
> +    Elf64_Shdr *shdr = (void *)buff;
> +    struct sections *sctn = sections;
> +    uint64_t off = s->section_offset;
> +
> +    if (!s390_is_pv()) {
> +        return 0;
> +    }
> +
> +    for (; sctn->sections_size_func; off += shdr->sh_size, sctn++, shdr++) {
> +        memset(shdr, 0, sizeof(*shdr));
> +        shdr->sh_type = SHT_PROGBITS;
> +        shdr->sh_offset = off;
> +        shdr->sh_size = sctn->sections_size_func(s);
> +        shdr->sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, sctn->sctn_str, sizeof(sctn->sctn_str));
> +    }
> +
> +    return (uintptr_t)shdr - (uintptr_t)buff;
> +}
> +
> +
> +/* Add arch specific number of sections and their respective sizes */
> +static void arch_sections_add(DumpState *s)
> +{
> +    struct sections *sctn = sections;
> +
> +    /*
> +     * We only do a PV dump if we are running a PV guest, KVM supports
> +     * the dump API and we got valid dump length information.
> +     */
> +    if (!s390_is_pv() || !kvm_s390_get_protected_dump() ||
> +        !kvm_s390_pv_info_basic_valid()) {
> +        return;
> +    }
> +
> +    /*
> +     * Start the UV dump process by doing the initialize dump call via
> +     * KVM as the proxy.
> +     */
> +    if (!kvm_s390_dump_init()) {
> +            pv_dump_initialized = true;
> +    }
> +
> +    for (; sctn->sections_size_func; sctn++) {
> +        s->shdr_num += 1;
> +        s->elf_section_data_size += sctn->sections_size_func(s);
> +    }
> +
> +    /* We use the string table to identify the sections */
> +    s->string_table_usage = true;
> +}
> +
> +/*
> + * After the PV dump has been initialized, the CPU data has been
> + * fetched and memory has been dumped, we need to grab the tweak data
> + * and the completion data.
> + */
> +static void arch_sections_write(DumpState *s, uint8_t *buff)
> +{
> +    struct sections *sctn = sections;
> +
> +    /* shdr_num should only have been set > 1 if we are protected */
> +    assert(s390_is_pv());
> +
> +    for (; sctn->sections_size_func; sctn++) {
> +        sctn->sections_contents_func(s, buff);
> +        buff += sctn->sections_size_func(s);
> +    }
> +}
> +
>   int cpu_get_dump_info(ArchDumpInfo *info,
>                         const struct GuestPhysBlockList *guest_phys_blocks)
>   {
>       info->d_machine = EM_S390;
>       info->d_endian = ELFDATA2MSB;
>       info->d_class = ELFCLASS64;
> +    info->arch_sections_add_fn = *arch_sections_add;
> +    info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
> +    info->arch_sections_write_fn = *arch_sections_write;
>   
>       return 0;
>   }
> @@ -261,7 +448,7 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>   {
>       int name_size = 8; /* "LINUX" or "CORE" + pad */
>       size_t elf_note_size = 0;
> -    int note_head_size;
> +    int note_head_size, content_size;
>       const NoteFuncDesc *nf;
>   
>       assert(class == ELFCLASS64);
> @@ -270,12 +457,15 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>       note_head_size = sizeof(Elf64_Nhdr);
>   
>       for (nf = note_core; nf->note_contents_func; nf++) {
> -        elf_note_size = elf_note_size + note_head_size + name_size +
> -                        nf->contents_size;
> +        elf_note_size = elf_note_size + note_head_size + name_size + nf->contents_size;
>       }
>       for (nf = note_linux; nf->note_contents_func; nf++) {
> +        if (nf->pvonly && !s390_is_pv()) {
> +            continue;
> +        }
> +        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();
>           elf_note_size = elf_note_size + note_head_size + name_size +
> -                        nf->contents_size;
> +                        content_size;
>       }
>   
>       return (elf_note_size) * nr_cpus;

LGTM, and I can confirm this produces a valid pv-dump that can be
decrypted.

Please have a look on my comments.

With the nits fixed:
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [PATCH v5 15/18] s390x: Add protected dump cap

[Thomas Huth]

On 11/08/2022 14.11, Janosch Frank wrote:
> Add a protected dump capability for later feature checking.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   target/s390x/kvm/kvm.c       | 7 +++++++
>   target/s390x/kvm/kvm_s390x.h | 1 +
>   2 files changed, 8 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v5 16/18] s390x: Introduce PV query interface

[Thomas Huth]

On 11/08/2022 14.11, Janosch Frank wrote:
> Introduce an interface over which we can get information about UV data.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   hw/s390x/pv.c              | 61 ++++++++++++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c |  6 ++++
>   include/hw/s390x/pv.h      | 10 +++++++
>   3 files changed, 77 insertions(+)

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v5 18/18] s390x: pv: Add dump support

[Thomas Huth]

On 11/08/2022 14.11, Janosch Frank wrote:
> Sometimes dumping a guest from the outside is the only way to get the
> data that is needed. This can be the case if a dumping mechanism like
> KDUMP hasn't been configured or data needs to be fetched at a specific
> point. Dumping a protected guest from the outside without help from
> fw/hw doesn't yield sufficient data to be useful. Hence we now
> introduce PV dump support.
> 
> The PV dump support works by integrating the firmware into the dump
> process. New Ultravisor calls are used to initiate the dump process,
> dump cpu data, dump memory state and lastly complete the dump process.
> The UV calls are exposed by KVM via the new KVM_PV_DUMP command and
> its subcommands. The guest's data is fully encrypted and can only be
> decrypted by the entity that owns the customer communication key for
> the dumped guest. Also dumping needs to be allowed via a flag in the
> SE header.
> 
> On the QEMU side of things we store the PV dump data in the newly
> introduced architecture ELF sections (storage state and completion
> data) and the cpu notes (for cpu dump data).
> 
> Users can use the zgetdump tool to convert the encrypted QEMU dump to an
> unencrypted one.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
[...]
> @@ -207,22 +226,41 @@ static int s390x_write_elf64_notes(const char *note_name,
>                                          DumpState *s,
>                                          const NoteFuncDesc *funcs)
>   {
> -    Note note;
> +    Note note, *notep;
>       const NoteFuncDesc *nf;
> -    int note_size;
> +    int note_size, content_size;
>       int ret = -1;
>   
>       assert(strlen(note_name) < sizeof(note.name));
>   
>       for (nf = funcs; nf->note_contents_func; nf++) {
> -        memset(&note, 0, sizeof(note));
> -        note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> -        note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
> -        g_strlcpy(note.name, note_name, sizeof(note.name));
> -        (*nf->note_contents_func)(&note, cpu, id);
> +        notep = &note;
> +        if (nf->pvonly && !s390_is_pv()) {
> +            continue;
> +        }
>   
> -        note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
> -        ret = f(&note, note_size, s);
> +        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();
> +        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
> +
> +        /* Notes with dynamic sizes need to allocate a note */
> +        if (nf->note_size_func) {
> +            notep = g_malloc0(note_size);

Either use g_malloc() here (without the trailing "0") ...

> +        }
> +
> +        memset(notep, 0, sizeof(note));

... or put the memset() in an "else" block.

> +        /* Setup note header data */
> +        notep->hdr.n_descsz = cpu_to_be32(content_size);
> +        notep->hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> +        g_strlcpy(notep->name, note_name, sizeof(notep->name));
> +
> +        /* Get contents and write them out */
> +        (*nf->note_contents_func)(notep, cpu, id);
> +        ret = f(notep, note_size, s);
> +
> +        if (nf->note_size_func) {
> +            g_free(notep);
> +        }
>   
>           if (ret < 0) {
>               return -1;
> @@ -247,12 +285,161 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>       return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
>   }
>   
> +/* PV dump section size functions */
> +static uint64_t get_dump_stor_state_size_from_len(uint64_t len)
> +{
> +    return (len / (1 << 20)) * kvm_s390_pv_dmp_get_size_stor_state();

Use "MiB" instead of "1 << 20" ?

> +}
> +
> +static uint64_t get_size_stor_state(DumpState *s)
> +{
> +    return get_dump_stor_state_size_from_len(s->total_size);
> +}

  Thomas

Re: [PATCH v5 04/18] dump: Rework get_start_block

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:10 +0000, Janosch Frank wrote:
> get_start_block() returns the start address of the first memory block
> or -1.
> 
> With the GuestPhysBlock iterator conversion we don't need to set the
> start address and can therefore remove that code and the "start"
> DumpState struct member. The only functionality left is the validation
> of the start block so it only makes sense to re-name the function to
> validate_start_block()

Nit, since you don't return an address anymore, I find retaining the -
1/0 return value instead of true/false weird.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  dump/dump.c           | 20 ++++++--------------
>  include/sysemu/dump.h |  2 --
>  2 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 340de5a1e7..e204912a89 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1500,30 +1500,22 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
>      }
>  }
>  
> -static ram_addr_t get_start_block(DumpState *s)
> +static int validate_start_block(DumpState *s)
>  {
>      GuestPhysBlock *block;
>  
>      if (!s->has_filter) {
> -        s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>          return 0;
>      }
>  
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> +        /* This block is out of the range */
>          if (block->target_start >= s->begin + s->length ||
>              block->target_end <= s->begin) {
> -            /* This block is out of the range */
>              continue;
>          }
> -
> -        s->next_block = block;
> -        if (s->begin > block->target_start) {
> -            s->start = s->begin - block->target_start;
> -        } else {
> -            s->start = 0;
> -        }
> -        return s->start;
> -    }
> +        return 0;
> +   }
>  
>      return -1;
>  }
> @@ -1670,8 +1662,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          goto cleanup;
>      }
>  
> -    s->start = get_start_block(s);
> -    if (s->start == -1) {
> +    /* Is the filter filtering everything? */
> +    if (validate_start_block(s) == -1) {
>          error_setg(errp, QERR_INVALID_PARAMETER, "begin");
>          goto cleanup;
>      }
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index ffc2ea1072..7fce1d4af6 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -166,8 +166,6 @@ typedef struct DumpState {
>      hwaddr memory_offset;
>      int fd;
>  
> -    GuestPhysBlock *next_block;
> -    ram_addr_t start;
>      bool has_filter;
>      int64_t begin;
>      int64_t length;

Re: [PATCH v5 09/18] dump: Use a buffer for ELF section data and headers

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Currently we're writing the NULL section header if we overflow the
> physical header number in the ELF header. But in the future we'll add
> custom section headers AND section data.
> 
> To facilitate this we need to rearange section handling a bit. As with
> the other ELF headers we split the code into a prepare and a write
> step.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 83 +++++++++++++++++++++++++++++--------------
>  include/sysemu/dump.h |  2 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index a905316fe5..0051c71d08 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error **errp)
>      }
>  }
>  
> -static void write_elf_section(DumpState *s, int type, Error **errp)
> +static void prepare_elf_section_hdr_zero(DumpState *s)
>  {
> -    Elf32_Shdr shdr32;
> -    Elf64_Shdr shdr64;
> -    int shdr_size;
> -    void *shdr;
> +    if (dump_is_64bit(s)) {
> +        Elf64_Shdr *shdr64 = s->elf_section_hdrs;
> +
> +        shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
> +    } else {
> +        Elf32_Shdr *shdr32 = s->elf_section_hdrs;
> +
> +        shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
> +    }
> +}
> +
> +static void prepare_elf_section_hdrs(DumpState *s)
> +{
> +    size_t len, sizeof_shdr;
> +
> +    /*
> +     * Section ordering:
> +     * - HDR zero
> +     */
> +    sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +    len = sizeof_shdr * s->shdr_num;
> +    s->elf_section_hdrs = g_malloc0(len);

I'm not seeing this being freed.
> +
> +    /*
> +     * The first section header is ALWAYS a special initial section
> +     * header.
> +     *
> +     * The header should be 0 with one exception being that if
> +     * phdr_num is PN_XNUM then the sh_info field contains the real
> +     * number of segment entries.
> +     *
> +     * As we zero allocate the buffer we will only need to modify
> +     * sh_info for the PN_XNUM case.
> +     */
> +    if (s->phdr_num >= PN_XNUM) {
> +        prepare_elf_section_hdr_zero(s);
> +    }
> +}
> +
> +static void write_elf_section_headers(DumpState *s, Error **errp)

[...]

> @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>  
> +    /* write section headers to vmcore */
> +    write_elf_section_headers(s, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>      /* write PT_NOTE to vmcore */
>      write_elf_phdr_note(s, errp);
>      if (*errp) {
> @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>  
> -    /* write section to vmcore */
> -    if (s->shdr_num) {
> -        write_elf_section(s, 1, errp);
> -        if (*errp) {
> -            return;
> -        }
> -    }
> -

Here you change the order of the headers, but the elf header is only
fixed in patch 11.
I agree that this should be a separate patch, with an explanation on
why it's necessary. 
So basically squashed into patch 11, except I think the comment change
in that one should go into another patch.

>      /* write notes to vmcore */
>      write_elf_notes(s, errp);
>  }
> @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp)
>          return;
>      }
>  
> +    /* Iterate over memory and dump it to file */
>      dump_iterate(s, errp);
> +    if (*errp) {
> +        return;
> +    }
>  }
>  
>  static int write_start_flat_header(int fd)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b62513d87d..9995f65dc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -177,6 +177,8 @@ typedef struct DumpState {
>      int64_t filter_area_begin;  /* Start address of partial guest memory area */
>      int64_t filter_area_length; /* Length of partial guest memory area */
>  
> +    void *elf_section_hdrs;     /* Pointer to section header buffer */
> +
>      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 */

Re: [PATCH v5 12/18] dump/dump: Add section string table support

[Steffen Eiden]

Hi Janosch,

On 8/11/22 14:11, Janosch Frank wrote:
> As sections don't have a type like the notes do we need another way to
> determine their contents. The string table allows us to assign each
> section an identification string which architectures can then use to
> tag their sections with.
> 
> There will be no string table if the architecture doesn't add custom
> sections which are introduced in a following patch.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   dump/dump.c           | 71 +++++++++++++++++++++++++++++++++++++++++++
>   include/sysemu/dump.h |  4 +++
>   2 files changed, 75 insertions(+)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 31eb20108c..0d6dbf453a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
[ snip ]
>   }
>   
> +static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;
> +    int shdr_size;
> +    void *shdr;
> +
> +    if (dump_is_64bit(s)) {
> +        shdr_size = sizeof(Elf64_Shdr);
> +        memset(&shdr64, 0, shdr_size);
> +        shdr64.sh_type = SHT_STRTAB;
> +        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
> +        shdr64.sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
I think you mixed up .strtab and .shstrtab here.
'.shstrtab' should be used here.

The ELF specs define bots as follows (from man 5 elf) :

        .shstrtab
               This section holds section names.  This section is of type
               SHT_STRTAB.  No attribute types are used.

        .strtab
               This section holds strings, most commonly the strings that
               represent the names associated with symbol table entries.
               If the file has a loadable segment that includes the
               symbol string table, the section's attributes will include
               the SHF_ALLOC bit.  Otherwise, the bit will be off.  This
               section is of type SHT_STRTAB.

However, the name lookup works, as you correctly specified that this 
section holds the section header names via the 'e_shstrndx' field in the 
elf header.

> +        shdr64.sh_size = s->string_table_buf->len;
> +        shdr = &shdr64;
> +    } else {
> +        shdr_size = sizeof(Elf32_Shdr);
> +        memset(&shdr32, 0, shdr_size);
> +        shdr32.sh_type = SHT_STRTAB;
> +        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
> +        shdr32.sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> +        shdr32.sh_size = s->string_table_buf->len;
> +        shdr = &shdr32;
> +    }
> +
> +    memcpy(buff, shdr, shdr_size);
> +
[snip]
Also, with your patches the dump output places the headers in this ordering:
[elf hdr]
[section hdrs]
[program hdrs]

**normally** program hdrs are placed before section hdrs,
but this is just a convention IIRC.


Steffen

Re: [PATCH v5 12/18] dump/dump: Add section string table support

[Janosch Frank]

On 8/30/22 13:35, Steffen Eiden wrote:
> Hi Janosch,
> 
> On 8/11/22 14:11, Janosch Frank wrote:
>> As sections don't have a type like the notes do we need another way to
>> determine their contents. The string table allows us to assign each
>> section an identification string which architectures can then use to
>> tag their sections with.
>>
>> There will be no string table if the architecture doesn't add custom
>> sections which are introduced in a following patch.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    dump/dump.c           | 71 +++++++++++++++++++++++++++++++++++++++++++
>>    include/sysemu/dump.h |  4 +++
>>    2 files changed, 75 insertions(+)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 31eb20108c..0d6dbf453a 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
> [ snip ]
>>    }
>>    
>> +static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
>> +{
>> +    Elf32_Shdr shdr32;
>> +    Elf64_Shdr shdr64;
>> +    int shdr_size;
>> +    void *shdr;
>> +
>> +    if (dump_is_64bit(s)) {
>> +        shdr_size = sizeof(Elf64_Shdr);
>> +        memset(&shdr64, 0, shdr_size);
>> +        shdr64.sh_type = SHT_STRTAB;
>> +        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
>> +        shdr64.sh_name = s->string_table_buf->len;
>> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> I think you mixed up .strtab and .shstrtab here.
> '.shstrtab' should be used here.
> 
> The ELF specs define bots as follows (from man 5 elf) :
> 
>          .shstrtab
>                 This section holds section names.  This section is of type
>                 SHT_STRTAB.  No attribute types are used.
> 
>          .strtab
>                 This section holds strings, most commonly the strings that
>                 represent the names associated with symbol table entries.
>                 If the file has a loadable segment that includes the
>                 symbol string table, the section's attributes will include
>                 the SHF_ALLOC bit.  Otherwise, the bit will be off.  This
>                 section is of type SHT_STRTAB.
> 
> However, the name lookup works, as you correctly specified that this
> section holds the section header names via the 'e_shstrndx' field in the
> elf header.

Sigh
We can make this a shstrtab only strtab since that's effectively what it 
does right now. It annoys me that we'll need a second strtab if we ever 
want to name other structures. Or at least we'll need special handling.

> 
>> +        shdr64.sh_size = s->string_table_buf->len;
>> +        shdr = &shdr64;
>> +    } else {
>> +        shdr_size = sizeof(Elf32_Shdr);
>> +        memset(&shdr32, 0, shdr_size);
>> +        shdr32.sh_type = SHT_STRTAB;
>> +        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
>> +        shdr32.sh_name = s->string_table_buf->len;
>> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
>> +        shdr32.sh_size = s->string_table_buf->len;
>> +        shdr = &shdr32;
>> +    }
>> +
>> +    memcpy(buff, shdr, shdr_size);
>> +
> [snip]
> Also, with your patches the dump output places the headers in this ordering:
> [elf hdr]
> [section hdrs]
> [program hdrs]
> 
> **normally** program hdrs are placed before section hdrs,
> but this is just a convention IIRC.

I don't see why this should be a problem, that's what the offsets are for.

> 
> 
> Steffen
> 

Re: [PATCH v5 06/18] dump: Rework dump_calculate_size function

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:10 +0000, Janosch Frank wrote:
> dump_calculate_size() sums up all the sizes of the guest memory
> blocks. Since we already have a function that calculates the size of a
> single memory block (dump_get_memblock_size()) we can simply iterate
> over the blocks and use the function instead of calculating the size
> ourselves.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

> ---
>  dump/dump.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index b043337bc7..d82cc46d7d 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1548,25 +1548,19 @@ bool qemu_system_dump_in_progress(void)
>      return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE);
>  }
>  
> -/* calculate total size of memory to be dumped (taking filter into
> - * acoount.) */
> +/*
> + * calculate total size of memory to be dumped (taking filter into
> + * account.)
> + */
>  static int64_t dump_calculate_size(DumpState *s)
>  {
>      GuestPhysBlock *block;
> -    int64_t size = 0, total = 0, left = 0, right = 0;
> +    int64_t total = 0;
>  
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> -        if (dump_has_filter(s)) {
> -            /* calculate the overlapped region. */
> -            left = MAX(s->filter_area_begin, block->target_start);
> -            right = MIN(s->filter_area_begin + s->filter_area_length, block->target_end);
> -            size = right - left;
> -            size = size > 0 ? size : 0;
> -        } else {
> -            /* count the whole region in */
> -            size = (block->target_end - block->target_start);
> -        }
> -        total += size;
> +        total += dump_filtered_memblock_size(block,
> +                                             s->filter_area_begin,
> +                                             s->filter_area_length);
>      }
>  
>      return total;

Re: [PATCH v5 07/18] dump: Split elf header functions into prepare and write

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Let's split the write from the modification of the elf header so we
> can consolidate the write of the data in one function.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

This is cosmetic only, right?

Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Re: [PATCH v5 08/18] dump: Rename write_elf*_phdr_note to prepare_elf*_phdr_note

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> The functions in question do not actually write to the file descriptor
> they set up a buffer which is later written to the fd.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Re: [PATCH v5 12/18] dump/dump: Add section string table support

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> As sections don't have a type like the notes do we need another way to
> determine their contents. The string table allows us to assign each
> section an identification string which architectures can then use to
> tag their sections with.
> 
> There will be no string table if the architecture doesn't add custom
> sections which are introduced in a following patch.

Why? Is there any harm in always having a (possibly empty) string
table? Can't put garbage into sh_name then but that's not relevant.
Seems like it would make the code a bit simpler.

I would also expect the string data to be written in this patch,
instead you do that in the next.
With that and Steffen's .shstrtab addressed:
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Some minor suggestions below.

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 71 +++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/dump.h |  4 +++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 31eb20108c..0d6dbf453a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c

[...]

> @@ -393,17 +400,50 @@ static void prepare_elf_section_hdr_zero(DumpState *s)
>      }
>  }
>  
> +static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;

Could just = {} those and drop the memset below.

> +    int shdr_size;
> +    void *shdr;
> +
> +    if (dump_is_64bit(s)) {
> +        shdr_size = sizeof(Elf64_Shdr);
> +        memset(&shdr64, 0, shdr_size);
> +        shdr64.sh_type = SHT_STRTAB;
> +        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
> +        shdr64.sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));

Could put the ".shstrtab" in a char[] variable.

> +        shdr64.sh_size = s->string_table_buf->len;
> +        shdr = &shdr64;
> +    } else {
> +        shdr_size = sizeof(Elf32_Shdr);
> +        memset(&shdr32, 0, shdr_size);
> +        shdr32.sh_type = SHT_STRTAB;
> +        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
> +        shdr32.sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> +        shdr32.sh_size = s->string_table_buf->len;
> +        shdr = &shdr32;
> +    }
> +
> +    memcpy(buff, shdr, shdr_size);
> +}

[...]

> @@ -1844,6 +1903,18 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          }
>      }
>  
> +    /*
> +     * calculate shdr_num and elf_section_data_size so we know the offsets and

What is the elf_section_data_size thing about?

> +     * sizes of all parts.
> +     *
> +     * If phdr_num overflowed we have at least one section header
> +     * More sections/hdrs can be added by the architectures
> +     */
> +    if (s->shdr_num > 1) {
> +        /* Reserve the string table */
> +        s->shdr_num += 1;
> +    }
> +
>      if (dump_is_64bit(s)) {
>          s->shdr_offset = sizeof(Elf64_Ehdr);
>          s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;

[...]

Re: [PATCH v5 13/18] dump/dump: Add arch section support

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Add hooks which architectures can use to add arbitrary data to custom
> sections.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c                | 120 ++++++++++++++++++++++++++++++-------
>  include/sysemu/dump-arch.h |   3 +
>  2 files changed, 100 insertions(+), 23 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 0d6dbf453a..65b18fc602 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -430,7 +430,7 @@ static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
>      memcpy(buff, shdr, shdr_size);
>  }
>  
> -static void prepare_elf_section_hdrs(DumpState *s)
> +static void prepare_elf_section_hdrs(DumpState *s, Error **errp)
>  {
>      size_t len, sizeof_shdr;
>      void *buff_hdr;
> @@ -438,6 +438,7 @@ static void prepare_elf_section_hdrs(DumpState *s)
>      /*
>       * Section ordering:
>       * - HDR zero
> +     * - Arch section hdrs
>       * - String table hdr
>       */
>      sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> @@ -465,6 +466,16 @@ static void prepare_elf_section_hdrs(DumpState *s)
>          return;
>      }
>  
> +    /* Add architecture defined section headers */
> +    if (s->dump_info.arch_sections_write_hdr_fn) {
> +        buff_hdr += s->dump_info.arch_sections_write_hdr_fn(s, buff_hdr);
> +
> +        if (s->shdr_num >= SHN_LORESERVE) {

               /* TODO: raise limit by encoding via sh_link */

> +            error_setg_errno(errp, EINVAL, "dump: too many architecture defined sections");
> +            return;
> +        }
> +    }
> +
>      /*
>       * String table is the last section since strings are added via
>       * arch_sections_write_hdr().
> @@ -477,7 +488,10 @@ static void write_elf_section_headers(DumpState *s, Error **errp)
>      size_t sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
>      int ret;
>  
> -    prepare_elf_section_hdrs(s);
> +    prepare_elf_section_hdrs(s, errp);
> +    if (*errp) {

You're depending on errp not being NULL here, which it isn't, but it
doesn't seem like good style to me.
error.h recommends to also indicate success/failure via the return
value if possible. prepare_elf_section_hdrs returns void right now, so
it's easy so side step it this way.
(ERRP_GUARD would work too of course)

> +        return;
> +    }
>  
>      ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s);
>      if (ret < 0) {
> @@ -485,6 +499,30 @@ static void write_elf_section_headers(DumpState *s, Error **errp)
>      }
>  }
>  
> +static void write_elf_sections(DumpState *s, Error **errp)
> +{
> +    int ret;
> +
> +    if (!s->elf_section_data_size) {
> +        return;
> +    }
> +
> +    /* Write architecture section data */
> +    ret = fd_write_vmcore(s->elf_section_data,
> +                          s->elf_section_data_size, s);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "dump: failed to write architecture section  data");

Looks like two spaces between section and data.

> +        return;
> +    }
> +
> +    /* Write string table */
> +    ret = fd_write_vmcore(s->string_table_buf->data,
> +                          s->string_table_buf->len, s);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "dump: failed to write string table data");
> +    }
> +}
> +
>  static void write_data(DumpState *s, void *buf, int length, Error **errp)
>  {
>      int ret;
> @@ -744,6 +782,24 @@ static void dump_iterate(DumpState *s, Error **errp)
>      }
>  }
>  
> +static void dump_end(DumpState *s, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    if (!s->elf_section_data_size) {
> +        return;
> +    }
> +    s->elf_section_data = g_malloc0(s->elf_section_data_size);

Why zero initialize the memory, do you depend on that?

> +
> +    /* Adds the architecture defined section data to s->elf_section_data  */
> +    if (s->dump_info.arch_sections_write_fn) {
> +        s->dump_info.arch_sections_write_fn(s, s->elf_section_data);
> +    }
> +
> +    /* write sections to vmcore */
> +    write_elf_sections(s, errp);
> +}
> +
>  static void create_vmcore(DumpState *s, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -758,6 +814,9 @@ static void create_vmcore(DumpState *s, Error **errp)
>      if (*errp) {
>          return;
>      }
> +
> +    /* Write the section data */
> +    dump_end(s, errp);
>  }
>  
>  static int write_start_flat_header(int fd)
> @@ -1883,38 +1942,53 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      }
>  
>      /*
> -     * calculate phdr_num
> -     *
> -     * the type of ehdr->e_phnum is uint16_t, so we should avoid overflow
> +     * Adds the number of architecture sections to shdr_num, sets
> +     * string_table_usage and sets elf_section_data_size so we know
> +     * the offsets and sizes of all parts.
>       */
> -    s->phdr_num = 1; /* PT_NOTE */
> -    if (s->list.num < UINT16_MAX - 2) {
> -        s->shdr_num = 0;
> -        s->phdr_num += s->list.num;
> -    } else {
> -        /* sh_info of section 0 holds the real number of phdrs */
> -        s->shdr_num = 1;
> +    if (s->dump_info.arch_sections_add_fn) {
> +        s->dump_info.arch_sections_add_fn(s);
>  
> -        /* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
> -        if (s->list.num <= UINT32_MAX - 1) {
> -            s->phdr_num += s->list.num;
> -        } else {
> -            s->phdr_num = UINT32_MAX;
> +        if (s->shdr_num) {
> +            s->string_table_usage = true;
>          }
>      }

I'm not convinced that this is the right patch for the stuff below.

>      /*
> -     * calculate shdr_num and elf_section_data_size so we know the offsets and
> -     * sizes of all parts.
> +     * Calculate phdr_num
>       *
> -     * If phdr_num overflowed we have at least one section header
> -     * More sections/hdrs can be added by the architectures
> +     * The absolute maximum amount of phdrs is UINT32_MAX - 1 as
> +     * sh_info is 32 bit. There's special handling once we go over
> +     * UINT16_MAX - 1 but that is handled in the ehdr and section
> +     * code.
>       */
> -    if (s->shdr_num > 1) {
> -        /* Reserve the string table */
> +    s->phdr_num = 1; /* Reserve PT_NOTE */
> +    if (s->list.num <= UINT32_MAX - 1) {
> +        s->phdr_num += s->list.num;
> +    } else {
> +        s->phdr_num = UINT32_MAX;
> +    }
> +
> +    /*
> +     * The first section header is always a special one in which most
> +     * fields are 0.
> +     *
> +     * We need it if we have custom sections and if phdr_num >=
> +     * PN_XNUM so we can write the real phdr_num into sh_info.
> +     */
> +    if (s->shdr_num || s->phdr_num >= PN_XNUM) {
>          s->shdr_num += 1;
>      }
>  
> +    /* Reserve the string table for the arch sections if needed */
> +    if (s->string_table_usage) {
> +        s->shdr_num += 1;
> +    }
> +
> +    /*
> +     * Now that the number of section and program headers is known we
> +     * can calculate the offsets of the headers and data.
> +     */
>      if (dump_is_64bit(s)) {
>          s->shdr_offset = sizeof(Elf64_Ehdr);
>          s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
> diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
> index e25b02e990..824c0c1152 100644
> --- a/include/sysemu/dump-arch.h
> +++ b/include/sysemu/dump-arch.h
> @@ -21,6 +21,9 @@ typedef struct ArchDumpInfo {
>      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. */
> +    void (*arch_sections_add_fn)(DumpState *s);
> +    uint64_t (*arch_sections_write_hdr_fn)(DumpState *s, uint8_t *buff);
> +    void (*arch_sections_write_fn)(DumpState *s, uint8_t *buff);
>  } ArchDumpInfo;
>  
>  struct GuestPhysBlockList; /* memory_mapping.h */

Re: [PATCH v5 15/18] s390x: Add protected dump cap

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Add a protected dump capability for later feature checking.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Re: [PATCH v5 16/18] s390x: Introduce PV query interface

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Introduce an interface over which we can get information about UV data.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

With the below fixed:
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

> ---
>  hw/s390x/pv.c              | 61 ++++++++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c |  6 ++++
>  include/hw/s390x/pv.h      | 10 +++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 401b63d6cb..2b892b45e8 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -20,6 +20,11 @@
>  #include "exec/confidential-guest-support.h"
>  #include "hw/s390x/ipl.h"
>  #include "hw/s390x/pv.h"
> +#include "target/s390x/kvm/kvm_s390x.h"
> +
> +static bool info_valid;
> +static struct kvm_s390_pv_info_vm info_vm;
> +static struct kvm_s390_pv_info_dump info_dump;
>  
>  static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
>  {
> @@ -56,6 +61,42 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
>      }                                  \
>  }
>  
> +int s390_pv_query_info(void)
> +{
> +    struct kvm_s390_pv_info info = {
> +        .header.id = KVM_PV_INFO_VM,
> +        .header.len_max = sizeof(info.header) + sizeof(info.vm),
> +    };
> +    int rc;
> +
> +    /* Info API's first user is dump so they are bundled */
> +    if (!kvm_s390_get_protected_dump()) {
> +        return 0;
> +    }
> +
> +    rc = s390_pv_cmd(KVM_PV_INFO, &info);
> +    if (rc) {
> +        error_report("KVM PV INFO cmd %x failed: %s",
> +                     info.header.id, strerror(rc));

Should be strerror(-rc).

> +        return rc;
> +    }
> +    memcpy(&info_vm, &info.vm, sizeof(info.vm));
> +
> +    info.header.id = KVM_PV_INFO_DUMP;
> +    info.header.len_max = sizeof(info.header) + sizeof(info.dump);
> +    rc = s390_pv_cmd(KVM_PV_INFO, &info);
> +    if (rc) {
> +        error_report("KVM PV INFO cmd %x failed: %s",
> +                     info.header.id, strerror(rc));

Same here.

> +        return rc;
> +    }
> +
> +    memcpy(&info_dump, &info.dump, sizeof(info.dump));
> +    info_valid = true;
> +
> +    return rc;
> +}
> +
> 
[...]

Re: [PATCH v5 17/18] s390x: Add KVM PV dump interface

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Let's add a few bits of code which hide the new KVM PV dump API from
> us via new functions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Re: [PATCH v5 18/18] s390x: pv: Add dump support

[Janis Schoetterl-Glausch]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Sometimes dumping a guest from the outside is the only way to get the
> data that is needed. This can be the case if a dumping mechanism like
> KDUMP hasn't been configured or data needs to be fetched at a specific
> point. Dumping a protected guest from the outside without help from
> fw/hw doesn't yield sufficient data to be useful. Hence we now
> introduce PV dump support.
> 
> The PV dump support works by integrating the firmware into the dump
> process. New Ultravisor calls are used to initiate the dump process,
> dump cpu data, dump memory state and lastly complete the dump process.
> The UV calls are exposed by KVM via the new KVM_PV_DUMP command and
> its subcommands. The guest's data is fully encrypted and can only be
> decrypted by the entity that owns the customer communication key for
> the dumped guest. Also dumping needs to be allowed via a flag in the
> SE header.
> 
> On the QEMU side of things we store the PV dump data in the newly
> introduced architecture ELF sections (storage state and completion
> data) and the cpu notes (for cpu dump data).
> 
> Users can use the zgetdump tool to convert the encrypted QEMU dump to an
> unencrypted one.

Does PV dump work when memory is being filtered? Are there any
constraints on the filter parameters, alignment or so?
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c              |  12 +-
>  include/sysemu/dump.h    |   5 +
>  target/s390x/arch_dump.c | 242 ++++++++++++++++++++++++++++++++++-----
>  3 files changed, 227 insertions(+), 32 deletions(-)

[...]
>  
>  typedef struct NoteFuncDescStruct {
>      int contents_size;
> +    uint64_t (*note_size_func)(void); /* NULL for non-dynamic sized contents */
>      void (*note_contents_func)(Note *note, S390CPU *cpu, int id);
> +    bool pvonly;
>  } NoteFuncDesc;
>  
>  static const NoteFuncDesc note_core[] = {
> -    {sizeof_field(Note, contents.prstatus), s390x_write_elf64_prstatus},
> -    {sizeof_field(Note, contents.fpregset), s390x_write_elf64_fpregset},
> -    { 0, NULL}
> +    {sizeof_field(Note, contents.prstatus), NULL, s390x_write_elf64_prstatus, false},
> +    {sizeof_field(Note, contents.fpregset), NULL, s390x_write_elf64_fpregset, false},
> +    { 0, NULL, NULL}
>  };
>  
>  static const NoteFuncDesc note_linux[] = {
> -    {sizeof_field(Note, contents.prefix),   s390x_write_elf64_prefix},
> -    {sizeof_field(Note, contents.ctrs),     s390x_write_elf64_ctrs},
> -    {sizeof_field(Note, contents.timer),    s390x_write_elf64_timer},
> -    {sizeof_field(Note, contents.todcmp),   s390x_write_elf64_todcmp},
> -    {sizeof_field(Note, contents.todpreg),  s390x_write_elf64_todpreg},
> -    {sizeof_field(Note, contents.vregslo),  s390x_write_elf64_vregslo},
> -    {sizeof_field(Note, contents.vregshi),  s390x_write_elf64_vregshi},
> -    {sizeof_field(Note, contents.gscb),     s390x_write_elf64_gscb},
> -    { 0, NULL}
> +    {sizeof_field(Note, contents.prefix),   NULL, s390x_write_elf64_prefix,  false},
> +    {sizeof_field(Note, contents.ctrs),     NULL, s390x_write_elf64_ctrs,    false},
> +    {sizeof_field(Note, contents.timer),    NULL, s390x_write_elf64_timer,   false},
> +    {sizeof_field(Note, contents.todcmp),   NULL, s390x_write_elf64_todcmp,  false},
> +    {sizeof_field(Note, contents.todpreg),  NULL, s390x_write_elf64_todpreg, false},
> +    {sizeof_field(Note, contents.vregslo),  NULL, s390x_write_elf64_vregslo, false},
> +    {sizeof_field(Note, contents.vregshi),  NULL, s390x_write_elf64_vregshi, false},
> +    {sizeof_field(Note, contents.gscb),     NULL, s390x_write_elf64_gscb,    false},
> +    {0, kvm_s390_pv_dmp_get_size_cpu,       s390x_write_elf64_pv, true},
> +    { 0, NULL, NULL}
>  };
>  
>  static int s390x_write_elf64_notes(const char *note_name,
> @@ -207,22 +226,41 @@ static int s390x_write_elf64_notes(const char *note_name,
>                                         DumpState *s,
>                                         const NoteFuncDesc *funcs)
>  {
> -    Note note;
> +    Note note, *notep;
>      const NoteFuncDesc *nf;
> -    int note_size;
> +    int note_size, content_size;

Could make those size_t. I guess it's not necessary, but we're kind of
a dumb pipe for data from the ultravisor so there's something to be
said for not making assumptions.

>      int ret = -1;
>  
>      assert(strlen(note_name) < sizeof(note.name));
>  
>      for (nf = funcs; nf->note_contents_func; nf++) {
> -        memset(&note, 0, sizeof(note));
> -        note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> -        note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
> -        g_strlcpy(note.name, note_name, sizeof(note.name));
> -        (*nf->note_contents_func)(&note, cpu, id);
> +        notep = &note;
> +        if (nf->pvonly && !s390_is_pv()) {
> +            continue;
> +        }
>  
> -        note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
> -        ret = f(&note, note_size, s);
> +        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();

Your comment above says
/* NULL for non-dynamic sized contents */
So it makes sense to condition on that, i.e.
+        content_size = nf->contents_size_func ? nf->note_size_func() : nf->contents_size;
And it makes it consistent with the ifs below

> +        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
> +
> +        /* Notes with dynamic sizes need to allocate a note */
> +        if (nf->note_size_func) {
> +            notep = g_malloc0(note_size);
> +        }
> +
> +        memset(notep, 0, sizeof(note));
> +
> +        /* Setup note header data */
> +        notep->hdr.n_descsz = cpu_to_be32(content_size);
> +        notep->hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> +        g_strlcpy(notep->name, note_name, sizeof(notep->name));
> +
> +        /* Get contents and write them out */
> +        (*nf->note_contents_func)(notep, cpu, id);
> +        ret = f(notep, note_size, s);
> +
> +        if (nf->note_size_func) {
> +            g_free(notep);
> +        }
>  
>          if (ret < 0) {
>              return -1;
> @@ -247,12 +285,161 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>      return s390x_write_elf64_notes("LINUX", f, cpu, cpuid, s, note_linux);
>  }
>  
> +/* PV dump section size functions */
> +static uint64_t get_dump_stor_state_size_from_len(uint64_t len)
> +{
> +    return (len / (1 << 20)) * kvm_s390_pv_dmp_get_size_stor_state();
> +}
> +
> +static uint64_t get_size_stor_state(DumpState *s)
> +{
> +    return get_dump_stor_state_size_from_len(s->total_size);
> +}
> +
> +static uint64_t get_size_complete(DumpState *s)
> +{
> +    return kvm_s390_pv_dmp_get_size_complete();
> +}
> +
> +/* PV dump section data functions*/
> +static int get_data_complete(DumpState *s, uint8_t *buff)
> +{
> +    int rc;
> +
> +    if (!pv_dump_initialized) {
> +        return 0;
> +    }
> +    rc = kvm_s390_dump_complete(buff);
> +    if (!rc) {
> +            pv_dump_initialized = false;
> +    }
> +    return rc;
> +}
> +
> +static int dump_mem(DumpState *s, uint64_t gaddr, uint8_t *buff, uint64_t buff_len)

The DumpState arg is being ignored.

> +{
> +    /* We need the gaddr + len and something to write to */
> +    if (!pv_dump_initialized) {
> +        return 0;
> +    }
> +    return kvm_s390_dump_mem(gaddr, buff_len, buff);
> +}
> +
> +static int get_data_mem(DumpState *s, uint8_t *buff)
> +{
> +    int64_t memblock_size, memblock_start;
> +    GuestPhysBlock *block;
> +    uint64_t off;
> +
> +    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> +        memblock_start = dump_filtered_memblock_start(block, s->filter_area_begin,
> +                                                      s->filter_area_length);
> +        if (memblock_start == -1) {
> +            continue;
> +        }
> +
> +        memblock_size = dump_filtered_memblock_size(block, s->filter_area_begin,
> +                                                    s->filter_area_length);
> +
> +        off = get_dump_stor_state_size_from_len(block->target_start);
> +        dump_mem(s, block->target_start, buff + off,
> +                 get_dump_stor_state_size_from_len(memblock_size));

This ignores the return value, if this is intentional a comment
explaining it would be nice.

> +    }
> +
> +    return 0;
> +}
> +
> +struct sections {
> +    uint64_t (*sections_size_func)(DumpState *s);
> +    int (*sections_contents_func)(DumpState *s, uint8_t *buff);
> +    char sctn_str[12];
> +} sections[] = {
> +    { get_size_stor_state, get_data_mem, "pv_mem_meta"},
> +    { get_size_complete, get_data_complete, "pv_compl"},
> +    {NULL , NULL, ""}
> +};

Could be static right?

> +
> +static uint64_t arch_sections_write_hdr(DumpState *s, uint8_t *buff)
> +{
> +    Elf64_Shdr *shdr = (void *)buff;
> +    struct sections *sctn = sections;
> +    uint64_t off = s->section_offset;
> +
> +    if (!s390_is_pv()) {
> +        return 0;
> +    }
> +
> +    for (; sctn->sections_size_func; off += shdr->sh_size, sctn++, shdr++) {
> +        memset(shdr, 0, sizeof(*shdr));

This isn't necessary since the header mem is zero allocated, but you
might not want to make guarantees about that. Maybe consider doing a
normal malloc and memsetting just the special 0 index section header in
dump.c.

> +        shdr->sh_type = SHT_PROGBITS;
> +        shdr->sh_offset = off;
> +        shdr->sh_size = sctn->sections_size_func(s);
> +        shdr->sh_name = s->string_table_buf->len;
> +        g_array_append_vals(s->string_table_buf, sctn->sctn_str, sizeof(sctn->sctn_str));
> +    }
> +
> +    return (uintptr_t)shdr - (uintptr_t)buff;
> +}
> +
> +
> +/* Add arch specific number of sections and their respective sizes */
> +static void arch_sections_add(DumpState *s)
> +{
> +    struct sections *sctn = sections;
> +
> +    /*
> +     * We only do a PV dump if we are running a PV guest, KVM supports
> +     * the dump API and we got valid dump length information.
> +     */
> +    if (!s390_is_pv() || !kvm_s390_get_protected_dump() ||
> +        !kvm_s390_pv_info_basic_valid()) {
> +        return;
> +    }
> +
> +    /*
> +     * Start the UV dump process by doing the initialize dump call via
> +     * KVM as the proxy.
> +     */
> +    if (!kvm_s390_dump_init()) {
> +            pv_dump_initialized = true;
> +    }
> +
> +    for (; sctn->sections_size_func; sctn++) {
> +        s->shdr_num += 1;
> +        s->elf_section_data_size += sctn->sections_size_func(s);
> +    }
> +
> +    /* We use the string table to identify the sections */
> +    s->string_table_usage = true;

In dump.c this value is being set if shdr_num > 0, so this seems
redundant.

> +}
> +
> +/*
> + * After the PV dump has been initialized, the CPU data has been
> + * fetched and memory has been dumped, we need to grab the tweak data
> + * and the completion data.
> + */
> +static void arch_sections_write(DumpState *s, uint8_t *buff)
> +{
> +    struct sections *sctn = sections;
> +
> +    /* shdr_num should only have been set > 1 if we are protected */
> +    assert(s390_is_pv());
> +
> +    for (; sctn->sections_size_func; sctn++) {
> +        sctn->sections_contents_func(s, buff);

As above with regards to ignoring return values.

> +        buff += sctn->sections_size_func(s);
> +    }
> +}
> +
>  int cpu_get_dump_info(ArchDumpInfo *info,
>                        const struct GuestPhysBlockList *guest_phys_blocks)
>  {
>      info->d_machine = EM_S390;
>      info->d_endian = ELFDATA2MSB;
>      info->d_class = ELFCLASS64;
> +    info->arch_sections_add_fn = *arch_sections_add;
> +    info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
> +    info->arch_sections_write_fn = *arch_sections_write;
>  
>      return 0;
>  }
> @@ -261,7 +448,7 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>  {
>      int name_size = 8; /* "LINUX" or "CORE" + pad */
>      size_t elf_note_size = 0;
> -    int note_head_size;
> +    int note_head_size, content_size;
>      const NoteFuncDesc *nf;
>  
>      assert(class == ELFCLASS64);
> @@ -270,12 +457,15 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>      note_head_size = sizeof(Elf64_Nhdr);
>  
>      for (nf = note_core; nf->note_contents_func; nf++) {
> -        elf_note_size = elf_note_size + note_head_size + name_size +
> -                        nf->contents_size;
> +        elf_note_size = elf_note_size + note_head_size + name_size + nf->contents_size;
>      }
>      for (nf = note_linux; nf->note_contents_func; nf++) {
> +        if (nf->pvonly && !s390_is_pv()) {
> +            continue;
> +        }
> +        content_size = nf->contents_size ? nf->contents_size : nf->note_size_func();
>          elf_note_size = elf_note_size + note_head_size + name_size +
> -                        nf->contents_size;
> +                        content_size;
>      }
>  
>      return (elf_note_size) * nr_cpus;

Re: [PATCH v5 04/18] dump: Rework get_start_block

[Janosch Frank]

On 8/29/22 22:17, Janis Schoetterl-Glausch wrote:
> On Thu, 2022-08-11 at 12:10 +0000, Janosch Frank wrote:
>> get_start_block() returns the start address of the first memory block
>> or -1.
>>
>> With the GuestPhysBlock iterator conversion we don't need to set the
>> start address and can therefore remove that code and the "start"
>> DumpState struct member. The only functionality left is the validation
>> of the start block so it only makes sense to re-name the function to
>> validate_start_block()
> 
> Nit, since you don't return an address anymore, I find retaining the -
> 1/0 return value instead of true/false weird.

I'm trying to wrap up fixing things for a new version.
My fix for this is calling it is_start_block_valid() and making the 
return bool. That being said, having int for true/false is not uncommon 
in C but explicitly checking -1 makes it look weird.



diff --git i/dump/dump.c w/dump/dump.c
index e204912a89..2239bd324e 100644
--- i/dump/dump.c
+++ w/dump/dump.c
@@ -1500,12 +1500,12 @@ static void create_kdump_vmcore(DumpState *s, 
Error **errp)
      }
  }

-static int validate_start_block(DumpState *s)
+static bool is_start_block_valid(DumpState *s)
  {
      GuestPhysBlock *block;

      if (!s->has_filter) {
-        return 0;
+        return true;
      }

      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
@@ -1514,10 +1514,10 @@ static int validate_start_block(DumpState *s)
              block->target_end <= s->begin) {
              continue;
          }
-        return 0;
+        return true;
     }

-    return -1;
+    return false;
  }

  static void get_max_mapnr(DumpState *s)
@@ -1663,7 +1663,7 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
      }

      /* Is the filter filtering everything? */
-    if (validate_start_block(s) == -1) {
+    if (!is_start_block_valid(s)) {
          error_setg(errp, QERR_INVALID_PARAMETER, "begin");
          goto cleanup;
      }