[PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Naveen N. Rao]

On P9, trying to use data breakpoints throws the splat shown below (*).
This is because the check for a data breakpoint in DSISR is in
do_hash_page(). Move this check to handle_page_fault() so as to catch
data breakpoints in both hash and radix MMU modes.

While at it, also remove the label '11' that was made redundant by
commit a546498f3bf9aa ("powerpc: Call do_page_fault() with interrupts
off")

(*)
    Unable to handle kernel paging request for data at address 0xc000000000e19218
    Faulting instruction address: 0xc0000000001155e8
    cpu 0x0: Vector: 300 (Data Access) at [c0000000ef1e7b20]
    pc: c0000000001155e8: find_pid_ns+0x48/0xe0
    lr: c000000000116ac4: find_task_by_vpid+0x44/0x90
    sp: c0000000ef1e7da0
    msr: 9000000000009033
    dar: c000000000e19218
    dsisr: 400000
    current = 0xc0000000f1f59700
    paca    = 0xc00000000fd40000 softe: 0 irq_happened: 0x01
    pid   = 1192, comm = sh
    Linux version 4.12.0-rc3-nnr (root@ea605ec2993c) (gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.1) ) #74 SMP Tue Jun 13 16:52:49 UTC 2017
    enter ? for help
    [c0000000ef1e7dc0] c000000000116ac4 find_task_by_vpid+0x44/0x90
    [c0000000ef1e7de0] c000000000108800 SyS_setpgid+0x80/0x220
    [c0000000ef1e7e30] c00000000000ba6c system_call+0x38/0xfc
    --- Exception: c01 (System Call) at 00007fff94480890
    SP (7fffd91e7260) is in userspace

Fixes: caca285e5ab4a ("powerpc/mm/radix: Use STD_MMU_64 to properly
isolate hash related code")
Reported-by: Shriya R. Kulkarni <shriykul@in.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index ae418b85c17c..17ee701b8336 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
 	.balign	IFETCH_ALIGN_BYTES
 do_hash_page:
 #ifdef CONFIG_PPC_STD_MMU_64
-	andis.	r0,r4,0xa410		/* weird error? */
+	andis.	r0,r4,0xa450		/* weird error? */
 	bne-	handle_page_fault	/* if not, try to insert a HPTE */
-	andis.  r0,r4,DSISR_DABRMATCH@h
-	bne-    handle_dabr_fault
 	CURRENT_THREAD_INFO(r11, r1)
 	lwz	r0,TI_PREEMPT(r11)	/* If we're in an "NMI" */
 	andis.	r0,r0,NMI_MASK@h	/* (i.e. an irq when soft-disabled) */
@@ -1442,7 +1440,9 @@ do_hash_page:
 
 /* Here we have a page fault that hash_page can't handle. */
 handle_page_fault:
-11:	ld	r4,_DAR(r1)
+	andis.  r0,r4,DSISR_DABRMATCH@h
+	bne-    handle_dabr_fault
+	ld	r4,_DAR(r1)
 	ld	r5,_DSISR(r1)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_page_fault
-- 
2.12.2

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Aneesh Kumar K.V]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> On P9, trying to use data breakpoints throws the splat shown below (*).
> This is because the check for a data breakpoint in DSISR is in
> do_hash_page(). Move this check to handle_page_fault() so as to catch
> data breakpoints in both hash and radix MMU modes.
>
> While at it, also remove the label '11' that was made redundant by
> commit a546498f3bf9aa ("powerpc: Call do_page_fault() with interrupts
> off")
>
> (*)
>     Unable to handle kernel paging request for data at address 0xc000000000e19218
>     Faulting instruction address: 0xc0000000001155e8
>     cpu 0x0: Vector: 300 (Data Access) at [c0000000ef1e7b20]
>     pc: c0000000001155e8: find_pid_ns+0x48/0xe0
>     lr: c000000000116ac4: find_task_by_vpid+0x44/0x90
>     sp: c0000000ef1e7da0
>     msr: 9000000000009033
>     dar: c000000000e19218
>     dsisr: 400000
>     current = 0xc0000000f1f59700
>     paca    = 0xc00000000fd40000 softe: 0 irq_happened: 0x01
>     pid   = 1192, comm = sh
>     Linux version 4.12.0-rc3-nnr (root@ea605ec2993c) (gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.1) ) #74 SMP Tue Jun 13 16:52:49 UTC 2017
>     enter ? for help
>     [c0000000ef1e7dc0] c000000000116ac4 find_task_by_vpid+0x44/0x90
>     [c0000000ef1e7de0] c000000000108800 SyS_setpgid+0x80/0x220
>     [c0000000ef1e7e30] c00000000000ba6c system_call+0x38/0xfc
>     --- Exception: c01 (System Call) at 00007fff94480890
>     SP (7fffd91e7260) is in userspace
>
> Fixes: caca285e5ab4a ("powerpc/mm/radix: Use STD_MMU_64 to properly
> isolate hash related code")
> Reported-by: Shriya R. Kulkarni <shriykul@in.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index ae418b85c17c..17ee701b8336 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
>  	.balign	IFETCH_ALIGN_BYTES
>  do_hash_page:
>  #ifdef CONFIG_PPC_STD_MMU_64
> -	andis.	r0,r4,0xa410		/* weird error? */
> +	andis.	r0,r4,0xa450		/* weird error? */

Can we convert that to a #define value. Ram did try to do that here.

https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-June/158607.html


>  	bne-	handle_page_fault	/* if not, try to insert a HPTE */
> -	andis.  r0,r4,DSISR_DABRMATCH@h
> -	bne-    handle_dabr_fault
>  	CURRENT_THREAD_INFO(r11, r1)
>  	lwz	r0,TI_PREEMPT(r11)	/* If we're in an "NMI" */
>  	andis.	r0,r0,NMI_MASK@h	/* (i.e. an irq when soft-disabled) */
> @@ -1442,7 +1440,9 @@ do_hash_page:
>  
>  /* Here we have a page fault that hash_page can't handle. */
>  handle_page_fault:
> -11:	ld	r4,_DAR(r1)
> +	andis.  r0,r4,DSISR_DABRMATCH@h
> +	bne-    handle_dabr_fault
> +	ld	r4,_DAR(r1)
>  	ld	r5,_DSISR(r1)
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	bl	do_page_fault
> -- 
> 2.12.2


-aneesh

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Naveen N. Rao]

Hi Aneesh,

On 2017/06/14 08:38AM, Aneesh Kumar K.V wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > On P9, trying to use data breakpoints throws the splat shown below (*).
> > This is because the check for a data breakpoint in DSISR is in
> > do_hash_page(). Move this check to handle_page_fault() so as to catch
> > data breakpoints in both hash and radix MMU modes.
> >
> > While at it, also remove the label '11' that was made redundant by
> > commit a546498f3bf9aa ("powerpc: Call do_page_fault() with interrupts
> > off")
> >
> > (*)
> >     Unable to handle kernel paging request for data at address 0xc000000000e19218
> >     Faulting instruction address: 0xc0000000001155e8
> >     cpu 0x0: Vector: 300 (Data Access) at [c0000000ef1e7b20]
> >     pc: c0000000001155e8: find_pid_ns+0x48/0xe0
> >     lr: c000000000116ac4: find_task_by_vpid+0x44/0x90
> >     sp: c0000000ef1e7da0
> >     msr: 9000000000009033
> >     dar: c000000000e19218
> >     dsisr: 400000
> >     current = 0xc0000000f1f59700
> >     paca    = 0xc00000000fd40000 softe: 0 irq_happened: 0x01
> >     pid   = 1192, comm = sh
> >     Linux version 4.12.0-rc3-nnr (root@ea605ec2993c) (gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.1) ) #74 SMP Tue Jun 13 16:52:49 UTC 2017
> >     enter ? for help
> >     [c0000000ef1e7dc0] c000000000116ac4 find_task_by_vpid+0x44/0x90
> >     [c0000000ef1e7de0] c000000000108800 SyS_setpgid+0x80/0x220
> >     [c0000000ef1e7e30] c00000000000ba6c system_call+0x38/0xfc
> >     --- Exception: c01 (System Call) at 00007fff94480890
> >     SP (7fffd91e7260) is in userspace
> >
> > Fixes: caca285e5ab4a ("powerpc/mm/radix: Use STD_MMU_64 to properly
> > isolate hash related code")
> > Reported-by: Shriya R. Kulkarni <shriykul@in.ibm.com>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/exceptions-64s.S | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > index ae418b85c17c..17ee701b8336 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
> >  	.balign	IFETCH_ALIGN_BYTES
> >  do_hash_page:
> >  #ifdef CONFIG_PPC_STD_MMU_64
> > -	andis.	r0,r4,0xa410		/* weird error? */
> > +	andis.	r0,r4,0xa450		/* weird error? */
> 
> Can we convert that to a #define value. Ram did try to do that here.
> 
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-June/158607.html

Hmm... I feel it will be good to do that as part of Ram's series since 
he has already coded it up :)

Ram's patches will anyway require a rebase and the change I do here for 
detecting DAWR already has a #define, so it should be a simple matter of 
including DSISR_DABRMATCH in DSISR_PAGE_FAULT_MASK.

But, if you really feel that I should make that change here, please do 
let me know and I will re-spin with those changes.


Thanks for the review!
- Naveen

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Aneesh Kumar K.V]

On Wednesday 14 June 2017 10:41 AM, Naveen N. Rao wrote:
> Hi Aneesh,
> 
> On 2017/06/14 08:38AM, Aneesh Kumar K.V wrote:
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>>
>>> On P9, trying to use data breakpoints throws the splat shown below (*).
>>> This is because the check for a data breakpoint in DSISR is in
>>> do_hash_page(). Move this check to handle_page_fault() so as to catch
>>> data breakpoints in both hash and radix MMU modes.
>>>
>>> While at it, also remove the label '11' that was made redundant by
>>> commit a546498f3bf9aa ("powerpc: Call do_page_fault() with interrupts
>>> off")
>>>
>>> (*)
>>>      Unable to handle kernel paging request for data at address 0xc000000000e19218
>>>      Faulting instruction address: 0xc0000000001155e8
>>>      cpu 0x0: Vector: 300 (Data Access) at [c0000000ef1e7b20]
>>>      pc: c0000000001155e8: find_pid_ns+0x48/0xe0
>>>      lr: c000000000116ac4: find_task_by_vpid+0x44/0x90
>>>      sp: c0000000ef1e7da0
>>>      msr: 9000000000009033
>>>      dar: c000000000e19218
>>>      dsisr: 400000
>>>      current = 0xc0000000f1f59700
>>>      paca    = 0xc00000000fd40000 softe: 0 irq_happened: 0x01
>>>      pid   = 1192, comm = sh
>>>      Linux version 4.12.0-rc3-nnr (root@ea605ec2993c) (gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.1) ) #74 SMP Tue Jun 13 16:52:49 UTC 2017
>>>      enter ? for help
>>>      [c0000000ef1e7dc0] c000000000116ac4 find_task_by_vpid+0x44/0x90
>>>      [c0000000ef1e7de0] c000000000108800 SyS_setpgid+0x80/0x220
>>>      [c0000000ef1e7e30] c00000000000ba6c system_call+0x38/0xfc
>>>      --- Exception: c01 (System Call) at 00007fff94480890
>>>      SP (7fffd91e7260) is in userspace
>>>
>>> Fixes: caca285e5ab4a ("powerpc/mm/radix: Use STD_MMU_64 to properly
>>> isolate hash related code")
>>> Reported-by: Shriya R. Kulkarni <shriykul@in.ibm.com>
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/exceptions-64s.S | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>>> index ae418b85c17c..17ee701b8336 100644
>>> --- a/arch/powerpc/kernel/exceptions-64s.S
>>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>>> @@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
>>>   	.balign	IFETCH_ALIGN_BYTES
>>>   do_hash_page:
>>>   #ifdef CONFIG_PPC_STD_MMU_64
>>> -	andis.	r0,r4,0xa410		/* weird error? */
>>> +	andis.	r0,r4,0xa450		/* weird error? */
>>
>> Can we convert that to a #define value. Ram did try to do that here.
>>
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-June/158607.html
> 
> Hmm... I feel it will be good to do that as part of Ram's series since
> he has already coded it up :)
> 
> Ram's patches will anyway require a rebase and the change I do here for
> detecting DAWR already has a #define, so it should be a simple matter of
> including DSISR_DABRMATCH in DSISR_PAGE_FAULT_MASK.
> 
> But, if you really feel that I should make that change here, please do
> let me know and I will re-spin with those changes.
> 

The thing is that change from 0xa410 to 0xa450 is not clear at all. And 
it needs proper documentation. IMHO the best way to do that is switch to 
#define name for that constant.

-aneesh

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Ram Pai]

On Wed, Jun 14, 2017 at 10:43:30AM +0530, Aneesh Kumar K.V wrote:
> 
> 
> On Wednesday 14 June 2017 10:41 AM, Naveen N. Rao wrote:
> >Hi Aneesh,
> >
> >On 2017/06/14 08:38AM, Aneesh Kumar K.V wrote:
> >>"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> >>
> >>>On P9, trying to use data breakpoints throws the splat shown below (*).
> >>>This is because the check for a data breakpoint in DSISR is in
> >>>do_hash_page(). Move this check to handle_page_fault() so as to catch
> >>>data breakpoints in both hash and radix MMU modes.
> >>>
> >>>While at it, also remove the label '11' that was made redundant by
> >>>commit a546498f3bf9aa ("powerpc: Call do_page_fault() with interrupts
> >>>off")
> >>>
> >>>(*)
> >>>     Unable to handle kernel paging request for data at address 0xc000000000e19218
> >>>     Faulting instruction address: 0xc0000000001155e8
> >>>     cpu 0x0: Vector: 300 (Data Access) at [c0000000ef1e7b20]
> >>>     pc: c0000000001155e8: find_pid_ns+0x48/0xe0
> >>>     lr: c000000000116ac4: find_task_by_vpid+0x44/0x90
> >>>     sp: c0000000ef1e7da0
> >>>     msr: 9000000000009033
> >>>     dar: c000000000e19218
> >>>     dsisr: 400000
> >>>     current = 0xc0000000f1f59700
> >>>     paca    = 0xc00000000fd40000 softe: 0 irq_happened: 0x01
> >>>     pid   = 1192, comm = sh
> >>>     Linux version 4.12.0-rc3-nnr (root@ea605ec2993c) (gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.1) ) #74 SMP Tue Jun 13 16:52:49 UTC 2017
> >>>     enter ? for help
> >>>     [c0000000ef1e7dc0] c000000000116ac4 find_task_by_vpid+0x44/0x90
> >>>     [c0000000ef1e7de0] c000000000108800 SyS_setpgid+0x80/0x220
> >>>     [c0000000ef1e7e30] c00000000000ba6c system_call+0x38/0xfc
> >>>     --- Exception: c01 (System Call) at 00007fff94480890
> >>>     SP (7fffd91e7260) is in userspace
> >>>
> >>>Fixes: caca285e5ab4a ("powerpc/mm/radix: Use STD_MMU_64 to properly
> >>>isolate hash related code")
> >>>Reported-by: Shriya R. Kulkarni <shriykul@in.ibm.com>
> >>>Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> >>>---
> >>>  arch/powerpc/kernel/exceptions-64s.S | 8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> >>>index ae418b85c17c..17ee701b8336 100644
> >>>--- a/arch/powerpc/kernel/exceptions-64s.S
> >>>+++ b/arch/powerpc/kernel/exceptions-64s.S
> >>>@@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
> >>>  	.balign	IFETCH_ALIGN_BYTES
> >>>  do_hash_page:
> >>>  #ifdef CONFIG_PPC_STD_MMU_64
> >>>-	andis.	r0,r4,0xa410		/* weird error? */
> >>>+	andis.	r0,r4,0xa450		/* weird error? */
> >>
> >>Can we convert that to a #define value. Ram did try to do that here.
> >>
> >>https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-June/158607.html
> >
> >Hmm... I feel it will be good to do that as part of Ram's series since
> >he has already coded it up :)
> >
> >Ram's patches will anyway require a rebase and the change I do here for
> >detecting DAWR already has a #define, so it should be a simple matter of
> >including DSISR_DABRMATCH in DSISR_PAGE_FAULT_MASK.
> >
> >But, if you really feel that I should make that change here, please do
> >let me know and I will re-spin with those changes.
> >
> 
> The thing is that change from 0xa410 to 0xa450 is not clear at all.
> And it needs proper documentation. IMHO the best way to do that is
> switch to #define name for that constant.

Naveen,
	
Feel free to take the macro from my patch. I think the magic
number is a little ugly. The earlier it goes the better.

My patch set will probably go through a couple of iterations. So I will
rebase it on top of your changes anyway.

RP

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> On Wednesday 14 June 2017 10:41 AM, Naveen N. Rao wrote:
>> On 2017/06/14 08:38AM, Aneesh Kumar K.V wrote:
>>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>>>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>>>> index ae418b85c17c..17ee701b8336 100644
>>>> --- a/arch/powerpc/kernel/exceptions-64s.S
>>>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>>>> @@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
>>>>   	.balign	IFETCH_ALIGN_BYTES
>>>>   do_hash_page:
>>>>   #ifdef CONFIG_PPC_STD_MMU_64
>>>> -	andis.	r0,r4,0xa410		/* weird error? */
>>>> +	andis.	r0,r4,0xa450		/* weird error? */
>>>
>>> Can we convert that to a #define value. Ram did try to do that here.
>>>
>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-June/158607.html
>> 
>> Hmm... I feel it will be good to do that as part of Ram's series since
>> he has already coded it up :)
>> 
>> Ram's patches will anyway require a rebase and the change I do here for
>> detecting DAWR already has a #define, so it should be a simple matter of
>> including DSISR_DABRMATCH in DSISR_PAGE_FAULT_MASK.
>> 
>> But, if you really feel that I should make that change here, please do
>> let me know and I will re-spin with those changes.
>
> The thing is that change from 0xa410 to 0xa450 is not clear at all. And 
> it needs proper documentation. IMHO the best way to do that is switch to 
> #define name for that constant.

Not in this patch. It needs to be backported, so it should be as minimal
as possible.

The change from 0xa410 to 0xa450 does need a mention in the changelog,
I'll add that.

cheers

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Naveen N. Rao]

On 2017/06/14 04:41PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> > On Wednesday 14 June 2017 10:41 AM, Naveen N. Rao wrote:
> >> On 2017/06/14 08:38AM, Aneesh Kumar K.V wrote:
> >>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> >>>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> >>>> index ae418b85c17c..17ee701b8336 100644
> >>>> --- a/arch/powerpc/kernel/exceptions-64s.S
> >>>> +++ b/arch/powerpc/kernel/exceptions-64s.S
> >>>> @@ -1411,10 +1411,8 @@ USE_TEXT_SECTION()
> >>>>   	.balign	IFETCH_ALIGN_BYTES
> >>>>   do_hash_page:
> >>>>   #ifdef CONFIG_PPC_STD_MMU_64
> >>>> -	andis.	r0,r4,0xa410		/* weird error? */
> >>>> +	andis.	r0,r4,0xa450		/* weird error? */
> >>>
> >>> Can we convert that to a #define value. Ram did try to do that here.
> >>>
> >>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-June/158607.html
> >> 
> >> Hmm... I feel it will be good to do that as part of Ram's series since
> >> he has already coded it up :)
> >> 
> >> Ram's patches will anyway require a rebase and the change I do here for
> >> detecting DAWR already has a #define, so it should be a simple matter of
> >> including DSISR_DABRMATCH in DSISR_PAGE_FAULT_MASK.
> >> 
> >> But, if you really feel that I should make that change here, please do
> >> let me know and I will re-spin with those changes.
> >
> > The thing is that change from 0xa410 to 0xa450 is not clear at all. And 
> > it needs proper documentation. IMHO the best way to do that is switch to 
> > #define name for that constant.
> 
> Not in this patch. It needs to be backported, so it should be as minimal
> as possible.

Ok.

> 
> The change from 0xa410 to 0xa450 does need a mention in the changelog,
> I'll add that.

Thanks, Michael!
(emails just started flowing again...)

- Naveen

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Anshuman Khandual]

On 06/14/2017 12:12 AM, Naveen N. Rao wrote:
> On P9, trying to use data breakpoints throws the splat shown below (*).
> This is because the check for a data breakpoint in DSISR is in
> do_hash_page(). Move this check to handle_page_fault() so as to catch
> data breakpoints in both hash and radix MMU modes.

Why cant we check for DSISR inside do_hash_page() on P9 ?

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Michael Ellerman]

Anshuman Khandual <khandual@linux.vnet.ibm.com> writes:

> On 06/14/2017 12:12 AM, Naveen N. Rao wrote:
>> On P9, trying to use data breakpoints throws the splat shown below (*).
>> This is because the check for a data breakpoint in DSISR is in
>> do_hash_page(). Move this check to handle_page_fault() so as to catch
>> data breakpoints in both hash and radix MMU modes.
>
> Why cant we check for DSISR inside do_hash_page() on P9 ?

We can.

But we also need to check inside handle_page_fault(), because when we're
in Radix mode we don't go via do_hash_page().

So rather than doing it in two places, the patch changes the logic so we
check in handle_page_fault(), and teaches the do_hash_page() code to go
there if DSISR_DABRMATCH is set.

cheers

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index ae418b85c17c..17ee701b8336 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1442,7 +1440,9 @@ do_hash_page:
>  
>  /* Here we have a page fault that hash_page can't handle. */
>  handle_page_fault:
> -11:	ld	r4,_DAR(r1)
> +	andis.  r0,r4,DSISR_DABRMATCH@h
> +	bne-    handle_dabr_fault

This broke hash. Please test hash! :)

I added:

@@ -1438,11 +1436,16 @@ do_hash_page:
 
        /* Error */
        blt-    13f
+
+       /* Reload DSISR into r4 for the DABR check below */
+       ld      r4,_DSISR(r1)
 #endif /* CONFIG_PPC_STD_MMU_64 */
 
 /* Here we have a page fault that hash_page can't handle. */
 handle_page_fault:


cheers

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Naveen N. Rao]

On 2017/06/16 03:16PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > index ae418b85c17c..17ee701b8336 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -1442,7 +1440,9 @@ do_hash_page:
> >  
> >  /* Here we have a page fault that hash_page can't handle. */
> >  handle_page_fault:
> > -11:	ld	r4,_DAR(r1)
> > +	andis.  r0,r4,DSISR_DABRMATCH@h
> > +	bne-    handle_dabr_fault
> 
> This broke hash. Please test hash! :)

Gah! :double-face-palm:
I don't know how I missed this... (yes, I do)

> 
> I added:
> 
> @@ -1438,11 +1436,16 @@ do_hash_page:
> 
>         /* Error */
>         blt-    13f
> +
> +       /* Reload DSISR into r4 for the DABR check below */
> +       ld      r4,_DSISR(r1)
>  #endif /* CONFIG_PPC_STD_MMU_64 */
> 
>  /* Here we have a page fault that hash_page can't handle. */
>  handle_page_fault:

As always, thanks Michael!

I think we can optimize this a bit more to eliminate the loads in
handle_page_fault. Here's an incremental patch above your changes for
-next, this time boot-tested with radix and disable_radix.

- Naveen

-----
[PATCH] powerpc64/exceptions64s: Eliminate a few un-necessary memory loads

In do_hash_page(), we re-load DSISR from stack though it is still present
in register r4. Eliminate the memory load by preserving this register.

Furthermore, handler_page_fault() reloads DAR and DSISR from memory and
this is only required if we fall through from do_hash_page().
Otherwise, r3 and r4 already have DAR and DSISR loaded. Re-use those
and have do_hash_page() reload those registers when falling-through.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index dd619faab862..b5182a1ef3d6 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1426,8 +1426,8 @@ do_hash_page:
 	 *
 	 * at return r3 = 0 for success, 1 for page fault, negative for error
 	 */
+	mr	r6,r4
         mr 	r4,r12
-	ld      r6,_DSISR(r1)
 	bl	__hash_page		/* build HPTE if possible */
         cmpdi	r3,0			/* see if __hash_page succeeded */
 
@@ -1437,7 +1437,8 @@ do_hash_page:
 	/* Error */
 	blt-	13f
 
-	/* Reload DSISR into r4 for the DABR check below */
+	/* Reload DAR/DSISR for handle_page_fault */
+	ld	r3,_DAR(r1)
 	ld      r4,_DSISR(r1)
 #endif /* CONFIG_PPC_STD_MMU_64 */
 
@@ -1445,8 +1446,8 @@ do_hash_page:
 handle_page_fault:
 	andis.  r0,r4,DSISR_DABRMATCH@h
 	bne-    handle_dabr_fault
-	ld	r4,_DAR(r1)
-	ld	r5,_DSISR(r1)
+	mr	r5,r4
+	mr	r4,r3
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_page_fault
 	cmpdi	r3,0
-- 
2.13.1

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Aneesh Kumar K.V]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> On 2017/06/16 03:16PM, Michael Ellerman wrote:
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> 
>> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> > index ae418b85c17c..17ee701b8336 100644
>> > --- a/arch/powerpc/kernel/exceptions-64s.S
>> > +++ b/arch/powerpc/kernel/exceptions-64s.S
>> > @@ -1442,7 +1440,9 @@ do_hash_page:
>> >  
>> >  /* Here we have a page fault that hash_page can't handle. */
>> >  handle_page_fault:
>> > -11:	ld	r4,_DAR(r1)
>> > +	andis.  r0,r4,DSISR_DABRMATCH@h
>> > +	bne-    handle_dabr_fault
>> 
>> This broke hash. Please test hash! :)
>
> Gah! :double-face-palm:
> I don't know how I missed this... (yes, I do)
>
>> 
>> I added:
>> 
>> @@ -1438,11 +1436,16 @@ do_hash_page:
>> 
>>         /* Error */
>>         blt-    13f
>> +
>> +       /* Reload DSISR into r4 for the DABR check below */
>> +       ld      r4,_DSISR(r1)
>>  #endif /* CONFIG_PPC_STD_MMU_64 */
>> 
>>  /* Here we have a page fault that hash_page can't handle. */
>>  handle_page_fault:
>
> As always, thanks Michael!
>
> I think we can optimize this a bit more to eliminate the loads in
> handle_page_fault. Here's an incremental patch above your changes for
> -next, this time boot-tested with radix and disable_radix.
>
> - Naveen
>
> -----
> [PATCH] powerpc64/exceptions64s: Eliminate a few un-necessary memory loads
>
> In do_hash_page(), we re-load DSISR from stack though it is still present
> in register r4. Eliminate the memory load by preserving this register.
>
> Furthermore, handler_page_fault() reloads DAR and DSISR from memory and
> this is only required if we fall through from do_hash_page().
> Otherwise, r3 and r4 already have DAR and DSISR loaded. Re-use those
> and have do_hash_page() reload those registers when falling-through.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index dd619faab862..b5182a1ef3d6 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1426,8 +1426,8 @@ do_hash_page:
>  	 *
>  	 * at return r3 = 0 for success, 1 for page fault, negative for error
>  	 */
> +	mr	r6,r4
>          mr 	r4,r12
> -	ld      r6,_DSISR(r1)
>  	bl	__hash_page		/* build HPTE if possible */
>          cmpdi	r3,0			/* see if __hash_page succeeded */
>
> @@ -1437,7 +1437,8 @@ do_hash_page:
>  	/* Error */
>  	blt-	13f
>
> -	/* Reload DSISR into r4 for the DABR check below */
> +	/* Reload DAR/DSISR for handle_page_fault */
> +	ld	r3,_DAR(r1)
>  	ld      r4,_DSISR(r1)
>  #endif /* CONFIG_PPC_STD_MMU_64 */
>
> @@ -1445,8 +1446,8 @@ do_hash_page:
>  handle_page_fault:
>  	andis.  r0,r4,DSISR_DABRMATCH@h
>  	bne-    handle_dabr_fault
> -	ld	r4,_DAR(r1)
> -	ld	r5,_DSISR(r1)
> +	mr	r5,r4
> +	mr	r4,r3
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	bl	do_page_fault
>  	cmpdi	r3,0


Can we avoid that if we rearrange args of other functions calls, so that
we can use r3 and r4 as it is ?

-anees

Re: [PATCH] powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Naveen N. Rao]

On 2017/06/19 03:21PM, Aneesh Kumar K.V wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > On 2017/06/16 03:16PM, Michael Ellerman wrote:
> >> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> >> 
> >> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> >> > index ae418b85c17c..17ee701b8336 100644
> >> > --- a/arch/powerpc/kernel/exceptions-64s.S
> >> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> >> > @@ -1442,7 +1440,9 @@ do_hash_page:
> >> >  
> >> >  /* Here we have a page fault that hash_page can't handle. */
> >> >  handle_page_fault:
> >> > -11:	ld	r4,_DAR(r1)
> >> > +	andis.  r0,r4,DSISR_DABRMATCH@h
> >> > +	bne-    handle_dabr_fault
> >> 
> >> This broke hash. Please test hash! :)
> >
> > Gah! :double-face-palm:
> > I don't know how I missed this... (yes, I do)
> >
> >> 
> >> I added:
> >> 
> >> @@ -1438,11 +1436,16 @@ do_hash_page:
> >> 
> >>         /* Error */
> >>         blt-    13f
> >> +
> >> +       /* Reload DSISR into r4 for the DABR check below */
> >> +       ld      r4,_DSISR(r1)
> >>  #endif /* CONFIG_PPC_STD_MMU_64 */
> >> 
> >>  /* Here we have a page fault that hash_page can't handle. */
> >>  handle_page_fault:
> >
> > As always, thanks Michael!
> >
> > I think we can optimize this a bit more to eliminate the loads in
> > handle_page_fault. Here's an incremental patch above your changes for
> > -next, this time boot-tested with radix and disable_radix.
> >
> > - Naveen
> >
> > -----
> > [PATCH] powerpc64/exceptions64s: Eliminate a few un-necessary memory loads
> >
> > In do_hash_page(), we re-load DSISR from stack though it is still present
> > in register r4. Eliminate the memory load by preserving this register.
> >
> > Furthermore, handler_page_fault() reloads DAR and DSISR from memory and
> > this is only required if we fall through from do_hash_page().
> > Otherwise, r3 and r4 already have DAR and DSISR loaded. Re-use those
> > and have do_hash_page() reload those registers when falling-through.
> >
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/exceptions-64s.S | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > index dd619faab862..b5182a1ef3d6 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -1426,8 +1426,8 @@ do_hash_page:
> >  	 *
> >  	 * at return r3 = 0 for success, 1 for page fault, negative for error
> >  	 */
> > +	mr	r6,r4
> >          mr 	r4,r12
> > -	ld      r6,_DSISR(r1)
> >  	bl	__hash_page		/* build HPTE if possible */
> >          cmpdi	r3,0			/* see if __hash_page succeeded */
> >
> > @@ -1437,7 +1437,8 @@ do_hash_page:
> >  	/* Error */
> >  	blt-	13f
> >
> > -	/* Reload DSISR into r4 for the DABR check below */
> > +	/* Reload DAR/DSISR for handle_page_fault */
> > +	ld	r3,_DAR(r1)
> >  	ld      r4,_DSISR(r1)
> >  #endif /* CONFIG_PPC_STD_MMU_64 */
> >
> > @@ -1445,8 +1446,8 @@ do_hash_page:
> >  handle_page_fault:
> >  	andis.  r0,r4,DSISR_DABRMATCH@h
> >  	bne-    handle_dabr_fault
> > -	ld	r4,_DAR(r1)
> > -	ld	r5,_DSISR(r1)
> > +	mr	r5,r4
> > +	mr	r4,r3
> >  	addi	r3,r1,STACK_FRAME_OVERHEAD
> >  	bl	do_page_fault
> >  	cmpdi	r3,0
> 
> 
> Can we avoid that if we rearrange args of other functions calls, so that
> we can use r3 and r4 as it is ?

I looked at changing do_page_fault(), but it is called from other places 
(booke, entry_32, ..) so, re-arranging the arguments will need more 
intrusive changes there potentially slowing those down.

However, I do think we can change the exception vectors to load things 
up differently for do_page_fault() and handle_page_fault(). I will 
check.

Thanks for the review,
- Naveen

[PATCH v2] powerpc64/exceptions: Refactor code to eliminate a few memory loads

[Naveen N. Rao]

On 2017/06/19 03:21PM, Aneesh Kumar K.V wrote:
> > @@ -1445,8 +1446,8 @@ do_hash_page:
> >  handle_page_fault:
> >  	andis.  r0,r4,DSISR_DABRMATCH@h
> >  	bne-    handle_dabr_fault
> > -	ld	r4,_DAR(r1)
> > -	ld	r5,_DSISR(r1)
> > +	mr	r5,r4
> > +	mr	r4,r3
> >  	addi	r3,r1,STACK_FRAME_OVERHEAD
> >  	bl	do_page_fault
> >  	cmpdi	r3,0
> 
> 
> Can we avoid that if we rearrange args of other functions calls, so that
> we can use r3 and r4 as it is ?

Here's a version that does that. Again, boot tested with radix and 
disable_radix.

Thanks,
Naveen

-
Change data_access_common() and instruction_access_common() to load the
trap number in r3, DAR in r4 and DSISR in r5 (rather than in r5, r3 and
r4 respectively). This change allows us to eliminate a few un-necessary
memory loads and register move operations in handle_page_fault(),
handle_dabr_fault() and label '77'.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 38 +++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index b6fad9790784..4c5abe1d6f44 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -494,11 +494,11 @@ EXC_COMMON_BEGIN(data_access_common)
 	EXCEPTION_PROLOG_COMMON(0x300, PACA_EXGEN)
 	RECONCILE_IRQ_STATE(r10, r11)
 	ld	r12,_MSR(r1)
-	ld	r3,PACA_EXGEN+EX_DAR(r13)
-	lwz	r4,PACA_EXGEN+EX_DSISR(r13)
-	li	r5,0x300
-	std	r3,_DAR(r1)
-	std	r4,_DSISR(r1)
+	ld	r4,PACA_EXGEN+EX_DAR(r13)
+	lwz	r5,PACA_EXGEN+EX_DSISR(r13)
+	li	r3,0x300
+	std	r4,_DAR(r1)
+	std	r5,_DSISR(r1)
 BEGIN_MMU_FTR_SECTION
 	b	do_hash_page		/* Try to handle as hpte fault */
 MMU_FTR_SECTION_ELSE
@@ -562,11 +562,11 @@ EXC_COMMON_BEGIN(instruction_access_common)
 	EXCEPTION_PROLOG_COMMON(0x400, PACA_EXGEN)
 	RECONCILE_IRQ_STATE(r10, r11)
 	ld	r12,_MSR(r1)
-	ld	r3,_NIP(r1)
-	andis.	r4,r12,0x5820
-	li	r5,0x400
-	std	r3,_DAR(r1)
-	std	r4,_DSISR(r1)
+	ld	r4,_NIP(r1)
+	andis.	r5,r12,0x5820
+	li	r3,0x400
+	std	r4,_DAR(r1)
+	std	r5,_DSISR(r1)
 BEGIN_MMU_FTR_SECTION
 	b	do_hash_page		/* Try to handle as hpte fault */
 MMU_FTR_SECTION_ELSE
@@ -1474,7 +1474,7 @@ USE_TEXT_SECTION()
 	.balign	IFETCH_ALIGN_BYTES
 do_hash_page:
 #ifdef CONFIG_PPC_STD_MMU_64
-	andis.	r0,r4,0xa450		/* weird error? */
+	andis.	r0,r5,0xa450		/* weird error? */
 	bne-	handle_page_fault	/* if not, try to insert a HPTE */
 	CURRENT_THREAD_INFO(r11, r1)
 	lwz	r0,TI_PREEMPT(r11)	/* If we're in an "NMI" */
@@ -1489,8 +1489,10 @@ do_hash_page:
 	 *
 	 * at return r3 = 0 for success, 1 for page fault, negative for error
 	 */
+	mr	r6,r5
+	mr	r5,r3
+	mr	r3,r4
         mr 	r4,r12
-	ld      r6,_DSISR(r1)
 	bl	__hash_page		/* build HPTE if possible */
         cmpdi	r3,0			/* see if __hash_page succeeded */
 
@@ -1500,16 +1502,15 @@ do_hash_page:
 	/* Error */
 	blt-	13f
 
-	/* Reload DSISR into r4 for the DABR check below */
-	ld      r4,_DSISR(r1)
+	/* Reload DAR/DSISR for handle_page_fault */
+	ld	r4,_DAR(r1)
+	ld      r5,_DSISR(r1)
 #endif /* CONFIG_PPC_STD_MMU_64 */
 
 /* Here we have a page fault that hash_page can't handle. */
 handle_page_fault:
-	andis.  r0,r4,DSISR_DABRMATCH@h
+	andis.  r0,r5,DSISR_DABRMATCH@h
 	bne-    handle_dabr_fault
-	ld	r4,_DAR(r1)
-	ld	r5,_DSISR(r1)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_page_fault
 	cmpdi	r3,0
@@ -1524,8 +1525,6 @@ handle_page_fault:
 /* We have a data breakpoint exception - handle it */
 handle_dabr_fault:
 	bl	save_nvgprs
-	ld      r4,_DAR(r1)
-	ld      r5,_DSISR(r1)
 	addi    r3,r1,STACK_FRAME_OVERHEAD
 	bl      do_break
 12:	b       ret_from_except_lite
@@ -1551,7 +1550,6 @@ handle_dabr_fault:
  * the access, or panic if there isn't a handler.
  */
 77:	bl	save_nvgprs
-	mr	r4,r3
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	li	r5,SIGSEGV
 	bl	bad_page_fault
-- 
2.13.1

Re: [PATCH v2] powerpc64/exceptions: Refactor code to eliminate a few memory loads

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> On 2017/06/19 03:21PM, Aneesh Kumar K.V wrote:
>> > @@ -1445,8 +1446,8 @@ do_hash_page:
>> >  handle_page_fault:
>> >  	andis.  r0,r4,DSISR_DABRMATCH@h
>> >  	bne-    handle_dabr_fault
>> > -	ld	r4,_DAR(r1)
>> > -	ld	r5,_DSISR(r1)
>> > +	mr	r5,r4
>> > +	mr	r4,r3
>> >  	addi	r3,r1,STACK_FRAME_OVERHEAD
>> >  	bl	do_page_fault
>> >  	cmpdi	r3,0
>> 
>> 
>> Can we avoid that if we rearrange args of other functions calls, so that
>> we can use r3 and r4 as it is ?
>
> Here's a version that does that. Again, boot tested with radix and 
> disable_radix.
>
> Thanks,
> Naveen
>
> -
> Change data_access_common() and instruction_access_common() to load the
> trap number in r3, DAR in r4 and DSISR in r5 (rather than in r5, r3 and
> r4 respectively). This change allows us to eliminate a few un-necessary
> memory loads and register move operations in handle_page_fault(),
> handle_dabr_fault() and label '77'.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 38 +++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)

Sorry I missed this and now it doesn't apply. Do you mind rebasing.

cheers

Re: [PATCH v2] powerpc64/exceptions: Refactor code to eliminate a few memory loads

[Naveen N. Rao]

Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
>> On 2017/06/19 03:21PM, Aneesh Kumar K.V wrote:
>>> > @@ -1445,8 +1446,8 @@ do_hash_page:
>>> >  handle_page_fault:
>>> >  	andis.  r0,r4,DSISR_DABRMATCH@h
>>> >  	bne-    handle_dabr_fault
>>> > -	ld	r4,_DAR(r1)
>>> > -	ld	r5,_DSISR(r1)
>>> > +	mr	r5,r4
>>> > +	mr	r4,r3
>>> >  	addi	r3,r1,STACK_FRAME_OVERHEAD
>>> >  	bl	do_page_fault
>>> >  	cmpdi	r3,0
>>> 
>>> 
>>> Can we avoid that if we rearrange args of other functions calls, so that
>>> we can use r3 and r4 as it is ?
>>
>> Here's a version that does that. Again, boot tested with radix and 
>> disable_radix.
>>
>> Thanks,
>> Naveen
>>
>> -
>> Change data_access_common() and instruction_access_common() to load the
>> trap number in r3, DAR in r4 and DSISR in r5 (rather than in r5, r3 and
>> r4 respectively). This change allows us to eliminate a few un-necessary
>> memory loads and register move operations in handle_page_fault(),
>> handle_dabr_fault() and label '77'.
>>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/exceptions-64s.S | 38 +++++++++++++++++-------------------
>>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> Sorry I missed this and now it doesn't apply. Do you mind rebasing.

No problem - this is just a small refactoring after all :).
Here's a rebased version.  Boot tested on mambo with/without radix.

Thanks,
Naveen

--
[PATCH v3] powerpc/exceptions64s: Eliminate a few un-necessary memory  
loads

Change data_access_common() and instruction_access_common() to load the
trap number in r3, DAR in r4 and DSISR in r5 (rather than in r5, r3 and
r4 respectively). This change allows us to eliminate a few un-necessary
memory loads and register move operations in handle_page_fault(),
handle_dabr_fault() and label '77'.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 38 +++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 72cffa0c4af6..b8166b4fa728 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -499,11 +499,11 @@ EXC_COMMON_BEGIN(data_access_common)
 	EXCEPTION_PROLOG_COMMON(0x300, PACA_EXGEN)
 	RECONCILE_IRQ_STATE(r10, r11)
 	ld	r12,_MSR(r1)
-	ld	r3,PACA_EXGEN+EX_DAR(r13)
-	lwz	r4,PACA_EXGEN+EX_DSISR(r13)
-	li	r5,0x300
-	std	r3,_DAR(r1)
-	std	r4,_DSISR(r1)
+	ld	r4,PACA_EXGEN+EX_DAR(r13)
+	lwz	r5,PACA_EXGEN+EX_DSISR(r13)
+	li	r3,0x300
+	std	r4,_DAR(r1)
+	std	r5,_DSISR(r1)
 BEGIN_MMU_FTR_SECTION
 	b	do_hash_page		/* Try to handle as hpte fault */
 MMU_FTR_SECTION_ELSE
@@ -543,11 +543,11 @@ EXC_COMMON_BEGIN(instruction_access_common)
 	EXCEPTION_PROLOG_COMMON(0x400, PACA_EXGEN)
 	RECONCILE_IRQ_STATE(r10, r11)
 	ld	r12,_MSR(r1)
-	ld	r3,_NIP(r1)
-	andis.	r4,r12,DSISR_BAD_FAULT_64S@h
-	li	r5,0x400
-	std	r3,_DAR(r1)
-	std	r4,_DSISR(r1)
+	ld	r4,_NIP(r1)
+	andis.	r5,r12,DSISR_BAD_FAULT_64S@h
+	li	r3,0x400
+	std	r4,_DAR(r1)
+	std	r5,_DSISR(r1)
 BEGIN_MMU_FTR_SECTION
 	b	do_hash_page		/* Try to handle as hpte fault */
 MMU_FTR_SECTION_ELSE
@@ -1523,7 +1523,7 @@ do_hash_page:
 #ifdef CONFIG_PPC_BOOK3S_64
 	lis	r0,DSISR_BAD_FAULT_64S@h
 	ori	r0,r0,DSISR_BAD_FAULT_64S@l
-	and.	r0,r4,r0		/* weird error? */
+	and.	r0,r5,r0		/* weird error? */
 	bne-	handle_page_fault	/* if not, try to insert a HPTE */
 	CURRENT_THREAD_INFO(r11, r1)
 	lwz	r0,TI_PREEMPT(r11)	/* If we're in an "NMI" */
@@ -1538,8 +1538,10 @@ do_hash_page:
 	 *
 	 * at return r3 = 0 for success, 1 for page fault, negative for error
 	 */
+	mr	r6,r5
+	mr	r5,r3
+	mr	r3,r4
         mr 	r4,r12
-	ld      r6,_DSISR(r1)
 	bl	__hash_page		/* build HPTE if possible */
         cmpdi	r3,0			/* see if __hash_page succeeded */
 
@@ -1549,16 +1551,15 @@ do_hash_page:
 	/* Error */
 	blt-	13f
 
-	/* Reload DSISR into r4 for the DABR check below */
-	ld      r4,_DSISR(r1)
+	/* Reload DAR/DSISR for handle_page_fault */
+	ld	r4,_DAR(r1)
+	ld      r5,_DSISR(r1)
 #endif /* CONFIG_PPC_BOOK3S_64 */
 
 /* Here we have a page fault that hash_page can't handle. */
 handle_page_fault:
-11:	andis.  r0,r4,DSISR_DABRMATCH@h
+	andis.  r0,r5,DSISR_DABRMATCH@h
 	bne-    handle_dabr_fault
-	ld	r4,_DAR(r1)
-	ld	r5,_DSISR(r1)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_page_fault
 	cmpdi	r3,0
@@ -1573,8 +1574,6 @@ handle_page_fault:
 /* We have a data breakpoint exception - handle it */
 handle_dabr_fault:
 	bl	save_nvgprs
-	ld      r4,_DAR(r1)
-	ld      r5,_DSISR(r1)
 	addi    r3,r1,STACK_FRAME_OVERHEAD
 	bl      do_break
 12:	b       ret_from_except_lite
@@ -1600,7 +1599,6 @@ handle_dabr_fault:
  * the access, or panic if there isn't a handler.
  */
 77:	bl	save_nvgprs
-	mr	r4,r3
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	li	r5,SIGSEGV
 	bl	bad_page_fault
-- 
2.15.0

Re: [PATCH v2] powerpc64/exceptions: Refactor code to eliminate a few memory loads

[Christophe Leroy]

Le 13/11/2017 à 18:04, Naveen N. Rao a écrit :
> Michael Ellerman wrote:
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>>
>>> On 2017/06/19 03:21PM, Aneesh Kumar K.V wrote:
>>>>> @@ -1445,8 +1446,8 @@ do_hash_page:
>>>>>   handle_page_fault:
>>>>>   	andis.  r0,r4,DSISR_DABRMATCH@h
>>>>>   	bne-    handle_dabr_fault
>>>>> -	ld	r4,_DAR(r1)
>>>>> -	ld	r5,_DSISR(r1)
>>>>> +	mr	r5,r4
>>>>> +	mr	r4,r3
>>>>>   	addi	r3,r1,STACK_FRAME_OVERHEAD
>>>>>   	bl	do_page_fault
>>>>>   	cmpdi	r3,0
>>>>
>>>>
>>>> Can we avoid that if we rearrange args of other functions calls, so that
>>>> we can use r3 and r4 as it is ?
>>>
>>> Here's a version that does that. Again, boot tested with radix and
>>> disable_radix.
>>>
>>> Thanks,
>>> Naveen
>>>
>>> -
>>> Change data_access_common() and instruction_access_common() to load the
>>> trap number in r3, DAR in r4 and DSISR in r5 (rather than in r5, r3 and
>>> r4 respectively). This change allows us to eliminate a few un-necessary
>>> memory loads and register move operations in handle_page_fault(),
>>> handle_dabr_fault() and label '77'.
>>>
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/exceptions-64s.S | 38 +++++++++++++++++-------------------
>>>   1 file changed, 18 insertions(+), 20 deletions(-)
>>
>> Sorry I missed this and now it doesn't apply. Do you mind rebasing.
> 
> No problem - this is just a small refactoring after all :).
> Here's a rebased version.  Boot tested on mambo with/without radix.
> 
> Thanks,
> Naveen
> 
> --
> [PATCH v3] powerpc/exceptions64s: Eliminate a few un-necessary memory
> loads
> 
> Change data_access_common() and instruction_access_common() to load the
> trap number in r3, DAR in r4 and DSISR in r5 (rather than in r5, r3 and
> r4 respectively). This change allows us to eliminate a few un-necessary
> memory loads and register move operations in handle_page_fault(),
> handle_dabr_fault() and label '77'.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/exceptions-64s.S | 38 +++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 72cffa0c4af6..b8166b4fa728 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -499,11 +499,11 @@ EXC_COMMON_BEGIN(data_access_common)
>   	EXCEPTION_PROLOG_COMMON(0x300, PACA_EXGEN)
>   	RECONCILE_IRQ_STATE(r10, r11)
>   	ld	r12,_MSR(r1)
> -	ld	r3,PACA_EXGEN+EX_DAR(r13)
> -	lwz	r4,PACA_EXGEN+EX_DSISR(r13)
> -	li	r5,0x300
> -	std	r3,_DAR(r1)
> -	std	r4,_DSISR(r1)
> +	ld	r4,PACA_EXGEN+EX_DAR(r13)
> +	lwz	r5,PACA_EXGEN+EX_DSISR(r13)
> +	li	r3,0x300
> +	std	r4,_DAR(r1)
> +	std	r5,_DSISR(r1)
>   BEGIN_MMU_FTR_SECTION
>   	b	do_hash_page		/* Try to handle as hpte fault */
>   MMU_FTR_SECTION_ELSE
> @@ -543,11 +543,11 @@ EXC_COMMON_BEGIN(instruction_access_common)
>   	EXCEPTION_PROLOG_COMMON(0x400, PACA_EXGEN)
>   	RECONCILE_IRQ_STATE(r10, r11)
>   	ld	r12,_MSR(r1)
> -	ld	r3,_NIP(r1)
> -	andis.	r4,r12,DSISR_BAD_FAULT_64S@h
> -	li	r5,0x400
> -	std	r3,_DAR(r1)
> -	std	r4,_DSISR(r1)
> +	ld	r4,_NIP(r1)
> +	andis.	r5,r12,DSISR_BAD_FAULT_64S@h
> +	li	r3,0x400
> +	std	r4,_DAR(r1)
> +	std	r5,_DSISR(r1)
>   BEGIN_MMU_FTR_SECTION
>   	b	do_hash_page		/* Try to handle as hpte fault */
>   MMU_FTR_SECTION_ELSE
> @@ -1523,7 +1523,7 @@ do_hash_page:
>   #ifdef CONFIG_PPC_BOOK3S_64
>   	lis	r0,DSISR_BAD_FAULT_64S@h
>   	ori	r0,r0,DSISR_BAD_FAULT_64S@l
> -	and.	r0,r4,r0		/* weird error? */
> +	and.	r0,r5,r0		/* weird error? */
>   	bne-	handle_page_fault	/* if not, try to insert a HPTE */
>   	CURRENT_THREAD_INFO(r11, r1)
>   	lwz	r0,TI_PREEMPT(r11)	/* If we're in an "NMI" */
> @@ -1538,8 +1538,10 @@ do_hash_page:
>   	 *
>   	 * at return r3 = 0 for success, 1 for page fault, negative for error
>   	 */
> +	mr	r6,r5
> +	mr	r5,r3
> +	mr	r3,r4
>           mr 	r4,r12
> -	ld      r6,_DSISR(r1)
>   	bl	__hash_page		/* build HPTE if possible */
>           cmpdi	r3,0			/* see if __hash_page succeeded */
>   
> @@ -1549,16 +1551,15 @@ do_hash_page:
>   	/* Error */
>   	blt-	13f
>   
> -	/* Reload DSISR into r4 for the DABR check below */
> -	ld      r4,_DSISR(r1)
> +	/* Reload DAR/DSISR for handle_page_fault */
> +	ld	r4,_DAR(r1)
> +	ld      r5,_DSISR(r1)
>   #endif /* CONFIG_PPC_BOOK3S_64 */
>   
>   /* Here we have a page fault that hash_page can't handle. */
>   handle_page_fault:
> -11:	andis.  r0,r4,DSISR_DABRMATCH@h
> +	andis.  r0,r5,DSISR_DABRMATCH@h
>   	bne-    handle_dabr_fault
> -	ld	r4,_DAR(r1)
> -	ld	r5,_DSISR(r1)
>   	addi	r3,r1,STACK_FRAME_OVERHEAD
>   	bl	do_page_fault
>   	cmpdi	r3,0
> @@ -1573,8 +1574,6 @@ handle_page_fault:
>   /* We have a data breakpoint exception - handle it */
>   handle_dabr_fault:
>   	bl	save_nvgprs
> -	ld      r4,_DAR(r1)
> -	ld      r5,_DSISR(r1)
>   	addi    r3,r1,STACK_FRAME_OVERHEAD
>   	bl      do_break
>   12:	b       ret_from_except_lite
> @@ -1600,7 +1599,6 @@ handle_dabr_fault:
>    * the access, or panic if there isn't a handler.
>    */
>   77:	bl	save_nvgprs
> -	mr	r4,r3
>   	addi	r3,r1,STACK_FRAME_OVERHEAD
>   	li	r5,SIGSEGV
>   	bl	bad_page_fault
> 

Seems like it was never merged but was superseded in 2019 by 
https://lore.kernel.org/r/20190802105709.27696-37-npiggin@gmail.com

Re: powerpc64/hw_breakpoints: Handle data breakpoints in radix mode

[Michael Ellerman]

On Tue, 2017-06-13 at 18:42:00 UTC, "Naveen N. Rao" wrote:
> On P9, trying to use data breakpoints throws the splat shown below (*).
> This is because the check for a data breakpoint in DSISR is in
> do_hash_page(). Move this check to handle_page_fault() so as to catch
> data breakpoints in both hash and radix MMU modes.
> 
> While at it, also remove the label '11' that was made redundant by
> commit a546498f3bf9aa ("powerpc: Call do_page_fault() with interrupts
> off")
> 
> (*)
>     Unable to handle kernel paging request for data at address 0xc000000000e19218
>     Faulting instruction address: 0xc0000000001155e8
>     cpu 0x0: Vector: 300 (Data Access) at [c0000000ef1e7b20]
>     pc: c0000000001155e8: find_pid_ns+0x48/0xe0
>     lr: c000000000116ac4: find_task_by_vpid+0x44/0x90
>     sp: c0000000ef1e7da0
>     msr: 9000000000009033
>     dar: c000000000e19218
>     dsisr: 400000
>     current = 0xc0000000f1f59700
>     paca    = 0xc00000000fd40000 softe: 0 irq_happened: 0x01
>     pid   = 1192, comm = sh
>     Linux version 4.12.0-rc3-nnr (root@ea605ec2993c) (gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.1) ) #74 SMP Tue Jun 13 16:52:49 UTC 2017
>     enter ? for help
>     [c0000000ef1e7dc0] c000000000116ac4 find_task_by_vpid+0x44/0x90
>     [c0000000ef1e7de0] c000000000108800 SyS_setpgid+0x80/0x220
>     [c0000000ef1e7e30] c00000000000ba6c system_call+0x38/0xfc
>     --- Exception: c01 (System Call) at 00007fff94480890
>     SP (7fffd91e7260) is in userspace
> 
> Fixes: caca285e5ab4a ("powerpc/mm/radix: Use STD_MMU_64 to properly
> isolate hash related code")
> Reported-by: Shriya R. Kulkarni <shriykul@in.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/d89ba5353f301971dd7d2f9fdf25c4

cheers