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

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

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
>