Hi Ayan, On 05/01/2024 12:21, Ayan Kumar Halder wrote: > DBGOSLSR is 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. No need for this dot and yet another thus (it reads difficult). You explained the OSLK bit, but you are not emulating this reg as ro_raz. Instead you copied the code from AArch64 (ro_read_val) which also sets OSLM[1] bit. Do we want the same handling given that Linux on arm32 does not make use of it?
> Thus only MDCCSR_EL0 can be emulated (which is DBGDSCRINT on arm32). > DBGDSCRINT can be accessed at EL0 as DBGDSCREXT is emulated as RAZ (as > DBGOSLSR.OSLK == 0). DBGDSCRINT.TXfull is set to 1. Even though this patch comes after the one explaining the need of emulating DCC I would still expect some reasoning here. Someone reading the vcpreg code and checking the commit behind would not know the rationale behind this patch. Allowing access DBGDSCRINT from EL0 is a fix, so I would make it clear by starting a sentence with "Take the opportunity to fix the minimum EL for DBGDSCRINT ...". > > 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. > > Signed-off-by: Ayan Kumar Halder <[email protected]> > --- > 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. > > xen/arch/arm/include/asm/cpregs.h | 2 ++ > xen/arch/arm/vcpreg.c | 36 ++++++++++++++++++++++--------- > 2 files changed, 28 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..474f872b5f 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, > @@ -549,11 +548,24 @@ 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. > + * > + * Bit 29: TX full > + * > + * Accessible by EL0 if DBGDSCRext.UDCCdis is set to 0, which we > emulate > + * as RAZ/WI in the next case. > */ > - return handle_ro_raz(regs, regidx, cp32.read, hsr, 1); > + return handle_ro_read_val(regs, regidx, cp32.read, hsr, 0, 1U << 29); > + } > > case HSR_CPREG32(DBGDSCREXT): > /* > @@ -562,6 +574,13 @@ void do_cp14_32(struct cpu_user_regs *regs, const union > hsr hsr) > */ > return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); > > +#ifdef CONFIG_PARTIAL_EMULATION > + /* DBGDTR[TR]XINT share the same encoding */ > + case HSR_CPREG32(DBGDTRTXINT): > + if ( opt_partial_emulation ) > + return handle_raz_wi(regs, regidx, cp32.read, hsr, 0); > +#endif > + > case HSR_CPREG32(DBGVCR): > case HSR_CPREG32(DBGBVR0): > case HSR_CPREG32(DBGBCR0): > @@ -659,10 +678,7 @@ void do_cp14_dbg(struct cpu_user_regs *regs, const union > hsr hsr) > * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58 > * > * Unhandled: > - * DBGDTRTXint > - * DBGDTRRXint > - * > - * And all other unknown registers. > + * All unknown registers. > */ > gdprintk(XENLOG_ERR, > "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n", Same comments apply as for the arm64 patch. ~Michal
