Patchwork [PATCHv2,5/6] target-arm: add dump-guest-memory support

login
register
mail settings
Submitter Rabin Vincent
Date March 24, 2013, 5:27 p.m.
Message ID <1364146041-27041-6-git-send-email-rabin@rab.in>
Download mbox | patch
Permalink /patch/230472/
State New
Headers show

Comments

Rabin Vincent - March 24, 2013, 5:27 p.m.
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
Peter Maydell - March 24, 2013, 6:34 p.m.
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
Rabin Vincent - March 24, 2013, 7:26 p.m.
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.
Peter Maydell - March 24, 2013, 8:39 p.m.
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
Paolo Bonzini - April 4, 2013, 9:47 a.m.
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
Peter Maydell - April 4, 2013, 9:49 a.m.
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

Patch

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