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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 18/12/2023 21: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,
you mention status reg on arm64 and TX reg on arm32.
Instead, it should be DBGDTRTX_EL0 on arm64.

> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
> an undefined exception to the guest. And Linux crashes.
no need for a full stop before And

> 
> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
MDCCSR_EL0 (not DBGDTR_EL0)

> is emulated as TXfull.
> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
NIT: best practice is to refer to the latest version of a doc (i.e. J.a)

> "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
MDCCSR_EL0 (not DBGDTR_EL0)

> 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.
s/DBGDTRTX_EL0/DBGDTR[TR]X_EL0/
> 
> 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.

I can see in the code that you followed my suggestion to fix DBGDSCRINT,
so that it can be accessed at EL0 (because DBGDSCREXT is RAZ). However, you do 
not mention it anywhere.

> 
> 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
... is full, we would hint...

> +         * 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:
What is going on here? I see no use so please remove.

>          {
>              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);
> +
Leftover change? Please remove this prototype.

>  /* 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);
> +}
Leftover change? Please remove this function.

> +
> +
>  /* 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)
>      }
>  
Above DBGDIDR, there is a comment listing unhandled registers. It looks like 
you forgot to
update it.

>      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);
Why not 1U << 29 like in AArch64 case?

> +    }
>  
>      case HSR_CPREG32(DBGDSCREXT):
I would keep the comment that was here about MDBGen.

> +        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);
This should be moved between DBGOSLAR and DBGOSDLR. Also, you should update a 
comment
listing unhandled registers (above DBGOSLAR).

> +
Please add: /* DBGDTR[TR]XINT share the same encoding */
> +    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.
>           */
I struggle to understand this comment. The minimum EL for DBGDTR[TR]XINT is 0, 
which means it can
be accessed at any level. I see no need for this comment (just like there is 
none for arm64).

> -        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;
> +    fail:
same as before

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


~Michal