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

[Daniel Thompson]

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.