Hi Ayan, On 01/12/2023 19: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. > > We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0 > is emulated as TXfull | RXfull. TX/RX are status bits of MDCCSR_EL0, not DBGDTR_EL0. This applies here and elsewhere.
> 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. > > 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 Emulating DBGDSCREXT is not needed. See below. > to return a specific value (ie TXfull | RXfull) and ignore any writes > to this register. > > Signed-off-by: Michal Orzel <[email protected]> > Signed-off-by: Ayan Kumar Halder <[email protected]> > --- > 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): 1) Why did you invert DBGDSCRINT with DBGDSCREXT? The former should appear first to keep the correct order. 2) Take a look at what I did for arm64. I emulated TX/RX only in MDCCSR_EL0 and not in MDSCR_EL1 (on arm32, DBGDSCRINT, DBGDSCREXT respectively). This is because according to Arm ARM for MDSCR_EL1 TXfull/RXfull: "When OSLSR_EL1.OSLK == 0, software must treat this bit as UNK/SBZP" On arm64, we emulate OSLSR_EL1 as (1<<3) meaning OSLK is 0. UNK/SBZP means RAZ/WI. This implies that the only way to access the TXfull/RXfull flags is through MDCCSR_EL0. For arm32, we should do the same. Emulate only RINT and keep REXT as RAZ/WI. The only additional change would be to emulate (at the moment it is unhandled) DBGOSLSR similar to what we do for arm64. > + { > /* > - * 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). > */ > - 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); DBGDSCRINT is accessible at EL0 if DBGDSCREXT.UDCCdis is 0 (we emulate REXT as RAZ). There is even a comment about that which you removed (please restore). Therefore, the minimum EL passed to handle_ro_read_val should be 0 not 1. P.S. The current code is incorrect in this regard. > + } > > + case HSR_CPREG32(DBGDTRTXINT): This would be incorrect. Adding TXINT here would result in calling handle_raz_wi with minimum EL == 1, but TXINT can be accessed at EL0. This should be: /* DBGDTR[TR]XINT share the same encoding */ case HSR_CPREG32(DBGDTRTXINT): return handle_raz_wi(regs, regidx, cp32.read, hsr, 0); > case HSR_CPREG32(DBGVCR): > case HSR_CPREG32(DBGBVR0): > case HSR_CPREG32(DBGBCR0): ~Michal
