[XEN v4 0/3] xen/arm: Add emulation of Debug Data Transfer Registers

[XEN v4 0/3] xen/arm: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

Hi,

Refer https://lore.kernel.org/all/alpine.DEB.2.22.394.2312071341540.1265976@ubuntu-linux-20-04-desktop/T/
for the previous discussion on this issue.

Also, the linux earlycon hvc driver has been fixed.
See https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=0ec058ece2f933aed66b76bd5cb9b5e6764853c3

Changes from v1:-
1. Split the change across 3 patches as per the design consensus.

v2 :-
1. Reordered the patches.

v3 :- 
1. Change mentioned in individual patches.

Ayan Kumar Halder (2):
  xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation"
    cmd option
  xen/arm: arm32: Add emulation of Debug Data Transfer Registers

Michal Orzel (1):
  xen/arm: arm64: Add emulation of Debug Data Transfer Registers

 docs/misc/xen-command-line.pandoc    | 11 +++++++++
 xen/arch/arm/Kconfig                 |  9 +++++++
 xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++----
 xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
 xen/arch/arm/include/asm/cpregs.h    |  2 ++
 xen/arch/arm/include/asm/traps.h     |  6 +++++
 xen/arch/arm/traps.c                 |  9 +++++++
 xen/arch/arm/vcpreg.c                | 35 ++++++++++++++++++++--------
 8 files changed, 89 insertions(+), 14 deletions(-)

-- 
2.25.1

[XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Ayan Kumar Halder]

There can be situations when the registers cannot be emulated to their full
functionality. This can be due to the complexity involved. In such cases, one
can emulate those registers as RAZ/WI for example. We call them as partial
emulation.

Some registers are non-optional and as such there is nothing preventing an OS
from accessing them.
Instead of injecting undefined exception (thus crashing a guest), one may want
to prefer a partial emulation to let the guest running (in some cases accepting
the fact that it might result in unwanted behavior).

A suitable example of this (as seen in subsequent patches) is emulation of
DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
registers can be emulated as RAZ/WI and they can be enclosed within
CONFIG_PARTIAL_EMULATION.

Further, "partial-emulation" command line option allows us to
enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
enables support for partial emulation at compile time (i.e. adds code for
partial emulation), this option may be enabled or disabled by Yocto or other
build systems. However if the build system turns this option on, users
can use scripts like Imagebuilder to generate uboot-script which will append
"partial-emulation=false" to xen command line to turn off the partial
emulation. Thus, it helps to avoid rebuilding xen.

By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
This is done so that Xen supports partial emulation. However, customers are
fully aware when they enable partial emulation. It's important to note that
enabling such support might result in unwanted/non-spec compliant behavior.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from v1 :-
1. New patch introduced in v2.

v2 :-
1. Reordered the patches so that the config and command line option is
introduced in the first patch.

v3 :-
1. Defined a macro 'partial_emulation' to reduce if-defs.
2. Fixed style issues.

 docs/misc/xen-command-line.pandoc | 11 +++++++++++
 xen/arch/arm/Kconfig              |  9 +++++++++
 xen/arch/arm/include/asm/traps.h  |  6 ++++++
 xen/arch/arm/traps.c              |  9 +++++++++
 4 files changed, 35 insertions(+)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 8e65f8bd18..22c0d7c9f6 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1949,6 +1949,17 @@ This option is ignored in **pv-shim** mode.
 
 > Default: `on`
 
+### partial-emulation (arm)
+> `= <boolean>`
+
+> Default: `false`
+
+Flag to enable or disable partial emulation of system/coprocessor registers.
+Only effective if CONFIG_PARTIAL_EMULATION is enabled.
+
+**WARNING: Enabling this option might result in unwanted/non-spec compliant
+behavior.**
+
 ### pci
     = List of [ serr=<bool>, perr=<bool> ]
 
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 50e9bfae1a..8d8f668e7f 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -225,6 +225,15 @@ config STATIC_EVTCHN
 	  This option enables establishing static event channel communication
 	  between domains on a dom0less system (domU-domU as well as domU-dom0).
 
+config PARTIAL_EMULATION
+	bool "Enable partial emulation of system/coprocessor registers"
+	default y
+	help
+	  This option enables partial emulation of registers to prevent guests
+	  crashing when accessing registers which are not optional but have not been
+	  emulated to its complete functionality. Enabling this might result in
+	  unwanted/non-spec compliant behavior.
+
 endmenu
 
 menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 883dae368e..9a60dbf70e 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -10,6 +10,12 @@
 # include <asm/arm64/traps.h>
 #endif
 
+#ifdef CONFIG_PARTIAL_EMULATION
+extern bool partial_emulation;
+#else
+#define partial_emulation false
+#endif
+
 /*
  * GUEST_BUG_ON is intended for checking that the guest state has not been
  * corrupted in hardware and/or that the hardware behaves as we
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9c10e8f78c..d1c7a6c516 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -42,6 +42,15 @@
 #include <asm/vgic.h>
 #include <asm/vtimer.h>
 
+/*
+ * partial_emulation: If true, partial emulation for system/coprocessor
+ * registers will be enabled.
+ */
+#ifdef CONFIG_PARTIAL_EMULATION
+bool __ro_after_init partial_emulation = false;
+boolean_param("partial-emulation", partial_emulation);
+#endif
+
 /* The base of the stack must always be double-word aligned, which means
  * that both the kernel half of struct cpu_user_regs (which is pushed in
  * entry.S) and struct cpu_info (which lives at the bottom of a Xen
-- 
2.25.1

[XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

From: Michal Orzel <michal.orzel@amd.com>

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

To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
(these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.

Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
"If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".

Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
and returns -ENODEV in case TXfull bit is still set after writing a test
character. This way we prevent the guest from making use of HVC DCC as a
console.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from

v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
indication that the RX buffer is full and is waiting to be read.

2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.

3. Fixed the commit message and inline code comments.

v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
2. Removed the "fail" label.
3. Fixed the commit message.

v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether 
partial_emulation_enabled is true or not.

2. If partial_emulation_enabled is false, then access to HSR_SYSREG_DBGDTR_EL0,
HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception. 

 xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++++++++----
 xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..94f0a6c384 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
@@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
         return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_MDCCSR_EL0:
         /*
+         * 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 (only if partial emulation is enabled) to
+         * indicate the transmit buffer is full, we would hint the OS that the
+         * DCC is probably not working.
+         *
+         * Bit 29: TX full
+         *
          * 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,
+                                  partial_emulation ? (1U << 29) : 0);
+
+    case HSR_SYSREG_DBGDTR_EL0:
+    /* DBGDTR[TR]X_EL0 share the same encoding */
+    case HSR_SYSREG_DBGDTRTX_EL0:
+        if ( !partial_emulation )
+            goto fail;
+        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):
@@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
      * And all other unknown registers.
      */
     default:
+ fail:
         {
             const struct hsr_sysreg sysreg = hsr.sysreg;
 
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)
-- 
2.25.1

[XEN v4 3/3] xen/arm: arm32: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

When user enables HVC_DCC config option in Linux, it invokes access to debug
transfer register (i.e. DBGDTRTXINT). As this register is not emulated, Xen
injects an undefined exception to the guest and Linux crashes.
To prevent this crash, introduce a partial emulation of DBGDTRTXINT as RAZ/WI
and TXfull should be set to 1. So that Linux will see that TXfull is set, and
it will not access DBGDTRTXINT.

As a pre-requisite, DBGOSLSR should be emulated in the same way as its AArch64
variant (ie OSLSR_EL1).
This is to ensure that DBGOSLSR.OSLK is 0, thus MDSCR_EL1.TXfull is treated
as UNK/SBZP.
Only MDCCSR_EL0 can be emulated (which is DBGDSCRINT on arm32). DBGDSCRINT
can be accessed at EL0 as DBGDSCREXT is emulated as RAZ (as DBGOSLSR.OSLK
== 0). So, we tool the opportunity to fix the minimum EL for DBGDSCRINT.
 DBGDSCRINT.TXfull is set to 1.

Refer ARM DDI 0487J.a ID042523, G8.3.19, DBGDTRTXint
"If TXfull is set to 1, set DTRTX to UNKNOWN".
So, DBGDTR[TR]XINT is emulated as RAZ/WI.

Thus, any OS is expected to read DBGDSCRINT and check for TXfull before using
DBGDTRTXINT.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from

v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
indication that the RX buffer is full and is waiting to be read.

2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.

3. Fixed the commit message and inline code comments.

v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
2. Fixed in line comments and style related issues.
3. Updated commit message to mention DBGDSCRINT handling.

v3 :- 1. The original emulation of DBGDSCRINT is retained when
'partial_emulation' is false.

2. If 'partial_emulation' is false, then access to DBGDTRTXINT will
lead to undefined exception.

 xen/arch/arm/include/asm/cpregs.h |  2 ++
 xen/arch/arm/vcpreg.c             | 35 ++++++++++++++++++++++---------
 2 files changed, 27 insertions(+), 10 deletions(-)

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/vcpreg.c b/xen/arch/arm/vcpreg.c
index a2d0500704..87df4bd238 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -493,11 +493,12 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
      *
      * Unhandled:
-     *    DBGOSLSR
      *    DBGPRCR
      */
     case HSR_CPREG32(DBGOSLAR):
         return handle_wo_wi(regs, regidx, cp32.read, hsr, 1);
+    case HSR_CPREG32(DBGOSLSR):
+        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1, 1U << 3);
     case HSR_CPREG32(DBGOSDLR):
         return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
 
@@ -509,8 +510,6 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      *
      * Unhandled:
      *    DBGDCCINT
-     *    DBGDTRRXint
-     *    DBGDTRTXint
      *    DBGWFAR
      *    DBGDTRTXext
      *    DBGDTRRXext,
@@ -550,10 +549,22 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 
     case HSR_CPREG32(DBGDSCRINT):
         /*
-         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
-         * is set to 0, which we emulated below.
+         * 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.
+         *
+         * Bit 29: TX full
+         *
+         * Accessible by EL0 if DBGDSCRext.UDCCdis is set to 0, which we
+         * emulate as RAZ/WI in the next case.
          */
-        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
+        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 0,
+                                  partial_emulation ? (1U << 29) : 0);
 
     case HSR_CPREG32(DBGDSCREXT):
         /*
@@ -562,6 +573,12 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
          */
         return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
 
+    /* DBGDTR[TR]XINT share the same encoding */
+    case HSR_CPREG32(DBGDTRTXINT):
+        if ( !partial_emulation )
+            goto fail;
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 0);
+
     case HSR_CPREG32(DBGVCR):
     case HSR_CPREG32(DBGBVR0):
     case HSR_CPREG32(DBGBCR0):
@@ -591,6 +608,7 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      * And all other unknown registers.
      */
     default:
+ fail:
         gdprintk(XENLOG_ERR,
                  "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
                   cp32.read ? "mrc" : "mcr",
@@ -659,10 +677,7 @@ void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
      * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
      *
      * Unhandled:
-     *    DBGDTRTXint
-     *    DBGDTRRXint
-     *
-     * And all other unknown registers.
+     * All unknown registers.
      */
     gdprintk(XENLOG_ERR,
              "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
-- 
2.25.1

Re: [XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Michal Orzel]

Hi Ayan,

On 31/01/2024 13:10, Ayan Kumar Halder wrote:
> There can be situations when the registers cannot be emulated to their full
> functionality. This can be due to the complexity involved. In such cases, one
> can emulate those registers as RAZ/WI for example. We call them as partial
> emulation.
> 
> Some registers are non-optional and as such there is nothing preventing an OS
> from accessing them.
> Instead of injecting undefined exception (thus crashing a guest), one may want
> to prefer a partial emulation to let the guest running (in some cases accepting
> the fact that it might result in unwanted behavior).
> 
> A suitable example of this (as seen in subsequent patches) is emulation of
> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
> registers can be emulated as RAZ/WI and they can be enclosed within
> CONFIG_PARTIAL_EMULATION.
> 
> Further, "partial-emulation" command line option allows us to
> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
> enables support for partial emulation at compile time (i.e. adds code for
> partial emulation), this option may be enabled or disabled by Yocto or other
> build systems. However if the build system turns this option on, users
> can use scripts like Imagebuilder to generate uboot-script which will append
> "partial-emulation=false" to xen command line to turn off the partial
NIT: given that the option is false by default, it would make more sense to give example
with setting it to true to enable it.

> emulation. Thus, it helps to avoid rebuilding xen.
> 
> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
> This is done so that Xen supports partial emulation. However, customers are
> fully aware when they enable partial emulation. It's important to note that
> enabling such support might result in unwanted/non-spec compliant behavior.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from v1 :-
> 1. New patch introduced in v2.
> 
> v2 :-
> 1. Reordered the patches so that the config and command line option is
> introduced in the first patch.
> 
> v3 :-
> 1. Defined a macro 'partial_emulation' to reduce if-defs.
> 2. Fixed style issues.
> 
>  docs/misc/xen-command-line.pandoc | 11 +++++++++++
>  xen/arch/arm/Kconfig              |  9 +++++++++
>  xen/arch/arm/include/asm/traps.h  |  6 ++++++
>  xen/arch/arm/traps.c              |  9 +++++++++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 8e65f8bd18..22c0d7c9f6 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1949,6 +1949,17 @@ This option is ignored in **pv-shim** mode.
>  
>  > Default: `on`
>  
> +### partial-emulation (arm)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Flag to enable or disable partial emulation of system/coprocessor registers.
> +Only effective if CONFIG_PARTIAL_EMULATION is enabled.
> +
> +**WARNING: Enabling this option might result in unwanted/non-spec compliant
> +behavior.**
> +
>  ### pci
>      = List of [ serr=<bool>, perr=<bool> ]
>  
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 50e9bfae1a..8d8f668e7f 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -225,6 +225,15 @@ config STATIC_EVTCHN
>  	  This option enables establishing static event channel communication
>  	  between domains on a dom0less system (domU-domU as well as domU-dom0).
>  
> +config PARTIAL_EMULATION
> +	bool "Enable partial emulation of system/coprocessor registers"
> +	default y
> +	help
> +	  This option enables partial emulation of registers to prevent guests
> +	  crashing when accessing registers which are not optional but have not been
> +	  emulated to its complete functionality. Enabling this might result in
NIT: s/its/their

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

Hi Ayan,

On 31/01/2024 13:10, Ayan Kumar Halder wrote:
> From: Michal Orzel <michal.orzel@amd.com>
> 
> Currently, if user enables HVC_DCC config option in Linux, it invokes access
> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
> arm32). As these registers are not emulated, Xen injects an undefined
> exception to the guest and Linux crashes.
> 
> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.
> 
> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
> 
> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
> and returns -ENODEV in case TXfull bit is still set after writing a test
> character. This way we prevent the guest from making use of HVC DCC as a
> console.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v4 3/3] xen/arm: arm32: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

Hi Ayan,

On 31/01/2024 13:10, Ayan Kumar Halder wrote:
> When user enables HVC_DCC config option in Linux, it invokes access to debug
> transfer register (i.e. DBGDTRTXINT). As this register is not emulated, Xen
> injects an undefined exception to the guest and Linux crashes.
> To prevent this crash, introduce a partial emulation of DBGDTRTXINT as RAZ/WI
> and TXfull should be set to 1. So that Linux will see that TXfull is set, and
> it will not access DBGDTRTXINT.
It will access it at least once to later check if TXfull is set.

> 
> As a pre-requisite, DBGOSLSR should be emulated in the same way as its AArch64
> variant (ie OSLSR_EL1).
> This is to ensure that DBGOSLSR.OSLK is 0, thus MDSCR_EL1.TXfull is treated
> as UNK/SBZP.
For that you could just emulate it as RAZ. Instead you also set OSLM[1]. So at least
I would make it clear that you do that for consistency.

> Only MDCCSR_EL0 can be emulated (which is DBGDSCRINT on arm32). DBGDSCRINT
I would write: This way, we only need to emulate DBGDSCRINT.

> can be accessed at EL0 as DBGDSCREXT is emulated as RAZ (as DBGOSLSR.OSLK
> == 0). So, we tool the opportunity to fix the minimum EL for DBGDSCRINT.
>  DBGDSCRINT.TXfull is set to 1.
I'm not sure why you are mixing AArch64 names with AArch32 ones. It reads a bit difficult.
Furthermore, fixing lowest EL deserves a separate section.
Also s/tool/took.

> 
> Refer ARM DDI 0487J.a ID042523, G8.3.19, DBGDTRTXint
> "If TXfull is set to 1, set DTRTX to UNKNOWN".
> So, DBGDTR[TR]XINT is emulated as RAZ/WI.
> 
> Thus, any OS is expected to read DBGDSCRINT and check for TXfull before using
> DBGDTRTXINT.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from
> 
> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
> indication that the RX buffer is full and is waiting to be read.
> 
> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.
> 
> 3. Fixed the commit message and inline code comments.
> 
> v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
> 2. Fixed in line comments and style related issues.
> 3. Updated commit message to mention DBGDSCRINT handling.
> 
> v3 :- 1. The original emulation of DBGDSCRINT is retained when
> 'partial_emulation' is false.
> 
> 2. If 'partial_emulation' is false, then access to DBGDTRTXINT will
> lead to undefined exception.
> 
>  xen/arch/arm/include/asm/cpregs.h |  2 ++
>  xen/arch/arm/vcpreg.c             | 35 ++++++++++++++++++++++---------
>  2 files changed, 27 insertions(+), 10 deletions(-)
> 
> 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/vcpreg.c b/xen/arch/arm/vcpreg.c
> index a2d0500704..87df4bd238 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -493,11 +493,12 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>       * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
>       *
>       * Unhandled:
> -     *    DBGOSLSR
>       *    DBGPRCR
>       */
>      case HSR_CPREG32(DBGOSLAR):
>          return handle_wo_wi(regs, regidx, cp32.read, hsr, 1);
> +    case HSR_CPREG32(DBGOSLSR):
> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1, 1U << 3);
>      case HSR_CPREG32(DBGOSDLR):
>          return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
>  
> @@ -509,8 +510,6 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>       *
>       * Unhandled:
>       *    DBGDCCINT
> -     *    DBGDTRRXint
> -     *    DBGDTRTXint
>       *    DBGWFAR
>       *    DBGDTRTXext
>       *    DBGDTRRXext,
> @@ -550,10 +549,22 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>  
>      case HSR_CPREG32(DBGDSCRINT):
>          /*
> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> -         * is set to 0, which we emulated below.
> +         * 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.
I'm a bit suprised that these messages differ. Why not to use the same text as arm64 but with
register names updated?

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Julien Grall]

Hi Ayan,

On 31/01/2024 12:10, Ayan Kumar Halder wrote:
> There can be situations when the registers cannot be emulated to their full
> functionality. This can be due to the complexity involved. In such cases, one
> can emulate those registers as RAZ/WI for example. We call them as partial
> emulation.
> 
> Some registers are non-optional and as such there is nothing preventing an OS
> from accessing them.
> Instead of injecting undefined exception (thus crashing a guest), one may want
> to prefer a partial emulation to let the guest running (in some cases accepting
> the fact that it might result in unwanted behavior).
> 
> A suitable example of this (as seen in subsequent patches) is emulation of
> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
> registers can be emulated as RAZ/WI and they can be enclosed within
> CONFIG_PARTIAL_EMULATION.
> 
> Further, "partial-emulation" command line option allows us to
> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
> enables support for partial emulation at compile time (i.e. adds code for
> partial emulation), this option may be enabled or disabled by Yocto or other
> build systems. However if the build system turns this option on, users
> can use scripts like Imagebuilder to generate uboot-script which will append
> "partial-emulation=false" to xen command line to turn off the partial
> emulation. Thus, it helps to avoid rebuilding xen.
> 
> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
> This is done so that Xen supports partial emulation. However, customers are
> fully aware when they enable partial emulation. It's important to note that
> enabling such support might result in unwanted/non-spec compliant behavior.

Can you remind me why this is built by default? In particular...

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from v1 :-
> 1. New patch introduced in v2.
> 
> v2 :-
> 1. Reordered the patches so that the config and command line option is
> introduced in the first patch.
> 
> v3 :-
> 1. Defined a macro 'partial_emulation' to reduce if-defs.
> 2. Fixed style issues.
> 
>   docs/misc/xen-command-line.pandoc | 11 +++++++++++
>   xen/arch/arm/Kconfig              |  9 +++++++++
>   xen/arch/arm/include/asm/traps.h  |  6 ++++++
>   xen/arch/arm/traps.c              |  9 +++++++++
>   4 files changed, 35 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 8e65f8bd18..22c0d7c9f6 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1949,6 +1949,17 @@ This option is ignored in **pv-shim** mode.
>   
>   > Default: `on`
>   
> +### partial-emulation (arm)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Flag to enable or disable partial emulation of system/coprocessor registers.
> +Only effective if CONFIG_PARTIAL_EMULATION is enabled.
> +
> +**WARNING: Enabling this option might result in unwanted/non-spec compliant
> +behavior.**

... leads me to think that the default config should have it off. Still 
letting the integrator optionally opt-in.

It also wants some explanation about the security support statement. Is 
the goal to support any security issue that may arise from someone 
adding 'partial-emulation=true'?

> +
>   ### pci
>       = List of [ serr=<bool>, perr=<bool> ]
>   
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 50e9bfae1a..8d8f668e7f 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -225,6 +225,15 @@ config STATIC_EVTCHN
>   	  This option enables establishing static event channel communication
>   	  between domains on a dom0less system (domU-domU as well as domU-dom0).
>   
> +config PARTIAL_EMULATION
> +	bool "Enable partial emulation of system/coprocessor registers"
> +	default y
> +	help
> +	  This option enables partial emulation of registers to prevent guests
> +	  crashing when accessing registers which are not optional but have not been

I think we need to list somewhere (possibly in the command line 
documentation) which registers are partially implemented. This will help 
the admin to quickly figure out whether this option makes sense for them.

> +	  emulated to its complete functionality. Enabling this might result in
> +	  unwanted/non-spec compliant behavior.

The description leads me to think if this is selected, then Xen will do 
the partial emulation. However, this is not matching the code.

Selecting this option doesn't result to unwanted/non-spec compliant 
behavior. What trigger the unwanted behavior if the command line option.

So I would suggest to reword the Kconfig to:

"Partial emulation support"

"Some of the required registers are not properly emulated by Xen. This 
option will allow the admin to select at runtime (via the command line 
'partial-emulation' whether the registers are unimplemented (i.e. a 
fault will be injected on access) or they will be partially emulated.

Partial emulation could be useful if a guest is trying to access any of 
the registers (see XXX for the list).

> +
>   endmenu
>   
>   menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 883dae368e..9a60dbf70e 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -10,6 +10,12 @@
>   # include <asm/arm64/traps.h>
>   #endif
>   
> +#ifdef CONFIG_PARTIAL_EMULATION
> +extern bool partial_emulation;
> +#else
> +#define partial_emulation false
> +#endif
> +
>   /*
>    * GUEST_BUG_ON is intended for checking that the guest state has not been
>    * corrupted in hardware and/or that the hardware behaves as we
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 9c10e8f78c..d1c7a6c516 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -42,6 +42,15 @@
>   #include <asm/vgic.h>
>   #include <asm/vtimer.h>
>   
> +/*
> + * partial_emulation: If true, partial emulation for system/coprocessor
> + * registers will be enabled.
> + */
> +#ifdef CONFIG_PARTIAL_EMULATION
> +bool __ro_after_init partial_emulation = false;
> +boolean_param("partial-emulation", partial_emulation);
> +#endif

I think we should use warning_add() to print a message indicating the 
admin has enabled a configuration that is potentially unsafe for the guest.

> +
>   /* The base of the stack must always be double-word aligned, which means
>    * that both the kernel half of struct cpu_user_regs (which is pushed in
>    * entry.S) and struct cpu_info (which lives at the bottom of a Xen

Cheers,

-- 
Julien Grall

Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Julien Grall]

Hi Ayan,

On 31/01/2024 12:10, Ayan Kumar Halder wrote:
> From: Michal Orzel <michal.orzel@amd.com>
> 
> Currently, if user enables HVC_DCC config option in Linux, it invokes access
> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
> arm32). As these registers are not emulated, Xen injects an undefined
> exception to the guest and Linux crashes.
> 
> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.
> 
> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
> 
> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
> and returns -ENODEV in case TXfull bit is still set after writing a test
> character. This way we prevent the guest from making use of HVC DCC as a
> console.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from
> 
> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
> indication that the RX buffer is full and is waiting to be read.
> 
> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.
> 
> 3. Fixed the commit message and inline code comments.
> 
> v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
> 2. Removed the "fail" label.
> 3. Fixed the commit message.
> 
> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
> partial_emulation_enabled is true or not.
> 
> 2. If partial_emulation_enabled is false, then access to HSR_SYSREG_DBGDTR_EL0,
> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
> 
>   xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++++++++----
>   xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>   2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..94f0a6c384 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
> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>           return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>       case HSR_SYSREG_MDCCSR_EL0:
>           /*
> +         * 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 (only if partial emulation is enabled) to
> +         * indicate the transmit buffer is full, we would hint the OS that the
> +         * DCC is probably not working.
> +         *
> +         * Bit 29: TX full
> +         *
>            * 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.

The sentence "we emulate that register as ..." seems to be stale?

>            */
> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
> +                                  partial_emulation ? (1U << 29) : 0);
> +
> +    case HSR_SYSREG_DBGDTR_EL0:
> +    /* DBGDTR[TR]X_EL0 share the same encoding */
> +    case HSR_SYSREG_DBGDTRTX_EL0:
> +        if ( !partial_emulation )
> +            goto fail;
> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);

AFAICT, all the emulation helpers have an explanation why we are using 
them. But here this is not the case. Can you add one?

> +
>       HSR_SYSREG_DBG_CASES(DBGBVR):
>       HSR_SYSREG_DBG_CASES(DBGBCR):
>       HSR_SYSREG_DBG_CASES(DBGWVR):
> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>        * And all other unknown registers.
>        */
>       default:
> + fail:

AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet 
(?) accepted the rule, but I don't see we would not given I feel this is 
similar to what Rule 16.2 is trying to prevent and we accepted it.

I think case, I move all the code within default outside. And then call 
"goto fail" from the default label.

>           {
>               const struct hsr_sysreg sysreg = hsr.sysreg;
>   
> 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)

Cheers,

[1] 
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_03.c
[2] 
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_02.c

-- 
Julien Grall

Re: [XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Michal Orzel]

Hi Julien,

On 06/02/2024 19:49, Julien Grall wrote:
> 
> 
> Hi Ayan,
> 
> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>> There can be situations when the registers cannot be emulated to their full
>> functionality. This can be due to the complexity involved. In such cases, one
>> can emulate those registers as RAZ/WI for example. We call them as partial
>> emulation.
>>
>> Some registers are non-optional and as such there is nothing preventing an OS
>> from accessing them.
>> Instead of injecting undefined exception (thus crashing a guest), one may want
>> to prefer a partial emulation to let the guest running (in some cases accepting
>> the fact that it might result in unwanted behavior).
>>
>> A suitable example of this (as seen in subsequent patches) is emulation of
>> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
>> registers can be emulated as RAZ/WI and they can be enclosed within
>> CONFIG_PARTIAL_EMULATION.
>>
>> Further, "partial-emulation" command line option allows us to
>> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
>> enables support for partial emulation at compile time (i.e. adds code for
>> partial emulation), this option may be enabled or disabled by Yocto or other
>> build systems. However if the build system turns this option on, users
>> can use scripts like Imagebuilder to generate uboot-script which will append
>> "partial-emulation=false" to xen command line to turn off the partial
>> emulation. Thus, it helps to avoid rebuilding xen.
>>
>> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
>> This is done so that Xen supports partial emulation. However, customers are
>> fully aware when they enable partial emulation. It's important to note that
>> enabling such support might result in unwanted/non-spec compliant behavior.
> 
> Can you remind me why this is built by default? In particular...
This is the result of RFC discussion we had, where both Bertrand and Stefano agreed on having
the Kconfig enabled by default to improve user experience:
Bertrand:
https://lore.kernel.org/xen-devel/C0ADC33B-1966-4D3E-B081-A3AA0C3AE76D@arm.com/
Stefano:
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2312081514450.1703076@ubuntu-linux-20-04-desktop/

~Michal

Re: [XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Julien Grall]

Hi Michal,

On 07/02/2024 07:45, Michal Orzel wrote:
> On 06/02/2024 19:49, Julien Grall wrote:
>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>> There can be situations when the registers cannot be emulated to their full
>>> functionality. This can be due to the complexity involved. In such cases, one
>>> can emulate those registers as RAZ/WI for example. We call them as partial
>>> emulation.
>>>
>>> Some registers are non-optional and as such there is nothing preventing an OS
>>> from accessing them.
>>> Instead of injecting undefined exception (thus crashing a guest), one may want
>>> to prefer a partial emulation to let the guest running (in some cases accepting
>>> the fact that it might result in unwanted behavior).
>>>
>>> A suitable example of this (as seen in subsequent patches) is emulation of
>>> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
>>> registers can be emulated as RAZ/WI and they can be enclosed within
>>> CONFIG_PARTIAL_EMULATION.
>>>
>>> Further, "partial-emulation" command line option allows us to
>>> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
>>> enables support for partial emulation at compile time (i.e. adds code for
>>> partial emulation), this option may be enabled or disabled by Yocto or other
>>> build systems. However if the build system turns this option on, users
>>> can use scripts like Imagebuilder to generate uboot-script which will append
>>> "partial-emulation=false" to xen command line to turn off the partial
>>> emulation. Thus, it helps to avoid rebuilding xen.
>>>
>>> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
>>> This is done so that Xen supports partial emulation. However, customers are
>>> fully aware when they enable partial emulation. It's important to note that
>>> enabling such support might result in unwanted/non-spec compliant behavior.
>>
>> Can you remind me why this is built by default? In particular...
> This is the result of RFC discussion we had, where both Bertrand and Stefano agreed on having
> the Kconfig enabled by default to improve user experience:
> Bertrand:
> https://lore.kernel.org/xen-devel/C0ADC33B-1966-4D3E-B081-A3AA0C3AE76D@arm.com/
> Stefano:
> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2312081514450.1703076@ubuntu-linux-20-04-desktop/

Thanks for the pointer. I thought a bit more and per-se the default of 
the Kconfig doesn't really matter too much. So I am fine to keep it on 
by default.

That said, I think we need to detail the security support for the 
command line in SUPPORT.md. I think we want to consider to not security 
support any issue that would allow the userland to attack the guest OS 
due to a bug in the partial emulation.

I would be fine with security supporting any issue that would 
DoS/compromise Xen.

Cheers,

-- 
Julien Grall

Re: [XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Michal Orzel]

On 07/02/2024 11:06, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 07/02/2024 07:45, Michal Orzel wrote:
>> On 06/02/2024 19:49, Julien Grall wrote:
>>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>>> There can be situations when the registers cannot be emulated to their full
>>>> functionality. This can be due to the complexity involved. In such cases, one
>>>> can emulate those registers as RAZ/WI for example. We call them as partial
>>>> emulation.
>>>>
>>>> Some registers are non-optional and as such there is nothing preventing an OS
>>>> from accessing them.
>>>> Instead of injecting undefined exception (thus crashing a guest), one may want
>>>> to prefer a partial emulation to let the guest running (in some cases accepting
>>>> the fact that it might result in unwanted behavior).
>>>>
>>>> A suitable example of this (as seen in subsequent patches) is emulation of
>>>> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
>>>> registers can be emulated as RAZ/WI and they can be enclosed within
>>>> CONFIG_PARTIAL_EMULATION.
>>>>
>>>> Further, "partial-emulation" command line option allows us to
>>>> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
>>>> enables support for partial emulation at compile time (i.e. adds code for
>>>> partial emulation), this option may be enabled or disabled by Yocto or other
>>>> build systems. However if the build system turns this option on, users
>>>> can use scripts like Imagebuilder to generate uboot-script which will append
>>>> "partial-emulation=false" to xen command line to turn off the partial
>>>> emulation. Thus, it helps to avoid rebuilding xen.
>>>>
>>>> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
>>>> This is done so that Xen supports partial emulation. However, customers are
>>>> fully aware when they enable partial emulation. It's important to note that
>>>> enabling such support might result in unwanted/non-spec compliant behavior.
>>>
>>> Can you remind me why this is built by default? In particular...
>> This is the result of RFC discussion we had, where both Bertrand and Stefano agreed on having
>> the Kconfig enabled by default to improve user experience:
>> Bertrand:
>> https://lore.kernel.org/xen-devel/C0ADC33B-1966-4D3E-B081-A3AA0C3AE76D@arm.com/
>> Stefano:
>> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2312081514450.1703076@ubuntu-linux-20-04-desktop/
> 
> Thanks for the pointer. I thought a bit more and per-se the default of
> the Kconfig doesn't really matter too much. So I am fine to keep it on
> by default.
> 
> That said, I think we need to detail the security support for the
> command line in SUPPORT.md. I think we want to consider to not security
> support any issue that would allow the userland to attack the guest OS
> due to a bug in the partial emulation.
> 
> I would be fine with security supporting any issue that would
> DoS/compromise Xen.
Sounds good to me. Something like:
### ARM/Partial emulation

Enable partial emulation of registers, otherwise considered unimplemented,
that would normally trigger a fault injection.

    Status: Supported, with caveats

Bugs allowing the userspace to attack the guest OS will not be considered
security vulnerabilities.

Bugs that could compromise Xen will be considered security vulnerabilities.

~Michal

Re: [XEN v4 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Julien Grall]

Hi Michal,

On 07/02/2024 11:52, Michal Orzel wrote:
> 
> 
> On 07/02/2024 11:06, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> On 07/02/2024 07:45, Michal Orzel wrote:
>>> On 06/02/2024 19:49, Julien Grall wrote:
>>>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>>>> There can be situations when the registers cannot be emulated to their full
>>>>> functionality. This can be due to the complexity involved. In such cases, one
>>>>> can emulate those registers as RAZ/WI for example. We call them as partial
>>>>> emulation.
>>>>>
>>>>> Some registers are non-optional and as such there is nothing preventing an OS
>>>>> from accessing them.
>>>>> Instead of injecting undefined exception (thus crashing a guest), one may want
>>>>> to prefer a partial emulation to let the guest running (in some cases accepting
>>>>> the fact that it might result in unwanted behavior).
>>>>>
>>>>> A suitable example of this (as seen in subsequent patches) is emulation of
>>>>> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
>>>>> registers can be emulated as RAZ/WI and they can be enclosed within
>>>>> CONFIG_PARTIAL_EMULATION.
>>>>>
>>>>> Further, "partial-emulation" command line option allows us to
>>>>> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
>>>>> enables support for partial emulation at compile time (i.e. adds code for
>>>>> partial emulation), this option may be enabled or disabled by Yocto or other
>>>>> build systems. However if the build system turns this option on, users
>>>>> can use scripts like Imagebuilder to generate uboot-script which will append
>>>>> "partial-emulation=false" to xen command line to turn off the partial
>>>>> emulation. Thus, it helps to avoid rebuilding xen.
>>>>>
>>>>> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
>>>>> This is done so that Xen supports partial emulation. However, customers are
>>>>> fully aware when they enable partial emulation. It's important to note that
>>>>> enabling such support might result in unwanted/non-spec compliant behavior.
>>>>
>>>> Can you remind me why this is built by default? In particular...
>>> This is the result of RFC discussion we had, where both Bertrand and Stefano agreed on having
>>> the Kconfig enabled by default to improve user experience:
>>> Bertrand:
>>> https://lore.kernel.org/xen-devel/C0ADC33B-1966-4D3E-B081-A3AA0C3AE76D@arm.com/
>>> Stefano:
>>> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2312081514450.1703076@ubuntu-linux-20-04-desktop/
>>
>> Thanks for the pointer. I thought a bit more and per-se the default of
>> the Kconfig doesn't really matter too much. So I am fine to keep it on
>> by default.
>>
>> That said, I think we need to detail the security support for the
>> command line in SUPPORT.md. I think we want to consider to not security
>> support any issue that would allow the userland to attack the guest OS
>> due to a bug in the partial emulation.
>>
>> I would be fine with security supporting any issue that would
>> DoS/compromise Xen.
> Sounds good to me. Something like:
> ### ARM/Partial emulation
> 
> Enable partial emulation of registers, otherwise considered unimplemented,
> that would normally trigger a fault injection.
> 
>      Status: Supported, with caveats
> 
> Bugs allowing the userspace to attack the guest OS will not be considered
> security vulnerabilities.
> 
> Bugs that could compromise Xen will be considered security vulnerabilities.

LGTM.

Cheers,

-- 
Julien Grall

Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

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


On 06/02/2024 19:05, Julien Grall wrote:
> Hi Ayan,
Hi Julien/Michal,
>
> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>> From: Michal Orzel <michal.orzel@amd.com>
>>
>> Currently, if user enables HVC_DCC config option in Linux, it invokes 
>> access
>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, 
>> DBGDTRTXINT on
>> arm32). As these registers are not emulated, Xen injects an undefined
>> exception to the guest and Linux crashes.
>>
>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as 
>> TXfull.
>>
>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>>
>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> 
>> hvc_dcc_check(),
>> and returns -ENODEV in case TXfull bit is still set after writing a test
>> character. This way we prevent the guest from making use of HVC DCC as a
>> console.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from
>>
>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving 
>> the OS any
>> indication that the RX buffer is full and is waiting to be read.
>>
>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at 
>> EL0 only.
>>
>> 3. Fixed the commit message and inline code comments.
>>
>> v2 :- 1. Split the patch into two (separate patches for arm64 and 
>> arm32).
>> 2. Removed the "fail" label.
>> 3. Fixed the commit message.
>>
>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
>> partial_emulation_enabled is true or not.
>>
>> 2. If partial_emulation_enabled is false, then access to 
>> HSR_SYSREG_DBGDTR_EL0,
>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>>
>>   xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++++++++----
>>   xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index b5d54c569b..94f0a6c384 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
>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>>           return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>       case HSR_SYSREG_MDCCSR_EL0:
>>           /*
>> +         * 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 (only if partial emulation is 
>> enabled) to
>> +         * indicate the transmit buffer is full, we would hint the 
>> OS that the
>> +         * DCC is probably not working.
>> +         *
>> +         * Bit 29: TX full
>> +         *
>>            * 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.
>
> The sentence "we emulate that register as ..." seems to be stale?
>
>>            */
>> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, 
>> hsr, 0,
>> +                                  partial_emulation ? (1U << 29) : 0);
>> +
>> +    case HSR_SYSREG_DBGDTR_EL0:
>> +    /* DBGDTR[TR]X_EL0 share the same encoding */
>> +    case HSR_SYSREG_DBGDTRTX_EL0:
>> +        if ( !partial_emulation )
>> +            goto fail;
>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>
> AFAICT, all the emulation helpers have an explanation why we are using 
> them. But here this is not the case. Can you add one?
This and..
>
>> +
>>       HSR_SYSREG_DBG_CASES(DBGBVR):
>>       HSR_SYSREG_DBG_CASES(DBGBCR):
>>       HSR_SYSREG_DBG_CASES(DBGWVR):
>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>        * And all other unknown registers.
>>        */
>>       default:
>> + fail:
>
> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet 
> (?) accepted the rule, but I don't see we would not given I feel this 
> is similar to what Rule 16.2 is trying to prevent and we accepted it.
>
> I think case, I move all the code within default outside. And then 
> call "goto fail" from the default label.

I am not sure if I have interpreted this correctly.

Is it ok if you can take a look at the attached patch and let me know if 
the explaination and the code change looks sane ?

- Ayan

>
>>           {
>>               const struct hsr_sysreg sysreg = hsr.sysreg;
>>   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)
>
> Cheers,
>
> [1] 
> https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_03.c
> [2] 
> https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_02.c
>

[-- Attachment #2: v5-0002-xen-arm-arm64-Add-emulation-of-Debug-Data-Transfe.patch --]
[-- Type: text/plain, Size: 6261 bytes --]

From 769efc903eed18839641a003aa63ce64de27bb5f Mon Sep 17 00:00:00 2001
From: Michal Orzel <michal.orzel@amd.com>
Date: Wed, 3 Jan 2024 14:44:22 +0000
Subject: [XEN v5 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer
 Registers
To: xen-devel@lists.xenproject.org
Cc: sstabellini@kernel.org,
    stefano.stabellini@amd.com,
    julien@xen.org,
    Volodymyr_Babchuk@epam.com,
    bertrand.marquis@arm.com,
    michal.orzel@amd.com

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

To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
(these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.

Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
"If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".

Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
and returns -ENODEV in case TXfull bit is still set after writing a test
character. This way we prevent the guest from making use of HVC DCC as a
console.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/arm64/vsysreg.c         | 69 +++++++++++++++++++---------
 xen/arch/arm/include/asm/arm64/hsr.h |  3 ++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..813b40425d 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
@@ -173,10 +170,40 @@ void do_sysreg(struct cpu_user_regs *regs,
         return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_MDCCSR_EL0:
         /*
+         * 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 (only if partial emulation is enabled) to
+         * indicate the transmit buffer is full, we would hint the OS that the
+         * DCC is probably not working.
+         *
+         * Bit 29: TX full
+         *
          * 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.
+         * register as RO at EL0 and above.
          */
-        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
+        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
+                                  partial_emulation ? (1U << 29) : 0);
+
+    case HSR_SYSREG_DBGDTR_EL0:
+    /* DBGDTR[TR]X_EL0 share the same encoding */
+    case HSR_SYSREG_DBGDTRTX_EL0:
+        /*
+         * As stated before, Xen does not support full emulation of Debug
+         * Communications Channel (DCC). Thus, if Xen does not support partial
+         * emulation and access to DBGDTRTX is trapped, it will inject an undef
+         * exception. Otherwise, Xen emulates this as RAZ/WI as it emulates the
+         * TX status as full. Thus, guests are not expected to use DBGDTRTX for
+         * reading/writing.
+         */
+        if ( !partial_emulation )
+            goto fail;
+        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):
@@ -394,23 +421,21 @@ void do_sysreg(struct cpu_user_regs *regs,
      * And all other unknown registers.
      */
     default:
-        {
-            const struct hsr_sysreg sysreg = hsr.sysreg;
-
-            gdprintk(XENLOG_ERR,
-                     "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
-                     sysreg.read ? "mrs" : "msr",
-                     sysreg.op0, sysreg.op1,
-                     sysreg.crn, sysreg.crm,
-                     sysreg.op2,
-                     sysreg.read ? "=>" : "<=",
-                     sysreg.reg, regs->pc);
-            gdprintk(XENLOG_ERR,
-                     "unhandled 64-bit sysreg access %#"PRIregister"\n",
-                     hsr.bits & HSR_SYSREG_REGS_MASK);
-            inject_undef_exception(regs, hsr);
-            return;
-        }
+        goto fail;
+ fail:
+
+    const struct hsr_sysreg sysreg = hsr.sysreg;
+
+    gdprintk(XENLOG_ERR,
+             "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
+             sysreg.read ? "mrs" : "msr", sysreg.op0, sysreg.op1, sysreg.crn,
+             sysreg.crm, sysreg.op2, sysreg.read ? "=>" : "<=", sysreg.reg,
+             regs->pc);
+    gdprintk(XENLOG_ERR,
+             "unhandled 64-bit sysreg access %#"PRIregister"\n",
+             hsr.bits & HSR_SYSREG_REGS_MASK);
+    inject_undef_exception(regs, hsr);
+    return;
     }
 
     regs->pc += 4;
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)
-- 
2.25.1

Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

Hi Ayan,

On 19/02/2024 15:45, Ayan Kumar Halder wrote:
> 
> On 06/02/2024 19:05, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien/Michal,
>>
>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>> From: Michal Orzel <michal.orzel@amd.com>
>>>
>>> Currently, if user enables HVC_DCC config option in Linux, it invokes 
>>> access
>>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, 
>>> DBGDTRTXINT on
>>> arm32). As these registers are not emulated, Xen injects an undefined
>>> exception to the guest and Linux crashes.
>>>
>>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
>>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as 
>>> TXfull.
>>>
>>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>>>
>>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
>>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> 
>>> hvc_dcc_check(),
>>> and returns -ENODEV in case TXfull bit is still set after writing a test
>>> character. This way we prevent the guest from making use of HVC DCC as a
>>> console.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>> Changes from
>>>
>>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving 
>>> the OS any
>>> indication that the RX buffer is full and is waiting to be read.
>>>
>>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at 
>>> EL0 only.
>>>
>>> 3. Fixed the commit message and inline code comments.
>>>
>>> v2 :- 1. Split the patch into two (separate patches for arm64 and 
>>> arm32).
>>> 2. Removed the "fail" label.
>>> 3. Fixed the commit message.
>>>
>>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
>>> partial_emulation_enabled is true or not.
>>>
>>> 2. If partial_emulation_enabled is false, then access to 
>>> HSR_SYSREG_DBGDTR_EL0,
>>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>>>
>>>   xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++++++++----
>>>   xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>>> index b5d54c569b..94f0a6c384 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
>>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>           return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>       case HSR_SYSREG_MDCCSR_EL0:
>>>           /*
>>> +         * 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 (only if partial emulation is 
>>> enabled) to
>>> +         * indicate the transmit buffer is full, we would hint the 
>>> OS that the
>>> +         * DCC is probably not working.
>>> +         *
>>> +         * Bit 29: TX full
>>> +         *
>>>            * 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.
>>
>> The sentence "we emulate that register as ..." seems to be stale?
I can see that you tried to handle Julien remark here. But I disagree. This statement
is not stale. It explains that because MDSCR_EL1 is RAZ/WI, MDCCSR_EL0 is RO at both
EL0 and EL1. This patch does not change this behavior.

>>
>>>            */
>>> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, 
>>> hsr, 0,
>>> +                                  partial_emulation ? (1U << 29) : 0);
>>> +
>>> +    case HSR_SYSREG_DBGDTR_EL0:
>>> +    /* DBGDTR[TR]X_EL0 share the same encoding */
>>> +    case HSR_SYSREG_DBGDTRTX_EL0:
>>> +        if ( !partial_emulation )
>>> +            goto fail;
>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>>
>> AFAICT, all the emulation helpers have an explanation why we are using 
>> them. But here this is not the case. Can you add one?
> This and..
>>
>>> +
>>>       HSR_SYSREG_DBG_CASES(DBGBVR):
>>>       HSR_SYSREG_DBG_CASES(DBGBCR):
>>>       HSR_SYSREG_DBG_CASES(DBGWVR):
>>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>        * And all other unknown registers.
>>>        */
>>>       default:
>>> + fail:
>>
>> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet 
>> (?) accepted the rule, but I don't see we would not given I feel this 
>> is similar to what Rule 16.2 is trying to prevent and we accepted it.
>>
>> I think case, I move all the code within default outside. And then 
>> call "goto fail" from the default label.
> 
> I am not sure if I have interpreted this correctly.
> 
> Is it ok if you can take a look at the attached patch and let me know if 
> the explaination and the code change looks sane ?
Looking at the attached patch and fail handling, I don't think it is what Julien meant.
In the default case you should jump to fail that would be defined outside of switch clause.

Something like:
    default:
        goto fail;
    }

    regs->pc += 4;
    return;

fail:
    gdprintk...

When it comes to explanation for HSR_SYSREG_DBGDTRTX_EL0, I will let Julien to provide a comment he believes is right.
To me, it feels strange to repeat almost the same information as for MDCCSR_EL0.

~Michal

Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Julien Grall]

Hi Michal,

On 19/02/2024 15:43, Michal Orzel wrote:

> On 19/02/2024 15:45, Ayan Kumar Halder wrote:
>>
>> On 06/02/2024 19:05, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien/Michal,
>>>
>>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>>> From: Michal Orzel <michal.orzel@amd.com>
>>>>
>>>> Currently, if user enables HVC_DCC config option in Linux, it invokes
>>>> access
>>>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64,
>>>> DBGDTRTXINT on
>>>> arm32). As these registers are not emulated, Xen injects an undefined
>>>> exception to the guest and Linux crashes.
>>>>
>>>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
>>>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as
>>>> TXfull.
>>>>
>>>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
>>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>>>>
>>>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
>>>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>>>> hvc_dcc_check(),
>>>> and returns -ENODEV in case TXfull bit is still set after writing a test
>>>> character. This way we prevent the guest from making use of HVC DCC as a
>>>> console.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>> Changes from
>>>>
>>>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving
>>>> the OS any
>>>> indication that the RX buffer is full and is waiting to be read.
>>>>
>>>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at
>>>> EL0 only.
>>>>
>>>> 3. Fixed the commit message and inline code comments.
>>>>
>>>> v2 :- 1. Split the patch into two (separate patches for arm64 and
>>>> arm32).
>>>> 2. Removed the "fail" label.
>>>> 3. Fixed the commit message.
>>>>
>>>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
>>>> partial_emulation_enabled is true or not.
>>>>
>>>> 2. If partial_emulation_enabled is false, then access to
>>>> HSR_SYSREG_DBGDTR_EL0,
>>>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>>>>
>>>>    xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++++++++----
>>>>    xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>>>>    2 files changed, 27 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>>>> index b5d54c569b..94f0a6c384 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
>>>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>>        case HSR_SYSREG_MDCCSR_EL0:
>>>>            /*
>>>> +         * 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 (only if partial emulation is
>>>> enabled) to
>>>> +         * indicate the transmit buffer is full, we would hint the
>>>> OS that the
>>>> +         * DCC is probably not working.
>>>> +         *
>>>> +         * Bit 29: TX full
>>>> +         *
>>>>             * 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.
>>>
>>> The sentence "we emulate that register as ..." seems to be stale?
> I can see that you tried to handle Julien remark here. But I disagree. This statement
> is not stale. It explains that because MDSCR_EL1 is RAZ/WI, MDCCSR_EL0 is RO at both
> EL0 and EL1. This patch does not change this behavior.

Indeed. I misread the comment. So what I wrote can be ignored here.

> 
>>>
>>>>             */
>>>> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>>>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read,
>>>> hsr, 0,
>>>> +                                  partial_emulation ? (1U << 29) : 0);
>>>> +
>>>> +    case HSR_SYSREG_DBGDTR_EL0:
>>>> +    /* DBGDTR[TR]X_EL0 share the same encoding */
>>>> +    case HSR_SYSREG_DBGDTRTX_EL0:
>>>> +        if ( !partial_emulation )
>>>> +            goto fail;
>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>>>
>>> AFAICT, all the emulation helpers have an explanation why we are using
>>> them. But here this is not the case. Can you add one?
>> This and..
>>>
>>>> +
>>>>        HSR_SYSREG_DBG_CASES(DBGBVR):
>>>>        HSR_SYSREG_DBG_CASES(DBGBCR):
>>>>        HSR_SYSREG_DBG_CASES(DBGWVR):
>>>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>         * And all other unknown registers.
>>>>         */
>>>>        default:
>>>> + fail:
>>>
>>> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet
>>> (?) accepted the rule, but I don't see we would not given I feel this
>>> is similar to what Rule 16.2 is trying to prevent and we accepted it.
>>>
>>> I think case, I move all the code within default outside. And then
>>> call "goto fail" from the default label.
>>
>> I am not sure if I have interpreted this correctly.
>>
>> Is it ok if you can take a look at the attached patch and let me know if
>> the explaination and the code change looks sane ?
> Looking at the attached patch and fail handling, I don't think it is what Julien meant.
> In the default case you should jump to fail that would be defined outside of switch clause.
> 
> Something like:
>      default:
>          goto fail;
>      }
> 
>      regs->pc += 4;
>      return;
> 
> fail:
>      gdprintk...

+1.

> 
> When it comes to explanation for HSR_SYSREG_DBGDTRTX_EL0, I will let Julien to provide a comment he believes is right.
> To me, it feels strange to repeat almost the same information as for MDCCSR_EL0.

It is not clear to me whether you are objecting of adding a comment or 
whether you would be ok with a comment that is not duplicating.

If the latter, you could move the first paragraph outside of the case 
and then have a register specific explanation of the implementation.

Cheers,

-- 
Julien Grall

Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Michal Orzel]

Hi Julien,

On 19/02/2024 19:48, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 19/02/2024 15:43, Michal Orzel wrote:
> 
>> On 19/02/2024 15:45, Ayan Kumar Halder wrote:
>>>
>>> On 06/02/2024 19:05, Julien Grall wrote:
>>>> Hi Ayan,
>>> Hi Julien/Michal,
>>>>
>>>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>>>> From: Michal Orzel <michal.orzel@amd.com>
>>>>>
>>>>> Currently, if user enables HVC_DCC config option in Linux, it invokes
>>>>> access
>>>>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64,
>>>>> DBGDTRTXINT on
>>>>> arm32). As these registers are not emulated, Xen injects an undefined
>>>>> exception to the guest and Linux crashes.
>>>>>
>>>>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
>>>>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as
>>>>> TXfull.
>>>>>
>>>>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
>>>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>>>>>
>>>>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
>>>>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>>>>> hvc_dcc_check(),
>>>>> and returns -ENODEV in case TXfull bit is still set after writing a test
>>>>> character. This way we prevent the guest from making use of HVC DCC as a
>>>>> console.
>>>>>
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>> ---
>>>>> Changes from
>>>>>
>>>>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving
>>>>> the OS any
>>>>> indication that the RX buffer is full and is waiting to be read.
>>>>>
>>>>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at
>>>>> EL0 only.
>>>>>
>>>>> 3. Fixed the commit message and inline code comments.
>>>>>
>>>>> v2 :- 1. Split the patch into two (separate patches for arm64 and
>>>>> arm32).
>>>>> 2. Removed the "fail" label.
>>>>> 3. Fixed the commit message.
>>>>>
>>>>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
>>>>> partial_emulation_enabled is true or not.
>>>>>
>>>>> 2. If partial_emulation_enabled is false, then access to
>>>>> HSR_SYSREG_DBGDTR_EL0,
>>>>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>>>>>
>>>>>    xen/arch/arm/arm64/vsysreg.c         | 28 ++++++++++++++++++++++++----
>>>>>    xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>>>>>    2 files changed, 27 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>>>>> index b5d54c569b..94f0a6c384 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
>>>>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>>            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>>>        case HSR_SYSREG_MDCCSR_EL0:
>>>>>            /*
>>>>> +         * 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 (only if partial emulation is
>>>>> enabled) to
>>>>> +         * indicate the transmit buffer is full, we would hint the
>>>>> OS that the
>>>>> +         * DCC is probably not working.
>>>>> +         *
>>>>> +         * Bit 29: TX full
>>>>> +         *
>>>>>             * 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.
>>>>
>>>> The sentence "we emulate that register as ..." seems to be stale?
>> I can see that you tried to handle Julien remark here. But I disagree. This statement
>> is not stale. It explains that because MDSCR_EL1 is RAZ/WI, MDCCSR_EL0 is RO at both
>> EL0 and EL1. This patch does not change this behavior.
> 
> Indeed. I misread the comment. So what I wrote can be ignored here.
> 
>>
>>>>
>>>>>             */
>>>>> -        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>>>>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read,
>>>>> hsr, 0,
>>>>> +                                  partial_emulation ? (1U << 29) : 0);
>>>>> +
>>>>> +    case HSR_SYSREG_DBGDTR_EL0:
>>>>> +    /* DBGDTR[TR]X_EL0 share the same encoding */
>>>>> +    case HSR_SYSREG_DBGDTRTX_EL0:
>>>>> +        if ( !partial_emulation )
>>>>> +            goto fail;
>>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>>>>
>>>> AFAICT, all the emulation helpers have an explanation why we are using
>>>> them. But here this is not the case. Can you add one?
>>> This and..
>>>>
>>>>> +
>>>>>        HSR_SYSREG_DBG_CASES(DBGBVR):
>>>>>        HSR_SYSREG_DBG_CASES(DBGBCR):
>>>>>        HSR_SYSREG_DBG_CASES(DBGWVR):
>>>>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>>         * And all other unknown registers.
>>>>>         */
>>>>>        default:
>>>>> + fail:
>>>>
>>>> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet
>>>> (?) accepted the rule, but I don't see we would not given I feel this
>>>> is similar to what Rule 16.2 is trying to prevent and we accepted it.
>>>>
>>>> I think case, I move all the code within default outside. And then
>>>> call "goto fail" from the default label.
>>>
>>> I am not sure if I have interpreted this correctly.
>>>
>>> Is it ok if you can take a look at the attached patch and let me know if
>>> the explaination and the code change looks sane ?
>> Looking at the attached patch and fail handling, I don't think it is what Julien meant.
>> In the default case you should jump to fail that would be defined outside of switch clause.
>>
>> Something like:
>>      default:
>>          goto fail;
>>      }
>>
>>      regs->pc += 4;
>>      return;
>>
>> fail:
>>      gdprintk...
> 
> +1.
> 
>>
>> When it comes to explanation for HSR_SYSREG_DBGDTRTX_EL0, I will let Julien to provide a comment he believes is right.
>> To me, it feels strange to repeat almost the same information as for MDCCSR_EL0.
> 
> It is not clear to me whether you are objecting of adding a comment or
> whether you would be ok with a comment that is not duplicating.
Adding a comment is always welcome. I was against duplication.

@Ayan:
Move a paragraph starting with "Xen doesn't expose" above case HSR_SYSREG_MDCCSR_EL0 and leave rest as is.
For HSR_SYSREG_DBGDTRTX_EL0, add sth like:

Emulate as RAZ/WI (only if partial emulation is enabled) to prevent injecting undefined exception.
Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that register as RAZ/WI.

~Michal