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) {

bt->flags |= BT_REGS_NOT_FOUND; return FALSE; } ptregs = &ms->panic_task_regs[bt->tc->processor]; - frame->sp = ptregs->sp; frame->pc = ptregs->pc; - frame->fp = ptregs->regs[29]; - - if (!is_kernel_text(frame->pc) && - in_user_stack(bt->tc->task, frame->sp)) + if (user_mode(ptregs)) { + frame->sp = user_stack_pointer(ptregs); + frame->fp = user_frame_pointer(ptregs); + if (is_kernel_text(frame->pc) || + !in_user_stack(bt->tc->task, frame->sp)) { + error(WARNING, "Corrupt prstatus? pstate=0x%lx, but no user frame found\n", + ptregs->pstate); + bt->flags |= BT_REGS_NOT_FOUND; + return FALSE; + } bt->flags |= BT_USER_SPACE; + } else { + frame->sp = ptregs->sp; + frame->fp = ptregs->regs[29]; + } if (arm64_in_kdump_text(bt, frame)) bt->flags |= BT_KDUMP_ADJUST; -- 1.8.3.1 -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

----- 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.

----- 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