[PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[David Howells]

Implement the GDB remote protocol 'p' command to retrieve a single register
value.

The 'p' command is tried by some gdb versions before they consider using the
'g' command (to batch-retrieve all the registers).  Before the following
commit:

	commit 7ca8b9c0dafd1cb36289aa4c92c7beae7adcd34f
	Author: David Howells <dhowells@redhat.com>
	Date:   Mon May 24 14:32:54 2010 -0700
	Subject: frv: extend gdbstub to support more features of gdb

the 'p' command just returned an empty reply, which causes gdb to then go and
use the 'g' command.  However, since that commit, the 'p' command returns an
error string, which causes gdb to abort its connection to the target.

Not all gdb versions are affected, some use try 'g' first, and if that works,
don't bother with 'p', and so don't see the error.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/frv/include/asm/gdb-stub.h |    2 +
 arch/frv/kernel/gdb-stub.c      |  116 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+), 0 deletions(-)


diff --git a/arch/frv/include/asm/gdb-stub.h b/arch/frv/include/asm/gdb-stub.h
index e6bedd0..8bb0288 100644
--- a/arch/frv/include/asm/gdb-stub.h
+++ b/arch/frv/include/asm/gdb-stub.h
@@ -60,6 +60,8 @@
 #define GDB_REG_SCR(N)	(141+(N))
 #define GDB_REG_LR	145
 #define GDB_REG_LCR	146
+#define GDB_REG_IACC0H	147
+#define GDB_REG_IACC0L	148
 #define GDB_REG_FSR0	149
 #define GDB_REG_ACC(N)	(150+(N))
 #define GDB_REG_ACCG(N)	(158+(N)/4)
diff --git a/arch/frv/kernel/gdb-stub.c b/arch/frv/kernel/gdb-stub.c
index 84d103c..1a65d7b 100644
--- a/arch/frv/kernel/gdb-stub.c
+++ b/arch/frv/kernel/gdb-stub.c
@@ -1789,6 +1789,104 @@ void gdbstub(int sigval)
 			flush_cache = 1;
 			break;
 
+			/* pNN: Read value of reg N and return it */
+		case 'p':
+			ptr = &input_buffer[1];
+
+			if (!hexToInt(&ptr, &addr)) {
+				gdbstub_strcpy(output_buffer, "E01");
+				break;
+			}
+
+			temp2 = 1;
+			switch (addr) {
+			case GDB_REG_GR(0):
+				temp = 0;
+				break;
+			case GDB_REG_GR(1) ... GDB_REG_GR(63):
+				temp = __debug_user_context->i.gr[addr - GDB_REG_GR(0)];
+				break;
+			case GDB_REG_FR(0) ... GDB_REG_FR(63):
+				temp = __debug_user_context->f.fr[addr - GDB_REG_FR(0)];
+				break;
+			case GDB_REG_PC:
+				temp = __debug_user_context->i.pc;
+				break;
+			case GDB_REG_PSR:
+				temp = __debug_user_context->i.psr;
+				break;
+			case GDB_REG_CCR:
+				temp = __debug_user_context->i.ccr;
+				break;
+			case GDB_REG_CCCR:
+				temp = __debug_user_context->i.cccr;
+				break;
+			case GDB_REG_BRR:
+				temp = __debug_status.brr;
+				break;
+			case GDB_REG_LR:
+				temp = __debug_user_context->i.lr;
+				break;
+			case GDB_REG_LCR:
+				temp = __debug_user_context->i.lcr;
+				break;
+			case GDB_REG_TBR:
+				temp = __debug_user_context->i.tbr;
+				break;
+			case GDB_REG_FSR0:
+				temp = __debug_user_context->f.fsr[0];
+				break;
+			case GDB_REG_ACC(0) ... GDB_REG_ACC(7):
+				temp = __debug_user_context->f.acc[addr - GDB_REG_ACC(0)];
+				break;
+			case GDB_REG_ACCG(0):
+				temp = *(uint32_t *) &__debug_user_context->f.accg[0];
+				break;
+			case GDB_REG_ACCG(4):
+				temp = *(uint32_t *) &__debug_user_context->f.accg[4];
+				break;
+			case GDB_REG_IACC0H:
+				temp = ((u32*)&__debug_frame->iacc0)[0];
+				break;
+			case GDB_REG_IACC0L:
+				temp = ((u32*)&__debug_frame->iacc0)[1];
+				break;
+			case GDB_REG_MSR(0) ... GDB_REG_MSR(1):
+				temp = __debug_user_context->f.msr[addr - GDB_REG_MSR(0)];
+				break;
+			case GDB_REG_GNER(0) ... GDB_REG_GNER(1):
+				temp = __debug_user_context->i.gner[addr - GDB_REG_GNER(0)];
+				break;
+			case GDB_REG_FNER(0) ... GDB_REG_FNER(1):
+				temp = __debug_user_context->f.fner[addr - GDB_REG_FNER(0)];
+				break;
+			case GDB_REG_DBAR(0):
+				asm volatile("movsg dbar0,%0" : "=r"(temp));
+				break;
+			case GDB_REG_DBAR(1):
+				asm volatile("movsg dbar1,%0" : "=r"(temp));
+				break;
+			case GDB_REG_DBAR(2):
+				asm volatile("movsg dbar2,%0" : "=r"(temp));
+				break;
+			case GDB_REG_DBAR(3):
+				asm volatile("movsg dbar3,%0" : "=r"(temp));
+				break;
+			default:
+				temp = 0;
+				temp2 = 0;
+				break;
+			}
+
+			if (temp2) {
+				ptr = output_buffer;
+				ptr = mem2hex(&temp, ptr, 4, 0);
+			}
+			else {
+				gdbstub_strcpy(output_buffer, "E02");
+			}
+			break;
+
 			/* PNN,=RRRRRRRR: Write value R to reg N return OK */
 		case 'P':
 			ptr = &input_buffer[1];
@@ -1844,6 +1942,12 @@ void gdbstub(int sigval)
 			case GDB_REG_ACCG(4):
 				*(uint32_t *) &__debug_user_context->f.accg[4] = temp;
 				break;
+			case GDB_REG_IACC0H:
+				((u32*)&__debug_frame->iacc0)[0] = temp;
+				break;
+			case GDB_REG_IACC0L:
+				((u32*)&__debug_frame->iacc0)[1] = temp;
+				break;
 			case GDB_REG_MSR(0) ... GDB_REG_MSR(1):
 				__debug_user_context->f.msr[addr - GDB_REG_MSR(0)] = temp;
 				break;
@@ -1853,6 +1957,18 @@ void gdbstub(int sigval)
 			case GDB_REG_FNER(0) ... GDB_REG_FNER(1):
 				__debug_user_context->f.fner[addr - GDB_REG_FNER(0)] = temp;
 				break;
+			case GDB_REG_DBAR(0):
+				asm volatile("movgs %0,dbar0" :: "r"(temp));
+				break;
+			case GDB_REG_DBAR(1):
+				asm volatile("movgs %0,dbar1" :: "r"(temp));
+				break;
+			case GDB_REG_DBAR(2):
+				asm volatile("movgs %0,dbar2" :: "r"(temp));
+				break;
+			case GDB_REG_DBAR(3):
+				asm volatile("movgs %0,dbar3" :: "r"(temp));
+				break;
 			default:
 				temp2 = 0;
 				break;

Re: [PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[Linus Torvalds]

On Wed, 9 Jun 2010, David Howells wrote:
>
> Implement the GDB remote protocol 'p' command to retrieve a single register
> value.

Is there any value to this? Apparently it used to work just because that 
command never did anything (rather than return an error). Why not just 
reinstate that behavior instead?

There's doesn't seem to be any reason to implement the single-register 
case unless there is some legacy use of it. There's a good reason gdb 
tends to use the "register block" thing, and it tends to be easier to 
implement too (no crazy case statements etc).

			Linus

Re: [PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[Frank Ch. Eigler]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>> Implement the GDB remote protocol 'p' command to retrieve a single register
>> value.
>
> Is there any value to this? [...]

Sure, much less data to transfer across a slowish serial link.

- FChE

Re: [PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[Linus Torvalds]

On Wed, 9 Jun 2010, Frank Ch. Eigler wrote:
> 
> Sure, much less data to transfer across a slowish serial link.

Umm. Or more? If you look at one register, you tend to look at several. 
And we're not exactly talking kilobytes here.

Anyway, the existing patch is certainly easy to make a judgment on: it's 
not a regression fix but a new feature, so I'm not applying it. I was 
suggesting _making_ it a regression fix, but I really don't care.

		Linus

Re: [PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[Jason Wessel]

On 06/09/2010 01:23 PM, Frank Ch. Eigler wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>   
>>> Implement the GDB remote protocol 'p' command to retrieve a single register
>>> value.
>>>       
>> Is there any value to this? [...]
>>     
>
> Sure, much less data to transfer across a slowish serial link.
>
>   

I have actually measured this before using a yet to be posted for review
version which works for x86, arm and mips.   The improvement is more
significant with the 64 bit archs vs the 32 because of the register
sizing.  The typical case of stepping is quite a bit more responsive
when you only need to read the pc, stack pointer and frame pointer.  At
the point in time I post the patches, I can also post the data about the
savings for two of the exact same work load sequences.

I had also tried out an implementation of the gdb extension which where
you can pass the pc, stack pointer and frame pointer along with the stop
code and this provides an additional speed up as well, but not nearly as
much of a gain as implementing the 'p' packets.

I don't know if I will have it ready for 2.6.36 or not, but the generic
use of the 'p' packet was also required so that the kdb shell could have
a mechanism to individually read and write registers.

I am a bit curious if the frv arch had considered migrating to the kgdb
core, vs continuing to use the separate gdbstub?   It would seem we are
duplicating the creation of the same sort of features.  The frv
architecture may not need it at all, but if you do make use of the kgdb
core, you get other features like kdb for free, plus any of the
interesting features you might want to add might be of interest to other
consumers of the kernel debugger.

Cheers,
Jason.

Re: [PATCH] FRV: Implement the GDB remote protocol 'p' command to retrieve a register

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Implement the GDB remote protocol 'p' command to retrieve a single register
> > value.
> 
> Is there any value to this? Apparently it used to work just because that 
> command never did anything (rather than return an error). Why not just 
> reinstate that behavior instead?

That's a reasonable argument.  The 'p' command would be more efficient if gdb
uses it when required to get single values, but once it has decided to use the
'g' command, it seems to stick with it.

> There's doesn't seem to be any reason to implement the single-register 
> case unless there is some legacy use of it. There's a good reason gdb 
> tends to use the "register block" thing, and it tends to be easier to 
> implement too (no crazy case statements etc).

It's slower to use 'g', however, if we only want the value in one register,
and I'm usually doing gdb protocol over serial.

However, as stated above, once it's decided to use 'g', gdb doesn't seem to
want to use 'p' any more.

David