linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
@ 2016-05-09 17:39 Daniel Thompson
  2016-05-10 10:28 ` Will Deacon
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Daniel Thompson @ 2016-05-09 17:39 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Daniel Thompson, linux-arm-kernel, linux-kernel, patches,
	linaro-kernel, Jason Wessel, Dave P Martin, Mark Rutland,
	Vijaya Kumar K

Current versions of gdb do not interoperate cleanly with kgdb on arm64
systems because gdb and kgdb do not use the same register description.
This patch modifies kgdb to work with recent releases of gdb (>= 7.8.1).

Compatibility with gdb (after the patch is applied) is as follows:

  gdb-7.6 and earlier  Ok
  gdb-7.7 series       Works if user provides custom target description
  gdb-7.8(.0)          Works if user provides custom target description
  gdb-7.8.1 and later  Ok

When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
introduced it was paired with a gdb patch that made an incompatible
change to the gdbserver protocol. This patch was eventually merged into
the gdb sources:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd

The change to the protocol was mostly made to simplify big-endian support
inside the kernel gdb stub. Unfortunately the gdb project released
gdb-7.7.x and gdb-7.8.0 before the protocol incompatibility was identified
and reversed:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bdc144174bcb11e808b4e73089b850cf9620a7ee

This leaves us in a position where kgdb still uses the no-longer-used
protocol; gdb-7.8.1, which restored the original behaviour,If was
released on 2014-10-29.

I don't believe it is possible to detect/correct the protocol
incompatiblity which means the kernel must take a view about which
version of the gdb remote protocol is "correct". This patch takes the
view that the original/current version of the protocol is correct
and that version found in gdb-7.7.x and gdb-7.8.0 is anomalous.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---

Notes:
    Vijaya: I really hope I have the history lesson in the commit
            message correct! I've done a fair bit of archeology to
    	unpick things.

 arch/arm64/include/asm/kgdb.h | 13 +++++--------
 arch/arm64/kernel/kgdb.c      |  7 ++++++-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
index f69f69c8120c..326fa8f44da5 100644
--- a/arch/arm64/include/asm/kgdb.h
+++ b/arch/arm64/include/asm/kgdb.h
@@ -43,20 +43,17 @@ extern int kgdb_fault_expected;
  * General purpose regs:
  *     r0-r30: 64 bit
  *     sp,pc : 64 bit
- *     pstate  : 64 bit
- *     Total: 34
+ *     pstate  : 32 bit
+ *     Total: 33 + 1
  * FPU regs:
  *     f0-f31: 128 bit
- *     Total: 32
- * Extra regs
  *     fpsr & fpcr: 32 bit
- *     Total: 2
- *
+ *     Total: 32 + 2
  */

-#define _GP_REGS		34
+#define _GP_REGS		33
 #define _FP_REGS		32
-#define _EXTRA_REGS		2
+#define _EXTRA_REGS		3
 /*
  * general purpose registers size in bytes.
  * pstate is only 4 bytes. subtract 4 bytes
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index b67531a13136..ec62a4e3c190 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -58,7 +58,11 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
 	{ "x30", 8, offsetof(struct pt_regs, regs[30])},
 	{ "sp", 8, offsetof(struct pt_regs, sp)},
 	{ "pc", 8, offsetof(struct pt_regs, pc)},
-	{ "pstate", 8, offsetof(struct pt_regs, pstate)},
+	{ "pstate", 4, offsetof(struct pt_regs, pstate)
+#ifdef CONFIG_CPU_BIG_ENDIAN
+							+ 4
+#endif
+	},
 	{ "v0", 16, -1 },
 	{ "v1", 16, -1 },
 	{ "v2", 16, -1 },
@@ -128,6 +132,7 @@ sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
 	memset((char *)gdb_regs, 0, NUMREGBYTES);
 	thread_regs = task_pt_regs(task);
 	memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
+	dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
 }

 void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
--
2.5.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-09 17:39 [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol Daniel Thompson
@ 2016-05-10 10:28 ` Will Deacon
  2016-05-10 14:05   ` Daniel Thompson
  2016-05-10 10:45 ` Mark Rutland
  2016-06-16 15:51 ` [PATCH 4.7-rc3 v2] " Daniel Thompson
  2 siblings, 1 reply; 8+ messages in thread
From: Will Deacon @ 2016-05-10 10:28 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Catalin Marinas, linux-arm-kernel, linux-kernel, patches,
	linaro-kernel, Jason Wessel, Dave P Martin, Mark Rutland,
	Vijaya Kumar K

Hi Daniel,

So does kgdb do something useful after this patch?

On Mon, May 09, 2016 at 06:39:26PM +0100, Daniel Thompson wrote:
> Current versions of gdb do not interoperate cleanly with kgdb on arm64
> systems because gdb and kgdb do not use the same register description.
> This patch modifies kgdb to work with recent releases of gdb (>= 7.8.1).
> 
> Compatibility with gdb (after the patch is applied) is as follows:
> 
>   gdb-7.6 and earlier  Ok
>   gdb-7.7 series       Works if user provides custom target description
>   gdb-7.8(.0)          Works if user provides custom target description
>   gdb-7.8.1 and later  Ok
> 
> When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
> introduced it was paired with a gdb patch that made an incompatible
> change to the gdbserver protocol. This patch was eventually merged into
> the gdb sources:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
> 
> The change to the protocol was mostly made to simplify big-endian support
> inside the kernel gdb stub. Unfortunately the gdb project released
> gdb-7.7.x and gdb-7.8.0 before the protocol incompatibility was identified
> and reversed:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bdc144174bcb11e808b4e73089b850cf9620a7ee
> 
> This leaves us in a position where kgdb still uses the no-longer-used
> protocol; gdb-7.8.1, which restored the original behaviour,If was
> released on 2014-10-29.
> 
> I don't believe it is possible to detect/correct the protocol
> incompatiblity which means the kernel must take a view about which
> version of the gdb remote protocol is "correct". This patch takes the
> view that the original/current version of the protocol is correct
> and that version found in gdb-7.7.x and gdb-7.8.0 is anomalous.

Urgh, this is filthy! Still, without a time machine, I guess there's
little we can do about it. Can I ask you to respin the patch but with
the rationale as a comment in the header file, and a pointer to the
comment from the C code too, please?

The code looks incorrect after this change, so we should justify how
we've ended up in this state and not everybody looks at the git log
for that rationale.

Cheers,

Will

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-09 17:39 [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol Daniel Thompson
  2016-05-10 10:28 ` Will Deacon
@ 2016-05-10 10:45 ` Mark Rutland
  2016-05-10 13:41   ` Daniel Thompson
  2016-06-16 15:51 ` [PATCH 4.7-rc3 v2] " Daniel Thompson
  2 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2016-05-10 10:45 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel,
	patches, linaro-kernel, Jason Wessel, Dave P Martin,
	Vijaya Kumar K

On Mon, May 09, 2016 at 06:39:26PM +0100, Daniel Thompson wrote:
> Current versions of gdb do not interoperate cleanly with kgdb on arm64
> systems because gdb and kgdb do not use the same register description.
> This patch modifies kgdb to work with recent releases of gdb (>= 7.8.1).
> 
> Compatibility with gdb (after the patch is applied) is as follows:
> 
>   gdb-7.6 and earlier  Ok
>   gdb-7.7 series       Works if user provides custom target description
>   gdb-7.8(.0)          Works if user provides custom target description
>   gdb-7.8.1 and later  Ok
> 
> When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
> introduced it was paired with a gdb patch that made an incompatible
> change to the gdbserver protocol. This patch was eventually merged into
> the gdb sources:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
> 
> The change to the protocol was mostly made to simplify big-endian support
> inside the kernel gdb stub.

While that was how we discovered the inconsistency, a major concern is
that SPSR_EL* (i.e. PSTATE), as accessed by MRS/MSR is a 64-bit
quantity, even if the upper 32 bits are RES0 today.

It is conceivable that the upper 32 bits could be used in future (as
happened with CLIDR_EL1), and for this reason we expose those upper 32
bits from the kernel, and treat system registers as 64-bit quantities
generally.

So this was also about ensuring the interface was consistent and to some
extent future-proof.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-10 10:45 ` Mark Rutland
@ 2016-05-10 13:41   ` Daniel Thompson
  2016-05-10 14:31     ` Mark Rutland
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Thompson @ 2016-05-10 13:41 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel,
	patches, linaro-kernel, Jason Wessel, Dave P Martin,
	Vijaya Kumar K

On 10/05/16 11:45, Mark Rutland wrote:
>> When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
>> introduced it was paired with a gdb patch that made an incompatible
>> change to the gdbserver protocol. This patch was eventually merged into
>> the gdb sources:
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
>>
>> The change to the protocol was mostly made to simplify big-endian support
>> inside the kernel gdb stub.
>
> While that was how we discovered the inconsistency, a major concern is
> that SPSR_EL* (i.e. PSTATE), as accessed by MRS/MSR is a 64-bit
> quantity, even if the upper 32 bits are RES0 today.
>
> It is conceivable that the upper 32 bits could be used in future (as
> happened with CLIDR_EL1), and for this reason we expose those upper 32
> bits from the kernel, and treat system registers as 64-bit quantities
> generally.

These cases are not exactly the same.

CLIDR_ELx is (or was) architecturally defined as a 64-bit register and 
explicitly marks the upper 32-bits as RES0.

That is not the case for SPSR_ELx; this register is architecturally 
defined to be 32-bit.

I know that doesn't *prevent* SPSR_ELx from being expanded in the future 
it is not unreasonable for gdb to design its wire protocol based on the 
description found in the architecture manual.


> So this was also about ensuring the interface was consistent and to some
> extent future-proof.

gdb remote protocol is already future proof and has never at any point 
contradicted the architecture.

However the changes to the protocol in 7.7.x and 7.8.0 were analogous to 
an unexpected ABI change rather than a carefully controlled introduction 
of a new feature. Like the kernel, once detected, they were reversed ;-) .


Daniel.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-10 10:28 ` Will Deacon
@ 2016-05-10 14:05   ` Daniel Thompson
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Thompson @ 2016-05-10 14:05 UTC (permalink / raw)
  To: Will Deacon
  Cc: Catalin Marinas, linux-arm-kernel, linux-kernel, patches,
	linaro-kernel, Jason Wessel, Dave P Martin, Mark Rutland,
	Vijaya Kumar K

On 10/05/16 11:28, Will Deacon wrote:
> Hi Daniel,
>
> So does kgdb do something useful after this patch?

On this occasion I was debugging it on behalf of another developer 
rather than trying to use it myself to debug something so I didn't do 
much additional testing beyond sanity tested the contents of the 
register set.

Naturally it will be *totally* awesome if I get the pseudo-NMI stuff 
working perfectly. ;-)


> On Mon, May 09, 2016 at 06:39:26PM +0100, Daniel Thompson wrote:
>> Current versions of gdb do not interoperate cleanly with kgdb on arm64
>> systems because gdb and kgdb do not use the same register description.
>> This patch modifies kgdb to work with recent releases of gdb (>= 7.8.1).
>>
>> Compatibility with gdb (after the patch is applied) is as follows:
>>
>>   gdb-7.6 and earlier  Ok
>>   gdb-7.7 series       Works if user provides custom target description
>>   gdb-7.8(.0)          Works if user provides custom target description
>>   gdb-7.8.1 and later  Ok
>>
>> When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
>> introduced it was paired with a gdb patch that made an incompatible
>> change to the gdbserver protocol. This patch was eventually merged into
>> the gdb sources:
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
>>
>> The change to the protocol was mostly made to simplify big-endian support
>> inside the kernel gdb stub. Unfortunately the gdb project released
>> gdb-7.7.x and gdb-7.8.0 before the protocol incompatibility was identified
>> and reversed:
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bdc144174bcb11e808b4e73089b850cf9620a7ee
>>
>> This leaves us in a position where kgdb still uses the no-longer-used
>> protocol; gdb-7.8.1, which restored the original behaviour,If was
>> released on 2014-10-29.
>>
>> I don't believe it is possible to detect/correct the protocol
>> incompatiblity which means the kernel must take a view about which
>> version of the gdb remote protocol is "correct". This patch takes the
>> view that the original/current version of the protocol is correct
>> and that version found in gdb-7.7.x and gdb-7.8.0 is anomalous.
>
> Urgh, this is filthy! Still, without a time machine, I guess there's
> little we can do about it. Can I ask you to respin the patch but with
> the rationale as a comment in the header file, and a pointer to the
> comment from the C code too, please?
>
> The code looks incorrect after this change, so we should justify how
> we've ended up in this state and not everybody looks at the git log
> for that rationale.

Will do.


Daniel.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-10 13:41   ` Daniel Thompson
@ 2016-05-10 14:31     ` Mark Rutland
  2016-05-12 13:08       ` Daniel Thompson
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2016-05-10 14:31 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel,
	patches, linaro-kernel, Jason Wessel, Dave P Martin,
	Vijaya Kumar K

On Tue, May 10, 2016 at 02:41:54PM +0100, Daniel Thompson wrote:
> On 10/05/16 11:45, Mark Rutland wrote:
> >>When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
> >>introduced it was paired with a gdb patch that made an incompatible
> >>change to the gdbserver protocol. This patch was eventually merged into
> >>the gdb sources:
> >>https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
> >>
> >>The change to the protocol was mostly made to simplify big-endian support
> >>inside the kernel gdb stub.
> >
> >While that was how we discovered the inconsistency, a major concern is
> >that SPSR_EL* (i.e. PSTATE), as accessed by MRS/MSR is a 64-bit
> >quantity, even if the upper 32 bits are RES0 today.
> >
> >It is conceivable that the upper 32 bits could be used in future (as
> >happened with CLIDR_EL1), and for this reason we expose those upper 32
> >bits from the kernel, and treat system registers as 64-bit quantities
> >generally.
> 
> These cases are not exactly the same.
> 
> CLIDR_ELx is (or was) architecturally defined as a 64-bit register
> and explicitly marks the upper 32-bits as RES0.
> 
> That is not the case for SPSR_ELx; this register is architecturally
> defined to be 32-bit.

The below doesn't necessarily change your subsequent argument, but that
isn't quite true.

The two cases are in fact identical if you dig into the history a bit
further. Take a look in an earlier revision of the ARM ARM (e.g. ARM DDI
0487A.b), where it was explicitly stated:

	Attributes
		CLIDR_EL1 is a 32-bit register.

This was subsequently upgraded to 64-bit with the addition of ICB.

Based on this, my PoV is that any register that the ARM ARM describes as
"a 32-bit register" is a 64-bit register for which the upper 32 bits are
RES0.

> I know that doesn't *prevent* SPSR_ELx from being expanded in the
> future it is not unreasonable for gdb to design its wire protocol
> based on the description found in the architecture manual.
> 
> >So this was also about ensuring the interface was consistent and to some
> >extent future-proof.
> 
> gdb remote protocol is already future proof and has never at any
> point contradicted the architecture.
> 
> However the changes to the protocol in 7.7.x and 7.8.0 were
> analogous to an unexpected ABI change rather than a carefully
> controlled introduction of a new feature. Like the kernel, once
> detected, they were reversed ;-) .

To be clear, I don't disagree with this. The ARM ARM is at best
amigibious w.r.t. what it means by "a 32-bit register", and that's the
only point of contention.

However, we should make note of the above as a key point of rationale,
as it affects other decisions we make in this area.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-10 14:31     ` Mark Rutland
@ 2016-05-12 13:08       ` Daniel Thompson
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Thompson @ 2016-05-12 13:08 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel,
	patches, linaro-kernel, Jason Wessel, Dave P Martin,
	Vijaya Kumar K

On 10/05/16 15:31, Mark Rutland wrote:
> On Tue, May 10, 2016 at 02:41:54PM +0100, Daniel Thompson wrote:
>> On 10/05/16 11:45, Mark Rutland wrote:
>>>> When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
>>>> introduced it was paired with a gdb patch that made an incompatible
>>>> change to the gdbserver protocol. This patch was eventually merged into
>>>> the gdb sources:
>>>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
>>>>
>>>> The change to the protocol was mostly made to simplify big-endian support
>>>> inside the kernel gdb stub.
>>>
>>> While that was how we discovered the inconsistency, a major concern is
>>> that SPSR_EL* (i.e. PSTATE), as accessed by MRS/MSR is a 64-bit
>>> quantity, even if the upper 32 bits are RES0 today.
>>>
>>> It is conceivable that the upper 32 bits could be used in future (as
>>> happened with CLIDR_EL1), and for this reason we expose those upper 32
>>> bits from the kernel, and treat system registers as 64-bit quantities
>>> generally.
>>
>> These cases are not exactly the same.
>>
>> CLIDR_ELx is (or was) architecturally defined as a 64-bit register
>> and explicitly marks the upper 32-bits as RES0.
>>
>> That is not the case for SPSR_ELx; this register is architecturally
>> defined to be 32-bit.
>
> The below doesn't necessarily change your subsequent argument, but that
> isn't quite true.
>
> The two cases are in fact identical if you dig into the history a bit
> further. Take a look in an earlier revision of the ARM ARM (e.g. ARM DDI
> 0487A.b), where it was explicitly stated:
>
> 	Attributes
> 		CLIDR_EL1 is a 32-bit register.

I see. I only when back as far as A.e ...


> Based on this, my PoV is that any register that the ARM ARM describes as
> "a 32-bit register" is a 64-bit register for which the upper 32 bits are
> RES0.

Understood.

>> I know that doesn't *prevent* SPSR_ELx from being expanded in the
>> future it is not unreasonable for gdb to design its wire protocol
>> based on the description found in the architecture manual.
>>
>>> So this was also about ensuring the interface was consistent and to some
>>> extent future-proof.
>>
>> gdb remote protocol is already future proof and has never at any
>> point contradicted the architecture.
>>
>> However the changes to the protocol in 7.7.x and 7.8.0 were
>> analogous to an unexpected ABI change rather than a carefully
>> controlled introduction of a new feature. Like the kernel, once
>> detected, they were reversed ;-) .
>
> To be clear, I don't disagree with this. The ARM ARM is at best
> amigibious w.r.t. what it means by "a 32-bit register", and that's the
> only point of contention.

Agree... such ambiguity strikes me as a particularly serious problem for 
switchable context registers since it takes *very* close attention to 
the ARM ARM for an existing OS to determine that it is required to treat 
the upper 32-bits of context should-be-zero-or-preserved.


> However, we should make note of the above as a key point of rationale,
> as it affects other decisions we make in this area.

Ok. I'll add comments describing how the gdbremote protocol is 
interpreting the spec.


Daniel.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4.7-rc3 v2] arm64: kgdb: Match pstate size with gdbserver protocol
  2016-05-09 17:39 [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol Daniel Thompson
  2016-05-10 10:28 ` Will Deacon
  2016-05-10 10:45 ` Mark Rutland
@ 2016-06-16 15:51 ` Daniel Thompson
  2 siblings, 0 replies; 8+ messages in thread
From: Daniel Thompson @ 2016-06-16 15:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Daniel Thompson, linux-arm-kernel, linux-kernel, patches,
	linaro-kernel, Jason Wessel, Dave P Martin, Mark Rutland,
	Vijaya Kumar K

Current versions of gdb do not interoperate cleanly with kgdb on arm64
systems because gdb and kgdb do not use the same register description.
This patch modifies kgdb to work with recent releases of gdb (>= 7.8.1).

Compatibility with gdb (after the patch is applied) is as follows:

  gdb-7.6 and earlier  Ok
  gdb-7.7 series       Works if user provides custom target description
  gdb-7.8(.0)          Works if user provides custom target description
  gdb-7.8.1 and later  Ok

When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
introduced it was paired with a gdb patch that made an incompatible
change to the gdbserver protocol. This patch was eventually merged into
the gdb sources:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd

The change to the protocol was mostly made to simplify big-endian support
inside the kernel gdb stub. Unfortunately the gdb project released
gdb-7.7.x and gdb-7.8.0 before the protocol incompatibility was identified
and reversed:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bdc144174bcb11e808b4e73089b850cf9620a7ee

This leaves us in a position where kgdb still uses the no-longer-used
protocol; gdb-7.8.1, which restored the original behaviour, was
released on 2014-10-29.

I don't believe it is possible to detect/correct the protocol
incompatiblity which means the kernel must take a view about which
version of the gdb remote protocol is "correct". This patch takes the
view that the original/current version of the protocol is correct
and that version found in gdb-7.7.x and gdb-7.8.0 is anomalous.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---

Notes:
    v2:
     * Lots of new comments to sit in the code (rather than the git history)
       explaining exactly why there is such confusion about how wide the
       SPSR_ELx registers actually are (Will Deacon and Mark Rutland).
    
    Vijaya: I really hope I have the history lesson in the commit message
            correct! I've did a fair bit of archaeology to unpick things
    	and I pretty sure I haven't missed anything important.

 arch/arm64/include/asm/kgdb.h | 45 +++++++++++++++++++++++++++++++++++--------
 arch/arm64/kernel/kgdb.c      | 14 +++++++++++++-
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
index f69f69c8120c..da84645525b9 100644
--- a/arch/arm64/include/asm/kgdb.h
+++ b/arch/arm64/include/asm/kgdb.h
@@ -38,25 +38,54 @@ extern int kgdb_fault_expected;
 #endif /* !__ASSEMBLY__ */

 /*
- * gdb is expecting the following registers layout.
+ * gdb remote procotol (well most versions of it) expects the following
+ * register layout.
  *
  * General purpose regs:
  *     r0-r30: 64 bit
  *     sp,pc : 64 bit
- *     pstate  : 64 bit
- *     Total: 34
+ *     pstate  : 32 bit
+ *     Total: 33 + 1
  * FPU regs:
  *     f0-f31: 128 bit
- *     Total: 32
- * Extra regs
  *     fpsr & fpcr: 32 bit
- *     Total: 2
+ *     Total: 32 + 2
  *
+ * To expand a little on the "most versions of it"... when the gdb remote
+ * protocol for AArch64 was developed it depended on a statement in the
+ * Architecture Reference Manual that claimed "SPSR_ELx is a 32-bit register".
+ * and, as a result, allocated only 32-bits for the PSTATE in the remote
+ * protocol. In fact this statement is still present in ARM DDI 0487A.i.
+ *
+ * Unfortunately "is a 32-bit register" has a very special meaning for
+ * system registers. It means that "the upper bits, bits[63:32], are
+ * RES0.". RES0 is heavily used in the ARM architecture documents as a
+ * way to leave space for future architecture changes. So to translate a
+ * little for people who don't spend their spare time reading ARM architecture
+ * manuals, what "is a 32-bit register" actually means in this context is
+ * "is a 64-bit register but one with no meaning allocated to any of the
+ * upper 32-bits... *yet*".
+ *
+ * Perhaps then we should not be surprised that this has led to some
+ * confusion. Specifically a patch, influenced by the above translation,
+ * that extended PSTATE to 64-bit was accepted into gdb-7.7 but the patch
+ * was reverted in gdb-7.8.1 and all later releases, when this was
+ * discovered to be an undocumented protocol change.
+ *
+ * So... it is *not* wrong for us to only allocate 32-bits to PSTATE
+ * here even though the kernel itself allocates 64-bits for the same
+ * state. That is because this bit of code tells the kernel how the gdb
+ * remote protocol (well most versions of it) describes the register state.
+ *
+ * Note that if you are using one of the versions of gdb that supports
+ * the gdb-7.7 version of the protocol you cannot use kgdb directly
+ * without providing a custom register description (gdb can load new
+ * protocol descriptions at runtime).
  */

-#define _GP_REGS		34
+#define _GP_REGS		33
 #define _FP_REGS		32
-#define _EXTRA_REGS		2
+#define _EXTRA_REGS		3
 /*
  * general purpose registers size in bytes.
  * pstate is only 4 bytes. subtract 4 bytes
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index b67531a13136..b5f063e5eff7 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -58,7 +58,17 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
 	{ "x30", 8, offsetof(struct pt_regs, regs[30])},
 	{ "sp", 8, offsetof(struct pt_regs, sp)},
 	{ "pc", 8, offsetof(struct pt_regs, pc)},
-	{ "pstate", 8, offsetof(struct pt_regs, pstate)},
+	/*
+	 * struct pt_regs thinks PSTATE is 64-bits wide but gdb remote
+	 * protocol disagrees. Therefore we must extract only the lower
+	 * 32-bits. Look for the big comment in asm/kgdb.h for more
+	 * detail.
+	 */
+	{ "pstate", 4, offsetof(struct pt_regs, pstate)
+#ifdef CONFIG_CPU_BIG_ENDIAN
+							+ 4
+#endif
+	},
 	{ "v0", 16, -1 },
 	{ "v1", 16, -1 },
 	{ "v2", 16, -1 },
@@ -128,6 +138,8 @@ sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
 	memset((char *)gdb_regs, 0, NUMREGBYTES);
 	thread_regs = task_pt_regs(task);
 	memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
+	/* Special case for PSTATE (check comments in asm/kgdb.h for details) */
+	dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
 }

 void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
--
2.5.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-16 15:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-09 17:39 [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol Daniel Thompson
2016-05-10 10:28 ` Will Deacon
2016-05-10 14:05   ` Daniel Thompson
2016-05-10 10:45 ` Mark Rutland
2016-05-10 13:41   ` Daniel Thompson
2016-05-10 14:31     ` Mark Rutland
2016-05-12 13:08       ` Daniel Thompson
2016-06-16 15:51 ` [PATCH 4.7-rc3 v2] " Daniel Thompson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).