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"  writes:
> 
> > On 2017/06/16 03:16PM, Michael Ellerman wrote:
> >> "Naveen N. Rao"  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 
> > ---
> >  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
> > addir3,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

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

[Aneesh Kumar K.V]

"Naveen N. Rao"  writes:

> On 2017/06/16 03:16PM, Michael Ellerman wrote:
>> "Naveen N. Rao"  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 
> ---
>  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 */
>  cmpdir3,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
>   addir3,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/16 03:16PM, Michael Ellerman wrote:
> "Naveen N. Rao"  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 
---
 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
addir3,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

[Michael Ellerman]

"Naveen N. Rao"  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/14 04:41PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V"  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"  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

[Michael Ellerman]

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

[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"  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 
> >>> 0xc0e19218
> >>> Faulting instruction address: 0xc01155e8
> >>> cpu 0x0: Vector: 300 (Data Access) at [c000ef1e7b20]
> >>> pc: c01155e8: find_pid_ns+0x48/0xe0
> >>> lr: c0116ac4: find_task_by_vpid+0x44/0x90
> >>> sp: c000ef1e7da0
> >>> msr: 90009033
> >>> dar: c0e19218
> >>> dsisr: 40
> >>> current = 0xc000f1f59700
> >>> paca= 0xcfd4 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
> >>> [c000ef1e7dc0] c0116ac4 find_task_by_vpid+0x44/0x90
> >>> [c000ef1e7de0] c0108800 SyS_setpgid+0x80/0x220
> >>> [c000ef1e7e30] c000ba6c system_call+0x38/0xfc
> >>> --- Exception: c01 (System Call) at 7fff94480890
> >>> 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 
> >>>Signed-off-by: Naveen N. Rao 
> >>>---
> >>>  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"  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"  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

[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

[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"  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 
0xc0e19218
 Faulting instruction address: 0xc01155e8
 cpu 0x0: Vector: 300 (Data Access) at [c000ef1e7b20]
 pc: c01155e8: find_pid_ns+0x48/0xe0
 lr: c0116ac4: find_task_by_vpid+0x44/0x90
 sp: c000ef1e7da0
 msr: 90009033
 dar: c0e19218
 dsisr: 40
 current = 0xc000f1f59700
 paca= 0xcfd4 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
 [c000ef1e7dc0] c0116ac4 find_task_by_vpid+0x44/0x90
 [c000ef1e7de0] c0108800 SyS_setpgid+0x80/0x220
 [c000ef1e7e30] c000ba6c system_call+0x38/0xfc
 --- Exception: c01 (System Call) at 7fff94480890
 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 
Signed-off-by: Naveen N. Rao 
---
  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

[Naveen N. Rao]

Hi Aneesh,

On 2017/06/14 08:38AM, Aneesh Kumar K.V wrote:
> "Naveen N. Rao"  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 
> > 0xc0e19218
> > Faulting instruction address: 0xc01155e8
> > cpu 0x0: Vector: 300 (Data Access) at [c000ef1e7b20]
> > pc: c01155e8: find_pid_ns+0x48/0xe0
> > lr: c0116ac4: find_task_by_vpid+0x44/0x90
> > sp: c000ef1e7da0
> > msr: 90009033
> > dar: c0e19218
> > dsisr: 40
> > current = 0xc000f1f59700
> > paca= 0xcfd4 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
> > [c000ef1e7dc0] c0116ac4 find_task_by_vpid+0x44/0x90
> > [c000ef1e7de0] c0108800 SyS_setpgid+0x80/0x220
> > [c000ef1e7e30] c000ba6c system_call+0x38/0xfc
> > --- Exception: c01 (System Call) at 7fff94480890
> > 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 
> > Signed-off-by: Naveen N. Rao 
> > ---
> >  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]

"Naveen N. Rao"  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 
> 0xc0e19218
> Faulting instruction address: 0xc01155e8
> cpu 0x0: Vector: 300 (Data Access) at [c000ef1e7b20]
> pc: c01155e8: find_pid_ns+0x48/0xe0
> lr: c0116ac4: find_task_by_vpid+0x44/0x90
> sp: c000ef1e7da0
> msr: 90009033
> dar: c0e19218
> dsisr: 40
> current = 0xc000f1f59700
> paca= 0xcfd4 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
> [c000ef1e7dc0] c0116ac4 find_task_by_vpid+0x44/0x90
> [c000ef1e7de0] c0108800 SyS_setpgid+0x80/0x220
> [c000ef1e7e30] c000ba6c system_call+0x38/0xfc
> --- Exception: c01 (System Call) at 7fff94480890
> 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 
> Signed-off-by: Naveen N. Rao 
> ---
>  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)
>   addir3,r1,STACK_FRAME_OVERHEAD
>   bl  do_page_fault
> -- 
> 2.12.2


-aneesh

[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 
0xc0e19218
Faulting instruction address: 0xc01155e8
cpu 0x0: Vector: 300 (Data Access) at [c000ef1e7b20]
pc: c01155e8: find_pid_ns+0x48/0xe0
lr: c0116ac4: find_task_by_vpid+0x44/0x90
sp: c000ef1e7da0
msr: 90009033
dar: c0e19218
dsisr: 40
current = 0xc000f1f59700
paca= 0xcfd4 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
[c000ef1e7dc0] c0116ac4 find_task_by_vpid+0x44/0x90
[c000ef1e7de0] c0108800 SyS_setpgid+0x80/0x220
[c000ef1e7e30] c000ba6c system_call+0x38/0xfc
--- Exception: c01 (System Call) at 7fff94480890
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 
Signed-off-by: Naveen N. Rao 
---
 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)
addir3,r1,STACK_FRAME_OVERHEAD
bl  do_page_fault
-- 
2.12.2