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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 05/01/2024 12:21, Ayan Kumar Halder wrote:
> From: Michal Orzel <michal.orzel@xxxxxxx>
> 
> 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 VM and Linux (running as VM) crashes.
> 
> We wish to avoid this crash by adding a partial emulation of DBGDTRTX_EL0.
> MDCCSR_EL0 is emulated 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(),
> it returns -ENODEV. In this way, we are preventing the VM from accessing
> DBGDTRTX_EL0 register.
AFAIK, we are preventing the VM from making use of HVC DCC as a console.
DBGDTRTX_EL0 and MDCCSR_EL0 will be accessed at least once as part of 
hvc_dcc_check().

> 
> We also emulate DBGDTR[TR]X_EL0 as RAZ/WI.
> 
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> 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.
> 
>  xen/arch/arm/arm64/vsysreg.c         | 25 +++++++++++++++++++++----
>  xen/arch/arm/include/asm/arm64/hsr.h |  3 +++
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..2f70eea2e5 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,31 @@ 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:
> +    {
>          /*
> +         * 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, 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,
> +                                  1U << 29);
I would assume that if partial emulation is not enabled, we should stick to 
ro_raz
instead of emulating TX set.

> +    }
> +#ifdef CONFIG_PARTIAL_EMULATION
> +    case HSR_SYSREG_DBGDTR_EL0:
> +    /* DBGDTR[TR]X_EL0 share the same encoding */
> +    case HSR_SYSREG_DBGDTRTX_EL0:
> +        if ( opt_partial_emulation )
> +            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
This is incorrect. What will happen if CONFIG_PARTIAL_EMULATION=y and 
opt_partial_emulation=false ?
You would fall through to cases below and end up in RAZ/WI. Instead, you should 
jump to default case,
so that we would inject undefined exception.

> +#endif
>      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)

~Michal