[RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

Currently if user enables HVC_DCC config option in Linux, it invokes
access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
an undefined exception to the guest. And Linux crashes.

We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
is emulated as TXfull | RXfull.
Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
"If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
Also D17.3.7 DBGDTRRX_EL0,
" If RXfull is set to 1, return the last value written to DTRRX."

Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
the guest to be aborted.

We also emulate DBGDTRTX_EL0 as RAZ/WI.

We have added emulation for AArch32 variant of these registers as well.
Also, we have added handle_read_val_wi() to emulate DBGDSCREXT register
to return a specific value (ie TXfull | RXfull) and ignore any writes
to this register.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/arm64/vsysreg.c         | 21 ++++++++++++++----
 xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
 xen/arch/arm/include/asm/cpregs.h    |  2 ++
 xen/arch/arm/include/asm/traps.h     |  4 ++++
 xen/arch/arm/traps.c                 | 18 +++++++++++++++
 xen/arch/arm/vcpreg.c                | 33 +++++++++++++++++++++-------
 6 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..5082dfb02e 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
      *
      * Unhandled:
      *    MDCCINT_EL1
-     *    DBGDTR_EL0
-     *    DBGDTRRX_EL0
-     *    DBGDTRTX_EL0
      *    OSDTRRX_EL1
      *    OSDTRTX_EL1
      *    OSECCR_EL1
@@ -172,11 +169,27 @@ void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_MDSCR_EL1:
         return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_MDCCSR_EL0:
+    {
+        /*
+         * Bit 29: TX full, bit 30: RX full
+         * Given that we emulate DCC registers as RAZ/WI, doing the same for
+         * MDCCSR_EL0 would cause a guest to continue using the DCC despite no
+         * real effect. Setting the TX/RX status bits should result in a probe
+         * fail (based on Linux behavior).
+         */
+        register_t guest_reg_value = (1U << 29) | (1U << 30);
+
         /*
          * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
          * register as RAZ/WI above. So RO at both EL0 and EL1.
          */
-        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
+        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
+                                  guest_reg_value);
+    }
+    case HSR_SYSREG_DBGDTR_EL0:
+    /* DBGDTR[TR]X_EL0 share the same encoding */
+    case HSR_SYSREG_DBGDTRTX_EL0:
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
     HSR_SYSREG_DBG_CASES(DBGBVR):
     HSR_SYSREG_DBG_CASES(DBGBCR):
     HSR_SYSREG_DBG_CASES(DBGWVR):
diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
index e691d41c17..1495ccddea 100644
--- a/xen/arch/arm/include/asm/arm64/hsr.h
+++ b/xen/arch/arm/include/asm/arm64/hsr.h
@@ -47,6 +47,9 @@
 #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
 #define HSR_SYSREG_DBGPRCR_EL1    HSR_SYSREG(2,0,c1,c4,4)
 #define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
+#define HSR_SYSREG_DBGDTR_EL0     HSR_SYSREG(2,3,c0,c4,0)
+#define HSR_SYSREG_DBGDTRTX_EL0   HSR_SYSREG(2,3,c0,c5,0)
+#define HSR_SYSREG_DBGDTRRX_EL0   HSR_SYSREG(2,3,c0,c5,0)
 
 #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
 #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index 6b083de204..aec9e8f329 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -75,6 +75,8 @@
 #define DBGDIDR         p14,0,c0,c0,0   /* Debug ID Register */
 #define DBGDSCRINT      p14,0,c0,c1,0   /* Debug Status and Control Internal */
 #define DBGDSCREXT      p14,0,c0,c2,2   /* Debug Status and Control External */
+#define DBGDTRRXINT     p14,0,c0,c5,0   /* Debug Data Transfer Register, Receive */
+#define DBGDTRTXINT     p14,0,c0,c5,0   /* Debug Data Transfer Register, Transmit */
 #define DBGVCR          p14,0,c0,c7,0   /* Vector Catch */
 #define DBGBVR0         p14,0,c0,c0,4   /* Breakpoint Value 0 */
 #define DBGBCR0         p14,0,c0,c0,5   /* Breakpoint Control 0 */
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 883dae368e..a2692722d5 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -56,6 +56,10 @@ void handle_ro_raz(struct cpu_user_regs *regs, int regidx, bool read,
 void handle_ro_read_val(struct cpu_user_regs *regs, int regidx, bool read,
                         const union hsr hsr, int min_el, register_t val);
 
+/* Read only as value provided with 'val' argument, write ignore */
+void handle_read_val_wi(struct cpu_user_regs *regs, int regidx,
+                        const union hsr hsr, int min_el, register_t val);
+
 /* Co-processor registers emulation (see arch/arm/vcpreg.c). */
 void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr);
 void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3784e8276e..f5ab555b19 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1676,6 +1676,24 @@ void handle_ro_read_val(struct cpu_user_regs *regs,
     advance_pc(regs, hsr);
 }
 
+/* Read as value provided with 'val' argument of this function, write ignore */
+void handle_read_val_wi(struct cpu_user_regs *regs,
+                        int regidx,
+                        const union hsr hsr,
+                        int min_el,
+                        register_t val)
+{
+    ASSERT((min_el == 0) || (min_el == 1));
+
+    if ( min_el > 0 && regs_mode_is_user(regs) )
+        return inject_undef_exception(regs, hsr);
+
+    set_user_reg(regs, regidx, val);
+
+    advance_pc(regs, hsr);
+}
+
+
 /* Read only as read as zero */
 void handle_ro_raz(struct cpu_user_regs *regs,
                    int regidx,
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 39aeda9dab..3f1276f96e 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
         break;
     }
 
-    case HSR_CPREG32(DBGDSCRINT):
+    case HSR_CPREG32(DBGDSCREXT):
+    {
         /*
-         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
-         * is set to 0, which we emulated below.
+         * Bit 29: TX full, bit 30: RX full
+         * Given that we emulate DCC registers as RAZ/WI, doing the same for
+         * DBGDSCRint would cause a guest to continue using the DCC despite no
+         * real effect. Setting the TX/RX status bits should result in a probe
+         * fail (based on Linux behavior).
          */
-        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
+        register_t guest_reg_value = (1U << 29) | (1U << 30);
 
-    case HSR_CPREG32(DBGDSCREXT):
+        return handle_read_val_wi(regs, regidx, hsr, 1,
+                                  guest_reg_value);
+    }
+
+    case HSR_CPREG32(DBGDSCRINT):
+    {
         /*
-         * Implement debug status and control register as RAZ/WI.
-         * The OS won't use Hardware debug if MDBGen not set.
+         * Bit 29: TX full, bit 30: RX full
+         * Given that we emulate DCC registers as RAZ/WI, doing the same for
+         * DBGDSCRint would cause a guest to continue using the DCC despite no
+         * real effect. Setting the TX/RX status bits should result in a probe
+         * fail (based on Linux behavior).
          */
-        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
+        register_t guest_reg_value = (1U << 29) | (1U << 30);
+
+        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1,
+                                  guest_reg_value);
+    }
 
+    case HSR_CPREG32(DBGDTRTXINT):
     case HSR_CPREG32(DBGVCR):
     case HSR_CPREG32(DBGBVR0):
     case HSR_CPREG32(DBGBCR0):
-- 
2.25.1

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

Hi Ayan,

On 01/12/2023 19:50, Ayan Kumar Halder wrote:
> Currently if user enables HVC_DCC config option in Linux, it invokes
> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
> an undefined exception to the guest. And Linux crashes.
> 
> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
> is emulated as TXfull | RXfull.
TX/RX are status bits of MDCCSR_EL0, not DBGDTR_EL0. This applies here and elsewhere.

> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
> Also D17.3.7 DBGDTRRX_EL0,
> " If RXfull is set to 1, return the last value written to DTRRX."
> 
> Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
> before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
> hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
> the guest to be aborted.
> 
> We also emulate DBGDTRTX_EL0 as RAZ/WI.
> 
> We have added emulation for AArch32 variant of these registers as well.
> Also, we have added handle_read_val_wi() to emulate DBGDSCREXT register
Emulating DBGDSCREXT is not needed. See below.

> to return a specific value (ie TXfull | RXfull) and ignore any writes
> to this register.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>  xen/arch/arm/arm64/vsysreg.c         | 21 ++++++++++++++----
>  xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>  xen/arch/arm/include/asm/cpregs.h    |  2 ++
>  xen/arch/arm/include/asm/traps.h     |  4 ++++
>  xen/arch/arm/traps.c                 | 18 +++++++++++++++
>  xen/arch/arm/vcpreg.c                | 33 +++++++++++++++++++++-------
>  6 files changed, 69 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..5082dfb02e 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
>       *
>       * Unhandled:
>       *    MDCCINT_EL1
> -     *    DBGDTR_EL0
> -     *    DBGDTRRX_EL0
> -     *    DBGDTRTX_EL0
>       *    OSDTRRX_EL1
>       *    OSDTRTX_EL1
>       *    OSECCR_EL1
> @@ -172,11 +169,27 @@ void do_sysreg(struct cpu_user_regs *regs,
>      case HSR_SYSREG_MDSCR_EL1:
>          return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>      case HSR_SYSREG_MDCCSR_EL0:
> +    {
> +        /*
> +         * Bit 29: TX full, bit 30: RX full
> +         * Given that we emulate DCC registers as RAZ/WI, doing the same for
> +         * MDCCSR_EL0 would cause a guest to continue using the DCC despite no
> +         * real effect. Setting the TX/RX status bits should result in a probe
> +         * fail (based on Linux behavior).
> +         */
> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
> +
>          /*
>           * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
>           * register as RAZ/WI above. So RO at both EL0 and EL1.
>           */
> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
> +                                  guest_reg_value);
> +    }
> +    case HSR_SYSREG_DBGDTR_EL0:
> +    /* DBGDTR[TR]X_EL0 share the same encoding */
> +    case HSR_SYSREG_DBGDTRTX_EL0:
> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>      HSR_SYSREG_DBG_CASES(DBGBVR):
>      HSR_SYSREG_DBG_CASES(DBGBCR):
>      HSR_SYSREG_DBG_CASES(DBGWVR):
> diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
> index e691d41c17..1495ccddea 100644
> --- a/xen/arch/arm/include/asm/arm64/hsr.h
> +++ b/xen/arch/arm/include/asm/arm64/hsr.h
> @@ -47,6 +47,9 @@
>  #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
>  #define HSR_SYSREG_DBGPRCR_EL1    HSR_SYSREG(2,0,c1,c4,4)
>  #define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
> +#define HSR_SYSREG_DBGDTR_EL0     HSR_SYSREG(2,3,c0,c4,0)
> +#define HSR_SYSREG_DBGDTRTX_EL0   HSR_SYSREG(2,3,c0,c5,0)
> +#define HSR_SYSREG_DBGDTRRX_EL0   HSR_SYSREG(2,3,c0,c5,0)
>  
>  #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
>  #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 6b083de204..aec9e8f329 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -75,6 +75,8 @@
>  #define DBGDIDR         p14,0,c0,c0,0   /* Debug ID Register */
>  #define DBGDSCRINT      p14,0,c0,c1,0   /* Debug Status and Control Internal */
>  #define DBGDSCREXT      p14,0,c0,c2,2   /* Debug Status and Control External */
> +#define DBGDTRRXINT     p14,0,c0,c5,0   /* Debug Data Transfer Register, Receive */
> +#define DBGDTRTXINT     p14,0,c0,c5,0   /* Debug Data Transfer Register, Transmit */
>  #define DBGVCR          p14,0,c0,c7,0   /* Vector Catch */
>  #define DBGBVR0         p14,0,c0,c0,4   /* Breakpoint Value 0 */
>  #define DBGBCR0         p14,0,c0,c0,5   /* Breakpoint Control 0 */
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 883dae368e..a2692722d5 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -56,6 +56,10 @@ void handle_ro_raz(struct cpu_user_regs *regs, int regidx, bool read,
>  void handle_ro_read_val(struct cpu_user_regs *regs, int regidx, bool read,
>                          const union hsr hsr, int min_el, register_t val);
>  
> +/* Read only as value provided with 'val' argument, write ignore */
> +void handle_read_val_wi(struct cpu_user_regs *regs, int regidx,
> +                        const union hsr hsr, int min_el, register_t val);
> +
>  /* Co-processor registers emulation (see arch/arm/vcpreg.c). */
>  void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr);
>  void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 3784e8276e..f5ab555b19 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1676,6 +1676,24 @@ void handle_ro_read_val(struct cpu_user_regs *regs,
>      advance_pc(regs, hsr);
>  }
>  
> +/* Read as value provided with 'val' argument of this function, write ignore */
> +void handle_read_val_wi(struct cpu_user_regs *regs,
> +                        int regidx,
> +                        const union hsr hsr,
> +                        int min_el,
> +                        register_t val)
> +{
> +    ASSERT((min_el == 0) || (min_el == 1));
> +
> +    if ( min_el > 0 && regs_mode_is_user(regs) )
> +        return inject_undef_exception(regs, hsr);
> +
> +    set_user_reg(regs, regidx, val);
> +
> +    advance_pc(regs, hsr);
> +}
> +
> +
>  /* Read only as read as zero */
>  void handle_ro_raz(struct cpu_user_regs *regs,
>                     int regidx,
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 39aeda9dab..3f1276f96e 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>          break;
>      }
>  
> -    case HSR_CPREG32(DBGDSCRINT):
> +    case HSR_CPREG32(DBGDSCREXT):
1) Why did you invert DBGDSCRINT with DBGDSCREXT? The former should appear first to keep the correct order.

2) Take a look at what I did for arm64. I emulated TX/RX only in MDCCSR_EL0 and not in MDSCR_EL1 (on arm32,
DBGDSCRINT, DBGDSCREXT respectively). This is because according to Arm ARM for MDSCR_EL1 TXfull/RXfull:
"When OSLSR_EL1.OSLK == 0, software must treat this bit as UNK/SBZP"
On arm64, we emulate OSLSR_EL1 as (1<<3) meaning OSLK is 0. UNK/SBZP means RAZ/WI. This implies that
the only way to access the TXfull/RXfull flags is through MDCCSR_EL0.

For arm32, we should do the same. Emulate only RINT and keep REXT as RAZ/WI. The only additional change would
be to emulate (at the moment it is unhandled) DBGOSLSR similar to what we do for arm64.

> +    {
>          /*
> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> -         * is set to 0, which we emulated below.
> +         * Bit 29: TX full, bit 30: RX full
> +         * Given that we emulate DCC registers as RAZ/WI, doing the same for
> +         * DBGDSCRint would cause a guest to continue using the DCC despite no
> +         * real effect. Setting the TX/RX status bits should result in a probe
> +         * fail (based on Linux behavior).
>           */
> -        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
>  
> -    case HSR_CPREG32(DBGDSCREXT):
> +        return handle_read_val_wi(regs, regidx, hsr, 1,
> +                                  guest_reg_value);
> +    }
> +
> +    case HSR_CPREG32(DBGDSCRINT):
> +    {
>          /*
> -         * Implement debug status and control register as RAZ/WI.
> -         * The OS won't use Hardware debug if MDBGen not set.
> +         * Bit 29: TX full, bit 30: RX full
> +         * Given that we emulate DCC registers as RAZ/WI, doing the same for
> +         * DBGDSCRint would cause a guest to continue using the DCC despite no
> +         * real effect. Setting the TX/RX status bits should result in a probe
> +         * fail (based on Linux behavior).
>           */
> -        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
> +
> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1,
> +                                  guest_reg_value);
DBGDSCRINT is accessible at EL0 if DBGDSCREXT.UDCCdis is 0 (we emulate REXT as RAZ). There is
even a comment about that which you removed (please restore). Therefore, the minimum EL passed
to handle_ro_read_val should be 0 not 1.

P.S. The current code is incorrect in this regard.

> +    }
>  
> +    case HSR_CPREG32(DBGDTRTXINT):
This would be incorrect. Adding TXINT here would result in calling handle_raz_wi with minimum EL == 1,
but TXINT can be accessed at EL0.

This should be:
    /* DBGDTR[TR]XINT share the same encoding */
    case HSR_CPREG32(DBGDTRTXINT):
        return handle_raz_wi(regs, regidx, cp32.read, hsr, 0);

>      case HSR_CPREG32(DBGVCR):
>      case HSR_CPREG32(DBGBVR0):
>      case HSR_CPREG32(DBGBCR0):


~Michal

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

Hi Ayan,

On 01/12/2023 18:50, Ayan Kumar Halder wrote:
> Currently if user enables HVC_DCC config option in Linux, it invokes
> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
> an undefined exception to the guest. And Linux crashes.

I am missing some data points here to be able to say whether I would be 
ok with emulating the registers. So some questions:
   * As you wrote below, HVC_DCC will return -ENODEV after this 
emulation. So may I ask what's the use case to enable it? (e.g. is there 
a distro turning this on?)
  * Linux is writing to the registers unconditionally, but is the spec 
mandating the implementation of the registers? (I couldn't find either way)
  * When was this check introduced in Linux? Did it ever changed?

> 
> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
> is emulated as TXfull | RXfull.

Skimming through the Arm Arm, I see that TXfull and Rxfull indicates 
that both buffers are full but it doesn't explicitly say this means the 
feature is not available.

I understand this is what Linux checks, but if we want to partially 
emulate the registers in Xen, then I'd like us to make sure this is 
backed by the Arm Arm rather than based on Linux implementation (which 
can change at any point).

> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
> Also D17.3.7 DBGDTRRX_EL0,
> " If RXfull is set to 1, return the last value written to DTRRX."
> 
> Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
> before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
> hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
> the guest to be aborted.

See above, what guarantees us that Linux will not change this behavior 
in the future?

> 
> We also emulate DBGDTRTX_EL0 as RAZ/WI.
> 
> We have added emulation for AArch32 variant of these registers as well.
> Also, we have added handle_read_val_wi() to emulate DBGDSCREXT register
> to return a specific value (ie TXfull | RXfull) and ignore any writes
> to this register.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

We usually expect the first Signed-off-by to also be the author. So 
should Michal be the author of this patch?

> ---
>   xen/arch/arm/arm64/vsysreg.c         | 21 ++++++++++++++----
>   xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>   xen/arch/arm/include/asm/cpregs.h    |  2 ++
>   xen/arch/arm/include/asm/traps.h     |  4 ++++
>   xen/arch/arm/traps.c                 | 18 +++++++++++++++
>   xen/arch/arm/vcpreg.c                | 33 +++++++++++++++++++++-------
>   6 files changed, 69 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..5082dfb02e 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
>        *
>        * Unhandled:
>        *    MDCCINT_EL1
> -     *    DBGDTR_EL0
> -     *    DBGDTRRX_EL0
> -     *    DBGDTRTX_EL0
>        *    OSDTRRX_EL1
>        *    OSDTRTX_EL1
>        *    OSECCR_EL1
> @@ -172,11 +169,27 @@ void do_sysreg(struct cpu_user_regs *regs,
>       case HSR_SYSREG_MDSCR_EL1:
>           return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>       case HSR_SYSREG_MDCCSR_EL0:
> +    {
> +        /*
> +         * Bit 29: TX full, bit 30: RX full
> +         * Given that we emulate DCC registers as RAZ/WI, doing the same for
> +         * MDCCSR_EL0 would cause a guest to continue using the DCC despite no
> +         * real effect. Setting the TX/RX status bits should result in a probe
> +         * fail (based on Linux behavior).
> +         */
> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
> +
>           /*
>            * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
>            * register as RAZ/WI above. So RO at both EL0 and EL1.
>            */
> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
> +                                  guest_reg_value);
> +    }
> +    case HSR_SYSREG_DBGDTR_EL0:
> +    /* DBGDTR[TR]X_EL0 share the same encoding */
> +    case HSR_SYSREG_DBGDTRTX_EL0:
> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>       HSR_SYSREG_DBG_CASES(DBGBVR):
>       HSR_SYSREG_DBG_CASES(DBGBCR):
>       HSR_SYSREG_DBG_CASES(DBGWVR):
> diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
> index e691d41c17..1495ccddea 100644
> --- a/xen/arch/arm/include/asm/arm64/hsr.h
> +++ b/xen/arch/arm/include/asm/arm64/hsr.h
> @@ -47,6 +47,9 @@
>   #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
>   #define HSR_SYSREG_DBGPRCR_EL1    HSR_SYSREG(2,0,c1,c4,4)
>   #define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
> +#define HSR_SYSREG_DBGDTR_EL0     HSR_SYSREG(2,3,c0,c4,0)
> +#define HSR_SYSREG_DBGDTRTX_EL0   HSR_SYSREG(2,3,c0,c5,0)
> +#define HSR_SYSREG_DBGDTRRX_EL0   HSR_SYSREG(2,3,c0,c5,0)
>   
>   #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
>   #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 6b083de204..aec9e8f329 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -75,6 +75,8 @@
>   #define DBGDIDR         p14,0,c0,c0,0   /* Debug ID Register */
>   #define DBGDSCRINT      p14,0,c0,c1,0   /* Debug Status and Control Internal */
>   #define DBGDSCREXT      p14,0,c0,c2,2   /* Debug Status and Control External */
> +#define DBGDTRRXINT     p14,0,c0,c5,0   /* Debug Data Transfer Register, Receive */
> +#define DBGDTRTXINT     p14,0,c0,c5,0   /* Debug Data Transfer Register, Transmit */
>   #define DBGVCR          p14,0,c0,c7,0   /* Vector Catch */
>   #define DBGBVR0         p14,0,c0,c0,4   /* Breakpoint Value 0 */
>   #define DBGBCR0         p14,0,c0,c0,5   /* Breakpoint Control 0 */
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 883dae368e..a2692722d5 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -56,6 +56,10 @@ void handle_ro_raz(struct cpu_user_regs *regs, int regidx, bool read,
>   void handle_ro_read_val(struct cpu_user_regs *regs, int regidx, bool read,
>                           const union hsr hsr, int min_el, register_t val);
>   
> +/* Read only as value provided with 'val' argument, write ignore */
> +void handle_read_val_wi(struct cpu_user_regs *regs, int regidx,
> +                        const union hsr hsr, int min_el, register_t val);
> +
>   /* Co-processor registers emulation (see arch/arm/vcpreg.c). */
>   void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr);
>   void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 3784e8276e..f5ab555b19 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1676,6 +1676,24 @@ void handle_ro_read_val(struct cpu_user_regs *regs,
>       advance_pc(regs, hsr);
>   }
>   
> +/* Read as value provided with 'val' argument of this function, write ignore */
> +void handle_read_val_wi(struct cpu_user_regs *regs,
> +                        int regidx,
> +                        const union hsr hsr,
> +                        int min_el,
> +                        register_t val)
> +{
> +    ASSERT((min_el == 0) || (min_el == 1));
> +
> +    if ( min_el > 0 && regs_mode_is_user(regs) )
> +        return inject_undef_exception(regs, hsr);
> +
> +    set_user_reg(regs, regidx, val);
> +
> +    advance_pc(regs, hsr);
> +}
> +
> +
>   /* Read only as read as zero */
>   void handle_ro_raz(struct cpu_user_regs *regs,
>                      int regidx,
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 39aeda9dab..3f1276f96e 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>           break;
>       }
>   
> -    case HSR_CPREG32(DBGDSCRINT):
> +    case HSR_CPREG32(DBGDSCREXT):
> +    {
>           /*
> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> -         * is set to 0, which we emulated below.
> +         * Bit 29: TX full, bit 30: RX full
> +         * Given that we emulate DCC registers as RAZ/WI, doing the same for
> +         * DBGDSCRint would cause a guest to continue using the DCC despite no
> +         * real effect. Setting the TX/RX status bits should result in a probe
> +         * fail (based on Linux behavior).
If you want to mention Linux then you need to be a bit more specific 
because Linux can change at any point. So you at least want to specify 
the Linux version and place in the code.

So this doesn't get stale as soon as the HVC_DCC driver changes.

>            */
> -        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
>   
> -    case HSR_CPREG32(DBGDSCREXT):
> +        return handle_read_val_wi(regs, regidx, hsr, 1,
> +                                  guest_reg_value);
> +    }
> +
> +    case HSR_CPREG32(DBGDSCRINT):
> +    {
>           /*
> -         * Implement debug status and control register as RAZ/WI.
> -         * The OS won't use Hardware debug if MDBGen not set.
> +         * Bit 29: TX full, bit 30: RX full
> +         * Given that we emulate DCC registers as RAZ/WI, doing the same for
> +         * DBGDSCRint would cause a guest to continue using the DCC despite no
> +         * real effect. Setting the TX/RX status bits should result in a probe
> +         * fail (based on Linux behavior).
>            */
> -        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
> +
> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1,
> +                                  guest_reg_value);
> +    }
>   
> +    case HSR_CPREG32(DBGDTRTXINT):
>       case HSR_CPREG32(DBGVCR):
>       case HSR_CPREG32(DBGBVR0):
>       case HSR_CPREG32(DBGBCR0):

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

On 04/12/2023 10:31, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>> Currently if user enables HVC_DCC config option in Linux, it invokes
>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>> an undefined exception to the guest. And Linux crashes.
>
> I am missing some data points here to be able to say whether I would 
> be ok with emulating the registers. So some questions:
>   * As you wrote below, HVC_DCC will return -ENODEV after this 
> emulation. So may I ask what's the use case to enable it? (e.g. is 
> there a distro turning this on?)

I am not aware of any distro using (or not using) this feature. This 
issue came to light during our internal testing, when HVC_DCC was 
enabled to use the debug console. When Linux runs without Xen, then we 
could observe the logs on the debug console. When Linux was running as a 
VM, it crashed.

My intention here was to do the bare minimum emulation so that the crash 
could be avoided.

>  * Linux is writing to the registers unconditionally, but is the spec 
> mandating the implementation of the registers? (I couldn't find either 
> way)

 From ARM DDI 0487I.a ID081822, H1.2, External debug,

"The Debug Communications Channel, DCC, passes data between the PE and 
the debugger:

— The DCC includes the data transfer registers, DTRRX and DTRTX, and 
associated flow-control flags.

— Although the DCC is an essential part of Debug state operation, it can 
also be used in Non-debug state."

 From this I infer that the spec mandates the implementation of these 
registers. IOW, these registers should always be present in any Arm 
compliant SoC.

>  * When was this check introduced in Linux? Did it ever changed?
>
This check was introduced in the following commit

"commit f377775dc083506e2fd7739d8615971c46b5246e
Author: Rob Herring <rob.herring@calxeda.com>
Date:   Tue Sep 24 21:05:58 2013 -0500

     TTY: hvc_dcc: probe for a JTAG connection before registering

     Enabling the ARM DCC console and using without a JTAG connection will
     simply hang the system. Since distros like to turn on all options, this
     is a reoccurring problem to debug. We can do better by checking if
     anything is attached and handling characters. There is no way to probe
     this, so send a newline and check that it is handled.
"

As of today, this check hasn't changed.

>>
>> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
>> is emulated as TXfull | RXfull.
>
> Skimming through the Arm Arm, I see that TXfull and Rxfull indicates 
> that both buffers are full but it doesn't explicitly say this means 
> the feature is not available.
We are not saying that the feature is not available. Rather ...
>
> I understand this is what Linux checks, but if we want to partially 
> emulate the registers in Xen, then I'd like us to make sure this is 
> backed by the Arm Arm rather than based on Linux implementation (which 
> can change at any point).
>
>> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
>> Also D17.3.7 DBGDTRRX_EL0,
>> " If RXfull is set to 1, return the last value written to DTRRX."
>>
>> Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
>> before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>> hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
>> the guest to be aborted.
>
> See above, what guarantees us that Linux will not change this behavior 
> in the future?

If I understand "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN" 
correctly, it seems that Arm ARM expects OS to check for TXfull.

If the condition is true, it should return some error.

Let me know if I misunderstood this.

>
>>
>> We also emulate DBGDTRTX_EL0 as RAZ/WI.
>>
>> We have added emulation for AArch32 variant of these registers as well.
>> Also, we have added handle_read_val_wi() to emulate DBGDSCREXT register
>> to return a specific value (ie TXfull | RXfull) and ignore any writes
>> to this register.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>
> We usually expect the first Signed-off-by to also be the author. So 
> should Michal be the author of this patch?
Yes, I will make Michal as the author.
>
>> ---
>>   xen/arch/arm/arm64/vsysreg.c         | 21 ++++++++++++++----
>>   xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>>   xen/arch/arm/include/asm/cpregs.h    |  2 ++
>>   xen/arch/arm/include/asm/traps.h     |  4 ++++
>>   xen/arch/arm/traps.c                 | 18 +++++++++++++++
>>   xen/arch/arm/vcpreg.c                | 33 +++++++++++++++++++++-------
>>   6 files changed, 69 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index b5d54c569b..5082dfb02e 100644
>> --- a/xen/arch/arm/arm64/vsysreg.c
>> +++ b/xen/arch/arm/arm64/vsysreg.c
>> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
>>        *
>>        * Unhandled:
>>        *    MDCCINT_EL1
>> -     *    DBGDTR_EL0
>> -     *    DBGDTRRX_EL0
>> -     *    DBGDTRTX_EL0
>>        *    OSDTRRX_EL1
>>        *    OSDTRTX_EL1
>>        *    OSECCR_EL1
>> @@ -172,11 +169,27 @@ void do_sysreg(struct cpu_user_regs *regs,
>>       case HSR_SYSREG_MDSCR_EL1:
>>           return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>       case HSR_SYSREG_MDCCSR_EL0:
>> +    {
>> +        /*
>> +         * Bit 29: TX full, bit 30: RX full
>> +         * Given that we emulate DCC registers as RAZ/WI, doing the 
>> same for
>> +         * MDCCSR_EL0 would cause a guest to continue using the DCC 
>> despite no
>> +         * real effect. Setting the TX/RX status bits should result 
>> in a probe
>> +         * fail (based on Linux behavior).
>> +         */
>> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
>> +
>>           /*
>>            * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We 
>> emulate that
>>            * register as RAZ/WI above. So RO at both EL0 and EL1.
>>            */
>> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, 
>> hsr, 0,
>> +                                  guest_reg_value);
>> +    }
>> +    case HSR_SYSREG_DBGDTR_EL0:
>> +    /* DBGDTR[TR]X_EL0 share the same encoding */
>> +    case HSR_SYSREG_DBGDTRTX_EL0:
>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>>       HSR_SYSREG_DBG_CASES(DBGBVR):
>>       HSR_SYSREG_DBG_CASES(DBGBCR):
>>       HSR_SYSREG_DBG_CASES(DBGWVR):
>> diff --git a/xen/arch/arm/include/asm/arm64/hsr.h 
>> b/xen/arch/arm/include/asm/arm64/hsr.h
>> index e691d41c17..1495ccddea 100644
>> --- a/xen/arch/arm/include/asm/arm64/hsr.h
>> +++ b/xen/arch/arm/include/asm/arm64/hsr.h
>> @@ -47,6 +47,9 @@
>>   #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
>>   #define HSR_SYSREG_DBGPRCR_EL1    HSR_SYSREG(2,0,c1,c4,4)
>>   #define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
>> +#define HSR_SYSREG_DBGDTR_EL0     HSR_SYSREG(2,3,c0,c4,0)
>> +#define HSR_SYSREG_DBGDTRTX_EL0   HSR_SYSREG(2,3,c0,c5,0)
>> +#define HSR_SYSREG_DBGDTRRX_EL0   HSR_SYSREG(2,3,c0,c5,0)
>>     #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
>>   #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
>> diff --git a/xen/arch/arm/include/asm/cpregs.h 
>> b/xen/arch/arm/include/asm/cpregs.h
>> index 6b083de204..aec9e8f329 100644
>> --- a/xen/arch/arm/include/asm/cpregs.h
>> +++ b/xen/arch/arm/include/asm/cpregs.h
>> @@ -75,6 +75,8 @@
>>   #define DBGDIDR         p14,0,c0,c0,0   /* Debug ID Register */
>>   #define DBGDSCRINT      p14,0,c0,c1,0   /* Debug Status and Control 
>> Internal */
>>   #define DBGDSCREXT      p14,0,c0,c2,2   /* Debug Status and Control 
>> External */
>> +#define DBGDTRRXINT     p14,0,c0,c5,0   /* Debug Data Transfer 
>> Register, Receive */
>> +#define DBGDTRTXINT     p14,0,c0,c5,0   /* Debug Data Transfer 
>> Register, Transmit */
>>   #define DBGVCR          p14,0,c0,c7,0   /* Vector Catch */
>>   #define DBGBVR0         p14,0,c0,c0,4   /* Breakpoint Value 0 */
>>   #define DBGBCR0         p14,0,c0,c0,5   /* Breakpoint Control 0 */
>> diff --git a/xen/arch/arm/include/asm/traps.h 
>> b/xen/arch/arm/include/asm/traps.h
>> index 883dae368e..a2692722d5 100644
>> --- a/xen/arch/arm/include/asm/traps.h
>> +++ b/xen/arch/arm/include/asm/traps.h
>> @@ -56,6 +56,10 @@ void handle_ro_raz(struct cpu_user_regs *regs, int 
>> regidx, bool read,
>>   void handle_ro_read_val(struct cpu_user_regs *regs, int regidx, 
>> bool read,
>>                           const union hsr hsr, int min_el, register_t 
>> val);
>>   +/* Read only as value provided with 'val' argument, write ignore */
>> +void handle_read_val_wi(struct cpu_user_regs *regs, int regidx,
>> +                        const union hsr hsr, int min_el, register_t 
>> val);
>> +
>>   /* Co-processor registers emulation (see arch/arm/vcpreg.c). */
>>   void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr);
>>   void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr);
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 3784e8276e..f5ab555b19 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1676,6 +1676,24 @@ void handle_ro_read_val(struct cpu_user_regs 
>> *regs,
>>       advance_pc(regs, hsr);
>>   }
>>   +/* Read as value provided with 'val' argument of this function, 
>> write ignore */
>> +void handle_read_val_wi(struct cpu_user_regs *regs,
>> +                        int regidx,
>> +                        const union hsr hsr,
>> +                        int min_el,
>> +                        register_t val)
>> +{
>> +    ASSERT((min_el == 0) || (min_el == 1));
>> +
>> +    if ( min_el > 0 && regs_mode_is_user(regs) )
>> +        return inject_undef_exception(regs, hsr);
>> +
>> +    set_user_reg(regs, regidx, val);
>> +
>> +    advance_pc(regs, hsr);
>> +}
>> +
>> +
>>   /* Read only as read as zero */
>>   void handle_ro_raz(struct cpu_user_regs *regs,
>>                      int regidx,
>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>> index 39aeda9dab..3f1276f96e 100644
>> --- a/xen/arch/arm/vcpreg.c
>> +++ b/xen/arch/arm/vcpreg.c
>> @@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs, 
>> const union hsr hsr)
>>           break;
>>       }
>>   -    case HSR_CPREG32(DBGDSCRINT):
>> +    case HSR_CPREG32(DBGDSCREXT):
>> +    {
>>           /*
>> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
>> -         * is set to 0, which we emulated below.
>> +         * Bit 29: TX full, bit 30: RX full
>> +         * Given that we emulate DCC registers as RAZ/WI, doing the 
>> same for
>> +         * DBGDSCRint would cause a guest to continue using the DCC 
>> despite no
>> +         * real effect. Setting the TX/RX status bits should result 
>> in a probe
>> +         * fail (based on Linux behavior).
> If you want to mention Linux then you need to be a bit more specific 
> because Linux can change at any point. So you at least want to specify 
> the Linux version and place in the code.
>
> So this doesn't get stale as soon as the HVC_DCC driver changes.

(based on Linux behavior since f377775dc083).

- Ayan

>
>>            */
>> -        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
>> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
>>   -    case HSR_CPREG32(DBGDSCREXT):
>> +        return handle_read_val_wi(regs, regidx, hsr, 1,
>> +                                  guest_reg_value);
>> +    }
>> +
>> +    case HSR_CPREG32(DBGDSCRINT):
>> +    {
>>           /*
>> -         * Implement debug status and control register as RAZ/WI.
>> -         * The OS won't use Hardware debug if MDBGen not set.
>> +         * Bit 29: TX full, bit 30: RX full
>> +         * Given that we emulate DCC registers as RAZ/WI, doing the 
>> same for
>> +         * DBGDSCRint would cause a guest to continue using the DCC 
>> despite no
>> +         * real effect. Setting the TX/RX status bits should result 
>> in a probe
>> +         * fail (based on Linux behavior).
>>            */
>> -        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
>> +        register_t guest_reg_value = (1U << 29) | (1U << 30);
>> +
>> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1,
>> +                                  guest_reg_value);
>> +    }
>>   +    case HSR_CPREG32(DBGDTRTXINT):
>>       case HSR_CPREG32(DBGVCR):
>>       case HSR_CPREG32(DBGBVR0):
>>       case HSR_CPREG32(DBGBCR0):
>
> Cheers,
>

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

On 04/12/2023 13:02, Ayan Kumar Halder wrote:
> 
> On 04/12/2023 10:31, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien,
>>
>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>> an undefined exception to the guest. And Linux crashes.
>>
>> I am missing some data points here to be able to say whether I would 
>> be ok with emulating the registers. So some questions:
>>   * As you wrote below, HVC_DCC will return -ENODEV after this 
>> emulation. So may I ask what's the use case to enable it? (e.g. is 
>> there a distro turning this on?)
> 
> I am not aware of any distro using (or not using) this feature. This 
> issue came to light during our internal testing, when HVC_DCC was 
> enabled to use the debug console. When Linux runs without Xen, then we 
> could observe the logs on the debug console. When Linux was running as a 
> VM, it crashed.
> 
> My intention here was to do the bare minimum emulation so that the crash 
> could be avoided.
This reminds me a bit the discussion around "xen/arm64: Decode ldr/str 
post increment operations". I don't want Xen to contain half-backed 
emulation just to please an OS in certain configuration that doesn't 
seem to be often used.

Also, AFAICT, KVM is in the same situation...

Given this is internal testing, have you considered to ask them to 
disable HVC_DCC?

> 
>>  * Linux is writing to the registers unconditionally, but is the spec 
>> mandating the implementation of the registers? (I couldn't find either 
>> way)
> 
>  From ARM DDI 0487I.a ID081822, H1.2, External debug,
> 
> "The Debug Communications Channel, DCC, passes data between the PE and 
> the debugger:
> 
> — The DCC includes the data transfer registers, DTRRX and DTRTX, and 
> associated flow-control flags.
> 
> — Although the DCC is an essential part of Debug state operation, it can 
> also be used in Non-debug state."
> 
>  From this I infer that the spec mandates the implementation of these 
> registers. IOW, these registers should always be present in any Arm 
> compliant SoC.
> 
>>  * When was this check introduced in Linux? Did it ever changed?
>>
> This check was introduced in the following commit
> 
> "commit f377775dc083506e2fd7739d8615971c46b5246e
> Author: Rob Herring <rob.herring@calxeda.com>
> Date:   Tue Sep 24 21:05:58 2013 -0500
> 
>      TTY: hvc_dcc: probe for a JTAG connection before registering
> 
>      Enabling the ARM DCC console and using without a JTAG connection will
>      simply hang the system. Since distros like to turn on all options, 
> this
>      is a reoccurring problem to debug. We can do better by checking if
>      anything is attached and handling characters. There is no way to probe
>      this, so send a newline and check that it is handled.
> "

I think this is the part I was missing from the commit message. I have 
proposed some wording below.

> 
> As of today, this check hasn't changed.
> 
>>>
>>> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
>>> is emulated as TXfull | RXfull.
>>
>> Skimming through the Arm Arm, I see that TXfull and Rxfull indicates 
>> that both buffers are full but it doesn't explicitly say this means 
>> the feature is not available.
> We are not saying that the feature is not available. Rather ...
>>
>> I understand this is what Linux checks, but if we want to partially 
>> emulate the registers in Xen, then I'd like us to make sure this is 
>> backed by the Arm Arm rather than based on Linux implementation (which 
>> can change at any point).
>>
>>> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
>>> Also D17.3.7 DBGDTRRX_EL0,
>>> " If RXfull is set to 1, return the last value written to DTRRX."
>>>
>>> Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
>>> before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>>> hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
>>> the guest to be aborted.
>>
>> See above, what guarantees us that Linux will not change this behavior 
>> in the future?
> 
> If I understand "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN" 
> correctly, it seems that Arm ARM expects OS to check for TXfull.
> 
> If the condition is true, it should return some error.
> 
> Let me know if I misunderstood this.

You understand the Arm spec correcly. I think we are disagreeing on the 
wording and whether we should accept basic emulation (see above).

I would like more opinion on that.

[...]

>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>>> index 39aeda9dab..3f1276f96e 100644
>>> --- a/xen/arch/arm/vcpreg.c
>>> +++ b/xen/arch/arm/vcpreg.c
>>> @@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs, 
>>> const union hsr hsr)
>>>           break;
>>>       }
>>>   -    case HSR_CPREG32(DBGDSCRINT):
>>> +    case HSR_CPREG32(DBGDSCREXT):
>>> +    {
>>>           /*
>>> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
>>> -         * is set to 0, which we emulated below.
>>> +         * Bit 29: TX full, bit 30: RX full
>>> +         * Given that we emulate DCC registers as RAZ/WI, doing the 
>>> same for
>>> +         * DBGDSCRint would cause a guest to continue using the DCC 
>>> despite no
>>> +         * real effect. Setting the TX/RX status bits should result 
>>> in a probe
>>> +         * fail (based on Linux behavior).
>> If you want to mention Linux then you need to be a bit more specific 
>> because Linux can change at any point. So you at least want to specify 
>> the Linux version and place in the code.
>>
>> So this doesn't get stale as soon as the HVC_DCC driver changes.
> 
> (based on Linux behavior since f377775dc083).

Base on the discussion above, I would like to suggest the following:

Xen doesn't expose a real (or emulated) Debug Communications Channel 
(DCC) to a domain. Yet the Arm Arm implies this is not an optional 
feature. So some domains may start to probe it. For instance, the 
HVC_DCC driver in Linux (since f377775dc083 and at least up to v6.7), 
will try to write some characters and check if the transmit buffer has 
emptied. By setting TX status bit to indicate the transmit buffer is 
full. This we would hint the OS that the DCC is probably not working.

This would give enough information for the reader to know what's going 
and how you emulate.

Also, while writing the proposed comment, I wonder why we need to set 
RX? Wouldn't this potentially indicate to the OS that there are some 
data to read?

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

Hi Julien,

On 04/12/2023 20:55, Julien Grall wrote:
> 
> 
> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>
>> On 04/12/2023 10:31, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien,
>>>
>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>> an undefined exception to the guest. And Linux crashes.
>>>
>>> I am missing some data points here to be able to say whether I would
>>> be ok with emulating the registers. So some questions:
>>>   * As you wrote below, HVC_DCC will return -ENODEV after this
>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>> there a distro turning this on?)
>>
>> I am not aware of any distro using (or not using) this feature. This
>> issue came to light during our internal testing, when HVC_DCC was
>> enabled to use the debug console. When Linux runs without Xen, then we
>> could observe the logs on the debug console. When Linux was running as a
>> VM, it crashed.
>>
>> My intention here was to do the bare minimum emulation so that the crash
>> could be avoided.
> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
> post increment operations". I don't want Xen to contain half-backed
> emulation just to please an OS in certain configuration that doesn't
> seem to be often used.
> 
> Also, AFAICT, KVM is in the same situation...
Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so there
will be no fault on an attempt to access the DCC.

In general, I think that if a register is not optional and does not depend on other register
to be checked first (e.g. a feature/control register we emulate), which implies that it is fully ok for a guest to
access it directly - we should at least try to do something not to crash a guest.

I agree that this feature is not widely used. In fact I can only find it implemented in Linux and U-BOOT
and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate REXT.UDCCdis as 0) only
proves that. At the same time, it does not cost us much to add this trivial support.

> 
> Given this is internal testing, have you considered to ask them to
> disable HVC_DCC?
> 
>>
>>>  * Linux is writing to the registers unconditionally, but is the spec
>>> mandating the implementation of the registers? (I couldn't find either
>>> way)
>>
>>  From ARM DDI 0487I.a ID081822, H1.2, External debug,
>>
>> "The Debug Communications Channel, DCC, passes data between the PE and
>> the debugger:
>>
>> — The DCC includes the data transfer registers, DTRRX and DTRTX, and
>> associated flow-control flags.
>>
>> — Although the DCC is an essential part of Debug state operation, it can
>> also be used in Non-debug state."
>>
>>  From this I infer that the spec mandates the implementation of these
>> registers. IOW, these registers should always be present in any Arm
>> compliant SoC.
>>
>>>  * When was this check introduced in Linux? Did it ever changed?
>>>
>> This check was introduced in the following commit
>>
>> "commit f377775dc083506e2fd7739d8615971c46b5246e
>> Author: Rob Herring <rob.herring@calxeda.com>
>> Date:   Tue Sep 24 21:05:58 2013 -0500
>>
>>      TTY: hvc_dcc: probe for a JTAG connection before registering
>>
>>      Enabling the ARM DCC console and using without a JTAG connection will
>>      simply hang the system. Since distros like to turn on all options,
>> this
>>      is a reoccurring problem to debug. We can do better by checking if
>>      anything is attached and handling characters. There is no way to probe
>>      this, so send a newline and check that it is handled.
>> "
> 
> I think this is the part I was missing from the commit message. I have
> proposed some wording below.
> 
>>
>> As of today, this check hasn't changed.
>>
>>>>
>>>> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
>>>> is emulated as TXfull | RXfull.
>>>
>>> Skimming through the Arm Arm, I see that TXfull and Rxfull indicates
>>> that both buffers are full but it doesn't explicitly say this means
>>> the feature is not available.
>> We are not saying that the feature is not available. Rather ...
>>>
>>> I understand this is what Linux checks, but if we want to partially
>>> emulate the registers in Xen, then I'd like us to make sure this is
>>> backed by the Arm Arm rather than based on Linux implementation (which
>>> can change at any point).
>>>
>>>> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
>>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
>>>> Also D17.3.7 DBGDTRRX_EL0,
>>>> " If RXfull is set to 1, return the last value written to DTRRX."
>>>>
>>>> Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
>>>> before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>>>> hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
>>>> the guest to be aborted.
>>>
>>> See above, what guarantees us that Linux will not change this behavior
>>> in the future?
>>
>> If I understand "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
>> correctly, it seems that Arm ARM expects OS to check for TXfull.
>>
>> If the condition is true, it should return some error.
>>
>> Let me know if I misunderstood this.
> 
> You understand the Arm spec correcly. I think we are disagreeing on the
> wording and whether we should accept basic emulation (see above).
> 
> I would like more opinion on that.
> 
> [...]
> 
>>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>>>> index 39aeda9dab..3f1276f96e 100644
>>>> --- a/xen/arch/arm/vcpreg.c
>>>> +++ b/xen/arch/arm/vcpreg.c
>>>> @@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs,
>>>> const union hsr hsr)
>>>>           break;
>>>>       }
>>>>   -    case HSR_CPREG32(DBGDSCRINT):
>>>> +    case HSR_CPREG32(DBGDSCREXT):
>>>> +    {
>>>>           /*
>>>> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
>>>> -         * is set to 0, which we emulated below.
>>>> +         * Bit 29: TX full, bit 30: RX full
>>>> +         * Given that we emulate DCC registers as RAZ/WI, doing the
>>>> same for
>>>> +         * DBGDSCRint would cause a guest to continue using the DCC
>>>> despite no
>>>> +         * real effect. Setting the TX/RX status bits should result
>>>> in a probe
>>>> +         * fail (based on Linux behavior).
>>> If you want to mention Linux then you need to be a bit more specific
>>> because Linux can change at any point. So you at least want to specify
>>> the Linux version and place in the code.
>>>
>>> So this doesn't get stale as soon as the HVC_DCC driver changes.
>>
>> (based on Linux behavior since f377775dc083).
> 
> Base on the discussion above, I would like to suggest the following:
> 
> Xen doesn't expose a real (or emulated) Debug Communications Channel
> (DCC) to a domain. Yet the Arm Arm implies this is not an optional
> feature. So some domains may start to probe it. For instance, the
> HVC_DCC driver in Linux (since f377775dc083 and at least up to v6.7),
> will try to write some characters and check if the transmit buffer has
> emptied. By setting TX status bit to indicate the transmit buffer is
> full. This we would hint the OS that the DCC is probably not working.
> 
> This would give enough information for the reader to know what's going
> and how you emulate.
> 
> Also, while writing the proposed comment, I wonder why we need to set
> RX? Wouldn't this potentially indicate to the OS that there are some
> data to read?
You're right. No need for that.

~Michal

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

On 05/12/2023 09:28, Michal Orzel wrote:
> Hi Julien,
> 
> On 04/12/2023 20:55, Julien Grall wrote:
>>
>>
>> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>>
>>> On 04/12/2023 10:31, Julien Grall wrote:
>>>> Hi Ayan,
>>> Hi Julien,
>>>>
>>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>>> an undefined exception to the guest. And Linux crashes.
>>>>
>>>> I am missing some data points here to be able to say whether I would
>>>> be ok with emulating the registers. So some questions:
>>>>    * As you wrote below, HVC_DCC will return -ENODEV after this
>>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>>> there a distro turning this on?)
>>>
>>> I am not aware of any distro using (or not using) this feature. This
>>> issue came to light during our internal testing, when HVC_DCC was
>>> enabled to use the debug console. When Linux runs without Xen, then we
>>> could observe the logs on the debug console. When Linux was running as a
>>> VM, it crashed.
>>>
>>> My intention here was to do the bare minimum emulation so that the crash
>>> could be avoided.
>> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
>> post increment operations". I don't want Xen to contain half-backed
>> emulation just to please an OS in certain configuration that doesn't
>> seem to be often used.
>>
>> Also, AFAICT, KVM is in the same situation...
> Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so there
> will be no fault on an attempt to access the DCC.

Does this mean a guest will think the JTAG is availabe?

> 
> In general, I think that if a register is not optional and does not depend on other register
> to be checked first (e.g. a feature/control register we emulate), which implies that it is fully ok for a guest to
> access it directly - we should at least try to do something not to crash a guest.

This is where we have opposing opinion. I view crashing a guest better 
than providing a wrong emulation because it gives a clear signal that 
the register they are trying to access will not function properly.

We had this exact same discussion a few years ago when Linux started to 
access GIC*_ACTIVER registers. I know that Stefano was for emulating 
them as RAZ but this had consequences on the domain side (Linux 
sometimes need to read them). We settled on printing a warning which is 
not great but better than claiming we properly emulate the register.

> 
> I agree that this feature is not widely used. In fact I can only find it implemented in Linux and U-BOOT
> and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate REXT.UDCCdis as 0) only
> proves that. At the same time, it does not cost us much to add this trivial support.

See above. If we provide an (even basic) emulation, we need to make sure 
it is correct and doesn't have a side effect on the guest. If we can't 
guarantee that (e.g. like for set/way when a device is assigned), then 
the best course of action is to crash the domain.

AFAICT, the proposed emulation would be ok. But I want to make clear 
that I would not generally be ok with this approach and the decision 
would need to be on the case by case basis.

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

On 05/12/2023 11:01, Julien Grall wrote:
> 
> 
> On 05/12/2023 09:28, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 04/12/2023 20:55, Julien Grall wrote:
>>>
>>>
>>> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>>>
>>>> On 04/12/2023 10:31, Julien Grall wrote:
>>>>> Hi Ayan,
>>>> Hi Julien,
>>>>>
>>>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>>>> an undefined exception to the guest. And Linux crashes.
>>>>>
>>>>> I am missing some data points here to be able to say whether I would
>>>>> be ok with emulating the registers. So some questions:
>>>>>    * As you wrote below, HVC_DCC will return -ENODEV after this
>>>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>>>> there a distro turning this on?)
>>>>
>>>> I am not aware of any distro using (or not using) this feature. This
>>>> issue came to light during our internal testing, when HVC_DCC was
>>>> enabled to use the debug console. When Linux runs without Xen, then we
>>>> could observe the logs on the debug console. When Linux was running as a
>>>> VM, it crashed.
>>>>
>>>> My intention here was to do the bare minimum emulation so that the crash
>>>> could be avoided.
>>> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
>>> post increment operations". I don't want Xen to contain half-backed
>>> emulation just to please an OS in certain configuration that doesn't
>>> seem to be often used.
>>>
>>> Also, AFAICT, KVM is in the same situation...
>> Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so there
>> will be no fault on an attempt to access the DCC.
> 
> Does this mean a guest will think the JTAG is availabe?
Yes, it will believe the DCC is available which is not a totally bad idea. Yes, it will not have
any effect but at least covers the polling loop. The solution proposed here sounds better but does not take
into account the busy while loop when sending the char. Linux DCC earlycon does not make an initial check that runtime
driver does and will keep waiting in the loop if TXfull is set. Emulating everything as RAZ/WI solves that.
As you can see, each solution has its flaws and depends on the OS implementation.

> 
>>
>> In general, I think that if a register is not optional and does not depend on other register
>> to be checked first (e.g. a feature/control register we emulate), which implies that it is fully ok for a guest to
>> access it directly - we should at least try to do something not to crash a guest.
> 
> This is where we have opposing opinion. I view crashing a guest better
> than providing a wrong emulation because it gives a clear signal that
> the register they are trying to access will not function properly.
> 
> We had this exact same discussion a few years ago when Linux started to
> access GIC*_ACTIVER registers. I know that Stefano was for emulating
> them as RAZ but this had consequences on the domain side (Linux
> sometimes need to read them). We settled on printing a warning which is
> not great but better than claiming we properly emulate the register.
> 
>>
>> I agree that this feature is not widely used. In fact I can only find it implemented in Linux and U-BOOT
>> and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate REXT.UDCCdis as 0) only
>> proves that. At the same time, it does not cost us much to add this trivial support.
> 
> See above. If we provide an (even basic) emulation, we need to make sure
> it is correct and doesn't have a side effect on the guest. If we can't
> guarantee that (e.g. like for set/way when a device is assigned), then
> the best course of action is to crash the domain.
> 
> AFAICT, the proposed emulation would be ok. But I want to make clear
> that I would not generally be ok with this approach and the decision
> would need to be on the case by case basis.
This goes without saying. Every issue requires separate investigation.

~Michal

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

On 05/12/2023 10:30, Michal Orzel wrote:
> 
> 
> On 05/12/2023 11:01, Julien Grall wrote:
>>
>>
>> On 05/12/2023 09:28, Michal Orzel wrote:
>>> Hi Julien,
>>>
>>> On 04/12/2023 20:55, Julien Grall wrote:
>>>>
>>>>
>>>> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>>>>
>>>>> On 04/12/2023 10:31, Julien Grall wrote:
>>>>>> Hi Ayan,
>>>>> Hi Julien,
>>>>>>
>>>>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>>>>> an undefined exception to the guest. And Linux crashes.
>>>>>>
>>>>>> I am missing some data points here to be able to say whether I would
>>>>>> be ok with emulating the registers. So some questions:
>>>>>>     * As you wrote below, HVC_DCC will return -ENODEV after this
>>>>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>>>>> there a distro turning this on?)
>>>>>
>>>>> I am not aware of any distro using (or not using) this feature. This
>>>>> issue came to light during our internal testing, when HVC_DCC was
>>>>> enabled to use the debug console. When Linux runs without Xen, then we
>>>>> could observe the logs on the debug console. When Linux was running as a
>>>>> VM, it crashed.
>>>>>
>>>>> My intention here was to do the bare minimum emulation so that the crash
>>>>> could be avoided.
>>>> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
>>>> post increment operations". I don't want Xen to contain half-backed
>>>> emulation just to please an OS in certain configuration that doesn't
>>>> seem to be often used.
>>>>
>>>> Also, AFAICT, KVM is in the same situation...
>>> Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so there
>>> will be no fault on an attempt to access the DCC.
>>
>> Does this mean a guest will think the JTAG is availabe?
> Yes, it will believe the DCC is available which is not a totally bad idea. Yes, it will not have
> any effect but at least covers the polling loop. The solution proposed here sounds better but does not take
> into account the busy while loop when sending the char. Linux DCC earlycon does not make an initial check that runtime
> driver does and will keep waiting in the loop if TXfull is set. Emulating everything as RAZ/WI solves that.
> As you can see, each solution has its flaws and depends on the OS implementation.

Right, which prove my earlier point. You are providing an emulation just 
to please a specific driver in Linux (not even the whole Linux). This is 
what I was the most concern of.

So ...

>>> In general, I think that if a register is not optional and does not depend on other register
>>> to be checked first (e.g. a feature/control register we emulate), which implies that it is fully ok for a guest to
>>> access it directly - we should at least try to do something not to crash a guest.
>>
>> This is where we have opposing opinion. I view crashing a guest better
>> than providing a wrong emulation because it gives a clear signal that
>> the register they are trying to access will not function properly.
>>
>> We had this exact same discussion a few years ago when Linux started to
>> access GIC*_ACTIVER registers. I know that Stefano was for emulating
>> them as RAZ but this had consequences on the domain side (Linux
>> sometimes need to read them). We settled on printing a warning which is
>> not great but better than claiming we properly emulate the register.
>>
>>>
>>> I agree that this feature is not widely used. In fact I can only find it implemented in Linux and U-BOOT
>>> and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate REXT.UDCCdis as 0) only
>>> proves that. At the same time, it does not cost us much to add this trivial support.
>>
>> See above. If we provide an (even basic) emulation, we need to make sure
>> it is correct and doesn't have a side effect on the guest. If we can't
>> guarantee that (e.g. like for set/way when a device is assigned), then
>> the best course of action is to crash the domain.
>>
>> AFAICT, the proposed emulation would be ok.

... I will need to revise this statement. I am now against this patch.

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

On 05/12/2023 11:42, Julien Grall wrote:
> 
> 
> On 05/12/2023 10:30, Michal Orzel wrote:
>>
>>
>> On 05/12/2023 11:01, Julien Grall wrote:
>>>
>>>
>>> On 05/12/2023 09:28, Michal Orzel wrote:
>>>> Hi Julien,
>>>>
>>>> On 04/12/2023 20:55, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>>>>>
>>>>>> On 04/12/2023 10:31, Julien Grall wrote:
>>>>>>> Hi Ayan,
>>>>>> Hi Julien,
>>>>>>>
>>>>>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>>>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>>>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>>>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>>>>>> an undefined exception to the guest. And Linux crashes.
>>>>>>>
>>>>>>> I am missing some data points here to be able to say whether I would
>>>>>>> be ok with emulating the registers. So some questions:
>>>>>>>     * As you wrote below, HVC_DCC will return -ENODEV after this
>>>>>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>>>>>> there a distro turning this on?)
>>>>>>
>>>>>> I am not aware of any distro using (or not using) this feature. This
>>>>>> issue came to light during our internal testing, when HVC_DCC was
>>>>>> enabled to use the debug console. When Linux runs without Xen, then we
>>>>>> could observe the logs on the debug console. When Linux was running as a
>>>>>> VM, it crashed.
>>>>>>
>>>>>> My intention here was to do the bare minimum emulation so that the crash
>>>>>> could be avoided.
>>>>> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
>>>>> post increment operations". I don't want Xen to contain half-backed
>>>>> emulation just to please an OS in certain configuration that doesn't
>>>>> seem to be often used.
>>>>>
>>>>> Also, AFAICT, KVM is in the same situation...
>>>> Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so there
>>>> will be no fault on an attempt to access the DCC.
>>>
>>> Does this mean a guest will think the JTAG is availabe?
>> Yes, it will believe the DCC is available which is not a totally bad idea. Yes, it will not have
>> any effect but at least covers the polling loop. The solution proposed here sounds better but does not take
>> into account the busy while loop when sending the char. Linux DCC earlycon does not make an initial check that runtime
>> driver does and will keep waiting in the loop if TXfull is set. Emulating everything as RAZ/WI solves that.
>> As you can see, each solution has its flaws and depends on the OS implementation.
> 
> Right, which prove my earlier point. You are providing an emulation just
> to please a specific driver in Linux (not even the whole Linux). This is
> what I was the most concern of.
> 
> So ...
> 
>>>> In general, I think that if a register is not optional and does not depend on other register
>>>> to be checked first (e.g. a feature/control register we emulate), which implies that it is fully ok for a guest to
>>>> access it directly - we should at least try to do something not to crash a guest.
>>>
>>> This is where we have opposing opinion. I view crashing a guest better
>>> than providing a wrong emulation because it gives a clear signal that
>>> the register they are trying to access will not function properly.
>>>
>>> We had this exact same discussion a few years ago when Linux started to
>>> access GIC*_ACTIVER registers. I know that Stefano was for emulating
>>> them as RAZ but this had consequences on the domain side (Linux
>>> sometimes need to read them). We settled on printing a warning which is
>>> not great but better than claiming we properly emulate the register.
>>>
>>>>
>>>> I agree that this feature is not widely used. In fact I can only find it implemented in Linux and U-BOOT
>>>> and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate REXT.UDCCdis as 0) only
>>>> proves that. At the same time, it does not cost us much to add this trivial support.
>>>
>>> See above. If we provide an (even basic) emulation, we need to make sure
>>> it is correct and doesn't have a side effect on the guest. If we can't
>>> guarantee that (e.g. like for set/way when a device is assigned), then
>>> the best course of action is to crash the domain.
>>>
>>> AFAICT, the proposed emulation would be ok.
> 
> ... I will need to revise this statement. I am now against this patch.
Yes, the problem was tricky from the very beginning and I somewhat agree. I prepared a POC with one solution
that Ayan extended and sent to gather feedback (hence RFC). I think we should still wait for others
opinion (@Stefano, @Bertrand). I think the thread contains all the necessary information
to decide what to do:
- do nothing* (guest crashes)
- emulate DCC the same way as KVM i.e. RAZ/WI (no crash, no busy loop, guest keeps using DCC with no effect)
- emulate DCC with TXfull set to 1 (no crash, runtime DCC in Linux returns -ENODEV, earlycon busy loop issue)

* I still think we should fix DBGDSCRINT but I can send a separate patch (not really related to the DCC problem)

~Michal

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

Hi Julien/All,

On 05/12/2023 11:02, Michal Orzel wrote:
>
> On 05/12/2023 11:42, Julien Grall wrote:
>>
>> On 05/12/2023 10:30, Michal Orzel wrote:
>>>
>>> On 05/12/2023 11:01, Julien Grall wrote:
>>>>
>>>> On 05/12/2023 09:28, Michal Orzel wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On 04/12/2023 20:55, Julien Grall wrote:
>>>>>>
>>>>>> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>>>>>> On 04/12/2023 10:31, Julien Grall wrote:
>>>>>>>> Hi Ayan,
>>>>>>> Hi Julien,
>>>>>>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>>>>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>>>>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>>>>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>>>>>>> an undefined exception to the guest. And Linux crashes.
>>>>>>>> I am missing some data points here to be able to say whether I would
>>>>>>>> be ok with emulating the registers. So some questions:
>>>>>>>>      * As you wrote below, HVC_DCC will return -ENODEV after this
>>>>>>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>>>>>>> there a distro turning this on?)
>>>>>>> I am not aware of any distro using (or not using) this feature. This
>>>>>>> issue came to light during our internal testing, when HVC_DCC was
>>>>>>> enabled to use the debug console. When Linux runs without Xen, then we
>>>>>>> could observe the logs on the debug console. When Linux was running as a
>>>>>>> VM, it crashed.
>>>>>>>
>>>>>>> My intention here was to do the bare minimum emulation so that the crash
>>>>>>> could be avoided.
>>>>>> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
>>>>>> post increment operations". I don't want Xen to contain half-backed
>>>>>> emulation just to please an OS in certain configuration that doesn't
>>>>>> seem to be often used.
>>>>>>
>>>>>> Also, AFAICT, KVM is in the same situation...
>>>>> Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so there
>>>>> will be no fault on an attempt to access the DCC.
>>>> Does this mean a guest will think the JTAG is availabe?
>>> Yes, it will believe the DCC is available which is not a totally bad idea. Yes, it will not have
>>> any effect but at least covers the polling loop. The solution proposed here sounds better but does not take
>>> into account the busy while loop when sending the char. Linux DCC earlycon does not make an initial check that runtime
>>> driver does and will keep waiting in the loop if TXfull is set. Emulating everything as RAZ/WI solves that.
>>> As you can see, each solution has its flaws and depends on the OS implementation.
>> Right, which prove my earlier point. You are providing an emulation just
>> to please a specific driver in Linux (not even the whole Linux). This is
>> what I was the most concern of.
I have sent out a patch ("[PATCH] tty: hvc: dcc: Check for TXfull 
condition while setting up early console") to fix this.
>>
>> So ...
>>
>>>>> In general, I think that if a register is not optional and does not depend on other register
>>>>> to be checked first (e.g. a feature/control register we emulate), which implies that it is fully ok for a guest to
>>>>> access it directly - we should at least try to do something not to crash a guest.
>>>> This is where we have opposing opinion. I view crashing a guest better
>>>> than providing a wrong emulation because it gives a clear signal that
>>>> the register they are trying to access will not function properly.
>>>>
>>>> We had this exact same discussion a few years ago when Linux started to
>>>> access GIC*_ACTIVER registers. I know that Stefano was for emulating
>>>> them as RAZ but this had consequences on the domain side (Linux
>>>> sometimes need to read them). We settled on printing a warning which is
>>>> not great but better than claiming we properly emulate the register.
>>>>
>>>>> I agree that this feature is not widely used. In fact I can only find it implemented in Linux and U-BOOT
>>>>> and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate REXT.UDCCdis as 0) only
>>>>> proves that. At the same time, it does not cost us much to add this trivial support.
>>>> See above. If we provide an (even basic) emulation, we need to make sure
>>>> it is correct and doesn't have a side effect on the guest. If we can't
>>>> guarantee that (e.g. like for set/way when a device is assigned), then
>>>> the best course of action is to crash the domain.
>>>>
>>>> AFAICT, the proposed emulation would be ok.
>> ... I will need to revise this statement. I am now against this patch.
> Yes, the problem was tricky from the very beginning and I somewhat agree. I prepared a POC with one solution
> that Ayan extended and sent to gather feedback (hence RFC). I think we should still wait for others
> opinion (@Stefano, @Bertrand). I think the thread contains all the necessary information
> to decide what to do:
> - do nothing* (guest crashes)
> - emulate DCC the same way as KVM i.e. RAZ/WI (no crash, no busy loop, guest keeps using DCC with no effect)
> - emulate DCC with TXfull set to 1 (no crash, runtime DCC in Linux returns -ENODEV, earlycon busy loop issue)
>
> * I still think we should fix DBGDSCRINT but I can send a separate patch (not really related to the DCC problem)

Regardless if the linux hvc earlycon is fixed or not

@Julien , would you be ok with option 2 or 3 with a suitable warning ?

Also will wait for Stefano's and Bertrand's opinions.

Crashing the guest would seem quite severe imo if there can be a better 
way (option 2 or 3) to tell that DCC is not available.

- Ayan

>
> ~Michal
>

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

Hi Ayan,

On 05/12/2023 12:50, Ayan Kumar Halder wrote:
> Hi Julien/All,
> 
> On 05/12/2023 11:02, Michal Orzel wrote:
>>
>> On 05/12/2023 11:42, Julien Grall wrote:
>>>
>>> On 05/12/2023 10:30, Michal Orzel wrote:
>>>>
>>>> On 05/12/2023 11:01, Julien Grall wrote:
>>>>>
>>>>> On 05/12/2023 09:28, Michal Orzel wrote:
>>>>>> Hi Julien,
>>>>>>
>>>>>> On 04/12/2023 20:55, Julien Grall wrote:
>>>>>>>
>>>>>>> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>>>>>>> On 04/12/2023 10:31, Julien Grall wrote:
>>>>>>>>> Hi Ayan,
>>>>>>>> Hi Julien,
>>>>>>>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>>>>>>>> Currently if user enables HVC_DCC config option in Linux, it 
>>>>>>>>>> invokes
>>>>>>>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on 
>>>>>>>>>> arm64,
>>>>>>>>>> DBGDTRTXINT on arm32). As these registers are not emulated, 
>>>>>>>>>> Xen injects
>>>>>>>>>> an undefined exception to the guest. And Linux crashes.
>>>>>>>>> I am missing some data points here to be able to say whether I 
>>>>>>>>> would
>>>>>>>>> be ok with emulating the registers. So some questions:
>>>>>>>>>      * As you wrote below, HVC_DCC will return -ENODEV after this
>>>>>>>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>>>>>>>> there a distro turning this on?)
>>>>>>>> I am not aware of any distro using (or not using) this feature. 
>>>>>>>> This
>>>>>>>> issue came to light during our internal testing, when HVC_DCC was
>>>>>>>> enabled to use the debug console. When Linux runs without Xen, 
>>>>>>>> then we
>>>>>>>> could observe the logs on the debug console. When Linux was 
>>>>>>>> running as a
>>>>>>>> VM, it crashed.
>>>>>>>>
>>>>>>>> My intention here was to do the bare minimum emulation so that 
>>>>>>>> the crash
>>>>>>>> could be avoided.
>>>>>>> This reminds me a bit the discussion around "xen/arm64: Decode 
>>>>>>> ldr/str
>>>>>>> post increment operations". I don't want Xen to contain half-backed
>>>>>>> emulation just to please an OS in certain configuration that doesn't
>>>>>>> seem to be often used.
>>>>>>>
>>>>>>> Also, AFAICT, KVM is in the same situation...
>>>>>> Well, KVM is not in the same situation. It emulates all DCC regs 
>>>>>> as RAZ/WI, so there
>>>>>> will be no fault on an attempt to access the DCC.
>>>>> Does this mean a guest will think the JTAG is availabe?
>>>> Yes, it will believe the DCC is available which is not a totally bad 
>>>> idea. Yes, it will not have
>>>> any effect but at least covers the polling loop. The solution 
>>>> proposed here sounds better but does not take
>>>> into account the busy while loop when sending the char. Linux DCC 
>>>> earlycon does not make an initial check that runtime
>>>> driver does and will keep waiting in the loop if TXfull is set. 
>>>> Emulating everything as RAZ/WI solves that.
>>>> As you can see, each solution has its flaws and depends on the OS 
>>>> implementation.
>>> Right, which prove my earlier point. You are providing an emulation just
>>> to please a specific driver in Linux (not even the whole Linux). This is
>>> what I was the most concern of.
> I have sent out a patch ("[PATCH] tty: hvc: dcc: Check for TXfull 
> condition while setting up early console") to fix this.
>>>
>>> So ...
>>>
>>>>>> In general, I think that if a register is not optional and does 
>>>>>> not depend on other register
>>>>>> to be checked first (e.g. a feature/control register we emulate), 
>>>>>> which implies that it is fully ok for a guest to
>>>>>> access it directly - we should at least try to do something not to 
>>>>>> crash a guest.
>>>>> This is where we have opposing opinion. I view crashing a guest better
>>>>> than providing a wrong emulation because it gives a clear signal that
>>>>> the register they are trying to access will not function properly.
>>>>>
>>>>> We had this exact same discussion a few years ago when Linux 
>>>>> started to
>>>>> access GIC*_ACTIVER registers. I know that Stefano was for emulating
>>>>> them as RAZ but this had consequences on the domain side (Linux
>>>>> sometimes need to read them). We settled on printing a warning 
>>>>> which is
>>>>> not great but better than claiming we properly emulate the register.
>>>>>
>>>>>> I agree that this feature is not widely used. In fact I can only 
>>>>>> find it implemented in Linux and U-BOOT
>>>>>> and the issue I found in DBGDSCRINT (no access from EL0, even 
>>>>>> though we emulate REXT.UDCCdis as 0) only
>>>>>> proves that. At the same time, it does not cost us much to add 
>>>>>> this trivial support.
>>>>> See above. If we provide an (even basic) emulation, we need to make 
>>>>> sure
>>>>> it is correct and doesn't have a side effect on the guest. If we can't
>>>>> guarantee that (e.g. like for set/way when a device is assigned), then
>>>>> the best course of action is to crash the domain.
>>>>>
>>>>> AFAICT, the proposed emulation would be ok.
>>> ... I will need to revise this statement. I am now against this patch.
>> Yes, the problem was tricky from the very beginning and I somewhat 
>> agree. I prepared a POC with one solution
>> that Ayan extended and sent to gather feedback (hence RFC). I think we 
>> should still wait for others
>> opinion (@Stefano, @Bertrand). I think the thread contains all the 
>> necessary information
>> to decide what to do:
>> - do nothing* (guest crashes)
>> - emulate DCC the same way as KVM i.e. RAZ/WI (no crash, no busy loop, 
>> guest keeps using DCC with no effect)
>> - emulate DCC with TXfull set to 1 (no crash, runtime DCC in Linux 
>> returns -ENODEV, earlycon busy loop issue)
>>
>> * I still think we should fix DBGDSCRINT but I can send a separate 
>> patch (not really related to the DCC problem)
> 
> Regardless if the linux hvc earlycon is fixed or not
> 
> @Julien , would you be ok with option 2 or 3 with a suitable warning ?

I am afraid the answer is no.

> Also will wait for Stefano's and Bertrand's opinions.
> 
> Crashing the guest would seem quite severe imo if there can be a better 
> way (option 2 or 3) to tell that DCC is not available.

Well in option 2, you don't tell the DCC is not available. You just lie 
to it claiming there is one but it is not behaving properly.

I agree that crashing a guest is bad, but is lying to the domain really 
better? The consequence here is not that bad and hopefully it would be 
fairly easy to find. But this is not always the case. So I definitely 
would place a half-backed emulation more severe than a guest crash.

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Stefano Stabellini]

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

On Tue, 5 Dec 2023, Julien Grall wrote:
> Hi Ayan,
> 
> On 05/12/2023 12:50, Ayan Kumar Halder wrote:
> > Hi Julien/All,
> > 
> > On 05/12/2023 11:02, Michal Orzel wrote:
> > > 
> > > On 05/12/2023 11:42, Julien Grall wrote:
> > > > 
> > > > On 05/12/2023 10:30, Michal Orzel wrote:
> > > > > 
> > > > > On 05/12/2023 11:01, Julien Grall wrote:
> > > > > > 
> > > > > > On 05/12/2023 09:28, Michal Orzel wrote:
> > > > > > > Hi Julien,
> > > > > > > 
> > > > > > > On 04/12/2023 20:55, Julien Grall wrote:
> > > > > > > > 
> > > > > > > > On 04/12/2023 13:02, Ayan Kumar Halder wrote:
> > > > > > > > > On 04/12/2023 10:31, Julien Grall wrote:
> > > > > > > > > > Hi Ayan,
> > > > > > > > > Hi Julien,
> > > > > > > > > > On 01/12/2023 18:50, Ayan Kumar Halder wrote:
> > > > > > > > > > > Currently if user enables HVC_DCC config option in Linux,
> > > > > > > > > > > it invokes
> > > > > > > > > > > access to debug data transfer registers (ie DBGDTRTX_EL0
> > > > > > > > > > > on arm64,
> > > > > > > > > > > DBGDTRTXINT on arm32). As these registers are not
> > > > > > > > > > > emulated, Xen injects
> > > > > > > > > > > an undefined exception to the guest. And Linux crashes.
> > > > > > > > > > I am missing some data points here to be able to say whether
> > > > > > > > > > I would
> > > > > > > > > > be ok with emulating the registers. So some questions:
> > > > > > > > > >      * As you wrote below, HVC_DCC will return -ENODEV after
> > > > > > > > > > this
> > > > > > > > > > emulation. So may I ask what's the use case to enable it?
> > > > > > > > > > (e.g. is
> > > > > > > > > > there a distro turning this on?)
> > > > > > > > > I am not aware of any distro using (or not using) this
> > > > > > > > > feature. This
> > > > > > > > > issue came to light during our internal testing, when HVC_DCC
> > > > > > > > > was
> > > > > > > > > enabled to use the debug console. When Linux runs without Xen,
> > > > > > > > > then we
> > > > > > > > > could observe the logs on the debug console. When Linux was
> > > > > > > > > running as a
> > > > > > > > > VM, it crashed.
> > > > > > > > > 
> > > > > > > > > My intention here was to do the bare minimum emulation so that
> > > > > > > > > the crash
> > > > > > > > > could be avoided.
> > > > > > > > This reminds me a bit the discussion around "xen/arm64: Decode
> > > > > > > > ldr/str
> > > > > > > > post increment operations". I don't want Xen to contain
> > > > > > > > half-backed
> > > > > > > > emulation just to please an OS in certain configuration that
> > > > > > > > doesn't
> > > > > > > > seem to be often used.
> > > > > > > > 
> > > > > > > > Also, AFAICT, KVM is in the same situation...
> > > > > > > Well, KVM is not in the same situation. It emulates all DCC regs
> > > > > > > as RAZ/WI, so there
> > > > > > > will be no fault on an attempt to access the DCC.
> > > > > > Does this mean a guest will think the JTAG is availabe?
> > > > > Yes, it will believe the DCC is available which is not a totally bad
> > > > > idea. Yes, it will not have
> > > > > any effect but at least covers the polling loop. The solution proposed
> > > > > here sounds better but does not take
> > > > > into account the busy while loop when sending the char. Linux DCC
> > > > > earlycon does not make an initial check that runtime
> > > > > driver does and will keep waiting in the loop if TXfull is set.
> > > > > Emulating everything as RAZ/WI solves that.
> > > > > As you can see, each solution has its flaws and depends on the OS
> > > > > implementation.
> > > > Right, which prove my earlier point. You are providing an emulation just
> > > > to please a specific driver in Linux (not even the whole Linux). This is
> > > > what I was the most concern of.
> > I have sent out a patch ("[PATCH] tty: hvc: dcc: Check for TXfull condition
> > while setting up early console") to fix this.
> > > > 
> > > > So ...
> > > > 
> > > > > > > In general, I think that if a register is not optional and does
> > > > > > > not depend on other register
> > > > > > > to be checked first (e.g. a feature/control register we emulate),
> > > > > > > which implies that it is fully ok for a guest to
> > > > > > > access it directly - we should at least try to do something not to
> > > > > > > crash a guest.
> > > > > > This is where we have opposing opinion. I view crashing a guest
> > > > > > better
> > > > > > than providing a wrong emulation because it gives a clear signal
> > > > > > that
> > > > > > the register they are trying to access will not function properly.
> > > > > > 
> > > > > > We had this exact same discussion a few years ago when Linux started
> > > > > > to
> > > > > > access GIC*_ACTIVER registers. I know that Stefano was for emulating
> > > > > > them as RAZ but this had consequences on the domain side (Linux
> > > > > > sometimes need to read them). We settled on printing a warning which
> > > > > > is
> > > > > > not great but better than claiming we properly emulate the register.
> > > > > > 
> > > > > > > I agree that this feature is not widely used. In fact I can only
> > > > > > > find it implemented in Linux and U-BOOT
> > > > > > > and the issue I found in DBGDSCRINT (no access from EL0, even
> > > > > > > though we emulate REXT.UDCCdis as 0) only
> > > > > > > proves that. At the same time, it does not cost us much to add
> > > > > > > this trivial support.
> > > > > > See above. If we provide an (even basic) emulation, we need to make
> > > > > > sure
> > > > > > it is correct and doesn't have a side effect on the guest. If we
> > > > > > can't
> > > > > > guarantee that (e.g. like for set/way when a device is assigned),
> > > > > > then
> > > > > > the best course of action is to crash the domain.
> > > > > > 
> > > > > > AFAICT, the proposed emulation would be ok.
> > > > ... I will need to revise this statement. I am now against this patch.
> > > Yes, the problem was tricky from the very beginning and I somewhat agree.
> > > I prepared a POC with one solution
> > > that Ayan extended and sent to gather feedback (hence RFC). I think we
> > > should still wait for others
> > > opinion (@Stefano, @Bertrand). I think the thread contains all the
> > > necessary information
> > > to decide what to do:
> > > - do nothing* (guest crashes)
> > > - emulate DCC the same way as KVM i.e. RAZ/WI (no crash, no busy loop,
> > > guest keeps using DCC with no effect)
> > > - emulate DCC with TXfull set to 1 (no crash, runtime DCC in Linux returns
> > > -ENODEV, earlycon busy loop issue)
> > > 
> > > * I still think we should fix DBGDSCRINT but I can send a separate patch
> > > (not really related to the DCC problem)
> > 
> > Regardless if the linux hvc earlycon is fixed or not
> > 
> > @Julien , would you be ok with option 2 or 3 with a suitable warning ?
> 
> I am afraid the answer is no.
> 
> > Also will wait for Stefano's and Bertrand's opinions.
> > 
> > Crashing the guest would seem quite severe imo if there can be a better way
> > (option 2 or 3) to tell that DCC is not available.
> 
> Well in option 2, you don't tell the DCC is not available. You just lie to it
> claiming there is one but it is not behaving properly.
> 
> I agree that crashing a guest is bad, but is lying to the domain really
> better? The consequence here is not that bad and hopefully it would be fairly
> easy to find. But this is not always the case. So I definitely would place a
> half-backed emulation more severe than a guest crash.


I see where Julien is coming from, but I would go with option two:
"emulate DCC the same way as KVM". That's because I don't think we can
get away with crashing the guest in all cases. Although the issue came
up with a Linux guest, it could have been triggered by a proprietary
operating system that we cannot change, and I think Xen should support
running unmodified operating systems.

If we go with a "half-backed emulation" solution, as Julien wrote, then
it is better to be more similar to other hypervisors, that's why I chose
option two instead of option three.

But at the same time I recognize the validity of Julien's words and it
makes me wonder if we should have a KCONFIG option or command line
option to switch the Xen behavior. We could use it to gate all the
"half-backed emulation" we do for compatibility.  Something like:

config PARTIAL_EMULATION
    bool "Partial Emulation"
    ---help---
     
    Enables partial, not spec compliant, emulation of certain register
    interfaces (e.g DCC UART) for guest compatibility. If you disable
    this option, Xen will crash the guest if the guest tries to access
    interfaces not fully emulated or virtualized.

    If you enable this option, the guest might misbehave due to non-spec
    compliant emulation done by Xen.

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

Hi Stefano,

On 05/12/2023 23:21, Stefano Stabellini wrote:
> On Tue, 5 Dec 2023, Julien Grall wrote:
>> I agree that crashing a guest is bad, but is lying to the domain really
>> better? The consequence here is not that bad and hopefully it would be fairly
>> easy to find. But this is not always the case. So I definitely would place a
>> half-backed emulation more severe than a guest crash.
> 
> 
> I see where Julien is coming from, but I would go with option two:
> "emulate DCC the same way as KVM". That's because I don't think we can
> get away with crashing the guest in all cases. Although the issue came
> up with a Linux guest, it could have been triggered by a proprietary
> operating system that we cannot change, and I think Xen should support
> running unmodified operating systems.
> 
> If we go with a "half-backed emulation" solution, as Julien wrote, then
> it is better to be more similar to other hypervisors, that's why I chose
> option two instead of option three.
> 
> But at the same time I recognize the validity of Julien's words and it
> makes me wonder if we should have a KCONFIG option or command line
> option to switch the Xen behavior. We could use it to gate all the
> "half-backed emulation" we do for compatibility.  Something like:
> 
> config PARTIAL_EMULATION
>      bool "Partial Emulation"
>      ---help---
>       
>      Enables partial, not spec compliant, emulation of certain register
>      interfaces (e.g DCC UART) for guest compatibility. If you disable
>      this option, Xen will crash the guest if the guest tries to access
>      interfaces not fully emulated or virtualized.
> 
>      If you enable this option, the guest might misbehave due to non-spec
>      compliant emulation done by Xen.

As I wrote to Ayan on Matrix today, I am not in favor of the emulation. 
Yet, I am not going to oppose (as in Nack it) if the other maintainers 
agree with it.

The KConfig would be nice, the question is whether we want to (security) 
support such configuration? E.g. could this potentially introduce a 
security issue in the guest?

Regarding the  emulation itself, I actually prefer 3 because at least 
the Linux drivers will be able to bail out rather than trying to use them.

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Stefano Stabellini]

On Thu, 7 Dec 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 05/12/2023 23:21, Stefano Stabellini wrote:
> > On Tue, 5 Dec 2023, Julien Grall wrote:
> > > I agree that crashing a guest is bad, but is lying to the domain really
> > > better? The consequence here is not that bad and hopefully it would be
> > > fairly
> > > easy to find. But this is not always the case. So I definitely would place
> > > a
> > > half-backed emulation more severe than a guest crash.
> > 
> > 
> > I see where Julien is coming from, but I would go with option two:
> > "emulate DCC the same way as KVM". That's because I don't think we can
> > get away with crashing the guest in all cases. Although the issue came
> > up with a Linux guest, it could have been triggered by a proprietary
> > operating system that we cannot change, and I think Xen should support
> > running unmodified operating systems.
> > 
> > If we go with a "half-backed emulation" solution, as Julien wrote, then
> > it is better to be more similar to other hypervisors, that's why I chose
> > option two instead of option three.
> > 
> > But at the same time I recognize the validity of Julien's words and it
> > makes me wonder if we should have a KCONFIG option or command line
> > option to switch the Xen behavior. We could use it to gate all the
> > "half-backed emulation" we do for compatibility.  Something like:
> > 
> > config PARTIAL_EMULATION
> >      bool "Partial Emulation"
> >      ---help---
> >            Enables partial, not spec compliant, emulation of certain
> > register
> >      interfaces (e.g DCC UART) for guest compatibility. If you disable
> >      this option, Xen will crash the guest if the guest tries to access
> >      interfaces not fully emulated or virtualized.
> > 
> >      If you enable this option, the guest might misbehave due to non-spec
> >      compliant emulation done by Xen.
> 
> As I wrote to Ayan on Matrix today, I am not in favor of the emulation. Yet, I
> am not going to oppose (as in Nack it) if the other maintainers agree with it.

Thanks for being flexible


> The KConfig would be nice, the question is whether we want to (security)
> support such configuration? E.g. could this potentially introduce a security
> issue in the guest?

The important question is whether it could introduce a security issue in
Xen. If we think it wouldn't increase the attack surface significantly
then I would security support it otherwise not.


> Regarding the  emulation itself, I actually prefer 3 because at least the
> Linux drivers will be able to bail out rather than trying to use them.

I don't have a strong opinion between 2 and 3

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Bertrand Marquis]

Hi All,

Sorry for coming back late on this thread.

> On 7 Dec 2023, at 22:41, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 7 Dec 2023, Julien Grall wrote:
>> Hi Stefano,
>> 
>> On 05/12/2023 23:21, Stefano Stabellini wrote:
>>> On Tue, 5 Dec 2023, Julien Grall wrote:
>>>> I agree that crashing a guest is bad, but is lying to the domain really
>>>> better? The consequence here is not that bad and hopefully it would be
>>>> fairly
>>>> easy to find. But this is not always the case. So I definitely would place
>>>> a
>>>> half-backed emulation more severe than a guest crash.
>>> 
>>> 
>>> I see where Julien is coming from, but I would go with option two:
>>> "emulate DCC the same way as KVM". That's because I don't think we can
>>> get away with crashing the guest in all cases. Although the issue came
>>> up with a Linux guest, it could have been triggered by a proprietary
>>> operating system that we cannot change, and I think Xen should support
>>> running unmodified operating systems.
>>> 
>>> If we go with a "half-backed emulation" solution, as Julien wrote, then
>>> it is better to be more similar to other hypervisors, that's why I chose
>>> option two instead of option three.
>>> 
>>> But at the same time I recognize the validity of Julien's words and it
>>> makes me wonder if we should have a KCONFIG option or command line
>>> option to switch the Xen behavior. We could use it to gate all the
>>> "half-backed emulation" we do for compatibility.  Something like:
>>> 
>>> config PARTIAL_EMULATION
>>>     bool "Partial Emulation"
>>>     ---help---
>>>           Enables partial, not spec compliant, emulation of certain
>>> register
>>>     interfaces (e.g DCC UART) for guest compatibility. If you disable
>>>     this option, Xen will crash the guest if the guest tries to access
>>>     interfaces not fully emulated or virtualized.
>>> 
>>>     If you enable this option, the guest might misbehave due to non-spec
>>>     compliant emulation done by Xen.
>> 
>> As I wrote to Ayan on Matrix today, I am not in favor of the emulation. Yet, I
>> am not going to oppose (as in Nack it) if the other maintainers agree with it.
> 
> Thanks for being flexible
> 
> 
>> The KConfig would be nice, the question is whether we want to (security)
>> support such configuration? E.g. could this potentially introduce a security
>> issue in the guest?
> 
> The important question is whether it could introduce a security issue in
> Xen. If we think it wouldn't increase the attack surface significantly
> then I would security support it otherwise not.
> 
> 
>> Regarding the  emulation itself, I actually prefer 3 because at least the
>> Linux drivers will be able to bail out rather than trying to use them.
> 
> I don't have a strong opinion between 2 and 3

Here is my view on it:
- providing a wrong emulation to please guests is not wrong as it might end
up hidding something that will be hard to debug so on that point I agree with
Julien.
- choosing a solution which might just crash a guest without any other solution
than recompiling or modifying xen is not something acceptable if we want Xen
to thrive.

So i would suggest the following solution:
- have a Kconfig to surround this code so that "correct" guests can disable it.
- have a command line option to activate this behavior and turn it off by default.
One encountering the problem will have to explicitly set a command line parameter
so cannot do this without knowing.
- activate the Kconfig option by default and security support it as it is only active if
a command line parameter is passed.

The Kconfig parameter should be more generic so that this could apply to a bunch of
registers we would emulate with RAZ/WI so I am happy with that proposal if we say
that this must be activated through a command line option passed to Xen at boot.

Regards
Bertrand

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers\

[Stefano Stabellini]

On Fri, 8 Dec 2023, Bertrand Marquis wrote:
> Hi All,
> 
> Sorry for coming back late on this thread.
> 
> > On 7 Dec 2023, at 22:41, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Thu, 7 Dec 2023, Julien Grall wrote:
> >> Hi Stefano,
> >> 
> >> On 05/12/2023 23:21, Stefano Stabellini wrote:
> >>> On Tue, 5 Dec 2023, Julien Grall wrote:
> >>>> I agree that crashing a guest is bad, but is lying to the domain really
> >>>> better? The consequence here is not that bad and hopefully it would be
> >>>> fairly
> >>>> easy to find. But this is not always the case. So I definitely would place
> >>>> a
> >>>> half-backed emulation more severe than a guest crash.
> >>> 
> >>> 
> >>> I see where Julien is coming from, but I would go with option two:
> >>> "emulate DCC the same way as KVM". That's because I don't think we can
> >>> get away with crashing the guest in all cases. Although the issue came
> >>> up with a Linux guest, it could have been triggered by a proprietary
> >>> operating system that we cannot change, and I think Xen should support
> >>> running unmodified operating systems.
> >>> 
> >>> If we go with a "half-backed emulation" solution, as Julien wrote, then
> >>> it is better to be more similar to other hypervisors, that's why I chose
> >>> option two instead of option three.
> >>> 
> >>> But at the same time I recognize the validity of Julien's words and it
> >>> makes me wonder if we should have a KCONFIG option or command line
> >>> option to switch the Xen behavior. We could use it to gate all the
> >>> "half-backed emulation" we do for compatibility.  Something like:
> >>> 
> >>> config PARTIAL_EMULATION
> >>>     bool "Partial Emulation"
> >>>     ---help---
> >>>           Enables partial, not spec compliant, emulation of certain
> >>> register
> >>>     interfaces (e.g DCC UART) for guest compatibility. If you disable
> >>>     this option, Xen will crash the guest if the guest tries to access
> >>>     interfaces not fully emulated or virtualized.
> >>> 
> >>>     If you enable this option, the guest might misbehave due to non-spec
> >>>     compliant emulation done by Xen.
> >> 
> >> As I wrote to Ayan on Matrix today, I am not in favor of the emulation. Yet, I
> >> am not going to oppose (as in Nack it) if the other maintainers agree with it.
> > 
> > Thanks for being flexible
> > 
> > 
> >> The KConfig would be nice, the question is whether we want to (security)
> >> support such configuration? E.g. could this potentially introduce a security
> >> issue in the guest?
> > 
> > The important question is whether it could introduce a security issue in
> > Xen. If we think it wouldn't increase the attack surface significantly
> > then I would security support it otherwise not.
> > 
> > 
> >> Regarding the  emulation itself, I actually prefer 3 because at least the
> >> Linux drivers will be able to bail out rather than trying to use them.
> > 
> > I don't have a strong opinion between 2 and 3
> 
> Here is my view on it:
> - providing a wrong emulation to please guests is not wrong as it might end
> up hidding something that will be hard to debug so on that point I agree with
> Julien.
> - choosing a solution which might just crash a guest without any other solution
> than recompiling or modifying xen is not something acceptable if we want Xen
> to thrive.
> 
> So i would suggest the following solution:
> - have a Kconfig to surround this code so that "correct" guests can disable it.
> - have a command line option to activate this behavior and turn it off by default.
> One encountering the problem will have to explicitly set a command line parameter
> so cannot do this without knowing.
> - activate the Kconfig option by default and security support it as it is only active if
> a command line parameter is passed.
> 
> The Kconfig parameter should be more generic so that this could apply to a bunch of
> registers we would emulate with RAZ/WI so I am happy with that proposal if we say
> that this must be activated through a command line option passed to Xen at boot.

You are suggesting both a Kconfig and a command line option.

The Kconfig would be useful so that in a strict configuration we can
disable the code even from the build (useful for instance in
configurations for safety certifications.)

The Kconfig option would be enabled by default (which is important for
the out of the box experience otherwise users would have to manually
rebuild Xen to run their guests.)

However, the actual partial emulation is only enabled if a command line
option is passed. The command line option would be off by default. So
the code is there, but wouldn't be used unless the user enables the
command line option explicitly. This way, tools aimed at making the out
of the box experience better (like ImageBuilder) might pass the command
line option by default but production build systems (like Yocto)
wouldn't.

Yes I think this is the best compromise. If it was just for this patch I
would say it is a bit too much but we have seen a few of these cases so
I think this is a general framework that will be useful in multiple
instances. I am fine with this.

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Julien Grall]

Hi,

On 07/12/2023 21:41, Stefano Stabellini wrote:
> On Thu, 7 Dec 2023, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 05/12/2023 23:21, Stefano Stabellini wrote:
>>> On Tue, 5 Dec 2023, Julien Grall wrote:
>>>> I agree that crashing a guest is bad, but is lying to the domain really
>>>> better? The consequence here is not that bad and hopefully it would be
>>>> fairly
>>>> easy to find. But this is not always the case. So I definitely would place
>>>> a
>>>> half-backed emulation more severe than a guest crash.
>>>
>>>
>>> I see where Julien is coming from, but I would go with option two:
>>> "emulate DCC the same way as KVM". That's because I don't think we can
>>> get away with crashing the guest in all cases. Although the issue came
>>> up with a Linux guest, it could have been triggered by a proprietary
>>> operating system that we cannot change, and I think Xen should support
>>> running unmodified operating systems.
>>>
>>> If we go with a "half-backed emulation" solution, as Julien wrote, then
>>> it is better to be more similar to other hypervisors, that's why I chose
>>> option two instead of option three.
>>>
>>> But at the same time I recognize the validity of Julien's words and it
>>> makes me wonder if we should have a KCONFIG option or command line
>>> option to switch the Xen behavior. We could use it to gate all the
>>> "half-backed emulation" we do for compatibility.  Something like:
>>>
>>> config PARTIAL_EMULATION
>>>       bool "Partial Emulation"
>>>       ---help---
>>>             Enables partial, not spec compliant, emulation of certain
>>> register
>>>       interfaces (e.g DCC UART) for guest compatibility. If you disable
>>>       this option, Xen will crash the guest if the guest tries to access
>>>       interfaces not fully emulated or virtualized.
>>>
>>>       If you enable this option, the guest might misbehave due to non-spec
>>>       compliant emulation done by Xen.
>>
>> As I wrote to Ayan on Matrix today, I am not in favor of the emulation. Yet, I
>> am not going to oppose (as in Nack it) if the other maintainers agree with it.
> 
> Thanks for being flexible
> 
> 
>> The KConfig would be nice, the question is whether we want to (security)
>> support such configuration? E.g. could this potentially introduce a security
>> issue in the guest?
> 
> The important question is whether it could introduce a security issue in
> Xen. If we think it wouldn't increase the attack surface significantly
> then I would security support it otherwise not.

For this specific emulation, it is unlikely. But I can't make a generic 
statement here. So we would need to do a case by case basis.

Furthermore, our security statement is also covering a guest userspace 
attacking a guest OS. We would issue an XSA if this is feasible because 
of an issue in the hypervisor.

With half-backed emulation, it becomes more difficult to know whether we 
are not opening a hole and replacing a guest crashes at boot by 
something worse.

Again unlikely here. But those kind of bugs are no unheard. So this is 
something to take into account when you want to claim security support 
for half-backed emulation.

Cheers,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

On 11/12/2023 09:33, Julien Grall wrote:
> Hi,

Hi Julien/Stefano/Bertrand/Michal,

It is a great discussion, thanks for your suggestions.

I think we have an agreement. :-)

>
> On 07/12/2023 21:41, Stefano Stabellini wrote:
>> On Thu, 7 Dec 2023, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 05/12/2023 23:21, Stefano Stabellini wrote:
>>>> On Tue, 5 Dec 2023, Julien Grall wrote:
>>>>> I agree that crashing a guest is bad, but is lying to the domain 
>>>>> really
>>>>> better? The consequence here is not that bad and hopefully it 
>>>>> would be
>>>>> fairly
>>>>> easy to find. But this is not always the case. So I definitely 
>>>>> would place
>>>>> a
>>>>> half-backed emulation more severe than a guest crash.
>>>>
>>>>
>>>> I see where Julien is coming from, but I would go with option two:
>>>> "emulate DCC the same way as KVM". That's because I don't think we can
>>>> get away with crashing the guest in all cases. Although the issue came
>>>> up with a Linux guest, it could have been triggered by a proprietary
>>>> operating system that we cannot change, and I think Xen should support
>>>> running unmodified operating systems.
>>>>
>>>> If we go with a "half-backed emulation" solution, as Julien wrote, 
>>>> then
>>>> it is better to be more similar to other hypervisors, that's why I 
>>>> chose
>>>> option two instead of option three.
>>>>
>>>> But at the same time I recognize the validity of Julien's words and it
>>>> makes me wonder if we should have a KCONFIG option or command line
>>>> option to switch the Xen behavior. We could use it to gate all the
>>>> "half-backed emulation" we do for compatibility.  Something like:
>>>>
>>>> config PARTIAL_EMULATION
>>>>       bool "Partial Emulation"
>>>>       ---help---
>>>>             Enables partial, not spec compliant, emulation of certain
>>>> register
>>>>       interfaces (e.g DCC UART) for guest compatibility. If you 
>>>> disable
>>>>       this option, Xen will crash the guest if the guest tries to 
>>>> access
>>>>       interfaces not fully emulated or virtualized.
>>>>
>>>>       If you enable this option, the guest might misbehave due to 
>>>> non-spec
>>>>       compliant emulation done by Xen.
>>>
>>> As I wrote to Ayan on Matrix today, I am not in favor of the 
>>> emulation. Yet, I
>>> am not going to oppose (as in Nack it) if the other maintainers 
>>> agree with it.
>>
>> Thanks for being flexible
>>
>>
>>> The KConfig would be nice, the question is whether we want to 
>>> (security)
>>> support such configuration? E.g. could this potentially introduce a 
>>> security
>>> issue in the guest?
>>
>> The important question is whether it could introduce a security issue in
>> Xen. If we think it wouldn't increase the attack surface significantly
>> then I would security support it otherwise not.
>
> For this specific emulation, it is unlikely. But I can't make a 
> generic statement here. So we would need to do a case by case basis.
>
> Furthermore, our security statement is also covering a guest userspace 
> attacking a guest OS. We would issue an XSA if this is feasible 
> because of an issue in the hypervisor.
>
> With half-backed emulation, it becomes more difficult to know whether 
> we are not opening a hole and replacing a guest crashes at boot by 
> something worse.
>
> Again unlikely here. But those kind of bugs are no unheard. So this is 
> something to take into account when you want to claim security support 
> for half-backed emulation.

For this specific emulation, I think we all agree that there is no 
security risk. So we need not add any security support for this.

With regards to partial emulation, we all agree that there is no perfect 
solution.


However, the approach on which we all seem to have consensus :-

1. Emulate DCC with TXfull set to 1 (no crash, DCC driver in Linux/Uboot 
returns -ENODEV/-EAGAIN).

2. Introduce a Kconfig (say "CONFIG_PARTIAL_EMULATION") option to 
surround this code which will be specific for Arm and enabled by 
default. This should be turned off by a vendor who does not want to 
provide any form of partial emulation.

3. Introduce a hypervisor command line option ("partial_emulation" , 
disabled by default) so that this cen be enabled at run time using 
Imagebuilder/uboot scripts.

The #2 and #3 can be extended in future to cover all forms of partial 
emulation.

I will send out a patch implementing this approach.

- Ayan

>
> Cheers,
>

Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Stefano Stabellini]

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

On Mon, 11 Dec 2023, Ayan Kumar Halder wrote:
> On 11/12/2023 09:33, Julien Grall wrote:
> > Hi,
> 
> Hi Julien/Stefano/Bertrand/Michal,
> 
> It is a great discussion, thanks for your suggestions.
> 
> I think we have an agreement. :-)
> 
> > 
> > On 07/12/2023 21:41, Stefano Stabellini wrote:
> > > On Thu, 7 Dec 2023, Julien Grall wrote:
> > > > Hi Stefano,
> > > > 
> > > > On 05/12/2023 23:21, Stefano Stabellini wrote:
> > > > > On Tue, 5 Dec 2023, Julien Grall wrote:
> > > > > > I agree that crashing a guest is bad, but is lying to the domain
> > > > > > really
> > > > > > better? The consequence here is not that bad and hopefully it would
> > > > > > be
> > > > > > fairly
> > > > > > easy to find. But this is not always the case. So I definitely would
> > > > > > place
> > > > > > a
> > > > > > half-backed emulation more severe than a guest crash.
> > > > > 
> > > > > 
> > > > > I see where Julien is coming from, but I would go with option two:
> > > > > "emulate DCC the same way as KVM". That's because I don't think we can
> > > > > get away with crashing the guest in all cases. Although the issue came
> > > > > up with a Linux guest, it could have been triggered by a proprietary
> > > > > operating system that we cannot change, and I think Xen should support
> > > > > running unmodified operating systems.
> > > > > 
> > > > > If we go with a "half-backed emulation" solution, as Julien wrote,
> > > > > then
> > > > > it is better to be more similar to other hypervisors, that's why I
> > > > > chose
> > > > > option two instead of option three.
> > > > > 
> > > > > But at the same time I recognize the validity of Julien's words and it
> > > > > makes me wonder if we should have a KCONFIG option or command line
> > > > > option to switch the Xen behavior. We could use it to gate all the
> > > > > "half-backed emulation" we do for compatibility.  Something like:
> > > > > 
> > > > > config PARTIAL_EMULATION
> > > > >       bool "Partial Emulation"
> > > > >       ---help---
> > > > >             Enables partial, not spec compliant, emulation of certain
> > > > > register
> > > > >       interfaces (e.g DCC UART) for guest compatibility. If you
> > > > > disable
> > > > >       this option, Xen will crash the guest if the guest tries to
> > > > > access
> > > > >       interfaces not fully emulated or virtualized.
> > > > > 
> > > > >       If you enable this option, the guest might misbehave due to
> > > > > non-spec
> > > > >       compliant emulation done by Xen.
> > > > 
> > > > As I wrote to Ayan on Matrix today, I am not in favor of the emulation.
> > > > Yet, I
> > > > am not going to oppose (as in Nack it) if the other maintainers agree
> > > > with it.
> > > 
> > > Thanks for being flexible
> > > 
> > > 
> > > > The KConfig would be nice, the question is whether we want to (security)
> > > > support such configuration? E.g. could this potentially introduce a
> > > > security
> > > > issue in the guest?
> > > 
> > > The important question is whether it could introduce a security issue in
> > > Xen. If we think it wouldn't increase the attack surface significantly
> > > then I would security support it otherwise not.
> > 
> > For this specific emulation, it is unlikely. But I can't make a generic
> > statement here. So we would need to do a case by case basis.
> > 
> > Furthermore, our security statement is also covering a guest userspace
> > attacking a guest OS. We would issue an XSA if this is feasible because of
> > an issue in the hypervisor.
> > 
> > With half-backed emulation, it becomes more difficult to know whether we are
> > not opening a hole and replacing a guest crashes at boot by something worse.
> > 
> > Again unlikely here. But those kind of bugs are no unheard. So this is
> > something to take into account when you want to claim security support for
> > half-backed emulation.
> 
> For this specific emulation, I think we all agree that there is no security
> risk. So we need not add any security support for this.

Rather than "no security risk" I would say "unlikely" as Julien wrote
(one never knows...) Also you wrote "we need not add any security
support" but I think you probably meant the opposite: we could add
security support for it.


> With regards to partial emulation, we all agree that there is no perfect
> solution.
> 
> 
> However, the approach on which we all seem to have consensus :-
> 
> 1. Emulate DCC with TXfull set to 1 (no crash, DCC driver in Linux/Uboot
> returns -ENODEV/-EAGAIN).
> 
> 2. Introduce a Kconfig (say "CONFIG_PARTIAL_EMULATION") option to surround
> this code which will be specific for Arm and enabled by default. This should
> be turned off by a vendor who does not want to provide any form of partial
> emulation.
> 
> 3. Introduce a hypervisor command line option ("partial_emulation" , disabled
> by default) so that this cen be enabled at run time using Imagebuilder/uboot
> scripts.
> 
> The #2 and #3 can be extended in future to cover all forms of partial
> emulation.
> 
> I will send out a patch implementing this approach.

Yes, sounds good