Re: [Qemu-devel] [PATCHv2 5/6] target-arm: add dump-guest-memory support

[Peter Maydell]

On 24 March 2013 17:27, Rabin Vincent <address@hidden> 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