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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 31/01/2024 13:10, Ayan Kumar Halder wrote:
> When user enables HVC_DCC config option in Linux, it invokes access to debug
> transfer register (i.e. DBGDTRTXINT). As this register is not emulated, Xen
> injects an undefined exception to the guest and Linux crashes.
> To prevent this crash, introduce a partial emulation of DBGDTRTXINT as RAZ/WI
> and TXfull should be set to 1. So that Linux will see that TXfull is set, and
> it will not access DBGDTRTXINT.
It will access it at least once to later check if TXfull is set.

> 
> As a pre-requisite, DBGOSLSR should be 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.
For that you could just emulate it as RAZ. Instead you also set OSLM[1]. So at 
least
I would make it clear that you do that for consistency.

> Only MDCCSR_EL0 can be emulated (which is DBGDSCRINT on arm32). DBGDSCRINT
I would write: This way, we only need to emulate DBGDSCRINT.

> can be accessed at EL0 as DBGDSCREXT is emulated as RAZ (as DBGOSLSR.OSLK
> == 0). So, we tool the opportunity to fix the minimum EL for DBGDSCRINT.
>  DBGDSCRINT.TXfull is set to 1.
I'm not sure why you are mixing AArch64 names with AArch32 ones. It reads a bit 
difficult.
Furthermore, fixing lowest EL deserves a separate section.
Also s/tool/took.

> 
> 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.
> 
> Thus, any OS is expected to read DBGDSCRINT and check for TXfull before using
> DBGDTRTXINT.
> 
> 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.
> 
> v3 :- 1. The original emulation of DBGDSCRINT is retained when
> 'partial_emulation' is false.
> 
> 2. If 'partial_emulation' is false, then access to DBGDTRTXINT will
> lead to undefined exception.
> 
>  xen/arch/arm/include/asm/cpregs.h |  2 ++
>  xen/arch/arm/vcpreg.c             | 35 ++++++++++++++++++++++---------
>  2 files changed, 27 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..87df4bd238 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,
> @@ -550,10 +549,22 @@ 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.
I'm a bit suprised that these messages differ. Why not to use the same text as 
arm64 but with
register names updated?

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal