Re: [PATCH v2 1/3] xen/arm: Add emulation of Debug Data Transfer Registers

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 18 Dec 2023, at 20:32, Michal.Orzel@xxxxxxx 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 MDCCSR_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.
> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
> 
> 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 in the case of AArch32, DBGOSLSR is 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.
> Thus only MDCCSR_EL0 can be emulated (which is DBGDTRTXINT on arm32).
> So, DBGDTR[TR]XINT is emulated 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.
> 
> xen/arch/arm/arm64/vsysreg.c         | 26 +++++++++++++++----
> 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                | 38 +++++++++++++++++++++++-----
> 6 files changed, 79 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..ebeb83dd65 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,29 @@ 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. This 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);
> +    }
> +    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):
> @@ -393,7 +408,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>      *
>      * And all other unknown registers.
>      */
> -    default:
> +    default: goto fail;
> +    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)
> 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..5087125111 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -549,18 +549,41 @@ 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, 1 << 29);
> +    }
> 
>     case HSR_CPREG32(DBGDSCREXT):
> +        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
> +
> +    case HSR_CPREG32(DBGOSLSR):
> +         return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1, 1 << 3);
> +
> +    case HSR_CPREG32(DBGDTRTXINT):
> +    {
>         /*
> -         * Implement debug status and control register as RAZ/WI.
> -         * The OS won't use Hardware debug if MDBGen not set.
> +         * As DBGDSCRINT is emulated which is architecturally mapped to 
> AArch64
> +         * register MDCCSR_EL0. MDSCR_EL1 is not emulated. So DBGDTR[TR]XINT 
> can
> +         * only be accessed as EL0 level.
>          */
> -        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
> +        return handle_raz_wi(regs, regidx, cp32.read, hsr, 0);
> +    }
> 
>     case HSR_CPREG32(DBGVCR):
>     case HSR_CPREG32(DBGBVR0):
> @@ -590,7 +613,8 @@ void do_cp14_32(struct cpu_user_regs *regs, const union 
> hsr hsr)
>      *
>      * And all other unknown registers.
>      */
> -    default:
> +    default: goto fail;

I think this label doesn’t belong here, it is used only in patch 3.

> +    fail:
>         gdprintk(XENLOG_ERR,
>                  "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
>                   cp32.read ? "mrc" : "mcr",
> -- 
> 2.25.1
> 
>