Messages in this thread | | | Subject | Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol | From | Daniel Thompson <> | Date | Thu, 12 May 2016 14:08:29 +0100 |
| |
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.
| |