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

[Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>]

On 04/30/2013 02:02 PM, Alexander Graf wrote:
> 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 <tumanova@linux.vnet.ibm.com>
>>>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>>>> ---
>>>> 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 <tumanova@linux.vnet.ibm.com>
>>>> + *
>>>> + * 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
>
I thought you were talking about be/le. Agreed. Will fix this. Thanks!

Regards,
Kate.