Patchwork [4/4] target-arm: add minimal dump-guest-memory support

login
register
mail settings
Submitter Rabin Vincent
Date June 20, 2012, 5:28 p.m.
Message ID <1340213303-596-4-git-send-email-rabin@rab.in>
Download mbox | patch
Permalink /patch/166135/
State New
Headers show

Comments

Rabin Vincent - June 20, 2012, 5:28 p.m.
Add a minimal dump-guest-memory support for ARM.  The -p option is not
supported and we don't add any QEMU-specific notes.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 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
Peter Maydell - June 28, 2012, 4:46 p.m.
On 20 June 2012 18:28, Rabin Vincent <rabin@rab.in> 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 <rabin@rab.in>
> ---
>  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?)

> +{
> +    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;
> +}
> +
> +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.)

-- PMM
Andreas Färber - June 29, 2012, 12:42 p.m.
Am 28.06.2012 18:46, schrieb Peter Maydell:
> On 20 June 2012 18:28, Rabin Vincent <rabin@rab.in> 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 <rabin@rab.in>
>> ---
>>  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.

>> +{
>> +    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()...

>> +
>> +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
Peter Maydell - June 29, 2012, 1:09 p.m.
On 29 June 2012 13:42, Andreas Färber <afaerber@suse.de> wrote:
> Am 28.06.2012 18:46, schrieb Peter Maydell:
>> On 20 June 2012 18:28, Rabin Vincent <rabin@rab.in> wrote:
>>> +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?

There's already an x86 implementation so the API is not new
(see memory_mapping.h). I was just wondering if it was something
we ought to be converting.

> 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.

I think that it's in the former category but there's no documentation :-(

-- PMM
Rabin Vincent - July 1, 2012, 6:22 a.m.
On Thu, Jun 28, 2012 at 05:46:02PM +0100, Peter Maydell wrote:
> On 20 June 2012 18:28, Rabin Vincent <rabin@rab.in> 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 <rabin@rab.in>
> > ---
> >  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.

> 
> > +{
> > +    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.

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.
Wen Congyang - July 4, 2012, 2:48 a.m.
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 <rabin@rab.in> 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 <rabin@rab.in>
>>> ---
>>>  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.
>
Wen Congyang - July 4, 2012, 2:57 a.m.
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 <rabin@rab.in> 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 <rabin@rab.in>
>>> ---
>>>  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
>

Patch

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;
+
+int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env,
+                         int cpuid, void *opaque)
+{
+    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));
+    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;
+    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;
+}