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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 04/12/2023 10:31, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 01/12/2023 18: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.
> I am missing some data points here to be able to say whether I would 
> be ok with emulating the registers. So some questions:
>   * As you wrote below, HVC_DCC will return -ENODEV after this 
> emulation. So may I ask what's the use case to enable it? (e.g. is 
> there a distro turning this on?)
I am not aware of any distro using (or not using) this feature. This 
issue came to light during our internal testing, when HVC_DCC was 
enabled to use the debug console. When Linux runs without Xen, then we 
could observe the logs on the debug console. When Linux was running as a 
VM, it crashed.
My intention here was to do the bare minimum emulation so that the crash 
could be avoided.
>  * Linux is writing to the registers unconditionally, but is the spec 
> mandating the implementation of the registers? (I couldn't find either 
> way)
From ARM DDI 0487I.a ID081822, H1.2, External debug,

"The Debug Communications Channel, DCC, passes data between the PE and 
the debugger:
— The DCC includes the data transfer registers, DTRRX and DTRTX, and 
associated flow-control flags.
— Although the DCC is an essential part of Debug state operation, it can 
also be used in Non-debug state."
From this I infer that the spec mandates the implementation of these 
registers. IOW, these registers should always be present in any Arm 
compliant SoC.
>  * When was this check introduced in Linux? Did it ever changed?
This check was introduced in the following commit

"commit f377775dc083506e2fd7739d8615971c46b5246e
Author: Rob Herring <rob.herring@xxxxxxxxxxx>
Date:   Tue Sep 24 21:05:58 2013 -0500

    TTY: hvc_dcc: probe for a JTAG connection before registering

    Enabling the ARM DCC console and using without a JTAG connection will
    simply hang the system. Since distros like to turn on all options, this
    is a reoccurring problem to debug. We can do better by checking if
    anything is attached and handling characters. There is no way to probe
    this, so send a newline and check that it is handled.
"

As of today, this check hasn't changed.

> > We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
> > is emulated as TXfull | RXfull.
> Skimming through the Arm Arm, I see that TXfull and Rxfull indicates 
> that both buffers are full but it doesn't explicitly say this means 
> the feature is not available.
We are not saying that the feature is not available. Rather ...
> I understand this is what Linux checks, but if we want to partially 
> emulate the registers in Xen, then I'd like us to make sure this is 
> backed by the Arm Arm rather than based on Linux implementation (which 
> can change at any point).
> > 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.
> See above, what guarantees us that Linux will not change this behavior 
> in the future?
If I understand "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN" 
correctly, it seems that Arm ARM expects OS to check for TXfull.
If the condition is true, it should return some error.

Let me know if I misunderstood this.

> > 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
> > 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>
> We usually expect the first Signed-off-by to also be the author. So 
> should Michal be the author of this patch?
Yes, I will make Michal as the author.
> > ---
> >   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):
> > +    {
> >           /*
> > -         * 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).
> If you want to mention Linux then you need to be a bit more specific 
> because Linux can change at any point. So you at least want to specify 
> the Linux version and place in the code.
> So this doesn't get stale as soon as the HVC_DCC driver changes.
(based on Linux behavior since f377775dc083).

- Ayan

> >            */
> > -        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);
> > +    }
> >   +    case HSR_CPREG32(DBGDTRTXINT):
> >       case HSR_CPREG32(DBGVCR):
> >       case HSR_CPREG32(DBGBVR0):
> >       case HSR_CPREG32(DBGBCR0):
> Cheers,