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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 05/01/2024 12:21, Ayan Kumar Halder wrote:
> 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.
No need for this dot and yet another thus (it reads difficult).
You explained the OSLK bit, but you are not emulating this reg as ro_raz. 
Instead you
copied the code from AArch64 (ro_read_val) which also sets OSLM[1] bit. Do we 
want the same handling
given that Linux on arm32 does not make use of it?

> Thus 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). DBGDSCRINT.TXfull is set to 1.
Even though this patch comes after the one explaining the need of emulating DCC
I would still expect some reasoning here. Someone reading the vcpreg code and 
checking the commit
behind would not know the rationale behind this patch.

Allowing access DBGDSCRINT from EL0 is a fix, so I would make it clear by 
starting a sentence
with "Take the opportunity to fix the minimum EL for DBGDSCRINT ...".

> 
> 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.
> 
> 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. Fixed in line comments and style related issues.
> 3. Updated commit message to mention DBGDSCRINT handling.
> 
>  xen/arch/arm/include/asm/cpregs.h |  2 ++
>  xen/arch/arm/vcpreg.c             | 36 ++++++++++++++++++++++---------
>  2 files changed, 28 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..474f872b5f 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,
> @@ -549,11 +548,24 @@ 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, 1U << 29);
> +    }
>  
>      case HSR_CPREG32(DBGDSCREXT):
>          /*
> @@ -562,6 +574,13 @@ void do_cp14_32(struct cpu_user_regs *regs, const union 
> hsr hsr)
>           */
>          return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
>  
> +#ifdef CONFIG_PARTIAL_EMULATION
> +    /* DBGDTR[TR]XINT share the same encoding */
> +    case HSR_CPREG32(DBGDTRTXINT):
> +        if ( opt_partial_emulation )
> +            return handle_raz_wi(regs, regidx, cp32.read, hsr, 0);
> +#endif
> +
>      case HSR_CPREG32(DBGVCR):
>      case HSR_CPREG32(DBGBVR0):
>      case HSR_CPREG32(DBGBCR0):
> @@ -659,10 +678,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",

Same comments apply as for the arm64 patch.

~Michal