[Crash-utility] [PATCH] arm64: support compat user mode prstatus

[Andrew Jones]

(Somehow I wasn't on CC for your reply.)

On Mon, Jan 04, 2016 at 04:14:51PM -0500, Dave Anderson wrote:
> 
> 
> ----- Original Message -----
> > On Fri, Dec 18, 2015 at 11:55:07PM +0100, Andrew Jones wrote:
> > > compat user mode prstatus already just works, almost. This missing
> > > pieces are that pt_regs->sp and pt_regs->fp are not in their usual
> > > locations. We need to pull them out of their architecturally mapped
> > > general purpose registers.
> > > ---
> > >  arm64.c | 42 ++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 34 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/arm64.c b/arm64.c
> > > index 183e768498fe8..3a82d20cdd465 100644
> > > --- a/arm64.c
> > > +++ b/arm64.c
> > > @@ -993,6 +993,25 @@ arm64_stackframe_init(void)
> > >  #define PSR_MODE_EL3h   0x0000000d
> > >  #define PSR_MODE_MASK   0x0000000f
> > >  
> > > +/* Architecturally defined mapping between AArch32 and AArch64 registers
> > > */
> > > +#define compat_usr(x)   regs[(x)]
> > > +#define compat_fp       regs[11]
> > > +#define compat_sp       regs[13]
> > > +#define compat_lr       regs[14]
> > > +
> > > +#define user_mode(ptregs) \
> > > +	(((ptregs)->pstate & PSR_MODE_MASK) == PSR_MODE_EL0t)
> > > +
> > > +#define compat_user_mode(ptregs)  \
> > > +	(((ptregs)->pstate & (PSR_MODE32_BIT | PSR_MODE_MASK)) == \
> > > +	 (PSR_MODE32_BIT | PSR_MODE_EL0t))
> > > +
> > > +#define user_stack_pointer(ptregs) \
> > > +	(!compat_user_mode(ptregs) ? (ptregs)->sp : (ptregs)->compat_sp)
> > > +
> > > +#define user_frame_pointer(ptregs) \
> > > +	(!compat_user_mode(ptregs) ? (ptregs)->regs[29] : (ptregs)->compat_fp)
> > > +
> > >  static int
> > >  arm64_is_kernel_exception_frame(struct bt_info *bt, ulong stkptr)
> > >  {
> > > @@ -1340,21 +1359,28 @@ arm64_get_dumpfile_stackframe(struct bt_info *bt,
> > > struct arm64_stackframe *frame
> > >  	struct machine_specific *ms = machdep->machspec;
> > >  	struct arm64_pt_regs *ptregs;
> > >  
> > > -	if (!ms->panic_task_regs ||
> > > -	    (!ms->panic_task_regs[bt->tc->processor].sp &&
> > > -	     !ms->panic_task_regs[bt->tc->processor].pc)) {
> > > +	if (!ms->panic_task_regs || !ms->panic_task_regs[bt->tc->processor].pc) {
> > 
> > Hi Dave,
> > 
> > I'm having second thoughts about keeping the
> > !ms->panic_task_regs[bt->tc->processor].pc test. pc being zero sounds
> > like a good reason to crash, or a potential status of a pc after a crash.
> > I don't think we should be too hasty to reject the rest of the registers.
> > Is there anyway we can relax the sanity checking, but still keep the crash
> > utility happy?
> > 
> > Thanks,
> > drew
> 
> Hi Drew,
> 
> OK, refresh my mind -- I'm not sure what the reasoning behind the patch snippet above?
> 
> Originally the code checked whether *both* the sp and pc are NULL, and if so, returns
> BT_REGS_NOT_FOUD (which seems fair to me).  Your patch stops checking for a NULL sp, but
> still checks for a NULL pc.  In this QEMU dump, the patch doesn't really accomplish anything, 
> since both incoming registers are non-NULL (albeit with a sp pointing to 16-bytes below the
> top of the stack).  It seems like we could leave it as-is.

The QEMU core generation is changing to only have a non-null aarch64 sp when
the prstatus is for aarch64 mode. Even so, we could still check for both sp
and pc being null, but then we'd toss all registers in the case where an
aarch32 guest ended up with a zero pc due to some bug. I agree the change
above is probably worse, as it also tosses registers for aarch64 though...

The core I gave you doesn't have a null sp as it was generated with a
different qemu patch, but "bad" core generations are always something
that can happen, and now sp==null is something that should happen (for
aarch32). I think the check should be revisited. Maybe we should just
sanity check pstate instead?

Thanks,
drew