Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol

[Daniel Thompson <daniel.thompson@linaro.org>]

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.