[PATCH RFC 0/3] Add DCC uart console support

[PATCH RFC 0/3] Add DCC uart console support

[Sai Pavan Boddu]

This patch series attaches a chardev to  arm Debug Communication channel registers,
As each cpu has it own DBGDTRTX/RX register just tried to hook each cpu
with chardev iff we find a chardev with id "dcc<cpu-id>".

ex:
    Below chardev switch would be connected to A53-0.

    ./qemu-system-aarch64 -M xlnx-zcu102 -kernel u-boot-dtb.bin
      -dtb zynqmp-zcu102-rev1.1.d -display none -m 2G
      -chardev stdio,id=dcc0


Sai Pavan Boddu (3):
  target/arm: Add dcc uart support
  target/arm: Enable dcc console for a53 and R5
  target/arm/debug_helper: Add fieldoffset for MDCCSR_EL0 reg

 target/arm/cpu.h          | 11 +++++
 target/arm/internals.h    |  4 ++
 target/arm/cpu64.c        |  1 +
 target/arm/debug-dcc.c    | 99 +++++++++++++++++++++++++++++++++++++++
 target/arm/debug_helper.c |  2 +-
 target/arm/helper.c       |  3 ++
 target/arm/tcg/cpu32.c    |  1 +
 target/arm/meson.build    |  1 +
 8 files changed, 121 insertions(+), 1 deletion(-)
 create mode 100644 target/arm/debug-dcc.c

-- 
2.34.1

[PATCH RFC 1/3] target/arm: Add dcc uart support

[Sai Pavan Boddu]

DCC is a debug port to transfer some data between debugger and
processor, we are using this feature to connect a chardev device.
Chardev frontends should be named as "dcc<cpu-index>" inorder to connect
to this interface.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
---
 target/arm/cpu.h       | 11 +++++
 target/arm/internals.h |  4 ++
 target/arm/debug-dcc.c | 99 ++++++++++++++++++++++++++++++++++++++++++
 target/arm/helper.c    |  3 ++
 target/arm/meson.build |  1 +
 5 files changed, 118 insertions(+)
 create mode 100644 target/arm/debug-dcc.c

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3841359d0f..6b3cb8e70e 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -30,6 +30,8 @@
 #include "qapi/qapi-types-common.h"
 #include "target/arm/multiprocessing.h"
 #include "target/arm/gtimer.h"
+#include "chardev/char.h"
+#include "chardev/char-fe.h"
 
 #ifdef TARGET_AARCH64
 #define KVM_HAVE_MCE_INJECTION 1
@@ -523,6 +525,11 @@ typedef struct CPUArchState {
 
         /* NV2 register */
         uint64_t vncr_el2;
+        /*
+         * Debug Trace regsiters
+         */
+        uint32_t dbgdtr_tx;
+        uint32_t dbgdtr_rx;
     } cp15;
 
     struct {
@@ -1097,6 +1104,9 @@ struct ArchCPU {
 
     /* Generic timer counter frequency, in Hz */
     uint64_t gt_cntfrq_hz;
+
+    /* dcc chardev */
+    CharBackend dcc;
 };
 
 typedef struct ARMCPUInfo {
@@ -2388,6 +2398,7 @@ enum arm_features {
      * CPU types added in future.
      */
     ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */
+    ARM_FEATURE_DCC,
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 11b5da2562..2fa797c5df 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1778,4 +1778,8 @@ uint64_t gt_get_countervalue(CPUARMState *env);
  * and CNTVCT_EL0 (this will be either 0 or the value of CNTVOFF_EL2).
  */
 uint64_t gt_virt_cnt_offset(CPUARMState *env);
+/*
+ * Initialise Coresight Debug interface
+ */
+void arm_dcc_init(ARMCPU *cpu);
 #endif
diff --git a/target/arm/debug-dcc.c b/target/arm/debug-dcc.c
new file mode 100644
index 0000000000..9144b54994
--- /dev/null
+++ b/target/arm/debug-dcc.c
@@ -0,0 +1,99 @@
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "internals.h"
+#include "cpregs.h"
+#include "chardev/char-fe.h"
+
+#define MDCCSR_EL0_RXFULL_MASK (1 << 30)
+#define MDCCSR_EL0_TXFULL_MASK (1 << 29)
+
+static void debug_dcc_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                        uint64_t value)
+{
+    ARMCPU *cpu = ri->opaque;
+    env->cp15.dbgdtr_tx = value;
+
+    if (qemu_chr_fe_get_driver(&cpu->dcc)) {
+        /*
+         * Usually dcc is used for putc/getc calls which expect only
+         * 1 byte from external debugger.
+         * TODO: This needs to be generalized for other use-cases.
+         */
+        qemu_chr_fe_write_all(&cpu->dcc, (uint8_t *)&env->cp15.dbgdtr_tx, 1);
+    }
+}
+
+static uint64_t debug_dcc_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint32_t ret = 0;
+    ARMCPU *cpu = ri->opaque;
+
+    if (env->cp15.mdscr_el1 & MDCCSR_EL0_RXFULL_MASK) {
+        ret = env->cp15.dbgdtr_rx;
+        env->cp15.dbgdtr_rx = 0;
+        env->cp15.mdscr_el1 &= ~MDCCSR_EL0_RXFULL_MASK;
+        qemu_chr_fe_accept_input(&cpu->dcc);
+    }
+    return ret;
+}
+
+static const ARMCPRegInfo dcc_cp_reginfo[] = {
+    /* DBGDTRTX_EL0/DBGDTRRX_EL0 depend on direction */
+    { .name = "DBGDTR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 5, .opc2 = 0,
+      .access = PL0_RW, .writefn = debug_dcc_write,
+      .readfn = debug_dcc_read,
+      .type = ARM_CP_OVERRIDE, .resetvalue = 0 },
+    /* DBGDTRTXint/DBGDTRRXint depend on direction */
+    { .name = "DBGDTRint", .state = ARM_CP_STATE_AA32, .cp = 14,
+      .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
+      .access = PL0_RW, .writefn = debug_dcc_write,
+      .readfn = debug_dcc_read,
+      .type = ARM_CP_OVERRIDE, .resetvalue = 0 },
+};
+
+
+static int dcc_chr_can_read(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    CPUARMState *env = &cpu->env;
+
+    if (!(env->cp15.mdscr_el1 & MDCCSR_EL0_RXFULL_MASK)) {
+        /*
+         * Usually dcc is used for putc/getc calls which expect only
+         * 1 byte from external debugger.
+         * TODO: This needs to be generalized for other use-cases.
+         */
+        return 1;
+    }
+
+    return 0;
+}
+
+static void dcc_chr_read(void *opaque, const uint8_t *buf, int size)
+{
+    ARMCPU *cpu = opaque;
+    CPUARMState *env = &cpu->env;
+
+    env->cp15.dbgdtr_rx = *buf;
+    env->cp15.mdscr_el1 |= MDCCSR_EL0_RXFULL_MASK;
+}
+
+void arm_dcc_init(ARMCPU *cpu)
+{
+    Chardev *chr;
+    char *dcc_name;
+    CPUState *p = CPU(cpu);
+
+    dcc_name = g_strdup_printf("dcc%d", p->cpu_index);
+    chr = qemu_chr_find(dcc_name);
+    define_arm_cp_regs_with_opaque(cpu, dcc_cp_reginfo, cpu);
+    if (chr) {
+        qemu_chr_fe_init(&cpu->dcc, chr, NULL);
+        qemu_chr_fe_set_handlers(&cpu->dcc,
+                      dcc_chr_can_read,
+                      dcc_chr_read,
+                      NULL, NULL, cpu, NULL, true);
+    }
+    g_free(dcc_name);
+}
diff --git a/target/arm/helper.c b/target/arm/helper.c
index ce31957235..2b594f91cb 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9268,6 +9268,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         }
     }
 
+    if (arm_feature(&cpu->env, ARM_FEATURE_DCC)) {
+        arm_dcc_init(cpu);
+    }
     /*
      * Register the base EL2 cpregs.
      * Pre v8, these registers are implemented only as part of the
diff --git a/target/arm/meson.build b/target/arm/meson.build
index 2e10464dbb..3ee38c6b45 100644
--- a/target/arm/meson.build
+++ b/target/arm/meson.build
@@ -5,6 +5,7 @@ arm_ss.add(files(
   'gdbstub.c',
   'helper.c',
   'vfp_helper.c',
+  'debug-dcc.c',
 ))
 arm_ss.add(zlib)
 
-- 
2.34.1

[PATCH RFC 2/3] target/arm: Enable dcc console for a53 and R5

[Sai Pavan Boddu]

This enabled DCC support.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
---
 target/arm/cpu64.c     | 1 +
 target/arm/tcg/cpu32.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 262a1d6c0b..e39740303b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -667,6 +667,7 @@ static void aarch64_a53_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_EL2);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     set_feature(&cpu->env, ARM_FEATURE_PMU);
+    set_feature(&cpu->env, ARM_FEATURE_DCC);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
     cpu->midr = 0x410fd034;
     cpu->revidr = 0x00000100;
diff --git a/target/arm/tcg/cpu32.c b/target/arm/tcg/cpu32.c
index bdd82d912a..b0ef51a9bf 100644
--- a/target/arm/tcg/cpu32.c
+++ b/target/arm/tcg/cpu32.c
@@ -562,6 +562,7 @@ static void cortex_r5_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7MP);
     set_feature(&cpu->env, ARM_FEATURE_PMSA);
     set_feature(&cpu->env, ARM_FEATURE_PMU);
+    set_feature(&cpu->env, ARM_FEATURE_DCC);
     cpu->midr = 0x411fc153; /* r1p3 */
     cpu->isar.id_pfr0 = 0x0131;
     cpu->isar.id_pfr1 = 0x001;
-- 
2.34.1

[PATCH RFC 3/3] target/arm/debug_helper: Add fieldoffset for MDCCSR_EL0 reg

[Sai Pavan Boddu]

MDCCSR_EL0 is aarch64 varient of DBGDSCRint, so utilize the same cpreg
offset.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
---
 target/arm/debug_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 7d856acddf..5deafa4d1f 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -970,7 +970,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
     { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
       .access = PL0_R, .accessfn = access_tdcc,
-      .type = ARM_CP_CONST, .resetvalue = 0 },
+      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1)}, /* Xilinx */
     /*
      * These registers belong to the Debug Communications Channel,
      * which is not implemented. However we implement RAZ/WI behaviour
-- 
2.34.1

Re: [PATCH RFC 0/3] Add DCC uart console support

[Peter Maydell]

On Fri, 14 Jun 2024 at 10:30, Sai Pavan Boddu <sai.pavan.boddu@amd.com> wrote:
>
> This patch series attaches a chardev to  arm Debug Communication channel registers,
> As each cpu has it own DBGDTRTX/RX register just tried to hook each cpu
> with chardev iff we find a chardev with id "dcc<cpu-id>".
>
> ex:
>     Below chardev switch would be connected to A53-0.
>
>     ./qemu-system-aarch64 -M xlnx-zcu102 -kernel u-boot-dtb.bin
>       -dtb zynqmp-zcu102-rev1.1.d -display none -m 2G
>       -chardev stdio,id=dcc0

This is neat -- I've thought before that it would be nice to be able
to use the DCC as a chardev destination, given that Linux has support
for earlycon-over-DCC. The thing that put me off was the question
of what the command-line syntax for setting it up ought to be, though.
I'm not sure we want "look for the chardev with a particular ID string"
but I don't know what existing in-tree precedent we have for this
kind of thing.

thanks
-- PMM

Re: [PATCH RFC 1/3] target/arm: Add dcc uart support

[Peter Maydell]

On Fri, 14 Jun 2024 at 10:30, Sai Pavan Boddu <sai.pavan.boddu@amd.com> wrote:
>
> DCC is a debug port to transfer some data between debugger and
> processor, we are using this feature to connect a chardev device.
> Chardev frontends should be named as "dcc<cpu-index>" inorder to connect
> to this interface.
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
> ---
>  target/arm/cpu.h       | 11 +++++
>  target/arm/internals.h |  4 ++
>  target/arm/debug-dcc.c | 99 ++++++++++++++++++++++++++++++++++++++++++
>  target/arm/helper.c    |  3 ++
>  target/arm/meson.build |  1 +
>  5 files changed, 118 insertions(+)
>  create mode 100644 target/arm/debug-dcc.c
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 3841359d0f..6b3cb8e70e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -30,6 +30,8 @@
>  #include "qapi/qapi-types-common.h"
>  #include "target/arm/multiprocessing.h"
>  #include "target/arm/gtimer.h"
> +#include "chardev/char.h"
> +#include "chardev/char-fe.h"
>
>  #ifdef TARGET_AARCH64
>  #define KVM_HAVE_MCE_INJECTION 1
> @@ -523,6 +525,11 @@ typedef struct CPUArchState {
>
>          /* NV2 register */
>          uint64_t vncr_el2;
> +        /*
> +         * Debug Trace regsiters
> +         */
> +        uint32_t dbgdtr_tx;
> +        uint32_t dbgdtr_rx;
>      } cp15;
>
>      struct {
> @@ -1097,6 +1104,9 @@ struct ArchCPU {
>
>      /* Generic timer counter frequency, in Hz */
>      uint64_t gt_cntfrq_hz;
> +
> +    /* dcc chardev */
> +    CharBackend dcc;
>  };
>
>  typedef struct ARMCPUInfo {
> @@ -2388,6 +2398,7 @@ enum arm_features {
>       * CPU types added in future.
>       */
>      ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */
> +    ARM_FEATURE_DCC,

Generally speaking we want to avoid adding new ARM_FEATURE bits
if there's some way to identify the presence of the feature via
the ID registers. In this case, the debug registers always exist
as long as we have ARM_FEATURE_V7, I think. So I think we should
always provide this as part of the registers we define in
debug_cp_reginfo[] in debug_helper.c, rather than having a
separate feature bit.

>  };
>
>  static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 11b5da2562..2fa797c5df 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1778,4 +1778,8 @@ uint64_t gt_get_countervalue(CPUARMState *env);
>   * and CNTVCT_EL0 (this will be either 0 or the value of CNTVOFF_EL2).
>   */
>  uint64_t gt_virt_cnt_offset(CPUARMState *env);
> +/*
> + * Initialise Coresight Debug interface
> + */
> +void arm_dcc_init(ARMCPU *cpu);
>  #endif
> diff --git a/target/arm/debug-dcc.c b/target/arm/debug-dcc.c
> new file mode 100644
> index 0000000000..9144b54994
> --- /dev/null
> +++ b/target/arm/debug-dcc.c
> @@ -0,0 +1,99 @@
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "internals.h"
> +#include "cpregs.h"
> +#include "chardev/char-fe.h"
> +
> +#define MDCCSR_EL0_RXFULL_MASK (1 << 30)
> +#define MDCCSR_EL0_TXFULL_MASK (1 << 29)
> +
> +static void debug_dcc_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                        uint64_t value)
> +{
> +    ARMCPU *cpu = ri->opaque;
> +    env->cp15.dbgdtr_tx = value;
> +
> +    if (qemu_chr_fe_get_driver(&cpu->dcc)) {

This is a weird function to have to call. Generally we set things
up so that you don't need to care if there's actually anything
connected to the chardev -- qemu_chr_fe_write() and friends will
do nothing if there's no chardev attached to this backend.

> +        /*
> +         * Usually dcc is used for putc/getc calls which expect only
> +         * 1 byte from external debugger.
> +         * TODO: This needs to be generalized for other use-cases.
> +         */

Is this referring to the way that the DCC is defined as a 64-bit
(or 32-bit for AArch32) channel but we're ignoring all but the bottom
8 bits of the values?

> +        qemu_chr_fe_write_all(&cpu->dcc, (uint8_t *)&env->cp15.dbgdtr_tx, 1);

The DCC has flow-control flags so we don't need to use _write_all, and
generally we should avoid that in new code because it might block.

> +    }
> +}
> +
> +static uint64_t debug_dcc_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint32_t ret = 0;
> +    ARMCPU *cpu = ri->opaque;
> +
> +    if (env->cp15.mdscr_el1 & MDCCSR_EL0_RXFULL_MASK) {
> +        ret = env->cp15.dbgdtr_rx;
> +        env->cp15.dbgdtr_rx = 0;
> +        env->cp15.mdscr_el1 &= ~MDCCSR_EL0_RXFULL_MASK;
> +        qemu_chr_fe_accept_input(&cpu->dcc);
> +    }
> +    return ret;
> +}
> +
> +static const ARMCPRegInfo dcc_cp_reginfo[] = {
> +    /* DBGDTRTX_EL0/DBGDTRRX_EL0 depend on direction */
> +    { .name = "DBGDTR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 5, .opc2 = 0,
> +      .access = PL0_RW, .writefn = debug_dcc_write,
> +      .readfn = debug_dcc_read,
> +      .type = ARM_CP_OVERRIDE, .resetvalue = 0 },
> +    /* DBGDTRTXint/DBGDTRRXint depend on direction */
> +    { .name = "DBGDTRint", .state = ARM_CP_STATE_AA32, .cp = 14,
> +      .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
> +      .access = PL0_RW, .writefn = debug_dcc_write,
> +      .readfn = debug_dcc_read,
> +      .type = ARM_CP_OVERRIDE, .resetvalue = 0 },
> +};

Other registers that need updates for a real DCC:
 * MDCCSR_EL0
 * OSDTRRX_EL1
 * OSDTRTX_EL1

(all of which we currently have dummy versions of in
debug_helper.c).

> +
> +
> +static int dcc_chr_can_read(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    CPUARMState *env = &cpu->env;
> +
> +    if (!(env->cp15.mdscr_el1 & MDCCSR_EL0_RXFULL_MASK)) {
> +        /*
> +         * Usually dcc is used for putc/getc calls which expect only
> +         * 1 byte from external debugger.
> +         * TODO: This needs to be generalized for other use-cases.
> +         */
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void dcc_chr_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    ARMCPU *cpu = opaque;
> +    CPUARMState *env = &cpu->env;
> +
> +    env->cp15.dbgdtr_rx = *buf;
> +    env->cp15.mdscr_el1 |= MDCCSR_EL0_RXFULL_MASK;
> +}
> +
> +void arm_dcc_init(ARMCPU *cpu)
> +{
> +    Chardev *chr;
> +    char *dcc_name;
> +    CPUState *p = CPU(cpu);
> +
> +    dcc_name = g_strdup_printf("dcc%d", p->cpu_index);
> +    chr = qemu_chr_find(dcc_name);
> +    define_arm_cp_regs_with_opaque(cpu, dcc_cp_reginfo, cpu);
> +    if (chr) {
> +        qemu_chr_fe_init(&cpu->dcc, chr, NULL);
> +        qemu_chr_fe_set_handlers(&cpu->dcc,
> +                      dcc_chr_can_read,
> +                      dcc_chr_read,
> +                      NULL, NULL, cpu, NULL, true);
> +    }
> +    g_free(dcc_name);
> +}

thanks
-- PMM

Re: [PATCH RFC 3/3] target/arm/debug_helper: Add fieldoffset for MDCCSR_EL0 reg

[Peter Maydell]

On Fri, 14 Jun 2024 at 10:30, Sai Pavan Boddu <sai.pavan.boddu@amd.com> wrote:
>
> MDCCSR_EL0 is aarch64 varient of DBGDSCRint, so utilize the same cpreg
> offset.
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
> ---
>  target/arm/debug_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> index 7d856acddf..5deafa4d1f 100644
> --- a/target/arm/debug_helper.c
> +++ b/target/arm/debug_helper.c
> @@ -970,7 +970,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>      { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>        .access = PL0_R, .accessfn = access_tdcc,
> -      .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1)}, /* Xilinx */

MDCCSR_EL0 and MDSCR_EL1 aren't the same thing -- MDCCSR_EL0
only has bits 30 and 29 (RXfull and TXfull), not the full contents
of MDSCR_EL1.

thanks
-- PMM