Re: [Qemu-devel] [PATCH] target-arm: add dump-guest-memory support

[Peter Maydell]

On 23 December 2014 at 23:29, Rabin Vincent <address@hidden> wrote:
> Enable support for the dump-guest-memory command on ARM and AArch64.
> The dumped files can be analyzed with crash or similar tools.
>
> Signed-off-by: Rabin Vincent <address@hidden>

Thanks -- looks pretty good. One nit and some annoying
corner cases you may not have thought of...

> +static size_t round4(size_t size)
> +{
> +    return ((size + 3) / 4) * 4;
> +}

Is this different from ROUND_UP(size, 4) ?
If we can use the standard macro from the headers we should;
if there's a real difference we should comment about what it is.


> +int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> +                             int cpuid, void *opaque)
> +{
> +    arm_elf_prstatus prstatus = {.pid = cpuid};
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    memcpy(&(prstatus.regs), cpu->env.regs, sizeof(cpu->env.regs));
> +    prstatus.regs[16] = cpsr_read(&cpu->env);
> +
> +    return write_elf_note("CORE", NT_PRSTATUS,
> +                          &prstatus, sizeof(prstatus),
> +                          f, opaque);
> +}
> +
> +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> +                             int cpuid, void *opaque)
> +{
> +    aarch64_elf_prstatus prstatus = {.pid = cpuid};
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    memcpy(&(prstatus.regs), cpu->env.xregs, sizeof(cpu->env.xregs));
> +    prstatus.pc = cpu->env.pc;
> +    prstatus.pstate = cpu->env.pstate;

You need to use the correct accessor function for pstate, not
all the bits are kept in env.pstate. Call pstate_read().

Can we get here when a 64-bit CPU is in AArch32 mode? (eg,
64 bit guest OS running a 32 bit compat process at the
point of taking the memory dump). If so, what sort of
core file should we be writing?

Assuming the answer is "still 64 bit core dump" you need
to do something here to sync the 32 bit TCG state into the
64 bit xregs array. (KVM can take care of itself.)

> +int cpu_get_dump_info(ArchDumpInfo *info,
> +                      const struct GuestPhysBlockList *guest_phys_blocks)
> +{
> +    info->d_machine = ELF_MACHINE;
> +    info->d_class = (info->d_machine == EM_ARM) ? ELFCLASS32 : ELFCLASS64;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    info->d_endian = ELFDATA2MSB;
> +#else
> +    info->d_endian = ELFDATA2LSB;
> +#endif

Note that in fact ARM is never going to be TARGET_WORDS_BIGENDIAN,
even if the guest is big-endian, because the #define represents
the bus endianness, not whether the CPU happens to currently be
doing byte-swizzling. Do you need to key d_endian off the CPU's
current endianness setting? The current endianness of EL1?
Something else?

thanks
-- PMM