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 <jf...@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 <jf...@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;
> }
>
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
