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

[Wen Congyang]

At 06/29/2012 08:42 PM, Andreas Färber Wrote:
> Am 28.06.2012 18:46, schrieb Peter Maydell:
>> 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...
>>
>>> 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?
>>
>>> +
>>> +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?)
> 
> I'm missing some context here. This is a new API? Where is it being used?
> 
> If it applies to multiple architectures and has separate
> architecture-specific implementations then CPUState argument please. If
> it's an ARM-internal API then, yes, ARMCPU please. If it's using common
> CPU fields in common code then CPUArchState is still the most fitting today.

It applies to multiple architectures, and use arch-spec fileds(For example:
register's value). Which is better? CPUArchState or XXXCPUState?

> 
>>> +{
>>> +    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?
>>
>>> +    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?
>>
>>> +    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;
>>> +}
> 
> What is this supposed to be? I think this is calling for a CPUClass
> field that gets overridden in target-arm/cpu.c:arm_cpu_class_init()...

No, it is for the monitor command dump-guest-memory's option -p. If the
user specifies this option, we will write guest physicall address and
virtual address mapping in the core(PT_LOAD). If the guest runs in real
mode(not use paging), the physical address is equal to virtual address.
otherwise, we should walk page table to get this mapping. This API
can tell us whether the guest runs in real mode or not.

Thanks
Wen Congyang

> 
>>> +
>>> +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 ?
>>
>> (memory_mapping.h could use some documentation comments
>> describing the purpose and API of these functions IMHO.)
> 
> Generally I'm not happy about an API defining new global per-arch
> cpu_*() functions, that conflicts with the QOM CPUState efforts.
> 
> Rabin, please keep my in the loop.
> 
> Thanks,
> Andreas
>