The ARM now defines 36 bits in SPSR_ELx in aarch64 mode, so
it's time to bite the bullet and extend PSTATE to match.
Most changes are straightforward, adjusting printf formats,
changing local variable types. More complex is migration,
where to maintain backward compatibility a new pstate64
record is introduced, and only when one of the extensions
that sets bits 32-35 are active.
The fate of gdbstub is left undecided for the moment.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 8 +++---
target/arm/tcg/translate.h | 20 ++++++-------
target/arm/cpu.c | 6 ++--
target/arm/gdbstub64.c | 1 +
target/arm/helper.c | 11 ++++----
target/arm/machine.c | 56 +++++++++++++++++++++++++++++++++++++
target/arm/tcg/helper-a64.c | 2 +-
7 files changed, 81 insertions(+), 23 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 98360b70b8..7769c4ae3c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -268,7 +268,7 @@ typedef struct CPUArchState {
uint64_t xregs[32];
uint64_t pc;
/* PSTATE isn't an architectural register for ARMv8. However, it is
- * convenient for us to assemble the underlying state into a 32 bit format
+ * convenient for us to assemble the underlying state into a 64 bit format
* identical to the architectural format used for the SPSR. (This is also
* what the Linux kernel's 'pstate' field in signal handlers and KVM's
* 'pstate' register are.) Of the PSTATE bits:
@@ -280,7 +280,7 @@ typedef struct CPUArchState {
* SM and ZA are kept in env->svcr
* all other bits are stored in their correct places in env->pstate
*/
- uint32_t pstate;
+ uint64_t pstate;
bool aarch64; /* True if CPU is in aarch64 state; inverse of PSTATE.nRW */
bool thumb; /* True if CPU is in thumb mode; cpsr[5] */
@@ -1556,7 +1556,7 @@ static inline unsigned int aarch64_pstate_mode(unsigned int el, bool handler)
* interprocessing, so we don't attempt to sync with the cpsr state used by
* the 32 bit decoder.
*/
-static inline uint32_t pstate_read(CPUARMState *env)
+static inline uint64_t pstate_read(CPUARMState *env)
{
int ZF;
@@ -1566,7 +1566,7 @@ static inline uint32_t pstate_read(CPUARMState *env)
| env->pstate | env->daif | (env->btype << 10);
}
-static inline void pstate_write(CPUARMState *env, uint32_t val)
+static inline void pstate_write(CPUARMState *env, uint64_t val)
{
env->ZF = (~val) & PSTATE_Z;
env->NF = val;
diff --git a/target/arm/tcg/translate.h b/target/arm/tcg/translate.h
index 3e63dad2b6..1479f5bf74 100644
--- a/target/arm/tcg/translate.h
+++ b/target/arm/tcg/translate.h
@@ -378,27 +378,27 @@ static inline TCGv_i32 get_ahp_flag(void)
}
/* Set bits within PSTATE. */
-static inline void set_pstate_bits(uint32_t bits)
+static inline void set_pstate_bits(uint64_t bits)
{
- TCGv_i32 p = tcg_temp_new_i32();
+ TCGv_i64 p = tcg_temp_new_i64();
tcg_debug_assert(!(bits & CACHED_PSTATE_BITS));
- tcg_gen_ld_i32(p, tcg_env, offsetof(CPUARMState, pstate));
- tcg_gen_ori_i32(p, p, bits);
- tcg_gen_st_i32(p, tcg_env, offsetof(CPUARMState, pstate));
+ tcg_gen_ld_i64(p, tcg_env, offsetof(CPUARMState, pstate));
+ tcg_gen_ori_i64(p, p, bits);
+ tcg_gen_st_i64(p, tcg_env, offsetof(CPUARMState, pstate));
}
/* Clear bits within PSTATE. */
-static inline void clear_pstate_bits(uint32_t bits)
+static inline void clear_pstate_bits(uint64_t bits)
{
- TCGv_i32 p = tcg_temp_new_i32();
+ TCGv_i64 p = tcg_temp_new_i64();
tcg_debug_assert(!(bits & CACHED_PSTATE_BITS));
- tcg_gen_ld_i32(p, tcg_env, offsetof(CPUARMState, pstate));
- tcg_gen_andi_i32(p, p, ~bits);
- tcg_gen_st_i32(p, tcg_env, offsetof(CPUARMState, pstate));
+ tcg_gen_ld_i64(p, tcg_env, offsetof(CPUARMState, pstate));
+ tcg_gen_andi_i64(p, p, ~bits);
+ tcg_gen_st_i64(p, tcg_env, offsetof(CPUARMState, pstate));
}
/* If the singlestep state is Active-not-pending, advance to Active-pending. */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index d9318c5325..ec63297165 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1208,7 +1208,7 @@ static void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
{
ARMCPU *cpu = ARM_CPU(cs);
CPUARMState *env = &cpu->env;
- uint32_t psr = pstate_read(env);
+ uint64_t psr = pstate_read(env);
int i, j;
int el = arm_current_el(env);
uint64_t hcr = arm_hcr_el2_eff(env);
@@ -1230,7 +1230,7 @@ static void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
} else {
ns_status = "";
}
- qemu_fprintf(f, "PSTATE=%08x %c%c%c%c %sEL%d%c",
+ qemu_fprintf(f, "PSTATE=%016" PRIx64 " %c%c%c%c %sEL%d%c",
psr,
psr & PSTATE_N ? 'N' : '-',
psr & PSTATE_Z ? 'Z' : '-',
@@ -1247,7 +1247,7 @@ static void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
(FIELD_EX64(env->svcr, SVCR, SM) ? 'S' : '-'));
}
if (cpu_isar_feature(aa64_bti, cpu)) {
- qemu_fprintf(f, " BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
+ qemu_fprintf(f, " BTYPE=%d", (int)(psr & PSTATE_BTYPE) >> 10);
}
qemu_fprintf(f, "%s%s%s",
(hcr & HCR_NV) ? " NV" : "",
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 64ee9b3b56..3cef47281a 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
case 32:
return gdb_get_reg64(mem_buf, env->pc);
case 33:
+ /* pstate is now a 64-bit value; can we simply adjust the xml? */
return gdb_get_reg32(mem_buf, pstate_read(env));
}
/* Unknown register. */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f006ecabf3..e404ba0f71 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9185,8 +9185,8 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
CPUARMState *env = &cpu->env;
unsigned int new_el = env->exception.target_el;
vaddr addr = env->cp15.vbar_el[new_el];
- unsigned int new_mode = aarch64_pstate_mode(new_el, true);
- unsigned int old_mode;
+ uint64_t new_mode = aarch64_pstate_mode(new_el, true);
+ uint64_t old_mode;
unsigned int cur_el = arm_current_el(env);
int rt;
@@ -9334,7 +9334,7 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
* If NV2 is disabled, change SPSR when NV,NV1 == 1,0 (I_ZJRNN)
* If NV2 is enabled, change SPSR when NV is 1 (I_DBTLM)
*/
- old_mode = deposit32(old_mode, 2, 2, 2);
+ old_mode = deposit64(old_mode, 2, 2, 2);
}
}
} else {
@@ -9347,7 +9347,7 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
}
env->banked_spsr[aarch64_banked_spsr_index(new_el)] = old_mode;
- qemu_log_mask(CPU_LOG_INT, "...with SPSR 0x%x\n", old_mode);
+ qemu_log_mask(CPU_LOG_INT, "...with SPSR 0x%" PRIx64 "\n", old_mode);
qemu_log_mask(CPU_LOG_INT, "...with ELR 0x%" PRIx64 "\n",
env->elr_el[new_el]);
@@ -9401,7 +9401,8 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
env->pc = addr;
- qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
+ qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64
+ " PSTATE 0x%" PRIx64 "\n",
new_el, env->pc, pstate_read(env));
}
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 8dbeca2867..9b00c14b4a 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -836,6 +836,61 @@ static const VMStateInfo vmstate_cpsr = {
.put = put_cpsr,
};
+static int get_pstate64_1(QEMUFile *f, void *opaque, size_t size,
+ const VMStateField *field)
+{
+ ARMCPU *cpu = opaque;
+ CPUARMState *env = &cpu->env;
+ uint64_t val = qemu_get_be64(f);
+
+ env->aarch64 = ((val & PSTATE_nRW) == 0);
+ pstate_write(env, val);
+ return 0;
+}
+
+static int put_pstate64_1(QEMUFile *f, void *opaque, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc)
+{
+ ARMCPU *cpu = opaque;
+ CPUARMState *env = &cpu->env;
+ uint64_t val = pstate_read(env);
+
+ qemu_put_be64(f, val);
+ return 0;
+}
+
+static const VMStateInfo vmstate_pstate64_1 = {
+ .name = "pstate64",
+ .get = get_pstate64_1,
+ .put = put_pstate64_1,
+};
+
+static bool pstate64_needed(void *opaque)
+{
+ ARMCPU *cpu = opaque;
+ CPUARMState *env = &cpu->env;
+
+ return is_a64(env) && pstate_read(env) > UINT32_MAX;
+}
+
+static const VMStateDescription vmstate_pstate64 = {
+ .name = "cpu/pstate64",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = pstate64_needed,
+ .fields = (const VMStateField[]) {
+ {
+ .name = "pstate64",
+ .version_id = 0,
+ .size = sizeof(uint64_t),
+ .info = &vmstate_pstate64_1,
+ .flags = VMS_SINGLE,
+ .offset = 0,
+ },
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static int get_power(QEMUFile *f, void *opaque, size_t size,
const VMStateField *field)
{
@@ -1119,6 +1174,7 @@ const VMStateDescription vmstate_arm_cpu = {
&vmstate_serror,
&vmstate_irq_line_state,
&vmstate_wfxt_timer,
+ &vmstate_pstate64,
NULL
}
};
diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
index 71c6c44ee8..f61adf1f80 100644
--- a/target/arm/tcg/helper-a64.c
+++ b/target/arm/tcg/helper-a64.c
@@ -639,7 +639,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
ARMCPU *cpu = env_archcpu(env);
int cur_el = arm_current_el(env);
unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
- uint32_t spsr = env->banked_spsr[spsr_idx];
+ uint64_t spsr = env->banked_spsr[spsr_idx];
int new_el;
bool return_to_aa64 = (spsr & PSTATE_nRW) == 0;
--
2.43.0On 7/27/25 1:02 AM, Richard Henderson wrote: > The ARM now defines 36 bits in SPSR_ELx in aarch64 mode, so > it's time to bite the bullet and extend PSTATE to match. > > Most changes are straightforward, adjusting printf formats, > changing local variable types. More complex is migration, > where to maintain backward compatibility a new pstate64 > record is introduced, and only when one of the extensions > that sets bits 32-35 are active. > > The fate of gdbstub is left undecided for the moment. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 8 +++--- > target/arm/tcg/translate.h | 20 ++++++------- > target/arm/cpu.c | 6 ++-- > target/arm/gdbstub64.c | 1 + > target/arm/helper.c | 11 ++++---- > target/arm/machine.c | 56 +++++++++++++++++++++++++++++++++++++ > target/arm/tcg/helper-a64.c | 2 +- > 7 files changed, 81 insertions(+), 23 deletions(-) Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
On 7/27/25 1:02 AM, Richard Henderson wrote:
> The ARM now defines 36 bits in SPSR_ELx in aarch64 mode, so
> it's time to bite the bullet and extend PSTATE to match.
>
> Most changes are straightforward, adjusting printf formats,
> changing local variable types. More complex is migration,
> where to maintain backward compatibility a new pstate64
> record is introduced, and only when one of the extensions
> that sets bits 32-35 are active.
>
> The fate of gdbstub is left undecided for the moment.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/cpu.h | 8 +++---
> target/arm/tcg/translate.h | 20 ++++++-------
> target/arm/cpu.c | 6 ++--
> target/arm/gdbstub64.c | 1 +
> target/arm/helper.c | 11 ++++----
> target/arm/machine.c | 56 +++++++++++++++++++++++++++++++++++++
> target/arm/tcg/helper-a64.c | 2 +-
> 7 files changed, 81 insertions(+), 23 deletions(-)
...
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 64ee9b3b56..3cef47281a 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> case 32:
> return gdb_get_reg64(mem_buf, env->pc);
> case 33:
> + /* pstate is now a 64-bit value; can we simply adjust the xml? */
> return gdb_get_reg32(mem_buf, pstate_read(env));
> }
If I'm correct, we currently don't expose PSTATE through gdbstub, but
only CPSR. This was a bit confusing for me, considering that CPSR is not
even supposed to exist in Aarch64.
Maybe it's a good opportunity to expose PSTATE instead, which could have
a 64 bits size. This way, we don't break any workflow.
...
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 8dbeca2867..9b00c14b4a 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -836,6 +836,61 @@ static const VMStateInfo vmstate_cpsr = {
> .put = put_cpsr,
> };
>
> +static int get_pstate64_1(QEMUFile *f, void *opaque, size_t size,
> + const VMStateField *field)
> +{
> + ARMCPU *cpu = opaque;
> + CPUARMState *env = &cpu->env;
> + uint64_t val = qemu_get_be64(f);
> +
> + env->aarch64 = ((val & PSTATE_nRW) == 0);
> + pstate_write(env, val);
> + return 0;
> +}
> +
> +static int put_pstate64_1(QEMUFile *f, void *opaque, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc)
> +{
> + ARMCPU *cpu = opaque;
> + CPUARMState *env = &cpu->env;
> + uint64_t val = pstate_read(env);
> +
> + qemu_put_be64(f, val);
> + return 0;
> +}
> +
> +static const VMStateInfo vmstate_pstate64_1 = {
> + .name = "pstate64",
> + .get = get_pstate64_1,
> + .put = put_pstate64_1,
> +};
> +
> +static bool pstate64_needed(void *opaque)
> +{
> + ARMCPU *cpu = opaque;
> + CPUARMState *env = &cpu->env;
> +
> + return is_a64(env) && pstate_read(env) > UINT32_MAX;
> +}
> +
> +static const VMStateDescription vmstate_pstate64 = {
> + .name = "cpu/pstate64",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = pstate64_needed,
> + .fields = (const VMStateField[]) {
> + {
> + .name = "pstate64",
> + .version_id = 0,
> + .size = sizeof(uint64_t),
> + .info = &vmstate_pstate64_1,
> + .flags = VMS_SINGLE,
> + .offset = 0,
> + },
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> static int get_power(QEMUFile *f, void *opaque, size_t size,
> const VMStateField *field)
> {
> @@ -1119,6 +1174,7 @@ const VMStateDescription vmstate_arm_cpu = {
> &vmstate_serror,
> &vmstate_irq_line_state,
> &vmstate_wfxt_timer,
> + &vmstate_pstate64,
> NULL
> }
> };
> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
> index 71c6c44ee8..f61adf1f80 100644
> --- a/target/arm/tcg/helper-a64.c
> +++ b/target/arm/tcg/helper-a64.c
> @@ -639,7 +639,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
> ARMCPU *cpu = env_archcpu(env);
> int cur_el = arm_current_el(env);
> unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
> - uint32_t spsr = env->banked_spsr[spsr_idx];
> + uint64_t spsr = env->banked_spsr[spsr_idx];
> int new_el;
> bool return_to_aa64 = (spsr & PSTATE_nRW) == 0;
>
Would that be better or worse to simply save the upper 32 bits,
considering that cpsr already holds the lower ones in Aarch64 mode?
On Thu, 31 Jul 2025 at 21:34, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote: > > On 7/27/25 1:02 AM, Richard Henderson wrote: > > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > > index 64ee9b3b56..3cef47281a 100644 > > --- a/target/arm/gdbstub64.c > > +++ b/target/arm/gdbstub64.c > > @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n) > > case 32: > > return gdb_get_reg64(mem_buf, env->pc); > > case 33: > > + /* pstate is now a 64-bit value; can we simply adjust the xml? */ > > return gdb_get_reg32(mem_buf, pstate_read(env)); > > } > > If I'm correct, we currently don't expose PSTATE through gdbstub, but > only CPSR. This was a bit confusing for me, considering that CPSR is not > even supposed to exist in Aarch64. > Maybe it's a good opportunity to expose PSTATE instead, which could have > a 64 bits size. This way, we don't break any workflow. Architecturally, PSTATE is simply an abstract bundling together of different information: it is not a particular format of a value, whether 32 or 64 bit or otherwise. (This makes it different to AArch32 CPSR, which really is a guest-visible register.) The thing that *is* defined architecturally is the SPSR_ELx format, which is where various bits of PSTATE get saved when reporting an exception up to a higher exception level (and which is pretty much the AArch32 CPSR format when the lower EL is AArch32). (Note that not *all* of PSTATE appears in the SPSR_ELx: for example the SME SM and ZA bits are considered part of PSTATE but they aren't saved into SPSR_ELx.) For convenience, various pieces of software pass around information in that SPSR_ELx format. Calling this either "CPSR" or "PSTATE" is not really correct, but either is perhaps less confusing than calling it SPSR when the context is that of the code running at the lower EL rather than the higher. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/kgdb.h#n41 suggests that expanding the existing pstate to 64 bits is probably likely to produce problems. Perhaps we should define a pstate_high or something for the top 32 bits? (I'll see if I can find out if anybody's already looked at this for the native debug case.) thanks -- PMM
On Fri, Aug 1, 2025 at 4:33 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 31 Jul 2025 at 21:34, Pierrick Bouvier > <pierrick.bouvier@linaro.org> wrote: > > > > On 7/27/25 1:02 AM, Richard Henderson wrote: > > > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > > > index 64ee9b3b56..3cef47281a 100644 > > > --- a/target/arm/gdbstub64.c > > > +++ b/target/arm/gdbstub64.c > > > @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n) > > > case 32: > > > return gdb_get_reg64(mem_buf, env->pc); > > > case 33: > > > + /* pstate is now a 64-bit value; can we simply adjust the xml? */ > > > return gdb_get_reg32(mem_buf, pstate_read(env)); > > > } > > > > If I'm correct, we currently don't expose PSTATE through gdbstub, but > > only CPSR. This was a bit confusing for me, considering that CPSR is not > > even supposed to exist in Aarch64. > > Maybe it's a good opportunity to expose PSTATE instead, which could have > > a 64 bits size. This way, we don't break any workflow. > > Architecturally, PSTATE is simply an abstract bundling together of > different information: it is not a particular format of a value, > whether 32 or 64 bit or otherwise. (This makes it different to > AArch32 CPSR, which really is a guest-visible register.) > > The thing that *is* defined architecturally is the SPSR_ELx format, which > is where various bits of PSTATE get saved when reporting an exception up > to a higher exception level (and which is pretty much the AArch32 CPSR > format when the lower EL is AArch32). (Note that not *all* of PSTATE > appears in the SPSR_ELx: for example the SME SM and ZA bits are > considered part of PSTATE but they aren't saved into SPSR_ELx.) > > For convenience, various pieces of software pass around information > in that SPSR_ELx format. Calling this either "CPSR" or "PSTATE" > is not really correct, but either is perhaps less confusing than > calling it SPSR when the context is that of the code running at the > lower EL rather than the higher. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/kgdb.h#n41 > suggests that expanding the existing pstate to 64 bits is probably > likely to produce problems. Perhaps we should define a pstate_high > or something for the top 32 bits? > IIUC, this is only a problem if you use the default gdb architecture register map, but QEMU ships its own as part of gdbstub, so it's free to change register definition of cspr. Or add a new PSTATE register. > (I'll see if I can find out if anybody's already looked at this > for the native debug case.) > > thanks > -- PMM >
Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:
> On Fri, Aug 1, 2025 at 4:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Thu, 31 Jul 2025 at 21:34, Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>> >
>> > On 7/27/25 1:02 AM, Richard Henderson wrote:
>> > > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
>> > > index 64ee9b3b56..3cef47281a 100644
>> > > --- a/target/arm/gdbstub64.c
>> > > +++ b/target/arm/gdbstub64.c
>> > > @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>> > > case 32:
>> > > return gdb_get_reg64(mem_buf, env->pc);
>> > > case 33:
>> > > + /* pstate is now a 64-bit value; can we simply adjust the xml? */
>> > > return gdb_get_reg32(mem_buf, pstate_read(env));
>> > > }
>> >
>> > If I'm correct, we currently don't expose PSTATE through gdbstub, but
>> > only CPSR. This was a bit confusing for me, considering that CPSR is not
>> > even supposed to exist in Aarch64.
>> > Maybe it's a good opportunity to expose PSTATE instead, which could have
>> > a 64 bits size. This way, we don't break any workflow.
>>
>> Architecturally, PSTATE is simply an abstract bundling together of
>> different information: it is not a particular format of a value,
>> whether 32 or 64 bit or otherwise. (This makes it different to
>> AArch32 CPSR, which really is a guest-visible register.)
>>
>> The thing that *is* defined architecturally is the SPSR_ELx format, which
>> is where various bits of PSTATE get saved when reporting an exception up
>> to a higher exception level (and which is pretty much the AArch32 CPSR
>> format when the lower EL is AArch32). (Note that not *all* of PSTATE
>> appears in the SPSR_ELx: for example the SME SM and ZA bits are
>> considered part of PSTATE but they aren't saved into SPSR_ELx.)
>>
>> For convenience, various pieces of software pass around information
>> in that SPSR_ELx format. Calling this either "CPSR" or "PSTATE"
>> is not really correct, but either is perhaps less confusing than
>> calling it SPSR when the context is that of the code running at the
>> lower EL rather than the higher.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/kgdb.h#n41
>> suggests that expanding the existing pstate to 64 bits is probably
>> likely to produce problems. Perhaps we should define a pstate_high
>> or something for the top 32 bits?
>>
>
> IIUC, this is only a problem if you use the default gdb architecture
> register map, but QEMU ships its own as part of gdbstub, so it's free
> to change register definition of cspr. Or add a new PSTATE register.
That is correct. The kgdb.h comment also mentions this:
* Note that if you are using one of the versions of gdb that supports
* the gdb-7.7 version of the protocol you cannot use kgdb directly
* without providing a custom register description (gdb can load new
* protocol descriptions at runtime).
"providing a custom register description" is the manual procedure the
user can perform that is equivalent to the gdbstub sending its own
register map XML ("target description" in GDB parlance) to GDB, as the
QEMU gdbstub does.
I checked and the only places where GDB hardcodes the CPSR size is in
Linux and FreeBSD specific code, when reading or writing a core file
because in that case the size is defined by the OS.
Note that the CPSR position in the register map is hardcoded, defining
its register number as 33.
--
Thiago
On 8/1/25 6:22 AM, Peter Maydell wrote: > On Thu, 31 Jul 2025 at 21:34, Pierrick Bouvier > <pierrick.bouvier@linaro.org> wrote: >> >> On 7/27/25 1:02 AM, Richard Henderson wrote: >>> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c >>> index 64ee9b3b56..3cef47281a 100644 >>> --- a/target/arm/gdbstub64.c >>> +++ b/target/arm/gdbstub64.c >>> @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n) >>> case 32: >>> return gdb_get_reg64(mem_buf, env->pc); >>> case 33: >>> + /* pstate is now a 64-bit value; can we simply adjust the xml? */ >>> return gdb_get_reg32(mem_buf, pstate_read(env)); >>> } >> >> If I'm correct, we currently don't expose PSTATE through gdbstub, but >> only CPSR. This was a bit confusing for me, considering that CPSR is not >> even supposed to exist in Aarch64. >> Maybe it's a good opportunity to expose PSTATE instead, which could have >> a 64 bits size. This way, we don't break any workflow. > > Architecturally, PSTATE is simply an abstract bundling together of > different information: it is not a particular format of a value, > whether 32 or 64 bit or otherwise. (This makes it different to > AArch32 CPSR, which really is a guest-visible register.) > I see. Then maybe what we really miss are the special registers associated to PSTATE (CurrentEL, DAIF, NZCV, ...) which are defined architecturally. When I tried to read EL for the first time using gdbstub, I was looking for CurrentEL, and finally found it accidently under CPSR. Would that make more sense? > The thing that *is* defined architecturally is the SPSR_ELx format, which > is where various bits of PSTATE get saved when reporting an exception up > to a higher exception level (and which is pretty much the AArch32 CPSR > format when the lower EL is AArch32). (Note that not *all* of PSTATE > appears in the SPSR_ELx: for example the SME SM and ZA bits are > considered part of PSTATE but they aren't saved into SPSR_ELx.) > > For convenience, various pieces of software pass around information > in that SPSR_ELx format. Calling this either "CPSR" or "PSTATE" > is not really correct, but either is perhaps less confusing than > calling it SPSR when the context is that of the code running at the > lower EL rather than the higher. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/kgdb.h#n41 > suggests that expanding the existing pstate to 64 bits is probably > likely to produce problems. Perhaps we should define a pstate_high > or something for the top 32 bits? > It seems we don't expose pstate at all now, and from your answer above, it would not make sense to expose it directly, as it's not formally defined. What is exposed by kgdb? > (I'll see if I can find out if anybody's already looked at this > for the native debug case.) > > thanks > -- PMM
On Fri, 1 Aug 2025 at 17:26, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote: > > On 8/1/25 6:22 AM, Peter Maydell wrote: > > On Thu, 31 Jul 2025 at 21:34, Pierrick Bouvier > > <pierrick.bouvier@linaro.org> wrote: > >> > >> On 7/27/25 1:02 AM, Richard Henderson wrote: > >>> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > >>> index 64ee9b3b56..3cef47281a 100644 > >>> --- a/target/arm/gdbstub64.c > >>> +++ b/target/arm/gdbstub64.c > >>> @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n) > >>> case 32: > >>> return gdb_get_reg64(mem_buf, env->pc); > >>> case 33: > >>> + /* pstate is now a 64-bit value; can we simply adjust the xml? */ > >>> return gdb_get_reg32(mem_buf, pstate_read(env)); > >>> } > >> > >> If I'm correct, we currently don't expose PSTATE through gdbstub, but > >> only CPSR. This was a bit confusing for me, considering that CPSR is not > >> even supposed to exist in Aarch64. > >> Maybe it's a good opportunity to expose PSTATE instead, which could have > >> a 64 bits size. This way, we don't break any workflow. > > > > Architecturally, PSTATE is simply an abstract bundling together of > > different information: it is not a particular format of a value, > > whether 32 or 64 bit or otherwise. (This makes it different to > > AArch32 CPSR, which really is a guest-visible register.) > > > > I see. > Then maybe what we really miss are the special registers associated to > PSTATE (CurrentEL, DAIF, NZCV, ...) which are defined architecturally. > When I tried to read EL for the first time using gdbstub, I was looking > for CurrentEL, and finally found it accidently under CPSR. > Would that make more sense? I think for backwards-compatibility reasons we should stick with the current cpsr format for the information that is in it. We do at least define the fields so you get a nice view of it: (gdb) print $cpsr $4 = [ EL=0 BTYPE=0 Z ] > > The thing that *is* defined architecturally is the SPSR_ELx format, which > > is where various bits of PSTATE get saved when reporting an exception up > > to a higher exception level (and which is pretty much the AArch32 CPSR > > format when the lower EL is AArch32). (Note that not *all* of PSTATE > > appears in the SPSR_ELx: for example the SME SM and ZA bits are > > considered part of PSTATE but they aren't saved into SPSR_ELx.) > > > > For convenience, various pieces of software pass around information > > in that SPSR_ELx format. Calling this either "CPSR" or "PSTATE" > > is not really correct, but either is perhaps less confusing than > > calling it SPSR when the context is that of the code running at the > > lower EL rather than the higher. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/kgdb.h#n41 > > suggests that expanding the existing pstate to 64 bits is probably > > likely to produce problems. Perhaps we should define a pstate_high > > or something for the top 32 bits? > > > > It seems we don't expose pstate at all now, and from your answer above, > it would not make sense to expose it directly, as it's not formally > defined. What is exposed by kgdb? kgdb currently does the same thing we do, and exposes a 32-bit "cpsr" value. -- PMM
On 8/1/25 9:37 AM, Peter Maydell wrote: > On Fri, 1 Aug 2025 at 17:26, Pierrick Bouvier > <pierrick.bouvier@linaro.org> wrote: >> >> On 8/1/25 6:22 AM, Peter Maydell wrote: >>> On Thu, 31 Jul 2025 at 21:34, Pierrick Bouvier >>> <pierrick.bouvier@linaro.org> wrote: >>>> >>>> On 7/27/25 1:02 AM, Richard Henderson wrote: >>>>> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c >>>>> index 64ee9b3b56..3cef47281a 100644 >>>>> --- a/target/arm/gdbstub64.c >>>>> +++ b/target/arm/gdbstub64.c >>>>> @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n) >>>>> case 32: >>>>> return gdb_get_reg64(mem_buf, env->pc); >>>>> case 33: >>>>> + /* pstate is now a 64-bit value; can we simply adjust the xml? */ >>>>> return gdb_get_reg32(mem_buf, pstate_read(env)); >>>>> } >>>> >>>> If I'm correct, we currently don't expose PSTATE through gdbstub, but >>>> only CPSR. This was a bit confusing for me, considering that CPSR is not >>>> even supposed to exist in Aarch64. >>>> Maybe it's a good opportunity to expose PSTATE instead, which could have >>>> a 64 bits size. This way, we don't break any workflow. >>> >>> Architecturally, PSTATE is simply an abstract bundling together of >>> different information: it is not a particular format of a value, >>> whether 32 or 64 bit or otherwise. (This makes it different to >>> AArch32 CPSR, which really is a guest-visible register.) >>> >> >> I see. >> Then maybe what we really miss are the special registers associated to >> PSTATE (CurrentEL, DAIF, NZCV, ...) which are defined architecturally. >> When I tried to read EL for the first time using gdbstub, I was looking >> for CurrentEL, and finally found it accidently under CPSR. >> Would that make more sense? > > I think for backwards-compatibility reasons we should stick > with the current cpsr format for the information that is in it. Yes, it's here anyway. My proposal was to add new registers. > We do at least define the fields so you get a nice view of it: > > (gdb) print $cpsr > $4 = [ EL=0 BTYPE=0 Z ] > Do you have specific pretty printers installed? On my debian trixie (gdb 16.3), there is no pretty printer for cpsr by default. >>> The thing that *is* defined architecturally is the SPSR_ELx format, which >>> is where various bits of PSTATE get saved when reporting an exception up >>> to a higher exception level (and which is pretty much the AArch32 CPSR >>> format when the lower EL is AArch32). (Note that not *all* of PSTATE >>> appears in the SPSR_ELx: for example the SME SM and ZA bits are >>> considered part of PSTATE but they aren't saved into SPSR_ELx.) >>> >>> For convenience, various pieces of software pass around information >>> in that SPSR_ELx format. Calling this either "CPSR" or "PSTATE" >>> is not really correct, but either is perhaps less confusing than >>> calling it SPSR when the context is that of the code running at the >>> lower EL rather than the higher. >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/kgdb.h#n41 >>> suggests that expanding the existing pstate to 64 bits is probably >>> likely to produce problems. Perhaps we should define a pstate_high >>> or something for the top 32 bits? >>> >> >> It seems we don't expose pstate at all now, and from your answer above, >> it would not make sense to expose it directly, as it's not formally >> defined. What is exposed by kgdb? > > kgdb currently does the same thing we do, and exposes a 32-bit > "cpsr" value. > > -- PMM
On Fri, 1 Aug 2025 at 17:41, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote: > > On 8/1/25 9:37 AM, Peter Maydell wrote: > > We do at least define the fields so you get a nice view of it: > > > > (gdb) print $cpsr > > $4 = [ EL=0 BTYPE=0 Z ] > > > > Do you have specific pretty printers installed? On my debian trixie (gdb > 16.3), there is no pretty printer for cpsr by default. I'm not aware of having installed any. This is GNU gdb (Ubuntu 15.0.50.20240403-0ubuntu1) 15.0.50.20240403-git I was assuming that this was just plain gdb doing something useful because the xml defines the field names and bitpositions (at least if your QEMU has commit 63070ce368e1a where Manos synced our xml to upstream gdb's to add these). -- PMM
On 8/1/25 9:44 AM, Peter Maydell wrote: > On Fri, 1 Aug 2025 at 17:41, Pierrick Bouvier > <pierrick.bouvier@linaro.org> wrote: >> >> On 8/1/25 9:37 AM, Peter Maydell wrote: >>> We do at least define the fields so you get a nice view of it: >>> >>> (gdb) print $cpsr >>> $4 = [ EL=0 BTYPE=0 Z ] >>> >> >> Do you have specific pretty printers installed? On my debian trixie (gdb >> 16.3), there is no pretty printer for cpsr by default. > > I'm not aware of having installed any. This is > GNU gdb (Ubuntu 15.0.50.20240403-0ubuntu1) 15.0.50.20240403-git > Trying with a ubuntu 24.04 container, I don't see it neither. Maybe something is missing in my gdbinit. > I was assuming that this was just plain gdb doing something > useful because the xml defines the field names and bitpositions > (at least if your QEMU has commit 63070ce368e1a where Manos > synced our xml to upstream gdb's to add these). > > -- PMM
On 8/1/25 9:53 AM, Pierrick Bouvier wrote: > On 8/1/25 9:44 AM, Peter Maydell wrote: >> On Fri, 1 Aug 2025 at 17:41, Pierrick Bouvier >> <pierrick.bouvier@linaro.org> wrote: >>> >>> On 8/1/25 9:37 AM, Peter Maydell wrote: >>>> We do at least define the fields so you get a nice view of it: >>>> >>>> (gdb) print $cpsr >>>> $4 = [ EL=0 BTYPE=0 Z ] >>>> >>> >>> Do you have specific pretty printers installed? On my debian trixie (gdb >>> 16.3), there is no pretty printer for cpsr by default. >> >> I'm not aware of having installed any. This is >> GNU gdb (Ubuntu 15.0.50.20240403-0ubuntu1) 15.0.50.20240403-git >> > > Trying with a ubuntu 24.04 container, I don't see it neither. Maybe > something is missing in my gdbinit. > My fault, I was using my system wide qemu binary. Using master branch it works well indeed. >> I was assuming that this was just plain gdb doing something >> useful because the xml defines the field names and bitpositions >> (at least if your QEMU has commit 63070ce368e1a where Manos >> synced our xml to upstream gdb's to add these). >> >> -- PMM >
On 8/1/25 05:13, Pierrick Bouvier wrote:
>> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
>> index 71c6c44ee8..f61adf1f80 100644
>> --- a/target/arm/tcg/helper-a64.c
>> +++ b/target/arm/tcg/helper-a64.c
>> @@ -639,7 +639,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
>> ARMCPU *cpu = env_archcpu(env);
>> int cur_el = arm_current_el(env);
>> unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
>> - uint32_t spsr = env->banked_spsr[spsr_idx];
>> + uint64_t spsr = env->banked_spsr[spsr_idx];
>> int new_el;
>> bool return_to_aa64 = (spsr & PSTATE_nRW) == 0;
>
> Would that be better or worse to simply save the upper 32 bits, considering that cpsr
> already holds the lower ones in Aarch64 mode?
I don't understand this comment.
(1) banked_spsr[] is already uint64_t
(2) SPSR_ELx is supposed to be uint64_t
(3) We were accidentally dropping the high bits of spsr here
because the local variable had the wrong type, before it
gets sent to pstate_write().
r~
On 7/31/25 9:29 PM, Richard Henderson wrote: > On 8/1/25 05:13, Pierrick Bouvier wrote: >>> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c >>> index 71c6c44ee8..f61adf1f80 100644 >>> --- a/target/arm/tcg/helper-a64.c >>> +++ b/target/arm/tcg/helper-a64.c >>> @@ -639,7 +639,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc) >>> ARMCPU *cpu = env_archcpu(env); >>> int cur_el = arm_current_el(env); >>> unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el); >>> - uint32_t spsr = env->banked_spsr[spsr_idx]; >>> + uint64_t spsr = env->banked_spsr[spsr_idx]; >>> int new_el; >>> bool return_to_aa64 = (spsr & PSTATE_nRW) == 0; >> >> Would that be better or worse to simply save the upper 32 bits, considering that cpsr >> already holds the lower ones in Aarch64 mode? > > I don't understand this comment. > > (1) banked_spsr[] is already uint64_t > (2) SPSR_ELx is supposed to be uint64_t > (3) We were accidentally dropping the high bits of spsr here > because the local variable had the wrong type, before it > gets sent to pstate_write(). > My comment was related to vmstate_pstate64. Sorry it was confusing as I left the last change concerning helper-a64.c, which was not related at all. So the comment was that vmstate_cpsr already contains the lower 32 bits of pstate, and thus we could save only the upper part in the new field. > > r~
On 8/2/25 02:12, Pierrick Bouvier wrote: > My comment was related to vmstate_pstate64. Sorry it was confusing as I left the last > change concerning helper-a64.c, which was not related at all. > > So the comment was that vmstate_cpsr already contains the lower 32 bits of pstate, and > thus we could save only the upper part in the new field. That was part laziness. By saving the lower half twice, I can simply do pstate_write(env, val) to load the data, rather than pstate_write(env, (val << 32) | pstate_read(env)); or something, to merge the halves. r~
On 8/1/25 05:13, Pierrick Bouvier wrote: > On 7/27/25 1:02 AM, Richard Henderson wrote: >> The ARM now defines 36 bits in SPSR_ELx in aarch64 mode, so >> it's time to bite the bullet and extend PSTATE to match. >> >> Most changes are straightforward, adjusting printf formats, >> changing local variable types. More complex is migration, >> where to maintain backward compatibility a new pstate64 >> record is introduced, and only when one of the extensions >> that sets bits 32-35 are active. >> >> The fate of gdbstub is left undecided for the moment. ... >> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c >> index 64ee9b3b56..3cef47281a 100644 >> --- a/target/arm/gdbstub64.c >> +++ b/target/arm/gdbstub64.c >> @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, >> int n) >> case 32: >> return gdb_get_reg64(mem_buf, env->pc); >> case 33: >> + /* pstate is now a 64-bit value; can we simply adjust the xml? */ >> return gdb_get_reg32(mem_buf, pstate_read(env)); >> } > > If I'm correct, we currently don't expose PSTATE through gdbstub, but only CPSR. This was > a bit confusing for me, considering that CPSR is not even supposed to exist in Aarch64. Correct. An old error, for sure. > Maybe it's a good opportunity to expose PSTATE instead, which could have a 64 bits size. > This way, we don't break any workflow. Hmm, perhaps adding a "org.gnu.gdb.aarch64.pstate" xml with just one 64-bit value is the best solution. Thiago? r~
On Fri, Aug 1, 2025 at 7:25 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/1/25 05:13, Pierrick Bouvier wrote: > > On 7/27/25 1:02 AM, Richard Henderson wrote: > >> The ARM now defines 36 bits in SPSR_ELx in aarch64 mode, so > >> it's time to bite the bullet and extend PSTATE to match. > >> > >> Most changes are straightforward, adjusting printf formats, > >> changing local variable types. More complex is migration, > >> where to maintain backward compatibility a new pstate64 > >> record is introduced, and only when one of the extensions > >> that sets bits 32-35 are active. > >> > >> The fate of gdbstub is left undecided for the moment. > ... > >> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > >> index 64ee9b3b56..3cef47281a 100644 > >> --- a/target/arm/gdbstub64.c > >> +++ b/target/arm/gdbstub64.c > >> @@ -47,6 +47,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, > >> int n) > >> case 32: > >> return gdb_get_reg64(mem_buf, env->pc); > >> case 33: > >> + /* pstate is now a 64-bit value; can we simply adjust the xml? */ > >> return gdb_get_reg32(mem_buf, pstate_read(env)); > >> } > > > > If I'm correct, we currently don't expose PSTATE through gdbstub, but only CPSR. This was > > a bit confusing for me, considering that CPSR is not even supposed to exist in Aarch64. > > Correct. An old error, for sure. > > > > Maybe it's a good opportunity to expose PSTATE instead, which could have a 64 bits size. > > This way, we don't break any workflow. > > Hmm, perhaps adding a "org.gnu.gdb.aarch64.pstate" xml with just one 64-bit value is the > best solution. Thiago? A single line in gdb-xml/aarch64-core.xml should be enough: <reg name="PSTATE" bitsize="64" /> I'd keep cpsr even if it's now effectively duplicated. It comes from gdb's upstream aarch64 schema so it's deeply ingrained by now. Then you can do `info registers $PSTATE` in gdb after adding the reg getter in gdbstub. I think it could be possible to define a <flags id="pstate_flags" size="8"> ... </flags> type that re-uses the cpsr_flags type definition by setting a field <field start="0" end="31" type = "cpsr_flags" /> but I haven't tried it. > > > r~ >
© 2016 - 2025 Red Hat, Inc.