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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

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.

>> +    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.

>> +    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)
> 
~Michal