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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

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

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

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

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

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

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

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

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

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

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


~Michal