On Fri, Dec 18, 2015 at 06:46:14PM +0000, Peter Maydell wrote: > On 18 December 2015 at 18:05, Andrew Jones <drjo...@redhat.com> wrote: > > On Fri, Dec 18, 2015 at 04:31:13PM +0000, Peter Maydell wrote: > >> On 18 December 2015 at 16:05, Andrew Jones <drjo...@redhat.com> 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