Re: [Qemu-devel] [PATCH 2/2] s390: dump guest memory implementation

[Alexander Graf]

On 29.04.2013, at 13:39, Ekaterina Tumanova wrote:

> On 04/26/2013 09:12 PM, Alexander Graf wrote:
>> On 23.04.2013, at 17:30, Jens Freimann wrote:
>> 
>>> Implement dump-guest-memory support for target s390x.
>>> 
>>> dump-guest-memory QEMU monitor command didn't work for s390 architecture.
>>> The result of the command is supposed to be ELF format 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.
>>> 
>>> Signed-off-by: Ekaterina Tumanova <address@hidden>
>>> Signed-off-by: Jens Freimann <address@hidden>
>>> ---
>>> configure                  |   2 +-
>>> include/elf.h              |   6 ++
>>> target-s390x/Makefile.objs |   2 +-
>>> target-s390x/arch_dump.c   | 231 
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> target-s390x/cpu-qom.h     |  21 +++++
>>> target-s390x/cpu.c         |   7 ++
>>> 6 files changed, 267 insertions(+), 2 deletions(-)
>>> create mode 100644 target-s390x/arch_dump.c
>>> 
>>> diff --git a/configure b/configure
>>> index ed49f91..90dc58b 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -4326,7 +4326,7 @@ fi
>>> if test "$target_softmmu" = "yes" ; then
>>>   echo "CONFIG_SOFTMMU=y" >> $config_target_mak
>>>   case "$target_arch2" in
>>> -    i386|x86_64)
>>> +    i386|x86_64|s390x)
>>>       echo "CONFIG_HAVE_CORE_DUMP=y" >> $config_target_mak
>>>   esac
>>> fi
>>> diff --git a/include/elf.h b/include/elf.h
>>> index a21ea53..ba4b3a7 100644
>>> --- a/include/elf.h
>>> +++ b/include/elf.h
>>> @@ -1219,11 +1219,17 @@ typedef struct elf64_shdr {
>>> 
>>> /* 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 register 
>>> */
>>> +#define NT_S390_TODCMP  0x302           /* s390 TOD clock comparator 
>>> register */
>>> +#define NT_S390_TIMER   0x301           /* s390 timer register */
>>> 
>>> 
>>> /* 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 += translate.o helper.o cpu.o interrupt.o
>>> obj-y += int_helper.o fpu_helper.o cc_helper.o mem_helper.o misc_helper.o
>>> -obj-$(CONFIG_SOFTMMU) += ioinst.o
>>> +obj-$(CONFIG_SOFTMMU) += ioinst.o arch_dump.o
>>> obj-$(CONFIG_KVM) += kvm.o
>>> diff --git a/target-s390x/arch_dump.c b/target-s390x/arch_dump.c
>>> new file mode 100644
>>> index 0000000..f908257
>>> --- /dev/null
>>> +++ b/target-s390x/arch_dump.c
>>> @@ -0,0 +1,231 @@
>>> +/*
>>> + * writing ELF notes for s390x arch
>>> + *
>>> + *
>>> + * Copyright IBM Corp. 2012
>>> + *
>>> + *     Ekaterina Tumanova <address@hidden>
>>> + *
>>> + * 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;
>>> +
>>> +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];
>>> +} 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;
>>> +
>>> +static int s390x_write_elf64_prstatus(note_t *note, CPUArchState *env)
>>> +{
>>> +    int i;
>>> +    s390x_user_regs *regs;
>>> +
>>> +    note->hdr.n_type = cpu_to_be32(NT_PRSTATUS);
>>> +
>>> +    regs = &(note->contents.prstatus.regs);
>>> +    regs->psw[0] =  cpu_to_be32(env->psw.mask);
>>> +    regs->psw[1] =  cpu_to_be32(env->psw.addr);
>>> +    for (i = 0; i <= 15; i++) {
>>> +        regs->acrs[i] = cpu_to_be32(env->aregs[i]);
>>> +        regs->gprs[i] = cpu_to_be32(env->regs[i]);
>> be32? Please verify whether you produce proper dumps on a little endian host.
>> 
>> 
>> Alex
> Hi,
> I don't think I have an opportunity to test this on x86 host.

Then ask someone else to do so.

> But logically as far as I understand, the code is correct here, since we need
> to produce big endian dump for s390 guest for any endian host had, and we use 
> cpu_to_be32 in the arch-specific part of code.

It's completely broken, as you truncate 64-bit registers to 32 bit.


Alex