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

Mon, 04 Dec 2023 02:32:31 -0800 

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 <[email protected]>
Signed-off-by: Ayan Kumar Halder <[email protected]>



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























					Reply via email to