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

[Peter Maydell]

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