[PATCH 0/2] 4xx xmon fixes

[PATCH 0/2] 4xx xmon fixes

[Josh Boyer]

Hi All,

A couple of simple fixes to improve xmon usage on 4xx boards.  I have no idea
if anyone else even uses this, but I find it handy from time to time.

josh

[PATCH 1/2] powerpc/4xx: Fix erroneous xmon warning on PowerPC 4xx

[Josh Boyer]

The xmon code relies on MSR_RI being non-zero to indicate that an exception
is recoverable.  If it is not, it prints a warning message.  However, the
PowerPC 4xx cores do not have an MSR_RI bit and this warning is produced for
every xmon event.

This introduces an unrecoverable_excp function to determine if an exception
is recoverable or not.  This gets rid of the erroneous warnings on 4xx.

Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
---
 arch/powerpc/xmon/xmon.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 0e09a45..c6f0a71 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -335,6 +335,16 @@ int cpus_are_in_xmon(void)
 }
 #endif
 
+static inline int unrecoverable_excp(struct pt_regs *regs)
+{
+#ifdef CONFIG_4xx
+	/* We have no MSR_RI bit on 4xx, so we simply return false */
+	return 0;
+#else
+	return ((regs->msr & MSR_RI) == 0);
+#endif
+}
+
 static int xmon_core(struct pt_regs *regs, int fromipi)
 {
 	int cmd = 0;
@@ -388,7 +398,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
 	bp = NULL;
 	if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF))
 		bp = at_breakpoint(regs->nip);
-	if (bp || (regs->msr & MSR_RI) == 0)
+	if (bp || unrecoverable_excp(regs))
 		fromipi = 0;
 
 	if (!fromipi) {
@@ -399,7 +409,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
 			       cpu, BP_NUM(bp));
 			xmon_print_symbol(regs->nip, " ", ")\n");
 		}
-		if ((regs->msr & MSR_RI) == 0)
+		if (unrecoverable_excp(regs))
 			printf("WARNING: exception is not recoverable, "
 			       "can't continue\n");
 		release_output_lock();
@@ -490,7 +500,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
 			printf("Stopped at breakpoint %x (", BP_NUM(bp));
 			xmon_print_symbol(regs->nip, " ", ")\n");
 		}
-		if ((regs->msr & MSR_RI) == 0)
+		if (unrecoverable_excp(regs))
 			printf("WARNING: exception is not recoverable, "
 			       "can't continue\n");
 		remove_bpts();
-- 
1.5.5.6

[PATCH 2/2] powerpc/44x: Fix xmon single step on PowerPC 44x

[Josh Boyer]

Prior to the arch/ppc -> arch/powerpc transition, xmon had support for single
stepping on 4xx boards.  The functionality was lost when arch/ppc was removed.
This patch restores single step support for 44x boards.

Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
---
 arch/powerpc/xmon/xmon.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index c6f0a71..fe2ad71 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -517,6 +517,15 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
 	in_xmon = 0;
 #endif
 
+#ifdef CONFIG_4xx
+	if ((regs->msr & (MSR_DE)) == (MSR_DE)) {
+		bp = at_breakpoint(regs->nip);
+		if (bp != NULL) {
+			regs->nip = (unsigned long) &bp->instr[0];
+			atomic_inc(&bp->ref_count);
+		}
+	}
+#else
 	if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF)) {
 		bp = at_breakpoint(regs->nip);
 		if (bp != NULL) {
@@ -530,7 +539,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
 			}
 		}
 	}
-
+#endif
 	insert_cpu_bpts();
 
 	local_irq_restore(flags);
@@ -894,6 +903,14 @@ cmds(struct pt_regs *excp)
 	}
 }
 
+#ifdef CONFIG_44x
+static int do_step(struct pt_regs *regs)
+{
+	regs->msr |= MSR_DE;
+	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
+	return 1;
+}
+#else
 /*
  * Step a single instruction.
  * Some instructions we emulate, others we execute with MSR_SE set.
@@ -924,6 +941,7 @@ static int do_step(struct pt_regs *regs)
 	regs->msr |= MSR_SE;
 	return 1;
 }
+#endif
 
 static void bootcmds(void)
 {
-- 
1.5.5.6

Re: [PATCH 2/2] powerpc/44x: Fix xmon single step on PowerPC 44x

[Benjamin Herrenschmidt]

On Wed, 2009-09-23 at 09:51 -0400, Josh Boyer wrote:
> Prior to the arch/ppc -> arch/powerpc transition, xmon had support for single
> stepping on 4xx boards.  The functionality was lost when arch/ppc was removed.
> This patch restores single step support for 44x boards.
> 
> Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
> ---
>  arch/powerpc/xmon/xmon.c |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index c6f0a71..fe2ad71 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -517,6 +517,15 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
>  	in_xmon = 0;
>  #endif
>  
> +#ifdef CONFIG_4xx
> +	if ((regs->msr & (MSR_DE)) == (MSR_DE)) {

Why not just if (regs->msr & MSR_DE) ?

> +		bp = at_breakpoint(regs->nip);
> +		if (bp != NULL) {
> +			regs->nip = (unsigned long) &bp->instr[0];
> +			atomic_inc(&bp->ref_count);
> +		}
> +	}
> +#else

Any reason why that couldn't be in CONFIG_BOOKE ?

>  	if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF)) {
>  		bp = at_breakpoint(regs->nip);
>  		if (bp != NULL) {
> @@ -530,7 +539,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
>  			}
>  		}
>  	}
> -
> +#endif
>  	insert_cpu_bpts();
>  
>  	local_irq_restore(flags);
> @@ -894,6 +903,14 @@ cmds(struct pt_regs *excp)
>  	}
>  }
>  
> +#ifdef CONFIG_44x

Same comment about BOOKE intead of 44x

> +static int do_step(struct pt_regs *regs)
> +{
> +	regs->msr |= MSR_DE;
> +	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);

I'm not sure about setting IDM... Won't that be a problem if you have
an external debugger connected ?

> +	return 1;
> +}
> +#else
>  /*
>   * Step a single instruction.
>   * Some instructions we emulate, others we execute with MSR_SE set.
> @@ -924,6 +941,7 @@ static int do_step(struct pt_regs *regs)
>  	regs->msr |= MSR_SE;
>  	return 1;
>  }
> +#endif

Cheers,
Ben.

 
>  static void bootcmds(void)
>  {

Re: [PATCH 2/2] powerpc/44x: Fix xmon single step on PowerPC 44x

[Josh Boyer]

On Thu, Sep 24, 2009 at 07:34:34AM +1000, Benjamin Herrenschmidt wrote:
>On Wed, 2009-09-23 at 09:51 -0400, Josh Boyer wrote:
>> Prior to the arch/ppc -> arch/powerpc transition, xmon had support for single
>> stepping on 4xx boards.  The functionality was lost when arch/ppc was removed.
>> This patch restores single step support for 44x boards.
>> 
>> Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/xmon/xmon.c |   20 +++++++++++++++++++-
>>  1 files changed, 19 insertions(+), 1 deletions(-)
>> 
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index c6f0a71..fe2ad71 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -517,6 +517,15 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
>>  	in_xmon = 0;
>>  #endif
>>  
>> +#ifdef CONFIG_4xx
>> +	if ((regs->msr & (MSR_DE)) == (MSR_DE)) {
>
>Why not just if (regs->msr & MSR_DE) ?

Blind duplication of existing if case.  Will fix.

>> +		bp = at_breakpoint(regs->nip);
>> +		if (bp != NULL) {
>> +			regs->nip = (unsigned long) &bp->instr[0];
>> +			atomic_inc(&bp->ref_count);
>> +		}
>> +	}
>> +#else
>
>Any reason why that couldn't be in CONFIG_BOOKE ?

Off the top of my head, no.  I haven't tested on 40x yet though.  Will try
and do that and revise.

>>  	if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF)) {
>>  		bp = at_breakpoint(regs->nip);
>>  		if (bp != NULL) {
>> @@ -530,7 +539,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
>>  			}
>>  		}
>>  	}
>> -
>> +#endif
>>  	insert_cpu_bpts();
>>  
>>  	local_irq_restore(flags);
>> @@ -894,6 +903,14 @@ cmds(struct pt_regs *excp)
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_44x
>
>Same comment about BOOKE intead of 44x

Nod.

>> +static int do_step(struct pt_regs *regs)
>> +{
>> +	regs->msr |= MSR_DE;
>> +	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
>
>I'm not sure about setting IDM... Won't that be a problem if you have
>an external debugger connected ?

It could be.  I have no external debugger, thus no way to check it.  You don't
get an exception without IDM set though, so it won't trap back into xmon like
it should.  This is how we did it in arch/ppc (which isn't always a great
thing) as well.

I don't see how you could get it working without IDM, unless you inserted a
trap (aka breakpoint) every time.  That seems sort of suboptimal when we have
the IC event we can use.

josh

Re: [PATCH 2/2] powerpc/44x: Fix xmon single step on PowerPC 44x

[Benjamin Herrenschmidt]

On Wed, 2009-09-23 at 18:35 -0400, Josh Boyer wrote:

> >Any reason why that couldn't be in CONFIG_BOOKE ?
> 
> Off the top of my head, no.  I haven't tested on 40x yet though.  Will try
> and do that and revise.

Does 40x have CONFIG_BOOKE ? I was thinking more about FSL etc... but
yeah, 40x is worth having a look too.

> It could be.  I have no external debugger, thus no way to check it.  You don't
> get an exception without IDM set though, so it won't trap back into xmon like
> it should.  This is how we did it in arch/ppc (which isn't always a great
> thing) as well.

Ok, let's leave it there for now.

> I don't see how you could get it working without IDM, unless you inserted a
> trap (aka breakpoint) every time.  That seems sort of suboptimal when we have
> the IC event we can use.

The question is more whether IDM should be set once for all at boot (or
not) but let's ignore that for now.

Ben.

Re: [PATCH 2/2] powerpc/44x: Fix xmon single step on PowerPC 44x

[David Gibson]

On Thu, Sep 24, 2009 at 07:34:34AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2009-09-23 at 09:51 -0400, Josh Boyer wrote:
[snip]
> > +static int do_step(struct pt_regs *regs)
> > +{
> > +	regs->msr |= MSR_DE;
> > +	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
> 
> I'm not sure about setting IDM... Won't that be a problem if you have
> an external debugger connected ?

Not in theory.  It should be possible to set both IDM and EDM
simultaneously.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson