Re: [Qemu-devel] [PATCH v2 5/6] target-arm: support QMP dump-guest-memory

[Peter Maydell <peter.maydell@linaro.org>]

On 3 December 2015 at 18:55, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Dec 03, 2015 at 11:44:05AM +0000, Peter Maydell wrote:
>> On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
>> > Add the support needed for creating prstatus elf notes. This
>> > allows us to use QMP dump-guest-memory.
>>
>> > +
>> > +    if (is_a64(env)) {
>> > +        for (i = 0; i < 31; ++i) {
>> > +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
>> > +        }
>> > +        note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
>> > +        note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
>> > +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate_read(env));
>> > +    } else {
>> > +        aarch64_sync_64_to_32(env);
>> > +        for (i = 0; i < 16; ++i) {
>> > +            note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->regs[i]);
>> > +        }
>> > +        note.prstatus.pr_reg.sp = note.prstatus.pr_reg.regs[13];
>> > +        note.prstatus.pr_reg.pc = note.prstatus.pr_reg.regs[15];
>> > +        note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
>> > +    }
>>
>> This doesn't look right. sync_64_to_32 is copying the state held
>> in the 64-bit env->xregs etc into the 32-bit env->regs. But if we're
>> in 32-bit state then the true state is in the 32-bit fields and
>> this will trash it. You want to sync_32_to_64 here, and then the
>> code to write the values to the dump is the same either way
>> (except for pstate vs cpsr which we haven't managed to clean up
>> and unify yet, sadly).
>
> Besides the unnecessary call to aarch64_sync_64_to_32(), then, for the
> KVM case, the above code is correct. However, for the TCG case, I now
> see why it's wrong.
>
> The KVM case starts with 64-bit state, because this function is dealing
> with 64-bit guest kernels. The TCG case, when userspace is running a
> 32-bit binary, starts with 32-bit state. In both cases we want to get
> 32-bit state into a 64-bit elf note. KVM needs aarch64_sync_64_to_32(),
> which is actually already done by cpu_synchronize_all_states(), and
> then to shoehorn the 32-bit registers into the 64-bit elf note, as done
> above. TCG, on the other hand, doesn't need to sync any state, it just
> needs to shoehorn. So the above aarch64_sync_64_to_32() call, which I
> actually added *for* TCG (since I misunderstood your comment on v1),
> actually makes it wrong. Needless to say, I didn't test TCG :-)
>
> Now, to fix it, we could do what you have here below
>
>>
>> I think you want
>>    uint64_t pstate;
>>    [...]
>>
>>    if (!is_a64(env)) {
>>        aarch64_sync_32_to_64(env);
>>        pstate = cpsr_read(env);
>>    } else {
>>        pstate = pstate_read(env);
>>    }
>>    for (i = 0; i < 31; ++i) {
>>        note.prstatus.pr_reg.regs[i] = cpu_to_dump64(s, env->xregs[i]);
>>    }
>
> But, this adds an unnecessary aarch64_sync_32_to_64() call to the kvm
> case (although it wouldn't hurt, as aarch64_sync_32_to_64 is the inverse
> of aarch64_sync_64_to_32, which we've already done earlier). It also
> always adds register values 16..30 to the elf note (which may not always
> be zero in the 32-bit userspace case?). The way I have it above makes
> sure those registers are zero in that case.

If you do that then you'll lose the information about the other
32-bit registers (the svc/irq/etc banked versions). Those aren't
relevant if the 32-bit code is in usermode, but probably you want
them if you're doing a dump of a 64-bit (emulated) hypervisor
that happens to be running a 32-bit guest kernel at point of dump.

> So, how about we just remove the aarch64_sync_64_to_32() from the code
> I have above? Won't that make it work for both KVM and TCG?
>
>
>>    note.prstatus.pr_reg.sp = cpu_to_dump64(s, env->xregs[31]);
>>    note.prstatus.pr_reg.pc = cpu_to_dump64(s, env->pc);
>>    note.prstatus.pr_reg.pstate = cpu_to_dump64(s, pstate);
>>
>> (Note that the 32-bit SP is not architecturally in X31;
>> it's in one of the other xregs, depending what mode the CPU
>> was in. For 32-bit userspace that will be USR and it's in X13.)
>
> Yup, that's why I was pulling it from x13 in the above code. In your
> version you can now use x31, due to the aarch64_sync_32_to_64().

Note that sync_32_to_64 does not copy regs[13] into x31, which was
my point. In a 64-bit-format dump of a VM that happens to be
running 32 bit code you should not expect pstate.sp to be the
32-bit process's SP...

>> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>> > +                             int cpuid, void *opaque)
>> > +{
>> > +    CPUARMState *env = &ARM_CPU(cs)->env;
>> > +    int ret;
>> > +
>> > +    if (arm_el_is_aa64(env, 1)) {
>> > +        ret = aarch64_write_elf64_note(f, env, cpuid, opaque);
>> > +    } else {
>> > +        ret = arm_write_elf32_note(f, env, cpuid, opaque);
>> > +    }
>>
>> This might produce the wrong kind of dump if we're in EL2
>> or EL3 at the point we take it (can only happen in emulation
>> and only once we add EL2 and EL3 emulation support, which isn't
>> active yet). Do we care?
>
> "care" is loaded word :-) If I can tweak this in some easy way now to get
> it ready for el2/el3 emulation, then I'm happy to do so. However, without
> a test environment, and without strong motivation to use this feature on
> emulation in the first place, then I'd rather not bother for the initial
> introduction of it. We can always modify it later.

For this test I think you can just say
  if (arm_feature(env, ARM_FEATURE_AARCH64)) {

which basically says "64-bit dump if the CPU supports 64-bit" (32-bit
KVM VMs won't have this feature bit). The other awkward part is
figuring out which endianness to use. I think there we can just put
in a comment
    /* We assume the relevant endianness is that of EL1; this is right
     * for kernels but might give the wrong answer if you're trying to
     * take a dump of a hypervisor that happens currently to be running
     * a wrong-endian kernel.
     */
and leave it for somebody who cares to try to figure out the right
semantics.

thanks
-- PMM