| Message ID | 1371724110-8250-2-git-send-email-tiejun.chen@windriver.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote: > We always alloc critical/machine/debug check exceptions. This is > different from the normal exception. So we should load these exception > stack properly like we did for booke. This is "booke". Do you mean like "like we did for 32-bit"? And the code is already trying to load the special stack; it just happens that it's loading from a different location than the C code placed the stack addresses. The changelog should point out the specific thing that is being fixed. > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > --- > arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S > index 4b23119..4d8e57f 100644 > --- a/arch/powerpc/kernel/exceptions-64e.S > +++ b/arch/powerpc/kernel/exceptions-64e.S > @@ -36,6 +36,37 @@ > */ > #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE > > +/* only on book3e */ > +#define DBG_STACK_BASE dbgirq_ctx > +#define MC_STACK_BASE mcheckirq_ctx > +#define CRIT_STACK_BASE critirq_ctx > + > +#ifdef CONFIG_RELOCATABLE > +#define LOAD_STACK_BASE(reg, level) \ > + tovirt(r2,r2); \ > + LOAD_REG_ADDR(reg, level##_STACK_BASE); > +#else > +#define LOAD_STACK_BASE(reg, level) \ > + LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE); > +#endif > + > +#ifdef CONFIG_SMP > +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \ > + mfspr r14,SPRN_PIR; \ > + slwi r14,r14,3; \ > + LOAD_STACK_BASE(r10, level); \ > + add r10,r10,r14; \ > + ld r10,0(r10); \ > + addi r10,r10,THREAD_SIZE; \ > + std r10,PACA_##level##_STACK(r13); > +#else > +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \ > + LOAD_STACK_BASE(r10, level); \ > + ld r10,0(r10); \ > + addi r10,r10,THREAD_SIZE; \ > + std r10,PACA_##level##_STACK(r13); > +#endif It looks like you're loading the stack from *irq_ctx, storing it in PACA_*_stack, and then (immediately after this in the caller) loading it back from PACA_*_STACK. Why not just load it from *irq_ctx and get rid of PACA_*_STACK altogether -- or change the C code to initialize the addresses in the PACA instead, and get ird of *irq_ctx on 64-bit? > /* Exception prolog code for all exceptions */ > #define EXCEPTION_PROLOG(n, intnum, type, addition) \ > mtspr SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */ \ > @@ -68,20 +99,32 @@ > #define SPRN_GDBELL_SRR1 SPRN_GSRR1 > > #define CRIT_SET_KSTACK \ > + andi. r10,r11,MSR_PR; \ > + bne 1f; \ > + BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT); \ > ld r1,PACA_CRIT_STACK(r13); \ > - subi r1,r1,SPECIAL_EXC_FRAME_SIZE; > + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \ The caller will already check MSR_PR and override this if coming from userspace; why do you need to check again here? -Scott
On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote: > We always alloc critical/machine/debug check exceptions. This is > different from the normal exception. So we should load these exception > stack properly like we did for booke. > > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > --- > arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S > index 4b23119..4d8e57f 100644 > --- a/arch/powerpc/kernel/exceptions-64e.S > +++ b/arch/powerpc/kernel/exceptions-64e.S > @@ -36,6 +36,37 @@ > */ > #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE > > +/* only on book3e */ > +#define DBG_STACK_BASE dbgirq_ctx > +#define MC_STACK_BASE mcheckirq_ctx > +#define CRIT_STACK_BASE critirq_ctx > + > +#ifdef CONFIG_RELOCATABLE > +#define LOAD_STACK_BASE(reg, level) \ > + tovirt(r2,r2); \ > + LOAD_REG_ADDR(reg, level##_STACK_BASE); Where does r2 come from here, where does it get used, and why do we need tovirt() on book3e? -Scott
On 10/19/2013 06:37 AM, Scott Wood wrote: > On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote: >> We always alloc critical/machine/debug check exceptions. This is >> different from the normal exception. So we should load these exception >> stack properly like we did for booke. > > This is "booke". Do you mean like "like we did for 32-bit"? Yes. > > And the code is already trying to load the special stack; it just > happens that it's loading from a different location than the C code > placed the stack addresses. The changelog should point out the specific > thing that is being fixed. Here I don't fix anything, and I just want to do the same thing as 32-bit. > >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >> --- >> arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++--- >> 1 file changed, 46 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S >> index 4b23119..4d8e57f 100644 >> --- a/arch/powerpc/kernel/exceptions-64e.S >> +++ b/arch/powerpc/kernel/exceptions-64e.S >> @@ -36,6 +36,37 @@ >> */ >> #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE >> >> +/* only on book3e */ >> +#define DBG_STACK_BASE dbgirq_ctx >> +#define MC_STACK_BASE mcheckirq_ctx >> +#define CRIT_STACK_BASE critirq_ctx >> + >> +#ifdef CONFIG_RELOCATABLE >> +#define LOAD_STACK_BASE(reg, level) \ >> + tovirt(r2,r2); \ >> + LOAD_REG_ADDR(reg, level##_STACK_BASE); >> +#else >> +#define LOAD_STACK_BASE(reg, level) \ >> + LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE); >> +#endif >> + >> +#ifdef CONFIG_SMP >> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \ >> + mfspr r14,SPRN_PIR; \ >> + slwi r14,r14,3; \ >> + LOAD_STACK_BASE(r10, level); \ >> + add r10,r10,r14; \ >> + ld r10,0(r10); \ >> + addi r10,r10,THREAD_SIZE; \ >> + std r10,PACA_##level##_STACK(r13); >> +#else >> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \ >> + LOAD_STACK_BASE(r10, level); \ >> + ld r10,0(r10); \ >> + addi r10,r10,THREAD_SIZE; \ >> + std r10,PACA_##level##_STACK(r13); >> +#endif > > It looks like you're loading the stack from *irq_ctx, storing it in > PACA_*_stack, and then (immediately after this in the caller) loading it > back from PACA_*_STACK. Why not just load it from *irq_ctx and get rid > of PACA_*_STACK altogether -- or change the C code to initialize the > addresses in the PACA instead, and get ird of *irq_ctx on 64-bit? Okay, I'd like to move forward the c code, please see next version. > >> /* Exception prolog code for all exceptions */ >> #define EXCEPTION_PROLOG(n, intnum, type, addition) \ >> mtspr SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */ \ >> @@ -68,20 +99,32 @@ >> #define SPRN_GDBELL_SRR1 SPRN_GSRR1 >> >> #define CRIT_SET_KSTACK \ >> + andi. r10,r11,MSR_PR; \ >> + bne 1f; \ >> + BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT); \ >> ld r1,PACA_CRIT_STACK(r13); \ >> - subi r1,r1,SPECIAL_EXC_FRAME_SIZE; >> + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \ > > The caller will already check MSR_PR and override this if coming from > userspace; why do you need to check again here? Looks this is redundant so this will be left out. Thanks, Tiejun
On 10/19/2013 07:55 AM, Scott Wood wrote: > On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote: >> We always alloc critical/machine/debug check exceptions. This is >> different from the normal exception. So we should load these exception >> stack properly like we did for booke. >> >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >> --- >> arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++--- >> 1 file changed, 46 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S >> index 4b23119..4d8e57f 100644 >> --- a/arch/powerpc/kernel/exceptions-64e.S >> +++ b/arch/powerpc/kernel/exceptions-64e.S >> @@ -36,6 +36,37 @@ >> */ >> #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE >> >> +/* only on book3e */ >> +#define DBG_STACK_BASE dbgirq_ctx >> +#define MC_STACK_BASE mcheckirq_ctx >> +#define CRIT_STACK_BASE critirq_ctx >> + >> +#ifdef CONFIG_RELOCATABLE >> +#define LOAD_STACK_BASE(reg, level) \ >> + tovirt(r2,r2); \ >> + LOAD_REG_ADDR(reg, level##_STACK_BASE); > > Where does r2 come from here, where does it get used, and why do we need > tovirt() on book3e? > As I remember this should be covered when we boot that capture kernel in kexec/kdump case. Now this is also gone away after move forward the c code. Thanks, Tiejun
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S index 4b23119..4d8e57f 100644 --- a/arch/powerpc/kernel/exceptions-64e.S +++ b/arch/powerpc/kernel/exceptions-64e.S @@ -36,6 +36,37 @@ */ #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE +/* only on book3e */ +#define DBG_STACK_BASE dbgirq_ctx +#define MC_STACK_BASE mcheckirq_ctx +#define CRIT_STACK_BASE critirq_ctx + +#ifdef CONFIG_RELOCATABLE +#define LOAD_STACK_BASE(reg, level) \ + tovirt(r2,r2); \ + LOAD_REG_ADDR(reg, level##_STACK_BASE); +#else +#define LOAD_STACK_BASE(reg, level) \ + LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE); +#endif + +#ifdef CONFIG_SMP +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \ + mfspr r14,SPRN_PIR; \ + slwi r14,r14,3; \ + LOAD_STACK_BASE(r10, level); \ + add r10,r10,r14; \ + ld r10,0(r10); \ + addi r10,r10,THREAD_SIZE; \ + std r10,PACA_##level##_STACK(r13); +#else +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \ + LOAD_STACK_BASE(r10, level); \ + ld r10,0(r10); \ + addi r10,r10,THREAD_SIZE; \ + std r10,PACA_##level##_STACK(r13); +#endif + /* Exception prolog code for all exceptions */ #define EXCEPTION_PROLOG(n, intnum, type, addition) \ mtspr SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */ \ @@ -68,20 +99,32 @@ #define SPRN_GDBELL_SRR1 SPRN_GSRR1 #define CRIT_SET_KSTACK \ + andi. r10,r11,MSR_PR; \ + bne 1f; \ + BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT); \ ld r1,PACA_CRIT_STACK(r13); \ - subi r1,r1,SPECIAL_EXC_FRAME_SIZE; + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \ +1: #define SPRN_CRIT_SRR0 SPRN_CSRR0 #define SPRN_CRIT_SRR1 SPRN_CSRR1 #define DBG_SET_KSTACK \ + andi. r10,r11,MSR_PR; \ + bne 1f; \ + BOOK3E_LOAD_EXC_LEVEL_STACK(DBG); \ ld r1,PACA_DBG_STACK(r13); \ - subi r1,r1,SPECIAL_EXC_FRAME_SIZE; + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \ +1: #define SPRN_DBG_SRR0 SPRN_DSRR0 #define SPRN_DBG_SRR1 SPRN_DSRR1 #define MC_SET_KSTACK \ + andi. r10,r11,MSR_PR; \ + bne 1f; \ + BOOK3E_LOAD_EXC_LEVEL_STACK(MC); \ ld r1,PACA_MC_STACK(r13); \ - subi r1,r1,SPECIAL_EXC_FRAME_SIZE; + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \ +1: #define SPRN_MC_SRR0 SPRN_MCSRR0 #define SPRN_MC_SRR1 SPRN_MCSRR1
We always alloc critical/machine/debug check exceptions. This is different from the normal exception. So we should load these exception stack properly like we did for booke. Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> --- arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-)