qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] arm: Add PSTATE field registers CurrentEL, NZCV, DAIF, SPSel to gdbstub
@ 2025-08-08 11:30 Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 1/4] gdbstub/aarch64: add CurrentEL register Manos Pitsidianakis
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Manos Pitsidianakis @ 2025-08-08 11:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm,
	Manos Pitsidianakis

Expose PSTATE field registers to gdbstub:

  (gdb) info registers
  <- snip ->
  sp             0x0                 0x0
  pc             0x40000000          0x40000000
  cpsr           0x400003c5          [ SP EL=1 F I A D BTYPE=0 Z ]
  fpsr           0x0                 0
  fpcr           0x0                 0
  vg             0x2                 2
  pauth_dmask    0xffff000000000000  -281474976710656
  pauth_cmask    0xffff000000000000  -281474976710656
  pauth_dmask_high 0xffff000000000000 -281474976710656
  pauth_cmask_high 0xffff000000000000 -281474976710656
  CurrentEL      0x4                 [ EL=1 ]
  NZCV           0x40000000          [ Z ]
  DAIF           0x3c0               [ F I A D ]
  SPSel          0x1                 [ SP ]
  <-snip->

This also would allow plugins to access those registers.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Manos Pitsidianakis (4):
      gdbstub/aarch64: add CurrentEL register
      gdbstub/aarch64: add NZCV register
      gdbstub/aarch64: add DAIF register
      gdbstub/aarch64: add SPSel register

 gdb-xml/aarch64-core.xml | 35 +++++++++++++++++++++++++++++++++++
 target/arm/cpu.h         |  1 +
 target/arm/gdbstub64.c   | 29 +++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)
---
base-commit: cd21ee5b27b22ae66c103d36516aa5077881aa3d
change-id: 20250808-gdbstub-aarch64-pstate-regs-e061c1911d85

--
γαῖα πυρί μιχθήτω



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/4] gdbstub/aarch64: add CurrentEL register
  2025-08-08 11:30 [PATCH 0/4] arm: Add PSTATE field registers CurrentEL, NZCV, DAIF, SPSel to gdbstub Manos Pitsidianakis
@ 2025-08-08 11:30 ` Manos Pitsidianakis
  2025-08-08 12:21   ` Peter Maydell
  2025-08-08 11:30 ` [PATCH 2/4] gdbstub/aarch64: add NZCV register Manos Pitsidianakis
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Manos Pitsidianakis @ 2025-08-08 11:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm,
	Manos Pitsidianakis

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 gdb-xml/aarch64-core.xml | 5 +++++
 target/arm/cpu.h         | 1 +
 target/arm/gdbstub64.c   | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
index b8046510b9a085d30463d37b3ecc8d435f5fb7a4..19ad743dc5607b4021fb795bfb9b8e9cf0adef68 100644
--- a/gdb-xml/aarch64-core.xml
+++ b/gdb-xml/aarch64-core.xml
@@ -91,4 +91,9 @@
   </flags>
   <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
 
+  <flags id="current_el_flags" size="8">
+    <!-- Exception Level.  -->
+    <field name="EL" start="2" end="3"/>
+  </flags>
+  <reg name="CurrentEL" bitsize="64" type="current_el"/>
 </feature>
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index dc9b6dce4c92297b2636d0d7c0dce580f1806d5b..c3070cd9863381fac40f5640e0a7a84dfa1c6e06 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1473,6 +1473,7 @@ void pmu_init(ARMCPU *cpu);
  * AArch32 mode SPSRs are basically CPSR-format.
  */
 #define PSTATE_SP (1U)
+#define PSTATE_EL (3U << 2)
 #define PSTATE_M (0xFU)
 #define PSTATE_nRW (1U << 4)
 #define PSTATE_F (1U << 6)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 08e28585396816ab90d6d8e460ff8171892fe8da..16b564e1a970cb5e854a705619f71ffc61545a73 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -48,6 +48,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         return gdb_get_reg64(mem_buf, env->pc);
     case 33:
         return gdb_get_reg32(mem_buf, pstate_read(env));
+    case 34:
+        /* CurrentEL */
+        return gdb_get_reg64(mem_buf, env->pstate & PSTATE_EL);
     }
     /* Unknown register.  */
     return 0;
@@ -77,6 +80,9 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         /* CPSR */
         pstate_write(env, tmp);
         return 4;
+    case 34:
+        /* CurrentEL */
+        return 0;
     }
     /* Unknown register.  */
     return 0;

-- 
2.47.2



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] gdbstub/aarch64: add NZCV register
  2025-08-08 11:30 [PATCH 0/4] arm: Add PSTATE field registers CurrentEL, NZCV, DAIF, SPSel to gdbstub Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 1/4] gdbstub/aarch64: add CurrentEL register Manos Pitsidianakis
@ 2025-08-08 11:30 ` Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 3/4] gdbstub/aarch64: add DAIF register Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 4/4] gdbstub/aarch64: add SPSel register Manos Pitsidianakis
  3 siblings, 0 replies; 10+ messages in thread
From: Manos Pitsidianakis @ 2025-08-08 11:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm,
	Manos Pitsidianakis

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 gdb-xml/aarch64-core.xml | 14 +++++++++++++-
 target/arm/gdbstub64.c   |  8 ++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
index 19ad743dc5607b4021fb795bfb9b8e9cf0adef68..2b74b87f908f792c24f76f212e4f7eaf335ddbc2 100644
--- a/gdb-xml/aarch64-core.xml
+++ b/gdb-xml/aarch64-core.xml
@@ -95,5 +95,17 @@
     <!-- Exception Level.  -->
     <field name="EL" start="2" end="3"/>
   </flags>
-  <reg name="CurrentEL" bitsize="64" type="current_el"/>
+  <reg name="CurrentEL" bitsize="64" type="current_el_flags"/>
+
+  <flags id="nzcv_flags" size="8">
+    <!-- Overflow Condition flag.  -->
+    <field name="V" start="28" end="28"/>
+    <!-- Carry Condition flag.  -->
+    <field name="C" start="29" end="29"/>
+    <!-- Zero Condition flag.  -->
+    <field name="Z" start="30" end="30"/>
+    <!-- Negative Condition flag.  -->
+    <field name="N" start="31" end="31"/>
+  </flags>
+  <reg name="NZCV" bitsize="64" type="nzcv_flags"/>
 </feature>
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 16b564e1a970cb5e854a705619f71ffc61545a73..dd3c6222a577efa03753cf07371afdedeefdb771 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -51,6 +51,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     case 34:
         /* CurrentEL */
         return gdb_get_reg64(mem_buf, env->pstate & PSTATE_EL);
+    case 35:
+        /* NZCV */
+        return gdb_get_reg64(mem_buf, pstate_read(env) & PSTATE_NZCV);
     }
     /* Unknown register.  */
     return 0;
@@ -83,6 +86,11 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     case 34:
         /* CurrentEL */
         return 0;
+    case 35:
+        /* NZCV */
+        tmp = (pstate_read(env) & ~PSTATE_NZCV) | (tmp & PSTATE_NZCV);
+        pstate_write(env, tmp);
+        return 8;
     }
     /* Unknown register.  */
     return 0;

-- 
2.47.2



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] gdbstub/aarch64: add DAIF register
  2025-08-08 11:30 [PATCH 0/4] arm: Add PSTATE field registers CurrentEL, NZCV, DAIF, SPSel to gdbstub Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 1/4] gdbstub/aarch64: add CurrentEL register Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 2/4] gdbstub/aarch64: add NZCV register Manos Pitsidianakis
@ 2025-08-08 11:30 ` Manos Pitsidianakis
  2025-08-08 11:30 ` [PATCH 4/4] gdbstub/aarch64: add SPSel register Manos Pitsidianakis
  3 siblings, 0 replies; 10+ messages in thread
From: Manos Pitsidianakis @ 2025-08-08 11:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm,
	Manos Pitsidianakis

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 gdb-xml/aarch64-core.xml | 12 ++++++++++++
 target/arm/gdbstub64.c   |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
index 2b74b87f908f792c24f76f212e4f7eaf335ddbc2..dffc92303fc7b7e8221cf6afd6009101f34352ed 100644
--- a/gdb-xml/aarch64-core.xml
+++ b/gdb-xml/aarch64-core.xml
@@ -108,4 +108,16 @@
     <field name="N" start="31" end="31"/>
   </flags>
   <reg name="NZCV" bitsize="64" type="nzcv_flags"/>
+
+  <flags id="daif_flags" size="8">
+    <!-- FIQ interrupt mask.  -->
+    <field name="F" start="6" end="6"/>
+    <!-- IRQ interrupt mask.  -->
+    <field name="I" start="7" end="7"/>
+    <!-- SError interrupt mask.  -->
+    <field name="A" start="8" end="8"/>
+    <!-- Debug exception mask.  -->
+    <field name="D" start="9" end="9"/>
+  </flags>
+  <reg name="DAIF" bitsize="64" type="daif_flags"/>
 </feature>
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index dd3c6222a577efa03753cf07371afdedeefdb771..6c424ed361e32e12836c6ef00a06397bd684bac4 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -54,6 +54,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     case 35:
         /* NZCV */
         return gdb_get_reg64(mem_buf, pstate_read(env) & PSTATE_NZCV);
+    case 36:
+        /* DAIF */
+        return gdb_get_reg64(mem_buf, env->daif);
     }
     /* Unknown register.  */
     return 0;
@@ -91,6 +94,10 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         tmp = (pstate_read(env) & ~PSTATE_NZCV) | (tmp & PSTATE_NZCV);
         pstate_write(env, tmp);
         return 8;
+    case 36:
+        /* DAIF */
+        env->daif = tmp & PSTATE_DAIF;
+        return 8;
     }
     /* Unknown register.  */
     return 0;

-- 
2.47.2



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] gdbstub/aarch64: add SPSel register
  2025-08-08 11:30 [PATCH 0/4] arm: Add PSTATE field registers CurrentEL, NZCV, DAIF, SPSel to gdbstub Manos Pitsidianakis
                   ` (2 preceding siblings ...)
  2025-08-08 11:30 ` [PATCH 3/4] gdbstub/aarch64: add DAIF register Manos Pitsidianakis
@ 2025-08-08 11:30 ` Manos Pitsidianakis
  3 siblings, 0 replies; 10+ messages in thread
From: Manos Pitsidianakis @ 2025-08-08 11:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm,
	Manos Pitsidianakis

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 gdb-xml/aarch64-core.xml | 6 ++++++
 target/arm/gdbstub64.c   | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
index dffc92303fc7b7e8221cf6afd6009101f34352ed..42793f063d9a9180d6233376a03835238c4a9c53 100644
--- a/gdb-xml/aarch64-core.xml
+++ b/gdb-xml/aarch64-core.xml
@@ -120,4 +120,10 @@
     <field name="D" start="9" end="9"/>
   </flags>
   <reg name="DAIF" bitsize="64" type="daif_flags"/>
+
+  <flags id="spsel_flags" size="8">
+    <!-- Stack Pointer.  -->
+    <field name="SP" start="0" end="0"/>
+  </flags>
+  <reg name="SPSel" bitsize="64" type="spsel_flags"/>
 </feature>
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 6c424ed361e32e12836c6ef00a06397bd684bac4..143e2b104f961da603404cdfb5247b044f0e84cd 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -57,6 +57,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     case 36:
         /* DAIF */
         return gdb_get_reg64(mem_buf, env->daif);
+    case 37:
+        /* SPSel */
+        return gdb_get_reg64(mem_buf, env->pstate & PSTATE_SP);
     }
     /* Unknown register.  */
     return 0;
@@ -98,6 +101,11 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         /* DAIF */
         env->daif = tmp & PSTATE_DAIF;
         return 8;
+    case 37:
+        /* SPSel */
+        tmp = (pstate_read(env) & ~PSTATE_SP) | (tmp & PSTATE_SP);
+        pstate_write(env, tmp);
+        return 8;
     }
     /* Unknown register.  */
     return 0;

-- 
2.47.2



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] gdbstub/aarch64: add CurrentEL register
  2025-08-08 11:30 ` [PATCH 1/4] gdbstub/aarch64: add CurrentEL register Manos Pitsidianakis
@ 2025-08-08 12:21   ` Peter Maydell
  2025-08-08 12:26     ` Manos Pitsidianakis
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2025-08-08 12:21 UTC (permalink / raw)
  To: Manos Pitsidianakis
  Cc: qemu-devel, Alex Bennée, Philippe Mathieu-Daudé,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm

On Fri, 8 Aug 2025 at 12:30, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  gdb-xml/aarch64-core.xml | 5 +++++
>  target/arm/cpu.h         | 1 +
>  target/arm/gdbstub64.c   | 6 ++++++
>  3 files changed, 12 insertions(+)
>
> diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
> index b8046510b9a085d30463d37b3ecc8d435f5fb7a4..19ad743dc5607b4021fb795bfb9b8e9cf0adef68 100644
> --- a/gdb-xml/aarch64-core.xml
> +++ b/gdb-xml/aarch64-core.xml
> @@ -91,4 +91,9 @@
>    </flags>
>    <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
>
> +  <flags id="current_el_flags" size="8">
> +    <!-- Exception Level.  -->
> +    <field name="EL" start="2" end="3"/>
> +  </flags>
> +  <reg name="CurrentEL" bitsize="64" type="current_el"/>
>  </feature>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index dc9b6dce4c92297b2636d0d7c0dce580f1806d5b..c3070cd9863381fac40f5640e0a7a84dfa1c6e06 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1473,6 +1473,7 @@ void pmu_init(ARMCPU *cpu);
>   * AArch32 mode SPSRs are basically CPSR-format.
>   */
>  #define PSTATE_SP (1U)
> +#define PSTATE_EL (3U << 2)
>  #define PSTATE_M (0xFU)
>  #define PSTATE_nRW (1U << 4)
>  #define PSTATE_F (1U << 6)
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 08e28585396816ab90d6d8e460ff8171892fe8da..16b564e1a970cb5e854a705619f71ffc61545a73 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -48,6 +48,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>          return gdb_get_reg64(mem_buf, env->pc);
>      case 33:
>          return gdb_get_reg32(mem_buf, pstate_read(env));
> +    case 34:
> +        /* CurrentEL */
> +        return gdb_get_reg64(mem_buf, env->pstate & PSTATE_EL);
>      }

The debugger already has this information in the 'cpsr'
register, so it could implement convenience views of
the subfields itself if it liked.

If we're going to do this I would prefer it to be because
we've gained some consensus with e.g. the gdb maintainers
that this is the "preferred" way to expose the CPU state.
The XML config stuff lets us do it in our own way if we
want to, but I think there is value in consistency across
stubs here.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] gdbstub/aarch64: add CurrentEL register
  2025-08-08 12:21   ` Peter Maydell
@ 2025-08-08 12:26     ` Manos Pitsidianakis
  2025-08-08 16:11       ` Pierrick Bouvier
  0 siblings, 1 reply; 10+ messages in thread
From: Manos Pitsidianakis @ 2025-08-08 12:26 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-devel, Alex Bennée, Philippe Mathieu-Daudé,
	Pierrick Bouvier, Gustavo Romero, Richard Henderson, qemu-arm

On Fri, Aug 8, 2025 at 3:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 8 Aug 2025 at 12:30, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> >
> > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > ---
> >  gdb-xml/aarch64-core.xml | 5 +++++
> >  target/arm/cpu.h         | 1 +
> >  target/arm/gdbstub64.c   | 6 ++++++
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
> > index b8046510b9a085d30463d37b3ecc8d435f5fb7a4..19ad743dc5607b4021fb795bfb9b8e9cf0adef68 100644
> > --- a/gdb-xml/aarch64-core.xml
> > +++ b/gdb-xml/aarch64-core.xml
> > @@ -91,4 +91,9 @@
> >    </flags>
> >    <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
> >
> > +  <flags id="current_el_flags" size="8">
> > +    <!-- Exception Level.  -->
> > +    <field name="EL" start="2" end="3"/>
> > +  </flags>
> > +  <reg name="CurrentEL" bitsize="64" type="current_el"/>
> >  </feature>
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index dc9b6dce4c92297b2636d0d7c0dce580f1806d5b..c3070cd9863381fac40f5640e0a7a84dfa1c6e06 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1473,6 +1473,7 @@ void pmu_init(ARMCPU *cpu);
> >   * AArch32 mode SPSRs are basically CPSR-format.
> >   */
> >  #define PSTATE_SP (1U)
> > +#define PSTATE_EL (3U << 2)
> >  #define PSTATE_M (0xFU)
> >  #define PSTATE_nRW (1U << 4)
> >  #define PSTATE_F (1U << 6)
> > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> > index 08e28585396816ab90d6d8e460ff8171892fe8da..16b564e1a970cb5e854a705619f71ffc61545a73 100644
> > --- a/target/arm/gdbstub64.c
> > +++ b/target/arm/gdbstub64.c
> > @@ -48,6 +48,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> >          return gdb_get_reg64(mem_buf, env->pc);
> >      case 33:
> >          return gdb_get_reg32(mem_buf, pstate_read(env));
> > +    case 34:
> > +        /* CurrentEL */
> > +        return gdb_get_reg64(mem_buf, env->pstate & PSTATE_EL);
> >      }
>
> The debugger already has this information in the 'cpsr'
> register, so it could implement convenience views of
> the subfields itself if it liked.

Yep, but consider: it is a register, architecturally, so it's nice to
include it for consistency. It's redundant only because gdb has cpsr
which is not a register. So this is about more about being technically
correct than correcting an actual problem.


>
> If we're going to do this I would prefer it to be because
> we've gained some consensus with e.g. the gdb maintainers
> that this is the "preferred" way to expose the CPU state.
> The XML config stuff lets us do it in our own way if we
> want to, but I think there is value in consistency across
> stubs here.

I plan to upstream the XML patches to gdb as well, separately. But
since we use custom target xml it's not like we depend on gdb to
change it.

>
> thanks
> -- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] gdbstub/aarch64: add CurrentEL register
  2025-08-08 12:26     ` Manos Pitsidianakis
@ 2025-08-08 16:11       ` Pierrick Bouvier
  2025-08-08 16:14         ` Peter Maydell
  0 siblings, 1 reply; 10+ messages in thread
From: Pierrick Bouvier @ 2025-08-08 16:11 UTC (permalink / raw)
  To: Manos Pitsidianakis, Peter Maydell
  Cc: qemu-devel, Alex Bennée, Philippe Mathieu-Daudé,
	Gustavo Romero, Richard Henderson, qemu-arm

On 8/8/25 5:26 AM, Manos Pitsidianakis wrote:
> On Fri, Aug 8, 2025 at 3:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 8 Aug 2025 at 12:30, Manos Pitsidianakis
>> <manos.pitsidianakis@linaro.org> wrote:
>>>
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> ---
>>>   gdb-xml/aarch64-core.xml | 5 +++++
>>>   target/arm/cpu.h         | 1 +
>>>   target/arm/gdbstub64.c   | 6 ++++++
>>>   3 files changed, 12 insertions(+)
>>>
>>> diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
>>> index b8046510b9a085d30463d37b3ecc8d435f5fb7a4..19ad743dc5607b4021fb795bfb9b8e9cf0adef68 100644
>>> --- a/gdb-xml/aarch64-core.xml
>>> +++ b/gdb-xml/aarch64-core.xml
>>> @@ -91,4 +91,9 @@
>>>     </flags>
>>>     <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
>>>
>>> +  <flags id="current_el_flags" size="8">
>>> +    <!-- Exception Level.  -->
>>> +    <field name="EL" start="2" end="3"/>
>>> +  </flags>
>>> +  <reg name="CurrentEL" bitsize="64" type="current_el"/>
>>>   </feature>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index dc9b6dce4c92297b2636d0d7c0dce580f1806d5b..c3070cd9863381fac40f5640e0a7a84dfa1c6e06 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -1473,6 +1473,7 @@ void pmu_init(ARMCPU *cpu);
>>>    * AArch32 mode SPSRs are basically CPSR-format.
>>>    */
>>>   #define PSTATE_SP (1U)
>>> +#define PSTATE_EL (3U << 2)
>>>   #define PSTATE_M (0xFU)
>>>   #define PSTATE_nRW (1U << 4)
>>>   #define PSTATE_F (1U << 6)
>>> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
>>> index 08e28585396816ab90d6d8e460ff8171892fe8da..16b564e1a970cb5e854a705619f71ffc61545a73 100644
>>> --- a/target/arm/gdbstub64.c
>>> +++ b/target/arm/gdbstub64.c
>>> @@ -48,6 +48,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>>           return gdb_get_reg64(mem_buf, env->pc);
>>>       case 33:
>>>           return gdb_get_reg32(mem_buf, pstate_read(env));
>>> +    case 34:
>>> +        /* CurrentEL */
>>> +        return gdb_get_reg64(mem_buf, env->pstate & PSTATE_EL);
>>>       }
>>
>> The debugger already has this information in the 'cpsr'
>> register, so it could implement convenience views of
>> the subfields itself if it liked.
> 
> Yep, but consider: it is a register, architecturally, so it's nice to
> include it for consistency. It's redundant only because gdb has cpsr
> which is not a register. So this is about more about being technically
> correct than correcting an actual problem.
> 

I agree with Manos on this.
As mentioned on a previous thread, cpsr is not even supposed to exist 
for aarch64. So adding architecturally defined registers, even if data 
is redundant with cpsr, should not be a problem.
I'm sure gdb folks can understand this too.

> 
>>
>> If we're going to do this I would prefer it to be because
>> we've gained some consensus with e.g. the gdb maintainers
>> that this is the "preferred" way to expose the CPU state.
>> The XML config stuff lets us do it in our own way if we
>> want to, but I think there is value in consistency across
>> stubs here.
> 
> I plan to upstream the XML patches to gdb as well, separately. But
> since we use custom target xml it's not like we depend on gdb to
> change it.
> 
>>
>> thanks
>> -- PMM



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] gdbstub/aarch64: add CurrentEL register
  2025-08-08 16:11       ` Pierrick Bouvier
@ 2025-08-08 16:14         ` Peter Maydell
  2025-08-08 17:14           ` Pierrick Bouvier
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2025-08-08 16:14 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: Manos Pitsidianakis, qemu-devel, Alex Bennée,
	Philippe Mathieu-Daudé, Gustavo Romero, Richard Henderson,
	qemu-arm

On Fri, 8 Aug 2025 at 17:11, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 8/8/25 5:26 AM, Manos Pitsidianakis wrote:
> > On Fri, Aug 8, 2025 at 3:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Fri, 8 Aug 2025 at 12:30, Manos Pitsidianakis
> >> <manos.pitsidianakis@linaro.org> wrote:
> >> The debugger already has this information in the 'cpsr'
> >> register, so it could implement convenience views of
> >> the subfields itself if it liked.
> >
> > Yep, but consider: it is a register, architecturally, so it's nice to
> > include it for consistency. It's redundant only because gdb has cpsr
> > which is not a register. So this is about more about being technically
> > correct than correcting an actual problem.
> >
>
> I agree with Manos on this.
> As mentioned on a previous thread, cpsr is not even supposed to exist
> for aarch64. So adding architecturally defined registers, even if data
> is redundant with cpsr, should not be a problem.
> I'm sure gdb folks can understand this too.

I'm not saying this is the wrong way to represent this.
I'm just saying we're not the only gdbstub in the world,
and it would be nice to have a wider discussion than just
QEMU folks so we are consistent about how we represent
PSTATE (including what we want to do about the new
bits that appear in the high 32 bits of an SPSR), before
we commit to any particular direction.

-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] gdbstub/aarch64: add CurrentEL register
  2025-08-08 16:14         ` Peter Maydell
@ 2025-08-08 17:14           ` Pierrick Bouvier
  0 siblings, 0 replies; 10+ messages in thread
From: Pierrick Bouvier @ 2025-08-08 17:14 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Manos Pitsidianakis, qemu-devel, Alex Bennée,
	Philippe Mathieu-Daudé, Gustavo Romero, Richard Henderson,
	qemu-arm

On 8/8/25 9:14 AM, Peter Maydell wrote:
> On Fri, 8 Aug 2025 at 17:11, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 8/8/25 5:26 AM, Manos Pitsidianakis wrote:
>>> On Fri, Aug 8, 2025 at 3:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Fri, 8 Aug 2025 at 12:30, Manos Pitsidianakis
>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>> The debugger already has this information in the 'cpsr'
>>>> register, so it could implement convenience views of
>>>> the subfields itself if it liked.
>>>
>>> Yep, but consider: it is a register, architecturally, so it's nice to
>>> include it for consistency. It's redundant only because gdb has cpsr
>>> which is not a register. So this is about more about being technically
>>> correct than correcting an actual problem.
>>>
>>
>> I agree with Manos on this.
>> As mentioned on a previous thread, cpsr is not even supposed to exist
>> for aarch64. So adding architecturally defined registers, even if data
>> is redundant with cpsr, should not be a problem.
>> I'm sure gdb folks can understand this too.
> 
> I'm not saying this is the wrong way to represent this.
> I'm just saying we're not the only gdbstub in the world,
> and it would be nice to have a wider discussion than just
> QEMU folks so we are consistent about how we represent
> PSTATE (including what we want to do about the new
> bits that appear in the high 32 bits of an SPSR), before
> we commit to any particular direction.
>

Considering we have our own set of gdb xml, is that really important to 
agree on pstate layout before we simply make those registers visible on 
our side?
The new registers added in this series on gdb/kgdb side at the moment, 
so we don't really break anything.

I agree it would be good to see this on gdb side, but my point is that 
we are not necessarily stuck and we can make this visible without 
waiting two releases. As well, it would be a good motivation to add this 
on gdb showing QEMU already exposes this.

> -- PMM



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-08-08 17:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-08 11:30 [PATCH 0/4] arm: Add PSTATE field registers CurrentEL, NZCV, DAIF, SPSel to gdbstub Manos Pitsidianakis
2025-08-08 11:30 ` [PATCH 1/4] gdbstub/aarch64: add CurrentEL register Manos Pitsidianakis
2025-08-08 12:21   ` Peter Maydell
2025-08-08 12:26     ` Manos Pitsidianakis
2025-08-08 16:11       ` Pierrick Bouvier
2025-08-08 16:14         ` Peter Maydell
2025-08-08 17:14           ` Pierrick Bouvier
2025-08-08 11:30 ` [PATCH 2/4] gdbstub/aarch64: add NZCV register Manos Pitsidianakis
2025-08-08 11:30 ` [PATCH 3/4] gdbstub/aarch64: add DAIF register Manos Pitsidianakis
2025-08-08 11:30 ` [PATCH 4/4] gdbstub/aarch64: add SPSel register Manos Pitsidianakis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).