[PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate

[PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate

[Yao Qi]

This patch is to change 32-bit cpsr to 64-bit pstate in aarch64 target
description.

When aarch64 GDB port was added into GDB, cpsr is 32-bit in
aarch64-core.xml.  Andrew Pinski changed it to 64-bit
https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html but forget
updating GDBserver side.  As a result, GDB thinks cpsr is 64-bit, but
GDBserver still thinks it is 32-bit, so contents of registers after
cpster, such as floating point registers, are wrong.  Catalin Udma
gave a patch https://sourceware.org/ml/gdb-patches/2014-08/msg00193.html
to change cpsr in GDBserver side to 64-bit.  After some discussions,
we decided to revert Andrew's patch, and let cpsr be 32-bit.

A problem "Remote 'g' packet reply is too long:" (when I connect gdb
to kgdb with aarch64 kernel) leads me to the 32-bit vs 64-bit issue
again.

cpsr is 32-bit, but it only exists aarch32 execution state.  As in
ARMv8 reference manual, "A1.3.1 Execution state"

"The A32 and T32
instruction sets include instructions that operate directly on various PSTATE elements, and
instructions that access PSTATE by using the Application Program Status Register (APSR)
or the Current Program Status Register (CPSR)".

In other words, there is no cpsr at all on aarch64 execution state.
On aarch64 execution state, pstate is an abstraction of process state
information, so it has no architecturally defined size or enconding.

However, the architecture does define how the PSTATE is encoded in the
SPSR when taking an exception, and that is the form of PSTATE in
ptrace.  Currently only bits 31:0 of SPSR are defined, but it is only
accessible via MSR/MRS instructions, which always transfer 64-bits,
so on ISA level, it should be regarded as 64-bit.

This change will break the compatibility for <new gdb, old gdbserver>
and <old gdb, new gdbserver>, but current GDB is wrong, we have to fix it.

gdb:

2015-09-03  Yao Qi  <yao.qi@linaro.org>

	* aarch64-tdep.c (aarch64_r_register_names): Rename cpsr to
	pstate.
	* features/aarch64-core.xmli (cpsr): Rename it to pstate, and
	change its size to 64-bit.
	* features/aarch64.c: Regenerate.
	* regformats/aarch64.dat: Likewise.
---
 gdb/aarch64-tdep.c            | 2 +-
 gdb/features/aarch64-core.xml | 2 +-
 gdb/features/aarch64.c        | 2 +-
 gdb/regformats/aarch64.dat    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5d8ffd4..04064cb 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -127,7 +127,7 @@ static const char *const aarch64_r_register_names[] =
   "x20", "x21", "x22", "x23",
   "x24", "x25", "x26", "x27",
   "x28", "x29", "x30", "sp",
-  "pc", "cpsr"
+  "pc", "pstate"
 };
 
 /* The FP/SIMD 'V' registers.  */
diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml
index c95049c..f0f455f 100644
--- a/gdb/features/aarch64-core.xml
+++ b/gdb/features/aarch64-core.xml
@@ -42,5 +42,5 @@
   <reg name="sp" bitsize="64" type="data_ptr"/>
 
   <reg name="pc" bitsize="64" type="code_ptr"/>
-  <reg name="cpsr" bitsize="32"/>
+  <reg name="pstate" bitsize="64"/>
 </feature>
diff --git a/gdb/features/aarch64.c b/gdb/features/aarch64.c
index 1e9a99d..643387d 100644
--- a/gdb/features/aarch64.c
+++ b/gdb/features/aarch64.c
@@ -50,7 +50,7 @@ initialize_tdesc_aarch64 (void)
   tdesc_create_reg (feature, "x30", 30, 1, NULL, 64, "int");
   tdesc_create_reg (feature, "sp", 31, 1, NULL, 64, "data_ptr");
   tdesc_create_reg (feature, "pc", 32, 1, NULL, 64, "code_ptr");
-  tdesc_create_reg (feature, "cpsr", 33, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pstate", 33, 1, NULL, 64, "int");
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.fpu");
   field_type = tdesc_named_type (feature, "ieee_double");
diff --git a/gdb/regformats/aarch64.dat b/gdb/regformats/aarch64.dat
index d4cea04..0db2f3a 100644
--- a/gdb/regformats/aarch64.dat
+++ b/gdb/regformats/aarch64.dat
@@ -36,7 +36,7 @@ expedite:x29,sp,pc
 64:x30
 64:sp
 64:pc
-32:cpsr
+64:pstate
 128:v0
 128:v1
 128:v2
-- 
1.9.1

Re: [PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate

[Richard Earnshaw]

On 03/09/15 13:56, Yao Qi wrote:
> This patch is to change 32-bit cpsr to 64-bit pstate in aarch64 target
> description.
> 
> When aarch64 GDB port was added into GDB, cpsr is 32-bit in
> aarch64-core.xml.  Andrew Pinski changed it to 64-bit
> https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html but forget
> updating GDBserver side.  As a result, GDB thinks cpsr is 64-bit, but
> GDBserver still thinks it is 32-bit, so contents of registers after
> cpster, such as floating point registers, are wrong.  Catalin Udma
> gave a patch https://sourceware.org/ml/gdb-patches/2014-08/msg00193.html
> to change cpsr in GDBserver side to 64-bit.  After some discussions,
> we decided to revert Andrew's patch, and let cpsr be 32-bit.
> 
> A problem "Remote 'g' packet reply is too long:" (when I connect gdb
> to kgdb with aarch64 kernel) leads me to the 32-bit vs 64-bit issue
> again.
> 
> cpsr is 32-bit, but it only exists aarch32 execution state.  As in
> ARMv8 reference manual, "A1.3.1 Execution state"
> 
> "The A32 and T32
> instruction sets include instructions that operate directly on various PSTATE elements, and
> instructions that access PSTATE by using the Application Program Status Register (APSR)
> or the Current Program Status Register (CPSR)".
> 
> In other words, there is no cpsr at all on aarch64 execution state.
> On aarch64 execution state, pstate is an abstraction of process state
> information, so it has no architecturally defined size or enconding.
> 
> However, the architecture does define how the PSTATE is encoded in the
> SPSR when taking an exception, and that is the form of PSTATE in
> ptrace.  Currently only bits 31:0 of SPSR are defined, but it is only
> accessible via MSR/MRS instructions, which always transfer 64-bits,
> so on ISA level, it should be regarded as 64-bit.
> 
> This change will break the compatibility for <new gdb, old gdbserver>
> and <old gdb, new gdbserver>, but current GDB is wrong, we have to fix it.
> 

Won't this also break any situation where the gdbserver knowledge is
embedded in a HW stub?  You're assuming that we can just update all
those servers and everything will be fine and dandy.  I don't think we
can't just change this arbitrarily.

I think kgdb should do what everyone else has been doing and simply
return the bottom 32 bits.

R.


> gdb:
> 
> 2015-09-03  Yao Qi  <yao.qi@linaro.org>
> 
> 	* aarch64-tdep.c (aarch64_r_register_names): Rename cpsr to
> 	pstate.
> 	* features/aarch64-core.xmli (cpsr): Rename it to pstate, and
> 	change its size to 64-bit.
> 	* features/aarch64.c: Regenerate.
> 	* regformats/aarch64.dat: Likewise.
> ---
>  gdb/aarch64-tdep.c            | 2 +-
>  gdb/features/aarch64-core.xml | 2 +-
>  gdb/features/aarch64.c        | 2 +-
>  gdb/regformats/aarch64.dat    | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 5d8ffd4..04064cb 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -127,7 +127,7 @@ static const char *const aarch64_r_register_names[] =
>    "x20", "x21", "x22", "x23",
>    "x24", "x25", "x26", "x27",
>    "x28", "x29", "x30", "sp",
> -  "pc", "cpsr"
> +  "pc", "pstate"
>  };
>  
>  /* The FP/SIMD 'V' registers.  */
> diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml
> index c95049c..f0f455f 100644
> --- a/gdb/features/aarch64-core.xml
> +++ b/gdb/features/aarch64-core.xml
> @@ -42,5 +42,5 @@
>    <reg name="sp" bitsize="64" type="data_ptr"/>
>  
>    <reg name="pc" bitsize="64" type="code_ptr"/>
> -  <reg name="cpsr" bitsize="32"/>
> +  <reg name="pstate" bitsize="64"/>
>  </feature>
> diff --git a/gdb/features/aarch64.c b/gdb/features/aarch64.c
> index 1e9a99d..643387d 100644
> --- a/gdb/features/aarch64.c
> +++ b/gdb/features/aarch64.c
> @@ -50,7 +50,7 @@ initialize_tdesc_aarch64 (void)
>    tdesc_create_reg (feature, "x30", 30, 1, NULL, 64, "int");
>    tdesc_create_reg (feature, "sp", 31, 1, NULL, 64, "data_ptr");
>    tdesc_create_reg (feature, "pc", 32, 1, NULL, 64, "code_ptr");
> -  tdesc_create_reg (feature, "cpsr", 33, 1, NULL, 32, "int");
> +  tdesc_create_reg (feature, "pstate", 33, 1, NULL, 64, "int");
>  
>    feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.fpu");
>    field_type = tdesc_named_type (feature, "ieee_double");
> diff --git a/gdb/regformats/aarch64.dat b/gdb/regformats/aarch64.dat
> index d4cea04..0db2f3a 100644
> --- a/gdb/regformats/aarch64.dat
> +++ b/gdb/regformats/aarch64.dat
> @@ -36,7 +36,7 @@ expedite:x29,sp,pc
>  64:x30
>  64:sp
>  64:pc
> -32:cpsr
> +64:pstate
>  128:v0
>  128:v1
>  128:v2
> 

Re: [PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate

[Yao Qi]

Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:

> Won't this also break any situation where the gdbserver knowledge is
> embedded in a HW stub?  You're assuming that we can just update all
> those servers and everything will be fine and dandy.  I don't think we
> can't just change this arbitrarily.

Yes, it will break any existing debugging stubs, including old gdbserver
and jtag probes, which think the register in the target description
(cpsr, psate or whatever) is 32-bit.

However, I don't assume "we can just update all those servers and
everything will be fine and dandy".  As I said in the first mail,
this change will break the compatibility.

AArch64 is still a new architecture, and there isn't much compatibility
burden as other old archs have.  Under this context, I think correctness
is more important than compatibility.  If we don't fix it now, we can't fix
it forever.  So it isn't an arbitrary change.

We did change the size of the register in the target description to
64-bit back and forth in 2013 and 2014, but I don't hear any complaints
on this from HW jtag probes providers.

>
> I think kgdb should do what everyone else has been doing and simply
> return the bottom 32 bits.

I think kgdb is correct.  I don't know what "everyone else" is.  As far
as I can see, openocd doesn't support aarch64 now, but there are some
patches to add aarch64 support http://openocd.zylin.com/#/c/2501/14
the register (named CPSR in the openocd patches) is 64-bit, which looks
right to me.

-- 
Yao (齐尧)

Re: [PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate

[Pedro Alves]

On 09/08/2015 09:20 AM, Yao Qi wrote:
> 
> AArch64 is still a new architecture, and there isn't much compatibility
> burden as other old archs have.

Under the same reasoning, you should be able to fix kgdb as well.

> Under this context, I think correctness
> is more important than compatibility.  If we don't fix it now, we can't fix
> it forever.  So it isn't an arbitrary change.

Simply cpsr->pstate like in your patch has user-visible changes as well.  Do
users really expect to see "pstate" in "info registers"?  If it really OK
that "print $cpsr" stops working?

But the change is really not OK as is.  You can't have a server _not_
send "cpsr" and claim that the target description supports
the "org.gnu.gdb.aarch64.core" feature:

  @node AArch64 Features
  @subsection AArch64 Features
  @cindex target descriptions, AArch64 features

  The @samp{org.gnu.gdb.aarch64.core} feature is required for AArch64
  targets.  It should contain registers @samp{x0} through @samp{x30},
  @samp{sp}, @samp{pc}, and @samp{cpsr}.

If we do go the cpsr->pstate direction, we'll have to add a new
target feature.

Thanks,
Pedro Alves

Re: [PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> But the change is really not OK as is.  You can't have a server _not_
> send "cpsr" and claim that the target description supports
> the "org.gnu.gdb.aarch64.core" feature:
>
>   @node AArch64 Features
>   @subsection AArch64 Features
>   @cindex target descriptions, AArch64 features
>
>   The @samp{org.gnu.gdb.aarch64.core} feature is required for AArch64
>   targets.  It should contain registers @samp{x0} through @samp{x30},
>   @samp{sp}, @samp{pc}, and @samp{cpsr}.
>
> If we do go the cpsr->pstate direction, we'll have to add a new
> target feature.

Yes, I realize that we need to add a new feature, like
"org.gnu.gdb.aarch64.core_pstate".  I withdraw this patch.

-- 
Yao (齐尧)