Re: [PATCH v3 03/21] powerpc/8xx: exception InstructionAccess does not exist on MPC8xx

[Joakim Tjernlund]

leroy christophe <christophe.leroy@xxxxxx> wrote on 2014/09/18 18:42:14:

> 
> Le 18/09/2014 17:15, Joakim Tjernlund a écrit :
> > Christophe Leroy <christophe.leroy@xxxxxx> wrote on 2014/09/17 
18:36:57:
> >> Exception InstructionAccess does not exist on MPC8xx. No need to 
branch
> > there from somewhere else.
> >> Handling can be done directly in InstructionTLBError Exception.
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> >>
> >> ---
> >> Changes in v2:
> >> - None
> >>
> >> Changes in v3:
> >> - arch/powerpc/mm/fault.c uses the vector number, so make sure it
> > understand
> >> the new ones.
> >>
> >>   arch/powerpc/kernel/head_8xx.S | 17 +++++++----------
> >>   arch/powerpc/mm/fault.c        |  1 +
> >>   2 files changed, 8 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/powerpc/kernel/head_8xx.S
> > b/arch/powerpc/kernel/head_8xx.S
> >> index 3af6db1..fbe5d10 100644
> >> --- a/arch/powerpc/kernel/head_8xx.S
> >> +++ b/arch/powerpc/kernel/head_8xx.S
> >> @@ -234,15 +234,9 @@ DataAccess:
> >>      EXC_XFER_LITE(0x300, handle_page_fault)
> >>
> >>   /* Instruction access exception.
> >> - * This is "never generated" by the MPC8xx.  We jump to it for other
> >> - * translation errors.
> >> + * This is "never generated" by the MPC8xx.
> >>    */
> >> -   . = 0x400
> >> -InstructionAccess:
> >> -   EXCEPTION_PROLOG
> >> -   mr   r4,r12
> >> -   mr   r5,r9
> >> -   EXC_XFER_LITE(0x400, handle_page_fault)
> >> +   EXCEPTION(0x400, InstructionAccess, unknown_exception, 
EXC_XFER_STD)
> >>
> >>   /* External interrupt */
> >>      EXCEPTION(0x500, HardwareInterrupt, do_IRQ, EXC_XFER_LITE)
> >> @@ -382,7 +376,7 @@ InstructionTLBMiss:
> >>   #endif
> >>      mfspr   r10, SPRN_SPRG_SCRATCH2
> >>      EXCEPTION_EPILOG_0
> >> -   b   InstructionAccess
> >> +   b   InstructionTLBError
> >>
> >>      . = 0x1200
> >>   DataStoreTLBMiss:
> >> @@ -477,7 +471,10 @@ DataStoreTLBMiss:
> >>    */
> >>      . = 0x1300
> >>   InstructionTLBError:
> >> -   b   InstructionAccess
> >> +   EXCEPTION_PROLOG
> >> +   mr   r4,r12
> >> +   mr   r5,r9
> >> +   EXC_XFER_LITE(0x1300, handle_page_fault)
> > Still don't like that you change the vector number, every other ppc 
uses
> > the
> > standard number.
> >
> > Can you not just lie here(EXC_XFER_LITE(0x400, handle_page_fault))?
> > Move the code to InstructionAccess too and let TLBError branch there.
> My issue was that if I do EXC_XFER_LITE(0x400, handle_page_fault), I 
> can't leave the
>      EXCEPTION(0x400, InstructionAccess, unknown_exception, 
> EXC_XFER_STD) at address .400
> Otherwise, I get twice the same label.
> 
> What about the following patch then ? Would this be acceptable ?
> 
> @@ -234,15 +234,13 @@ DataAccess:
>       EXC_XFER_LITE(0x300, handle_page_fault)
> 
>   /* Instruction access exception.
> - * This is "never generated" by the MPC8xx.  We jump to it for other
> - * translation errors.
> + * This is "never generated" by the MPC8xx.
>    */
>       . = 0x400
>   InstructionAccess:
>       EXCEPTION_PROLOG
> -    mr    r4,r12
> -    mr    r5,r9
> -    EXC_XFER_LITE(0x400, handle_page_fault)
> +    addi    r3,r1,STACK_FRAME_OVERHEAD
> +    EXC_XFER_STD(0x401, unknown_exception)

Since this is never generated I think it is OK. Nedds some more commenst
though, why the 401 etc.


> 
>   /* External interrupt */
>       EXCEPTION(0x500, HardwareInterrupt, do_IRQ, EXC_XFER_LITE)
> @@ -382,7 +380,7 @@ InstructionTLBMiss:
>   #endif
>       mfspr    r10, SPRN_SPRG_SCRATCH2
>       EXCEPTION_EPILOG_0
> -    b    InstructionAccess
> +    b    InstructionTLBError
> 
>       . = 0x1200
>   DataStoreTLBMiss:
> @@ -477,7 +475,11 @@ DataStoreTLBMiss:
>    */
>       . = 0x1300
>   InstructionTLBError:
> -    b    InstructionAccess
> +    EXCEPTION_PROLOG
> +    mr    r4,r12
> +    mr    r5,r9
> +    /* 0x400 is InstructionAccess exception, needed by bad_page_fault() 
*/
> +    EXC_XFER_LITE(0x400, handle_page_fault)

You should have the code in TLBMiss and have the TLBError branch there as
that is the common case. 

> 
>   /* This is the data TLB error on the MPC8xx.  This could be due to
>    * many reasons, including a dirty update to a pte.  We can catch that
> 
> 
> >
> > If not I think you should keep the old/current way?
> >
> > Same for DataTLB Miss/Error
> >
> >>   /* This is the data TLB error on the MPC8xx.  This could be due to
> >>    * many reasons, including a dirty update to a pte.  We can catch 
that
> >> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> >> index 51ab9e7..4d63c96 100644
> >> --- a/arch/powerpc/mm/fault.c
> >> +++ b/arch/powerpc/mm/fault.c
> >> @@ -526,6 +526,7 @@ void bad_page_fault(struct pt_regs *regs, 
unsigned
> > long address, int sig)
> >>         break;
> >>      case 0x400:
> >>      case 0x480:
> >> +   case 0x1300:
> >>         printk(KERN_ALERT "Unable to handle kernel paging request for 
"
> >>            "instruction fetch\n");
> >>         break;
> >> -- 
> >> 2.1.0
> >>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/