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

[Dave Anderson]

----- Original Message -----
> 
> 
> ----- Original Message -----
> > (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?
> 
> Yeah, I don't know -- I think the relevant part of your patch that fixes
> the handling of 32-bit register sets is quite good enough for now.  In the
> highly unlikely event where a double-NULL pc/sp pair does actually show up,
> the registers aren't really "tossed", given that you can still examine the
> register set from the dumpfile header with "help -r".
> 
> If you want to post a secondary patch that extends the sanity-checking to
> pstate or otherwise, please feel free to do so.
> 
> Thanks,
>   Dave

Queued for crash-7.1.5:
 
  https://github.com/crash-utility/crash/commit/4641ea1f610ebe67db706e7c657f9f6d93f9dc77

Thanks Drew,
  Dave