[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v3 6/9] target-arm: support QMP dump-
From: |
Andrew Jones |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v3 6/9] target-arm: support QMP dump-guest-memory |
Date: |
Fri, 18 Dec 2015 13:57:35 -0600 |
User-agent: |
Mutt/1.5.23.1 (2014-03-12) |
On Fri, Dec 18, 2015 at 06:46:14PM +0000, Peter Maydell wrote:
> On 18 December 2015 at 18:05, Andrew Jones <address@hidden> wrote:
> > On Fri, Dec 18, 2015 at 04:31:13PM +0000, Peter Maydell wrote:
> >> On 18 December 2015 at 16:05, Andrew Jones <address@hidden> wrote:
> >> > On Fri, Dec 18, 2015 at 11:59:39AM +0000, Peter Maydell wrote:
> >> >> I don't understand why we need to do this. If this is an
> >> >> AArch64 dump then we should just treat it as an AArch64
> >> >> dump, and presumably the consumer of the dump knows enough
> >> >> to know what the "hypervisor view" of a CPU that's currently
> >> >> in 32-bit mode is. It has to anyway to be able to figure
> >> >> out where all the other registers are, so why can't it
> >> >> also figure out what mode the CPU is currently in and thus
> >> >> where r13 is in the xregs array?
> >> >
> >> > You're probably right that this shouldn't be necessary. But, in order for
> >> > it not to be necessary, I'll need to write another crash patch.
> >> > Currently,
> >> > if you do a dump-guest-memory on a running guest, i.e. one where the
> >> > kernel
> >> > has not called panic(), and thus the cpus are actually in 32-bit
> >> > usermode,
> >> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
> >> > segfaults if sp == xregs[31]. crash does properly decode the registers
> >> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
> >> > to aarch64_compat_sp here also allows crash to work properly in the non-
> >> > panic'd case.
> >>
> >> If crash segfaults then that's clearly a bug in crash...
> >> What is it expecting to see in the SP field?
> >
> > A valid stack pointer
>
> But why? What does it do with it? (In any case a dump of a crashed
> system could have any random rubbish in SP so the tool has to handle
> it being something weird.)
The reason crash segfaults is because sp (xregs[31]) wasn't a user stack
address, and thus it expected the stack to include at least two frames,
which would mean fp would be non-zero, but it's not, and that leads to the
calculation of a bad stack pointer which then gets used as an offset into
the stack buffer, overflowing it. This is definitely a crash bug that
should be fixed. I'll report it.
Also, crash's arm64_get_dumpfile_stackframe() simply assumes sp will be
the stack pointer, and it doesn't check pstate for the MODE32 bit first.
I think it should be easy to fix this, as I only found two places that
should be changed. I'll send a patch for that.
>
> >> > So, I could teach crash to do what I'm doing here in qemu instead, but
> >> > there's still one more reason why it may make sense to do it here. That
> >> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
> >> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
> >> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
> >> > so why not set sp to the correct userspace sp?
> >>
> >> Well, what spec are we following here? The most logical view
> >
> > With the shoehorning approach we're following the aarch64 ptrace spec,
> > but putting arm32 registers in it. We then rely on the analysis tools
> > to do the right thing when they see pstate has PSR_MODE32_BIT set, which
> > all cpsr modes do.
>
> Right, so the analysis tool already has to cope with the MODE32 bit
> being set, and it can also figure out where the SP is (and which
> other registers are currently live for 32-bit).
>
> > The ptrace code in the kernel would return a real aarch32 view, i.e.
> > only registers up to r15 and a cpsr. We can't do that here because we've
> > committed to an EM_AARCH64 formatted core, and we only have one type of
> > PRSTATUS note for that core type. Furthermore, we want to be able to
> > return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.
> >
> >> to me seems to be to say "you get the view of the system
> >> that you get from a JTAG debugger or a hypervisor", which
> >> is to say you see a 64-bit set of registers and it's the
> >> debugger's job to decide which bits of those might be
> >> interesting to view as 32-bit and what is actually "live"
> >> 32 bit state.
> >
> > It appears that PRSTATUS aware tools don't currently work this way.
> >
> >>
> >> From that point of view there is no valid AArch64 SP register
> >> at this point in execution (xregs[31] for QEMU would be stale
> >> state, so not a good choice I think).
> >
> > I suppose zero or all 1's are the safest choices for "undefined", but
> > being undefined actually gives us freedom to use aarch64_compat_sp as
> > well, which, to me, looks like a safer and more useful value.
>
> It just doesn't really make sense to me to do this one bit of
> work for the debug analysis tools when they already have to know
> that 32-bit modes are special. We seem to end up with a function
> in QEMU whose only purpose is working around a bug in the thing
> consuming the coredump.
OK, I've come around to your point of view. What value would you like
me to put in sp? 0, 1's, or ??
Thanks,
drew
- [Qemu-arm] [PATCH v3 3/9] dump: allow target to set the page size, (continued)
[Qemu-arm] [PATCH v3 7/9] target-arm: dump-guest-memory: add prfpreg notes for aarch64, Andrew Jones, 2015/12/15
[Qemu-arm] [PATCH v3 8/9] elf: add arm note types, Andrew Jones, 2015/12/15
[Qemu-arm] [PATCH v3 9/9] target-arm: dump-guest-memory: add vfp notes for arm, Andrew Jones, 2015/12/15