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

[Rabin Vincent]

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