Re: [Qemu-devel] [PATCH 4/4] target-arm: add minimal dump-guest-memory support

[Wen Congyang]

At 07/01/2012 02:22 PM, Rabin Vincent Wrote:
> On Thu, Jun 28, 2012 at 05:46:02PM +0100, Peter Maydell wrote:
>> On 20 June 2012 18:28, Rabin Vincent <address@hidden> wrote:
>>> Add a minimal dump-guest-memory support for ARM.  The -p option is not
>>> supported and we don't add any QEMU-specific notes.
>>
>> So what does this patch give us? This commit message is pretty
>> short and I couldn't find a cover message for the patchset...
> 
> It makes the dump-guest-memory command work for arm-softmmu.  The
> resulting core dump can be analysed with a tool such as the crash
> utility.
> 
>>
>>> Signed-off-by: Rabin Vincent <address@hidden>
>>> ---
>>>  configure                        |    4 +--
>>>  target-arm/Makefile.objs         |    2 +-
>>>  target-arm/arch_dump.c           |   59 
>>> ++++++++++++++++++++++++++++++++++++++
>>>  target-arm/arch_memory_mapping.c |   13 +++++++++
>>>  4 files changed, 75 insertions(+), 3 deletions(-)
>>>  create mode 100644 target-arm/arch_dump.c
>>>  create mode 100644 target-arm/arch_memory_mapping.c
>>>
>>> diff --git a/configure b/configure
>>> index b68c0ca..a20ad19 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -3727,7 +3727,7 @@ case "$target_arch2" in
>>>     fi
>>>  esac
>>>  case "$target_arch2" in
>>> -  i386|x86_64)
>>> +  arm|i386|x86_64)
>>>     echo "CONFIG_HAVE_GET_MEMORY_MAPPING=y" >> $config_target_mak
>>>  esac
>>>  if test "$target_arch2" = "ppc64" -a "$fdt" = "yes"; then
>>> @@ -3746,7 +3746,7 @@ if test "$target_softmmu" = "yes" ; then
>>>     echo "subdir-$target: subdir-libcacard" >> $config_host_mak
>>>   fi
>>>   case "$target_arch2" in
>>> -    i386|x86_64)
>>> +    arm|i386|x86_64)
>>>       echo "CONFIG_HAVE_CORE_DUMP=y" >> $config_target_mak
>>>   esac
>>>  fi
>>> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
>>> index f447c4f..837b374 100644
>>> --- a/target-arm/Makefile.objs
>>> +++ b/target-arm/Makefile.objs
>>> @@ -1,5 +1,5 @@
>>>  obj-y += arm-semi.o
>>> -obj-$(CONFIG_SOFTMMU) += machine.o
>>> +obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o arch_dump.o
>>>  obj-y += translate.o op_helper.o helper.o cpu.o
>>>  obj-y += neon_helper.o iwmmxt_helper.o
>>>
>>> diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
>>> new file mode 100644
>>> index 0000000..47a7e40
>>> --- /dev/null
>>> +++ b/target-arm/arch_dump.c
>>> @@ -0,0 +1,59 @@
>>> +#include "cpu.h"
>>> +#include "cpu-all.h"
>>> +#include "dump.h"
>>> +#include "elf.h"
>>> +
>>> +typedef struct {
>>> +    char pad1[24];
>>> +    uint32_t pid;
>>> +    char pad2[44];
>>> +    uint32_t regs[18];
>>> +    char pad3[4];
>>> +} arm_elf_prstatus;
>>
>> I'm guessing this is following some specification's structure layout;
>> what specification?
> 
> struct elf_prstatus from the Linux kernel's include/linux/elfcore.h.
> 
>>
>>> +
>>> +int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env,
>>> +                         int cpuid, void *opaque)
>>
>> Should these APIs really be taking a CPUArchState* rather rather than
>> an ARMCPU* ? (Andreas?)
> 
> No idea.  Cc'ing Wen, who added the APIs.

These API is introduced by me. This API is for all targets, so I use
CPUArchState rather than XXXCPUState here.

> 
>>
>>> +{
>>> +    return -1;
>>> +}
>>> +
>>> +int cpu_write_elf32_note(write_core_dump_function f, CPUArchState *env,
>>> +                         int cpuid, void *opaque)
>>> +{
>>> +    arm_elf_prstatus prstatus;
>>> +
>>> +    memset(&prstatus, 0, sizeof(prstatus));
>>> +    memcpy(&(prstatus.regs), env->regs, sizeof(env->regs));
>>
>> This looks a bit odd -- env->regs[] is a 16 word array but
>> prstatus.regs is 18 words. What are the last two words for?
> 
> CPSR and orig_r0.  orig_r0 is not useful, but I think we can save the
> CPSR in there.
> 
>>
>>> +    prstatus.pid = cpuid;
>>> +
>>> +    return dump_write_elf_note(ELFCLASS32, "CORE", NT_PRSTATUS,
>>> +                               &prstatus, sizeof(prstatus),
>>> +                               f, opaque);
>>> +}
>>> +
>>> +int cpu_write_elf64_qemunote(write_core_dump_function f, CPUArchState *env,
>>> +                             void *opaque)
>>> +{
>>> +    return -1;
>>> +}
>>> +
>>> +int cpu_write_elf32_qemunote(write_core_dump_function f, CPUArchState *env,
>>> +                             void *opaque)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +int cpu_get_dump_info(ArchDumpInfo *info)
>>> +{
>>> +    info->d_machine = EM_ARM;
>>> +    info->d_endian = ELFDATA2LSB;
>>
>> ...even for big endian ARM?
> 
> I'll use TARGET_WORDS_BIGENDIAN to check.
> 
> Though it appears we don't have a armbe-softmmu?
> 
>>
>>> +    info->d_class = ELFCLASS32;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>>> +{
>>> +    return nr_cpus * dump_get_note_size(ELFCLASS32, "CORE",
>>> +                                        sizeof(arm_elf_prstatus));
>>> +}
>>> diff --git a/target-arm/arch_memory_mapping.c 
>>> b/target-arm/arch_memory_mapping.c
>>> new file mode 100644
>>> index 0000000..eeaaf09
>>> --- /dev/null
>>> +++ b/target-arm/arch_memory_mapping.c
>>> @@ -0,0 +1,13 @@
>>> +#include "cpu.h"
>>> +#include "cpu-all.h"
>>> +#include "memory_mapping.h"
>>> +
>>> +bool cpu_paging_enabled(CPUArchState *env)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
>>> +{
>>> +    return -1;
>>> +}
>>
>> Why do we need these null implementations and why do they
>> work better than the default ones in memory_mapping-stub.c ?
> 
> The implementations are to make the dump-guest-memory command build.  A
> full implementation would add support for the "-p" option which afaics
> is supposed to walk the page tables and dump only the pages which are
> mapped instead of the complete RAM.  I personally have no need for this
> option, so they are only null implementations which result in an error
> if this option is used.

If you want an error when this option is used, cpu_paging_enabeld should
return true, not false.

Thanks
Wen Congyang

> 
> The current config code keeps memory-mapping.c and memory-mapping-stub.c
> exclusive.  I think we should be able to make some changes there to
> allow us to use memory-mapping-stub.c instead of this
> arch_memory_mapping.c.
>