| Message ID | 1364146041-27041-6-git-send-email-rabin@rab.in |
|---|---|
| State | New |
| Headers | show |
On 24 March 2013 17:27, Rabin Vincent <rabin@rab.in> wrote: > Enable support for the dump-guest-memory monitor command for ARM. Hi. I'm afraid I'm not really familiar with the dump-guest-memory command/implementation so I have some possibly dumb comments below: > --- /dev/null > +++ b/target-arm/arch_dump.c > @@ -0,0 +1,61 @@ > +#include "cpu.h" > +#include "sysemu/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; Can you point me at the spec this struct corresponds to? > + > +int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env, > + int cpuid, void *opaque) > +{ > + return -1; > +} Is there any documentation of the API we're implementing here? (why does it require us to provide 64 bit functions that are never used?) > + > +int cpu_write_elf32_note(write_core_dump_function f, CPUArchState *env, > + int cpuid, void *opaque) > +{ > + arm_elf_prstatus prstatus = {.pid = cpuid}; > + > + memcpy(&(prstatus.regs), env->regs, sizeof(env->regs)); > + prstatus.regs[16] = cpsr_read(env); > + > + 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; > +} Why is it OK to implement this as a no-op? What could we be doing here that we don't do? What are the effects? > + > +int cpu_get_dump_info(ArchDumpInfo *info) > +{ > + info->d_machine = EM_ARM; > +#ifdef TARGET_WORDS_BIGENDIAN > + info->d_endian = ELFDATA2MSB; > +#else > + info->d_endian = ELFDATA2LSB; > +#endif > + info->d_class = ELFCLASS32; Most of this looks like it would be suitable for a default implementation that said "endian based on TARGET_WORDS_BIGENDIAN, machine is ELF_MACHINE, class based on TARGET_LONG_BITS". > + > + 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)); > +} > -- > 1.7.10.4 > thanks -- PMM
2013/3/24 Peter Maydell <peter.maydell@linaro.org>: > On 24 March 2013 17:27, Rabin Vincent <rabin@rab.in> wrote: >> --- /dev/null >> +++ b/target-arm/arch_dump.c >> @@ -0,0 +1,61 @@ >> +#include "cpu.h" >> +#include "sysemu/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; > > Can you point me at the spec this struct corresponds to? This is elf_prstatus from the Linux kernel's include/uapi/linux/elfcore.h, with the regset begin ARM regs in this case. I don't know if there's a spec. It doesn't sound like it from the comments in the kernel file: "This is mostly like the SVR4 structure, but more Linuxy, with things that Linux does not support and which gdb doesn't really use excluded." The x86 implementation in target-i386/arch_dump.c uses the same elf_prstatus with the x86 regs. > >> + >> +int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env, >> + int cpuid, void *opaque) >> +{ >> + return -1; >> +} > > Is there any documentation of the API we're implementing here? I coudn't find any documentation. It's only x86 that has the API implemented. > (why does it require us to provide 64 bit functions that are > never used?) I guess the API was made with x86 in mind. I will see if the requirement can be removed with some ifdefs in the dump.c file. (come to think of it, I guess this ARM code will need to use ELFCLASS64 when we have physical memory > 4GiB (LPAE)) >> +int cpu_write_elf32_qemunote(write_core_dump_function f, CPUArchState *env, >> + void *opaque) >> +{ >> + return 0; >> +} > > Why is it OK to implement this as a no-op? What could we > be doing here that we don't do? What are the effects? This is supposed to be used to add some additional information about the CPU state in an ELF note in a QEMU-specific structure, like the QEMUCPUState in target-i386/arm-state.c. It's only useful to implement this if someone sees a need to add any such information. > >> + >> +int cpu_get_dump_info(ArchDumpInfo *info) >> +{ >> + info->d_machine = EM_ARM; >> +#ifdef TARGET_WORDS_BIGENDIAN >> + info->d_endian = ELFDATA2MSB; >> +#else >> + info->d_endian = ELFDATA2LSB; >> +#endif >> + info->d_class = ELFCLASS32; > > Most of this looks like it would be suitable for a default > implementation that said "endian based on TARGET_WORDS_BIGENDIAN, > machine is ELF_MACHINE, class based on TARGET_LONG_BITS". I will see if this can be moved into the generic dump.c.
On 24 March 2013 19:26, Rabin Vincent <rabin@rab.in> wrote: > 2013/3/24 Peter Maydell <peter.maydell@linaro.org>: >> On 24 March 2013 17:27, Rabin Vincent <rabin@rab.in> wrote: So I guess I should prefix this email by saying that quite a bit of it is really comments on the existing dump code rather than the ARM related support you're adding here. I appreciate that it's annoying to get asked to fix up a bunch of existing code just because you happened to be the next person to want to add use case 2 to it. So you should feel free to push back a bit if any of this seems too burdensome or unnecessary. (I do think that the memory region stuff in the other patch does need fixing though, because (a) vexpress really does map RAM in two places in the physical address map and (b) we need to be careful about what we allow to be added to the memory API, so it remains coherent. So if you only fix up one thing it should be that.) >>> --- /dev/null >>> +++ b/target-arm/arch_dump.c >>> @@ -0,0 +1,61 @@ >>> +#include "cpu.h" >>> +#include "sysemu/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; >> >> Can you point me at the spec this struct corresponds to? > > This is elf_prstatus from the Linux kernel's > include/uapi/linux/elfcore.h, with the regset begin ARM regs in this > case. They don't look very similar to me -- this one has a lot of pad fields the elf_prstatus doesn't. Also, if the kernel's struct is a standard one with a cpu-specific regset field, why isn't QEMU also using a standard struct with a cpu-specific regset field? (it seems a bit bogus that we aren't saving the floating point registers too, but it looks like Linux doesn't do that either, so I guess there's nowhere in the core file for it.) > I don't know if there's a spec. It doesn't sound like it from the > comments in the kernel file: "This is mostly like the SVR4 structure, > but more Linuxy, with things that Linux does not support and which gdb > doesn't really use excluded." > > The x86 implementation in target-i386/arch_dump.c uses the same > elf_prstatus with the x86 regs. > >> >>> + >>> +int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env, >>> + int cpuid, void *opaque) >>> +{ >>> + return -1; >>> +} >> >> Is there any documentation of the API we're implementing here? > > I coudn't find any documentation. It's only x86 that has the API > implemented. It would be nice if somebody who understood it could add some doc comments to the header file. >> (why does it require us to provide 64 bit functions that are >> never used?) > > I guess the API was made with x86 in mind. I will see if the > requirement can be removed with some ifdefs in the dump.c file. > > (come to think of it, I guess this ARM code will need to use ELFCLASS64 > when we have physical memory > 4GiB (LPAE)) It would be good to check whether that is correct -- mostly core files are for dumps of a virtual address space so I don't know whether gdb/etc would understand an ARM corefile which claimed ELFCLASS64. (I mean understand it as an AArch32 corefile rather than AArch64.) >>> +int cpu_write_elf32_qemunote(write_core_dump_function f, CPUArchState *env, >>> + void *opaque) >>> +{ >>> + return 0; >>> +} >> >> Why is it OK to implement this as a no-op? What could we >> be doing here that we don't do? What are the effects? > > This is supposed to be used to add some additional information about the > CPU state in an ELF note in a QEMU-specific structure, like the > QEMUCPUState in target-i386/arm-state.c. It's only useful to implement > this if someone sees a need to add any such information. OK. >> >>> + >>> +int cpu_get_dump_info(ArchDumpInfo *info) >>> +{ >>> + info->d_machine = EM_ARM; >>> +#ifdef TARGET_WORDS_BIGENDIAN >>> + info->d_endian = ELFDATA2MSB; >>> +#else >>> + info->d_endian = ELFDATA2LSB; >>> +#endif >>> + info->d_class = ELFCLASS32; >> >> Most of this looks like it would be suitable for a default >> implementation that said "endian based on TARGET_WORDS_BIGENDIAN, >> machine is ELF_MACHINE, class based on TARGET_LONG_BITS". > > I will see if this can be moved into the generic dump.c. thanks -- PMM
Il 24/03/2013 21:39, Peter Maydell ha scritto: >> > >> > I guess the API was made with x86 in mind. I will see if the >> > requirement can be removed with some ifdefs in the dump.c file. >> > >> > (come to think of it, I guess this ARM code will need to use ELFCLASS64 >> > when we have physical memory > 4GiB (LPAE)) > It would be good to check whether that is correct -- mostly core > files are for dumps of a virtual address space so I don't know > whether gdb/etc would understand an ARM corefile which claimed > ELFCLASS64. (I mean understand it as an AArch32 corefile rather > than AArch64.) Note that in this case we can choose between physical and virtual address space dumps. Virtual address space is mostly useful with gdb; physical address space can be used for example with "crash", a post-mortem kernel debugger. Paolo
On 4 April 2013 10:47, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 24/03/2013 21:39, Peter Maydell ha scritto: >>> > (come to think of it, I guess this ARM code will need to use ELFCLASS64 >>> > when we have physical memory > 4GiB (LPAE)) >> It would be good to check whether that is correct -- mostly core >> files are for dumps of a virtual address space so I don't know >> whether gdb/etc would understand an ARM corefile which claimed >> ELFCLASS64. (I mean understand it as an AArch32 corefile rather >> than AArch64.) > > Note that in this case we can choose between physical and virtual > address space dumps. > > Virtual address space is mostly useful with gdb; physical address space > can be used for example with "crash", a post-mortem kernel debugger. Yeah, my point was really that either way we should be checking how the kernel and these tools expect to handle an LPAE dump. -- PMM
diff --git a/configure b/configure index 46a7594..f35786d 100755 --- a/configure +++ b/configure @@ -4184,7 +4184,7 @@ if test "$target_softmmu" = "yes" ; then echo "CONFIG_SOFTMMU=y" >> $config_target_mak echo "LIBS+=$libs_softmmu $target_libs_softmmu" >> $config_target_mak 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 d89b57c..93baa12 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_dump.o obj-$(CONFIG_KVM) += kvm.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..e568ffb --- /dev/null +++ b/target-arm/arch_dump.c @@ -0,0 +1,61 @@ +#include "cpu.h" +#include "sysemu/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 = {.pid = cpuid}; + + memcpy(&(prstatus.regs), env->regs, sizeof(env->regs)); + prstatus.regs[16] = cpsr_read(env); + + 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; +#ifdef TARGET_WORDS_BIGENDIAN + info->d_endian = ELFDATA2MSB; +#else + info->d_endian = ELFDATA2LSB; +#endif + 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)); +}
Enable support for the dump-guest-memory monitor command for ARM. Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Rabin Vincent <rabin@rab.in> --- configure | 2 +- target-arm/Makefile.objs | 2 +- target-arm/arch_dump.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 target-arm/arch_dump.c