[RFC] make write_elf_xx functions part of CPUClass, use CPUState

Hi Andreas,

not sure if this is what you had in mind. Does it at least go into the right direction?
It is pretty much untested and won't compile as is. I just wanted to get your opinion 
before putting more work into this.

This patch adds 4 write_elf_XX functions to the CPUClass which can be implemented
by XXXCPUState, e.g. S390CPUstate.

Would it make sense to have paging_enabled/HAVE_MEMORY_MAPPING also as a property
of the CPU object?

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 dump.c                   | 28 +++++++++++++++++++++++----
 include/qom/cpu.h        | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/dump.h    |  9 +--------
 qom/cpu.c                | 44 +++++++++++++++++++++++++++++++++++++++++++
 target-s390x/arch_dump.c | 25 ++++--------------------
 target-s390x/arch_dump.h | 14 ++++++++++++++
 target-s390x/cpu.c       |  6 ++++++
 7 files changed, 142 insertions(+), 33 deletions(-)
 create mode 100644 target-s390x/arch_dump.h

Hi Jens,

Am 09.04.2013 14:09, schrieb Jens Freimann:
> Hi Andreas,
> 
> not sure if this is what you had in mind. Does it at least go into the right direction?
> It is pretty much untested and won't compile as is. I just wanted to get your opinion 
> before putting more work into this.
> 
> This patch adds 4 write_elf_XX functions to the CPUClass which can be implemented
> by XXXCPUState, e.g. S390CPUstate.

That is mostly like I expected it, comments inline.

> 
> Would it make sense to have paging_enabled/HAVE_MEMORY_MAPPING also as a property
> of the CPU object?

Hm, I don't think QOM properties make much sense for the internal
restructuring - I'm not sure if we already have some indication whether
this can be used from QMP for a given target? That would be the only use
case for HAVE_MEMORY_MAPPING as QOM property that I could think of. for
paging_enabled() I think the only users would be internal so this could
be a QOM method/function just like the write_* functions.

> 
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
>  dump.c                   | 28 +++++++++++++++++++++++----
>  include/qom/cpu.h        | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/dump.h    |  9 +--------
>  qom/cpu.c                | 44 +++++++++++++++++++++++++++++++++++++++++++
>  target-s390x/arch_dump.c | 25 ++++--------------------
>  target-s390x/arch_dump.h | 14 ++++++++++++++
>  target-s390x/cpu.c       |  6 ++++++
>  7 files changed, 142 insertions(+), 33 deletions(-)
>  create mode 100644 target-s390x/arch_dump.h
> 
> diff --git a/dump.c b/dump.c
> index 574c292..df49845 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -272,13 +272,19 @@ static int write_elf64_notes(DumpState *s)
>  {
>      CPUArchState *env;
>      CPUState *cpu;
> +    CPUClass *cc;
>      int ret;
>      int id;
>  
>      for (env = first_cpu; env != NULL; env = env->next_cpu) {
>          cpu = ENV_GET_CPU(env);
> +        cc = CPU_GET_CLASS(cpu);
>          id = cpu_index(cpu);
> -        ret = cpu_write_elf64_note(fd_write_vmcore, env, id, s);
> +        if (cc->write_elf64_note)
> +            ret = cc->write_elf64_note(fd_write_vmcore, cpu, id, s);
> +        else {
> +            ret = 0;
> +        }

Why are you changing the logic here?

>          if (ret < 0) {
>              dump_error(s, "dump: failed to write elf notes.\n");
>              return -1;
> @@ -286,7 +292,11 @@ static int write_elf64_notes(DumpState *s)
>      }
>  
>      for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        ret = cpu_write_elf64_qemunote(fd_write_vmcore, env, s);
> +        if (cc->write_elf64_qemunote)
> +            ret = cc->write_elf64_qemunote(fd_write_vmcore, cpu, s);
> +        else {
> +            ret = 0;
> +        }

Dito?

>          if (ret < 0) {
>              dump_error(s, "dump: failed to write CPU status.\n");
>              return -1;
> @@ -324,13 +334,19 @@ static int write_elf32_notes(DumpState *s)
>  {
>      CPUArchState *env;
>      CPUState *cpu;
> +    CPUClass *cc;
>      int ret;
>      int id;
>  
>      for (env = first_cpu; env != NULL; env = env->next_cpu) {
>          cpu = ENV_GET_CPU(env);
> +        cc = CPU_GET_CLASS(cpu);
>          id = cpu_index(cpu);
> -        ret = cpu_write_elf32_note(fd_write_vmcore, env, id, s);
> +        if (cc->write_elf32_note) {
> +            ret = cc->write_elf32_note(fd_write_vmcore, cpu, id, s);
> +        } else {
> +            ret = 0; 
> +        }

Dito?

>          if (ret < 0) {
>              dump_error(s, "dump: failed to write elf notes.\n");
>              return -1;
> @@ -338,7 +354,11 @@ static int write_elf32_notes(DumpState *s)
>      }
>  
>      for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        ret = cpu_write_elf32_qemunote(fd_write_vmcore, env, s);
> +        if (cc->write_elf32_qemunote) {
> +            ret = cc->write_elf32_qemunote(fd_write_vmcore, cpu, s);
> +        } else {
> +            ret = 0; 
> +        }

Dito?

>          if (ret < 0) {
>              dump_error(s, "dump: failed to write CPU status.\n");
>              return -1;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index ab2657c..53199f1 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -24,6 +24,8 @@
>  #include "hw/qdev-core.h"
>  #include "qemu/thread.h"
>  
> +typedef int (*write_core_dump_function)(void *buf, size_t size, void *opaque);

You're only copying it (which may lead to duplicate typedef warnings
btw) but it would be nice to name it WriteCoreDump or so here.

> +
>  /**
>   * SECTION:cpu
>   * @section_id: QEMU-cpu
> @@ -55,6 +57,14 @@ typedef struct CPUClass {
>      ObjectClass *(*class_by_name)(const char *cpu_model);
>  
>      void (*reset)(CPUState *cpu);
> +    int (*write_elf64_note)(write_core_dump_function f, CPUState *cpu,
> +            int cpuid, void *opaque);
> +    int (*write_elf64_qemunote)(write_core_dump_function f, CPUState *cpu,
> +            void *opaque);
> +    int (*write_elf32_note)(write_core_dump_function f, CPUState *cpu,
> +            int cpuid, void *opaque);
> +    int (*write_elf32_qemunote)(write_core_dump_function f, CPUState *cpu,
> +            void *opaque);
>  } CPUClass;
>  
>  struct KVMState;
> @@ -116,6 +126,45 @@ struct CPUState {
>      int cpu_index; /* used by alpha TCG */
>  };
>  
> +/**
> + * cpu_write_elf64_note:
> + * @f: pointer to a function that writes memory to a file 
> + * @cpu: The CPU whose memory is to be dumped
> + * @cpuid: ID number of the CPU 
> + * @opaque: pointer to the CPUState struct
> + */
> +int cpu_write_elf64_note(write_core_dump_function f, CPUState *cpu,
> +        int cpuid, void *opaque);
> +
> +/**
> + * cpu_write_elf64_qemunote:
> + * @f: pointer to a function that writes memory to a file 
> + * @cpu: The CPU whose memory is to be dumped
> + * @cpuid: ID number of the CPU 
> + * @opaque: pointer to the CPUState struct
> + */
> +int cpu_write_elf64_qemunote(write_core_dump_function f, CPUState *cpu,
> +        void *opaque);
> +
> +/**
> + * cpu_write_elf32_note:
> + * @f: pointer to a function that writes memory to a file 
> + * @cpu: The CPU whose memory is to be dumped
> + * @cpuid: ID number of the CPU 
> + * @opaque: pointer to the CPUState struct
> + */
> +int cpu_write_elf32_note(write_core_dump_function f, CPUState *cpu,
> +        int cpuid, void *opaque);
> +
> +/**
> + * cpu_write_elf32_qemunote:
> + * @f: pointer to a function that writes memory to a file 
> + * @cpu: The CPU whose memory is to be dumped
> + * @cpuid: ID number of the CPU 
> + * @opaque: pointer to the CPUState struct
> + */
> +int cpu_write_elf32_qemunote(write_core_dump_function f, CPUState *cpu,
> +        void *opaque);
>  
>  /**
>   * cpu_reset:
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b03efd4..e5f9f7d 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -16,6 +16,7 @@
>  
>  #include "qapi/error.h"
>  #include "include/qapi/qmp/qerror.h"
> +#include "qom/cpu.h"
>  
>  typedef struct ArchDumpInfo {
>      int d_machine;  /* Architecture */
> @@ -24,14 +25,6 @@ typedef struct ArchDumpInfo {
>  } ArchDumpInfo;
>  
>  typedef int (*write_core_dump_function)(void *buf, size_t size, void *opaque);
> -int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env,
> -                                                  int cpuid, void *opaque);
> -int cpu_write_elf32_note(write_core_dump_function f, CPUArchState *env,
> -                                                  int cpuid, void *opaque);
> -int cpu_write_elf64_qemunote(write_core_dump_function f, CPUArchState *env,
> -                                                          void *opaque);
> -int cpu_write_elf32_qemunote(write_core_dump_function f, CPUArchState *env,
> -                                                          void *opaque);
>  int cpu_get_dump_info(ArchDumpInfo *info);
>  ssize_t cpu_get_note_size(int class, int machine, int nr_cpus);
>  
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 0a2194d..e2822d0 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -21,6 +21,50 @@
>  #include "qom/cpu.h"
>  #include "qemu-common.h"
>  
> +int cpu_write_elf32_qemunote(write_core_dump_function f, CPUState *cpu,
> +        void *opaque)
> +{
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
> +
> +    if (klass->write_elf32_qemunote != NULL) {
> +        return (*klass->write_elf32_qemunote)(f, cpu, opaque);
> +    }
> +    return -1;
> +}
> +
> +int cpu_write_elf32_note(write_core_dump_function f, CPUState *cpu,
> +        int cpuid, void *opaque)
> +{
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
> +
> +    if (klass->write_elf32_note != NULL) {
> +        return (*klass->write_elf32_note)(f, cpu, cpuid, opaque);
> +    }
> +    return -1;
> +}
> +
> +int cpu_write_elf64_qemunote(write_core_dump_function f, CPUState *cpu,
> +        void *opaque)
> +{
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
> +
> +    if (klass->write_elf64_note != NULL) {
> +        return (*klass->write_elf64_qemunote)(f, cpu, opaque);
> +    }
> +    return -1;
> +}
> +
> +int cpu_write_elf64_note(write_core_dump_function f, CPUState *cpu,
> +        int cpuid, void *opaque)
> +{
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
> +
> +    if (klass->write_elf64_note != NULL) {
> +        return (*klass->write_elf64_note)(f, cpu, cpuid, opaque);
> +    }
> +    return -1;
> +}
> +

I would rather make these four totally trivial, passing through the
return value always. See below.

>  void cpu_reset(CPUState *cpu)
>  {
>      CPUClass *klass = CPU_GET_CLASS(cpu);
> diff --git a/target-s390x/arch_dump.c b/target-s390x/arch_dump.c
> index 18f2652..c594874 100644
> --- a/target-s390x/arch_dump.c
> +++ b/target-s390x/arch_dump.c
> @@ -13,6 +13,7 @@
>  
>  #include "cpu.h"
>  #include "elf.h"
> +#include "arch_dump.h"
>  #include "exec/cpu-all.h"
>  #include "sysemu/dump.h"
>  #include "sysemu/kvm.h"
> @@ -186,10 +187,11 @@ static int s390x_write_all_elf64_notes(const char *note_name, write_core_dump_fu
>  }
>  
>  
> -int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env,
> +int s390_cpu_write_elf64_note(write_core_dump_function f, CPUState *cpu,
>                           int cpuid, void *opaque)
>  {
> -    return s390x_write_all_elf64_notes("CORE", f, env, cpuid, opaque);
> +    S390CPU *_cpu = S390_CPU(cpu);

To avoid this name conflict, please name the argument "cs" above, then
this can be just "cpu".

> +    return s390x_write_all_elf64_notes("CORE", f, &_cpu->env, cpuid, opaque);
>  }
>  
>  int cpu_get_dump_info(ArchDumpInfo *info)
> @@ -222,25 +224,6 @@ ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>  
>  }
>  
> -int cpu_write_elf32_note(write_core_dump_function f, CPUArchState *env,
> -                         int cpuid, void *opaque)
> -{
> -    return 0;
> -}
> -
> -
> -int cpu_write_elf64_qemunote(write_core_dump_function f, CPUArchState *env,
> -                             void *opaque)
> -{
> -    return 0;
> -}
> -
> -int cpu_write_elf32_qemunote(write_core_dump_function f, CPUArchState *env,
> -                             void *opaque)
> -{
> -    return 0;
> -}
> -
>  int arch_check_parameter(bool paging, bool has_filter, int64_t begin, int64_t length, 
>                            Error **errp)
>  {
> diff --git a/target-s390x/arch_dump.h b/target-s390x/arch_dump.h
> new file mode 100644
> index 0000000..8ebc24e
> --- /dev/null
> +++ b/target-s390x/arch_dump.h
> @@ -0,0 +1,14 @@
> +/*
> + * dump guest memory implementation 
> + *
> + * Copyright 2012 IBM Corp.
> + * Author(s): Jens Freimann <jfrei@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +int s390_cpu_write_elf64_note(write_core_dump_function f, CPUState *cpu,
> +                         int cpuid, void *opaque);

Suggest to place this into cpu-qom.h if not much more gets added to it.

> +
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 14a9e9d..14b25d8 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -27,6 +27,7 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/hw.h"
> +#include "arch_dump.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/s390x/sclp.h"
>  #include "sysemu/arch_init.h"
> @@ -231,6 +232,11 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      scc->parent_reset = cc->reset;
>      cc->reset = s390_cpu_reset;
>  
> +    cc->write_elf64_note = s390_cpu_write_elf64_note;
> +    cc->write_elf64_qemunote = NULL;
> +    cc->write_elf32_note = NULL;
> +    cc->write_elf32_qemunote = NULL;

Suggest to avoid NULL functions by implementing dummy versions in
qom/cpu.c, which return a value compatible with its call sites. Then
s390x etc. only overwrite the functions whose functionality they wish to
override, keeping the call sites (both wrappers and their callers) simpler.

cc->paging_enabled (or so) should also get overridden here.

And quite obviously this RFC is based on top of the s390x patch whereas
I was hoping to insert this rework before, to simplify both arm and
s390x patches.

Regards,
Andreas

> +
>      dc->vmsd = &vmstate_s390_cpu;
>  }
>  
>

Hi Andreas,

I have two patches to make dump-guest-memory related
cpu_write_elfxx_note functions members of CPUClass and pass CPUstate
as an argument instead of CPUArchState.

This is preparation for s390 patches which will follow on Monday.

Jens Freimann (2):
  qom: Convert cpu_write_elfXX_note functions to CPUState
  i386: use CPUClass->write_elf* functions

 dump-stub.c             | 28 ----------------------------
 dump.c                  |  8 ++++----
 include/qom/cpu.h       | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/dump.h   |  9 ---------
 qom/cpu.c               | 32 ++++++++++++++++++++++++++++++++
 target-i386/arch_dump.c | 25 +++++++++++++------------
 target-i386/cpu-qom.h   |  9 +++++++++
 target-i386/cpu.c       |  7 +++++++
 8 files changed, 114 insertions(+), 53 deletions(-)

Am 19.04.2013 16:45, schrieb Jens Freimann:
> Hi Andreas,
> 
> I have two patches to make dump-guest-memory related
> cpu_write_elfxx_note functions members of CPUClass and pass CPUstate
> as an argument instead of CPUArchState.
> 
> This is preparation for s390 patches which will follow on Monday.
> 
> Jens Freimann (2):
>   qom: Convert cpu_write_elfXX_note functions to CPUState
>   i386: use CPUClass->write_elf* functions

Thanks, squashed and applied to qom-cpu (with changes below):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

* squashed both for bisectability
* fixed argument indentation
* s/klass/cc/g in qom/cpu.c; TODO: class_init still uses k
* dropped including cpu-qom.h in target-i386/cpu.c
* dropped including qom/cpu.h in target-i386/arch_dump.c
* retained stubs from dump-stub.c in qom/cpu.c
* target-i386: reserve "cpu" for X86CPU and avoid env_ptr usage

Andreas