From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32971) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrVLH-0000Rp-Da for qemu-devel@nongnu.org; Tue, 25 Jun 2013 11:39:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UrVLE-00011E-Hk for qemu-devel@nongnu.org; Tue, 25 Jun 2013 11:39:39 -0400 Received: from cantor2.suse.de ([195.135.220.15]:58890 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrVLE-00010x-1f for qemu-devel@nongnu.org; Tue, 25 Jun 2013 11:39:36 -0400 Message-ID: <51C9B9B6.5050801@suse.de> Date: Tue, 25 Jun 2013 17:39:34 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <9DE6FF32-4B7E-4EFD-AC2F-63219CAC38E6@suse.de> <1372155462-22001-1-git-send-email-jfrei@linux.vnet.ibm.com> In-Reply-To: <1372155462-22001-1-git-send-email-jfrei@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] s390: Implement dump-guest-memory support for target s390x List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jens Freimann Cc: Christian Borntraeger , Ekaterina Tumanova , Alexander Graf , qemu-devel Hi Jens, Am 25.06.2013 12:17, schrieb Jens Freimann: > With this patch dump-guest-memory on s390 produces an ELF formatted, > crash-readable dump. > In order to implement this, the arch-specific part of dump-guest-memory > was added: > target-s390x/arch_dump.c contains the whole set of function for writing > Elf note sections of all types for s390x. >=20 > Signed-off-by: Ekaterina Tumanova > Signed-off-by: Jens Freimann Just some more nitpicks... > --- > include/elf.h | 6 ++ > target-s390x/Makefile.objs | 2 +- > target-s390x/arch_dump.c | 229 +++++++++++++++++++++++++++++++++++++= ++++++++ > target-s390x/cpu-qom.h | 5 + > target-s390x/cpu.c | 4 + > 5 files changed, 245 insertions(+), 1 deletion(-) > create mode 100644 target-s390x/arch_dump.c >=20 > diff --git a/include/elf.h b/include/elf.h > index cf0d3e2..58bfbf8 100644 > --- a/include/elf.h > +++ b/include/elf.h > @@ -1348,11 +1348,17 @@ typedef struct elf64_shdr { > =20 > /* Notes used in ET_CORE */ > #define NT_PRSTATUS 1 > +#define NT_FPREGSET 2 > #define NT_PRFPREG 2 > #define NT_PRPSINFO 3 > #define NT_TASKSTRUCT 4 > #define NT_AUXV 6 > #define NT_PRXFPREG 0x46e62b7f /* copied from gdb5.1/include/= elf/common.h */ > +#define NT_S390_PREFIX 0x305 /* s390 prefix register */ > +#define NT_S390_CTRS 0x304 /* s390 control registers */ > +#define NT_S390_TODPREG 0x303 /* s390 TOD programmable regis= ter */ > +#define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator r= egister */ > +#define NT_S390_TIMER 0x301 /* s390 timer register */ > =20 > =20 > /* Note header in a PT_NOTE section */ > diff --git a/target-s390x/Makefile.objs b/target-s390x/Makefile.objs > index 4e63417..c34f654 100644 > --- a/target-s390x/Makefile.objs > +++ b/target-s390x/Makefile.objs > @@ -1,4 +1,4 @@ > obj-y +=3D translate.o helper.o cpu.o interrupt.o > obj-y +=3D int_helper.o fpu_helper.o cc_helper.o mem_helper.o misc_hel= per.o > -obj-$(CONFIG_SOFTMMU) +=3D ioinst.o > +obj-$(CONFIG_SOFTMMU) +=3D ioinst.o arch_dump.o > obj-$(CONFIG_KVM) +=3D kvm.o > diff --git a/target-s390x/arch_dump.c b/target-s390x/arch_dump.c > new file mode 100644 > index 0000000..df95e46 > --- /dev/null > +++ b/target-s390x/arch_dump.c > @@ -0,0 +1,229 @@ > +/* > + * writing ELF notes for s390x arch > + * > + * > + * Copyright IBM Corp. 2012, 2013 > + * > + * Ekaterina Tumanova > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or = later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#include "cpu.h" > +#include "elf.h" > +#include "exec/cpu-all.h" > +#include "sysemu/dump.h" > +#include "sysemu/kvm.h" > + > + > +struct s390x_user_regs_struct { > + uint64_t psw[2]; > + uint64_t gprs[16]; > + uint32_t acrs[16]; > +} QEMU_PACKED; > + > +typedef struct s390x_user_regs_struct s390x_user_regs; Are these copied from the kernel or somewhere? QEMU expects CamelCase for both struct and typedef. > + > +struct s390x_elf_prstatus_struct { > + uint8_t pad1[32]; > + uint32_t pid; > + uint8_t pad2[76]; > + s390x_user_regs regs; > + uint8_t pad3[16]; > +} QEMU_PACKED; > + > +typedef struct s390x_elf_prstatus_struct s390x_elf_prstatus; > + > +struct s390x_elf_fpregset_struct { > + uint32_t fpc; > + uint32_t pad; > + uint64_t fprs[16]; Indentation? > +} QEMU_PACKED; > + > +typedef struct s390x_elf_fpregset_struct s390x_elf_fpregset; > + > + typedef struct note_struct { > + Elf64_Nhdr hdr; > + char name[5]; > + char pad3[3]; > + union { > + s390x_elf_prstatus prstatus; > + s390x_elf_fpregset fpregset; > + uint32_t prefix; > + uint64_t timer; > + uint64_t todcmp; > + uint32_t todpreg; > + uint64_t ctrs[16]; > + } contents; > + } QEMU_PACKED note_t; Why indented? *_t has been criticized as being reserved for POSIX. > + > +static int s390x_write_elf64_prstatus(note_t *note, CPUArchState *env) Please don't use CPUArchState in s390x-only code, use CPUS390XState where needed; for static helpers please prefer S390CPU. `git grep CPUArchState` flags to me where I still need to touch things, same for ENV_GET_CPU() (which Jason recently introduced, fix upcoming). > +{ > + int i; > + s390x_user_regs *regs; > + > + note->hdr.n_type =3D cpu_to_be32(NT_PRSTATUS); > + > + regs =3D &(note->contents.prstatus.regs); > + regs->psw[0] =3D cpu_to_be64(env->psw.mask); > + regs->psw[1] =3D cpu_to_be64(env->psw.addr); Two spaces accidental? > + for (i =3D 0; i <=3D 15; i++) { > + regs->acrs[i] =3D cpu_to_be32(env->aregs[i]); > + regs->gprs[i] =3D cpu_to_be64(env->regs[i]); > + } > + > + return 0; > +} Already returns 0, make it void? Same for all callbacks below. > + > +static int s390x_write_elf64_fpregset(note_t *note, CPUArchState *env) > +{ > + int i; > + > + note->hdr.n_type =3D cpu_to_be32(NT_FPREGSET); > + > + note->contents.fpregset.fpc =3D cpu_to_be32(env->fpc); > + for (i =3D 0; i <=3D 15; i++) { > + note->contents.fpregset.fprs[i] =3D cpu_to_be64(env->fregs[i]= .ll); > + } > + > + return 0; > +} > + > + > +static int s390x_write_elf64_timer(note_t *note, CPUArchState *env) > +{ > + note->hdr.n_type =3D cpu_to_be32(NT_S390_TIMER); > + > + note->contents.timer =3D cpu_to_be64((uint64_t)(env->cputm)); > + > + return 0; > +} > + > +static int s390x_write_elf64_todcmp(note_t *note, CPUArchState *env) > +{ > + note->hdr.n_type =3D cpu_to_be32(NT_S390_TODCMP); > + > + note->contents.todcmp =3D cpu_to_be64((uint64_t)(env->ckc)); > + > + return 0; > +} > + > +static int s390x_write_elf64_todpreg(note_t *note, CPUArchState *env) > +{ > + note->hdr.n_type =3D cpu_to_be32(NT_S390_TODPREG); > + > + note->contents.todpreg =3D cpu_to_be32((uint32_t)(env->todpr)); > + > + return 0; > +} > + > +static int s390x_write_elf64_ctrs(note_t *note, CPUArchState *env) > +{ > + int i; > + > + note->hdr.n_type =3D cpu_to_be32(NT_S390_CTRS); > + > + for (i =3D 0; i <=3D 15; i++) { > + note->contents.ctrs[i] =3D cpu_to_be64(env->cregs[i]); > + } > + > + return 0; > +} > + > +static int s390x_write_elf64_prefix(note_t *note, CPUArchState *env) > +{ > + note->hdr.n_type =3D cpu_to_be32(NT_S390_PREFIX); > + > + note->contents.prefix =3D cpu_to_be32((uint32_t)(env->psa)); > + > + return 0; > +} > + > + > +struct note_func_desc_struct { > + int contents_size; > + int (*note_contents_func)(note_t *note, CPUArchState *env); > +} note_func[] =3D { > + {sizeof(((note_t *)0)->contents.prstatus), s390x_write_elf64_prsta= tus}, > + {sizeof(((note_t *)0)->contents.prefix), s390x_write_elf64_prefi= x}, > + {sizeof(((note_t *)0)->contents.fpregset), s390x_write_elf64_fpreg= set}, > + {sizeof(((note_t *)0)->contents.ctrs), s390x_write_elf64_ctrs}= , > + {sizeof(((note_t *)0)->contents.timer), s390x_write_elf64_timer= }, > + {sizeof(((note_t *)0)->contents.todcmp), s390x_write_elf64_todcm= p}, > + {sizeof(((note_t *)0)->contents.todpreg), s390x_write_elf64_todpr= eg}, > + { 0, NULL} > +}; > + > + > +static int s390x_write_all_elf64_notes(const char *note_name, > + WriteCoreDumpFunction f, > + CPUArchState *env, int id, > + void *opaque) Odd indentation? > +{ > + note_t note; > + struct note_func_desc_struct *nf; typedef and CamelCase > + int note_size; > + int ret =3D -1; > + > + for (nf =3D note_func; nf->note_contents_func; nf++) { > + note.hdr.n_namesz =3D cpu_to_be32(sizeof(note.name)); > + note.hdr.n_descsz =3D cpu_to_be32(nf->contents_size); > + strncpy(note.name, note_name, sizeof(note.name)); > + ret =3D (*nf->note_contents_func)(¬e, env); > + > + note_size =3D sizeof(note) - sizeof(note.contents) + nf->conte= nts_size; > + ret =3D f(¬e, note_size, opaque); > + > + if (ret < 0) { > + return -1; > + } > + > + } > + > + return 0; > +} > + > + > +int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > + int cpuid, void *opaque) > +{ > + S390CPU *cpu =3D S390_CPU(cs); Here you have the S390CPU that you could just pass on. > + return s390x_write_all_elf64_notes("CORE", f, &cpu->env, cpuid, op= aque); > +} > + > +int cpu_get_dump_info(ArchDumpInfo *info) > +{ > + info->d_machine =3D EM_S390; > + info->d_endian =3D ELFDATA2MSB; > + info->d_class =3D ELFCLASS64; > + > + return 0; > +} > + > +ssize_t cpu_get_note_size(int class, int machine, int nr_cpus) > +{ > + int name_size =3D 8; /* "CORE" or "QEMU" rounded */ > + size_t elf_note_size =3D 0; > + int note_head_size; > + struct note_func_desc_struct *nf; > + > + assert(class =3D=3D ELFCLASS64); > + assert(machine =3D=3D EM_S390); > + > + note_head_size =3D sizeof(Elf64_Nhdr); > + > + for (nf =3D note_func; nf->note_contents_func; nf++) { > + elf_note_size =3D elf_note_size + note_head_size + name_size + > + nf->contents_size; > + } > + > + return (elf_note_size) * nr_cpus; > +} > + > +int s390_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, > + CPUState *env, void *opaque) Please don't mix env with CPUState. Indentation? > +{ > + return 0; > +} > diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h > index 34d45c2..6327802 100644 > --- a/target-s390x/cpu-qom.h > +++ b/target-s390x/cpu-qom.h > @@ -72,5 +72,10 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XStat= e *env) > #define ENV_OFFSET offsetof(S390CPU, env) > =20 > void s390_cpu_do_interrupt(CPUState *cpu); > +int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > + int cpuid, void *opaque); > + > +int s390_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, > + CPUState *env, void *opaque); Consistent indentation? > =20 > #endif > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c > index 23fe51f..9376a6c 100644 > --- a/target-s390x/cpu.c > +++ b/target-s390x/cpu.c > @@ -171,6 +171,10 @@ static void s390_cpu_class_init(ObjectClass *oc, v= oid *data) > =20 > cc->do_interrupt =3D s390_cpu_do_interrupt; > dc->vmsd =3D &vmstate_s390_cpu; > +#ifndef CONFIG_USER_ONLY > + cc->write_elf64_note =3D s390_cpu_write_elf64_note; > + cc->write_elf64_qemunote =3D s390_cpu_write_elf64_qemunote; > +#endif > } > =20 > static const TypeInfo s390_cpu_type_info =3D { Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg