| Message ID | 1340213303-596-4-git-send-email-rabin@rab.in |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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. >
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 >
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; +}
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