[PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Valentin Ghita]

The DEBUGEN bit is set by the debugger when it is connected to the
core.  Software can use this bit to check if a debug session is active.

Add a function in gdbstub to check if the debugger is attached to a CPU
and use this information when the DHCSR register is read in armv7m_nvic.

Signed-off-by: Valentin Ghita <valentinghita@google.com>
---
 gdbstub.c              | 10 ++++++++++
 hw/intc/armv7m_nvic.c  |  4 ++++
 include/exec/gdbstub.h |  6 ++++++
 3 files changed, 20 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 3c14c6a038..d4e39db8e7 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -3585,6 +3585,16 @@ int gdbserver_start(const char *device)
     return 0;
 }
 
+bool gdb_attached(CPUState *cpu)
+{
+    GDBProcess *process = gdb_get_cpu_process(cpu);
+    if (process != NULL) {
+        return process->attached;
+    }
+
+    return false;
+}
+
 static void register_types(void)
 {
     type_register_static(&char_gdb_type_info);
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 13df002ce4..d6fff94bca 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -21,6 +21,7 @@
 #include "sysemu/runstate.h"
 #include "target/arm/cpu.h"
 #include "exec/exec-all.h"
+#include "exec/gdbstub.h"
 #include "exec/memop.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -1510,6 +1511,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         }
         /* We provide minimal-RAS only: RFSR is RAZ/WI */
         return 0;
+    case 0xdf0: /* DHCSR */
+        /* Bit 0: DEBUGEN. */
+        return gdb_attached(CPU(cpu)) ? 1 : 0;
     case 0xf34: /* FPCCR */
         if (!cpu_isar_feature(aa32_vfp_simd, cpu)) {
             return 0;
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index a024a0350d..383f4e5224 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -177,6 +177,12 @@ static inline uint8_t * gdb_get_reg_ptr(GByteArray *buf, int len)
  */
 int gdbserver_start(const char *port_or_device);
 
+/**
+ * gdb_attached: check if GDB is attached to a given CPU.
+ * @cpu: the CPU to check if GDB is attached to.
+ */
+bool gdb_attached(CPUState *cpu);
+
 /**
  * gdb_has_xml:
  * This is an ugly hack to cope with both new and old gdb.
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Peter Maydell]

On Thu, 3 Feb 2022 at 15:53, Valentin Ghita <valentinghita@google.com> wrote:
>
> The DEBUGEN bit is set by the debugger when it is connected to the
> core.  Software can use this bit to check if a debug session is active.
>
> Add a function in gdbstub to check if the debugger is attached to a CPU
> and use this information when the DHCSR register is read in armv7m_nvic.

The DHCSR.C_DEBUGEN bit is for the architected Halting Debug, where
an external debugger uses the debug access port to control and
debug the CPU. This is conceptually similar to QEMU's gdb debugstub,
but not the same thing.

QEMU doesn't implement Halting Debug (this is architecturally
permitted), and so making this bit RAZ/WI is correct for us.

thanks
-- PMM

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Alex Bennée]

Valentin Ghita <valentinghita@google.com> writes:

> The DEBUGEN bit is set by the debugger when it is connected to the
> core.  Software can use this bit to check if a debug session is active.
>
> Add a function in gdbstub to check if the debugger is attached to a CPU
> and use this information when the DHCSR register is read in armv7m_nvic.
>
> Signed-off-by: Valentin Ghita <valentinghita@google.com>

Nack - use of the gdbstub should be transparent to the guest. It is not
trying to model any real JTAG/External debug hardware here.

> ---
>  gdbstub.c              | 10 ++++++++++
>  hw/intc/armv7m_nvic.c  |  4 ++++
>  include/exec/gdbstub.h |  6 ++++++
>  3 files changed, 20 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 3c14c6a038..d4e39db8e7 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3585,6 +3585,16 @@ int gdbserver_start(const char *device)
>      return 0;
>  }
>  
> +bool gdb_attached(CPUState *cpu)
> +{
> +    GDBProcess *process = gdb_get_cpu_process(cpu);
> +    if (process != NULL) {
> +        return process->attached;
> +    }
> +
> +    return false;
> +}
> +
>  static void register_types(void)
>  {
>      type_register_static(&char_gdb_type_info);
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 13df002ce4..d6fff94bca 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -21,6 +21,7 @@
>  #include "sysemu/runstate.h"
>  #include "target/arm/cpu.h"
>  #include "exec/exec-all.h"
> +#include "exec/gdbstub.h"
>  #include "exec/memop.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -1510,6 +1511,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>          }
>          /* We provide minimal-RAS only: RFSR is RAZ/WI */
>          return 0;
> +    case 0xdf0: /* DHCSR */
> +        /* Bit 0: DEBUGEN. */
> +        return gdb_attached(CPU(cpu)) ? 1 : 0;
>      case 0xf34: /* FPCCR */
>          if (!cpu_isar_feature(aa32_vfp_simd, cpu)) {
>              return 0;
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index a024a0350d..383f4e5224 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -177,6 +177,12 @@ static inline uint8_t * gdb_get_reg_ptr(GByteArray *buf, int len)
>   */
>  int gdbserver_start(const char *port_or_device);
>  
> +/**
> + * gdb_attached: check if GDB is attached to a given CPU.
> + * @cpu: the CPU to check if GDB is attached to.
> + */
> +bool gdb_attached(CPUState *cpu);
> +
>  /**
>   * gdb_has_xml:
>   * This is an ugly hack to cope with both new and old gdb.


-- 
Alex Bennée

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Valentin Ghita]

[-- Attachment #1: Type: text/plain, Size: 3217 bytes --]

On Thu, Feb 3, 2022 at 7:42 PM Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Valentin Ghita <valentinghita@google.com> writes:
>
> > The DEBUGEN bit is set by the debugger when it is connected to the
> > core.  Software can use this bit to check if a debug session is active.
> >
> > Add a function in gdbstub to check if the debugger is attached to a CPU
> > and use this information when the DHCSR register is read in armv7m_nvic.
> >
> > Signed-off-by: Valentin Ghita <valentinghita@google.com>
>
> Nack - use of the gdbstub should be transparent to the guest. It is not
> trying to model any real JTAG/External debug hardware here.
>
>
The goal was to support the following piece of code:

if (DHCSR.C_DEBUGEN) {
    __asm ("bkpt");
}

And I have another patch which handles the bkpt exception when the DEBUGEN
bit is set, by interrupting the CPU.

Any suggestions on how we can achieve this?

Thank you,
Valentin.

> ---
> >  gdbstub.c              | 10 ++++++++++
> >  hw/intc/armv7m_nvic.c  |  4 ++++
> >  include/exec/gdbstub.h |  6 ++++++
> >  3 files changed, 20 insertions(+)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 3c14c6a038..d4e39db8e7 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -3585,6 +3585,16 @@ int gdbserver_start(const char *device)
> >      return 0;
> >  }
> >
> > +bool gdb_attached(CPUState *cpu)
> > +{
> > +    GDBProcess *process = gdb_get_cpu_process(cpu);
> > +    if (process != NULL) {
> > +        return process->attached;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static void register_types(void)
> >  {
> >      type_register_static(&char_gdb_type_info);
> > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> > index 13df002ce4..d6fff94bca 100644
> > --- a/hw/intc/armv7m_nvic.c
> > +++ b/hw/intc/armv7m_nvic.c
> > @@ -21,6 +21,7 @@
> >  #include "sysemu/runstate.h"
> >  #include "target/arm/cpu.h"
> >  #include "exec/exec-all.h"
> > +#include "exec/gdbstub.h"
> >  #include "exec/memop.h"
> >  #include "qemu/log.h"
> >  #include "qemu/module.h"
> > @@ -1510,6 +1511,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t
> offset, MemTxAttrs attrs)
> >          }
> >          /* We provide minimal-RAS only: RFSR is RAZ/WI */
> >          return 0;
> > +    case 0xdf0: /* DHCSR */
> > +        /* Bit 0: DEBUGEN. */
> > +        return gdb_attached(CPU(cpu)) ? 1 : 0;
> >      case 0xf34: /* FPCCR */
> >          if (!cpu_isar_feature(aa32_vfp_simd, cpu)) {
> >              return 0;
> > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> > index a024a0350d..383f4e5224 100644
> > --- a/include/exec/gdbstub.h
> > +++ b/include/exec/gdbstub.h
> > @@ -177,6 +177,12 @@ static inline uint8_t * gdb_get_reg_ptr(GByteArray
> *buf, int len)
> >   */
> >  int gdbserver_start(const char *port_or_device);
> >
> > +/**
> > + * gdb_attached: check if GDB is attached to a given CPU.
> > + * @cpu: the CPU to check if GDB is attached to.
> > + */
> > +bool gdb_attached(CPUState *cpu);
> > +
> >  /**
> >   * gdb_has_xml:
> >   * This is an ugly hack to cope with both new and old gdb.
>
>
> --
> Alex Bennée
>

[-- Attachment #2: Type: text/html, Size: 4522 bytes --]

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Alex Bennée]

Valentin Ghita <valentinghita@google.com> writes:

> On Thu, Feb 3, 2022 at 7:42 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  Valentin Ghita <valentinghita@google.com> writes:
>
>  > The DEBUGEN bit is set by the debugger when it is connected to the
>  > core.  Software can use this bit to check if a debug session is active.
>  >
>  > Add a function in gdbstub to check if the debugger is attached to a CPU
>  > and use this information when the DHCSR register is read in armv7m_nvic.
>  >
>  > Signed-off-by: Valentin Ghita <valentinghita@google.com>
>
>  Nack - use of the gdbstub should be transparent to the guest. It is not
>  trying to model any real JTAG/External debug hardware here.
>
> The goal was to support the following piece of code:
>
> if (DHCSR.C_DEBUGEN) {
>     __asm ("bkpt");
> }
>  
> And I have another patch which handles the bkpt exception when the DEBUGEN
> bit is set, by interrupting the CPU.
>
> Any suggestions on how we can achieve this?

Assuming you are happy for the device to act as though a external
debugger is attached regardless of the gdbstub state you could use a CPU
property on the command line to enable this behaviour. We have some
examples for SVE for the 64 bit CPUs (see object_property_add for
sve-max-vq). So something like:

  -cpu cortex-m3,dhscr=true

You would probably want to model the behaviour of DHSCR.C_HALT as well
because that is something the core might do to itself if it detects it
is running under debug.

For completeness you might want to model the read values from the point
of view of the gdbstub although it wouldn't provide any view into the
system you can't already see as far as I can tell.

>
> Thank you,
> Valentin.
>
>  > ---
>  >  gdbstub.c              | 10 ++++++++++
>  >  hw/intc/armv7m_nvic.c  |  4 ++++
>  >  include/exec/gdbstub.h |  6 ++++++
>  >  3 files changed, 20 insertions(+)
>  >
>  > diff --git a/gdbstub.c b/gdbstub.c
>  > index 3c14c6a038..d4e39db8e7 100644
>  > --- a/gdbstub.c
>  > +++ b/gdbstub.c
>  > @@ -3585,6 +3585,16 @@ int gdbserver_start(const char *device)
>  >      return 0;
>  >  }
>  >  
>  > +bool gdb_attached(CPUState *cpu)
>  > +{
>  > +    GDBProcess *process = gdb_get_cpu_process(cpu);
>  > +    if (process != NULL) {
>  > +        return process->attached;
>  > +    }
>  > +
>  > +    return false;
>  > +}
>  > +
>  >  static void register_types(void)
>  >  {
>  >      type_register_static(&char_gdb_type_info);
>  > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>  > index 13df002ce4..d6fff94bca 100644
>  > --- a/hw/intc/armv7m_nvic.c
>  > +++ b/hw/intc/armv7m_nvic.c
>  > @@ -21,6 +21,7 @@
>  >  #include "sysemu/runstate.h"
>  >  #include "target/arm/cpu.h"
>  >  #include "exec/exec-all.h"
>  > +#include "exec/gdbstub.h"
>  >  #include "exec/memop.h"
>  >  #include "qemu/log.h"
>  >  #include "qemu/module.h"
>  > @@ -1510,6 +1511,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>  >          }
>  >          /* We provide minimal-RAS only: RFSR is RAZ/WI */
>  >          return 0;
>  > +    case 0xdf0: /* DHCSR */
>  > +        /* Bit 0: DEBUGEN. */
>  > +        return gdb_attached(CPU(cpu)) ? 1 : 0;
>  >      case 0xf34: /* FPCCR */
>  >          if (!cpu_isar_feature(aa32_vfp_simd, cpu)) {
>  >              return 0;
>  > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
>  > index a024a0350d..383f4e5224 100644
>  > --- a/include/exec/gdbstub.h
>  > +++ b/include/exec/gdbstub.h
>  > @@ -177,6 +177,12 @@ static inline uint8_t * gdb_get_reg_ptr(GByteArray *buf, int len)
>  >   */
>  >  int gdbserver_start(const char *port_or_device);
>  >  
>  > +/**
>  > + * gdb_attached: check if GDB is attached to a given CPU.
>  > + * @cpu: the CPU to check if GDB is attached to.
>  > + */
>  > +bool gdb_attached(CPUState *cpu);
>  > +
>  >  /**
>  >   * gdb_has_xml:
>  >   * This is an ugly hack to cope with both new and old gdb.
>
>  -- 
>  Alex Bennée


-- 
Alex Bennée

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Peter Maydell]

On Fri, 4 Feb 2022 at 09:28, Alex Bennée <alex.bennee@linaro.org> wrote:
> Assuming you are happy for the device to act as though a external
> debugger is attached regardless of the gdbstub state you could use a CPU
> property on the command line to enable this behaviour. We have some
> examples for SVE for the 64 bit CPUs (see object_property_add for
> sve-max-vq). So something like:
>
>   -cpu cortex-m3,dhscr=true
>
> You would probably want to model the behaviour of DHSCR.C_HALT as well
> because that is something the core might do to itself if it detects it
> is running under debug.

This is sounding pretty hacky to me. I think we should either have
a proper implementation of all of halting debug (probably opt-in,
with the default being that the gdbstub is transparent to the guest),
or we should just say that no, this isn't something we support,
and if you want gdb to get control when a particular bit of code
is executed then you should set a breakpoint there.

We don't even implement the guest-visible debug parts of the
architecture (eg architected single-step) yet, incidentally.

-- PMM

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 4 Feb 2022 at 09:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Assuming you are happy for the device to act as though a external
>> debugger is attached regardless of the gdbstub state you could use a CPU
>> property on the command line to enable this behaviour. We have some
>> examples for SVE for the 64 bit CPUs (see object_property_add for
>> sve-max-vq). So something like:
>>
>>   -cpu cortex-m3,dhscr=true
>>
>> You would probably want to model the behaviour of DHSCR.C_HALT as well
>> because that is something the core might do to itself if it detects it
>> is running under debug.
>
> This is sounding pretty hacky to me. I think we should either have
> a proper implementation of all of halting debug (probably opt-in,
> with the default being that the gdbstub is transparent to the guest),

So we could flip it and make it a property of gdbstub with transparency
being the default. Then any architecture that wanted to have this
behaviour could query the stub if enabled.

> or we should just say that no, this isn't something we support,
> and if you want gdb to get control when a particular bit of code
> is executed then you should set a breakpoint there.

It's a fairly niche use case but I don't see why we shouldn't assuming
someone is willing to write the code. However I suspect there is quite a
wide range of potential behaviours to model.

> We don't even implement the guest-visible debug parts of the
> architecture (eg architected single-step) yet, incidentally.

Is this just for Aarch32? Because for Aarch64 as far as I'm aware the
v8.0 debug works fine modulo bugs which I sent a fix for:

  Subject: [RFC PATCH] arm: force flag recalculation when messing with DAIF
  Date: Wed,  2 Feb 2022 12:23:53 +0000
  Message-Id: <20220202122353.457084-1-alex.bennee@linaro.org>

Implementing more of the debug functionality is on the list for
supporting Neoverse. We don't currently have a plan for how to deal with
the slowly growing series of PMU features though.

>
> -- PMM


-- 
Alex Bennée

Re: [PATCH] armv7m_nvic: set DHCSR.DEBUGEN when debugger is attached

[Peter Maydell]

On Fri, 4 Feb 2022 at 12:41, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 4 Feb 2022 at 09:28, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> Assuming you are happy for the device to act as though a external
> >> debugger is attached regardless of the gdbstub state you could use a CPU
> >> property on the command line to enable this behaviour. We have some
> >> examples for SVE for the 64 bit CPUs (see object_property_add for
> >> sve-max-vq). So something like:
> >>
> >>   -cpu cortex-m3,dhscr=true
> >>
> >> You would probably want to model the behaviour of DHSCR.C_HALT as well
> >> because that is something the core might do to itself if it detects it
> >> is running under debug.
> >
> > This is sounding pretty hacky to me. I think we should either have
> > a proper implementation of all of halting debug (probably opt-in,
> > with the default being that the gdbstub is transparent to the guest),
>
> So we could flip it and make it a property of gdbstub with transparency
> being the default. Then any architecture that wanted to have this
> behaviour could query the stub if enabled.

I don't particularly mind how we expose it to the user. But I
do think that we should either not implement this architectural
feature at all or we should implement it properly. "Just this
one tiny part that happens to be the bit this one user wanted"
isn't a long-term sensible place to land IMHO.

> > or we should just say that no, this isn't something we support,
> > and if you want gdb to get control when a particular bit of code
> > is executed then you should set a breakpoint there.
>
> It's a fairly niche use case but I don't see why we shouldn't assuming
> someone is willing to write the code. However I suspect there is quite a
> wide range of potential behaviours to model.
>
> > We don't even implement the guest-visible debug parts of the
> > architecture (eg architected single-step) yet, incidentally.
>
> Is this just for Aarch32? Because for Aarch64 as far as I'm aware the
> v8.0 debug works fine modulo bugs which I sent a fix for:

This is M-profile specifically. It has guest-controllable
single-step but the mechanism is (as usual with M-profile)
completely different from the A-profile equivalent.

-- PMM