Re: [XEN v5 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 26/02/2024 08:39, Michal Orzel wrote:
> Hi Ayan,
Hi,
> On 23/02/2024 16:41, Ayan Kumar Halder wrote:
> > Hi,
> >
> > On 20/02/2024 12:17, Ayan Kumar Halder 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 (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
> > > arm32). As these registers are not emulated, Xen injects an undefined
> > > exception to the guest and Linux crashes.
> > >
> > > To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
> > > (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.
> > >
> > > Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
> > > "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
> > >
> > > Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
> > > using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
> > > and returns -ENODEV in case TXfull bit is still set after writing a test
> > > character. This way we prevent the guest from making use of HVC DCC as a
> > > console.
> > >
> > > Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > > Reviewed-by: Michal Orzel <michal.orzel@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. Removed the "fail" label.
> > > 3. Fixed the commit message.
> > >
> > > v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
> > > partial_emulation_enabled is true or not.
> > >
> > > 2. If partial_emulation_enabled is false, then access to HSR_SYSREG_DBGDTR_EL0,
> > > HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
> > >
> > > v4 :- 1. Invoked "goto fail" from "default:" to ensure compliance with
> > > MISRA 15.3.
> > >
> > >    xen/arch/arm/arm64/vsysreg.c         | 68 +++++++++++++++++++---------
> > >    xen/arch/arm/include/asm/arm64/hsr.h |  3 ++
> > >    2 files changed, 50 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> > > index b5d54c569b..80918bc799 100644
> > > --- a/xen/arch/arm/arm64/vsysreg.c
> > > +++ b/xen/arch/arm/arm64/vsysreg.c
> > > @@ -82,6 +82,7 @@ TVM_REG(CONTEXTIDR_EL1)
> > >    void do_sysreg(struct cpu_user_regs *regs,
> > >                   const union hsr hsr)
> > >    {
> > > +    const struct hsr_sysreg sysreg = hsr.sysreg;
> > >        int regidx = hsr.sysreg.reg;
> > >        struct vcpu *v = current;
> > >    
> > > @@ -159,9 +160,6 @@ void do_sysreg(struct cpu_user_regs *regs,
> > >         *
> > >         * Unhandled:
> > >         *    MDCCINT_EL1
> > > -     *    DBGDTR_EL0
> > > -     *    DBGDTRRX_EL0
> > > -     *    DBGDTRTX_EL0
> > >         *    OSDTRRX_EL1
> > >         *    OSDTRTX_EL1
> > >         *    OSECCR_EL1
> > > @@ -171,12 +169,42 @@ void do_sysreg(struct cpu_user_regs *regs,
> > >         */
> > >        case HSR_SYSREG_MDSCR_EL1:
> > >            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> > > +
> > > +    /*
> > > +     * 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.
> > > +     */
> > >        case HSR_SYSREG_MDCCSR_EL0:
> > >            /*
> > > +         * By setting TX status bit (only if partial emulation is enabled) to
> > > +         * indicate the transmit buffer is full, 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,
> > > +                                  partial_emulation ? (1U << 29) : 0);
> > > +
> > > +    case HSR_SYSREG_DBGDTR_EL0:
> > > +    /* DBGDTR[TR]X_EL0 share the same encoding */
> > > +    case HSR_SYSREG_DBGDTRTX_EL0:
> > > +        /*
> > > +         * Emulate as RAZ/WI (only if partial emulation is enabled) to prevent
> > > +         * injecting undefined exception.
> > > +         * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate 
> > > that
> > > +         * register as RAZ/WI.
> > > +         */
> > > +        if ( !partial_emulation )
> > > +            goto fail;
> > > +        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):
> > > @@ -394,26 +422,24 @@ void do_sysreg(struct cpu_user_regs *regs,
> > >         * And all other unknown registers.
> > >         */
> > >        default:
> > > -        {
> > > -            const struct hsr_sysreg sysreg = hsr.sysreg;
> > > -
> > > -            gdprintk(XENLOG_ERR,
> > > -                     "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
> > > -                     sysreg.read ? "mrs" : "msr",
> > > -                     sysreg.op0, sysreg.op1,
> > > -                     sysreg.crn, sysreg.crm,
> > > -                     sysreg.op2,
> > > -                     sysreg.read ? "=>" : "<=",
> > > -                     sysreg.reg, regs->pc);
> > > -            gdprintk(XENLOG_ERR,
> > > -                     "unhandled 64-bit sysreg access %#"PRIregister"\n",
> > > -                     hsr.bits & HSR_SYSREG_REGS_MASK);
> > > -            inject_undef_exception(regs, hsr);
> > > -            return;
> > > -        }
> > > +        goto fail;
> > >        }
> > >    
> > >        regs->pc += 4;
> > > +    return;
> > > +
> > > + fail:
> > > +
> No need for this empty line.
Ack
> > > +    gdprintk(XENLOG_ERR,
> > > +             "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
> > > +             sysreg.read ? "mrs" : "msr", sysreg.op0, sysreg.op1, sysreg.crn,
> > > +             sysreg.crm, sysreg.op2, sysreg.read ? "=>" : "<=", sysreg.reg,
> > > +             regs->pc);
> The original formatting (i.e. placement of printk args) looked better. I'm not 
> sure why you changed it.
My bad. I changed it to align with 80 chars. I will revert it.
> > > +    gdprintk(XENLOG_ERR,
> > > +             "unhandled 64-bit sysreg access %#"PRIregister"\n",
> > > +             hsr.bits & HSR_SYSREG_REGS_MASK);
> > > +    inject_undef_exception(regs, hsr);
> > > +    return;
> > The last 'return' needs to be removed (spotted by Michal)
Ack

- Ayan

> >
> ~Michal