From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA Date: Mon, 6 Apr 2015 15:24:48 +0200 Message-ID: <55228920.6020304@citrix.com> References: <1427796446.2115.34.camel@citrix.com> <1427796462-24376-13-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1427796462-24376-13-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , xen-devel@lists.xen.org Cc: julien.grall@linaro.org, tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 31/03/2015 12:07, Ian Campbell wrote: > Signed-off-by: Ian Campbell > --- > xen/arch/arm/traps.c | 32 ++++++++++++++++++++++++++++++++ > xen/include/asm-arm/cpregs.h | 4 ++++ > xen/include/asm-arm/sysregs.h | 1 + > 3 files changed, 37 insertions(+) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 21bef01..7c37cec 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) > > switch ( hsr.bits & HSR_CP32_REGS_MASK ) > { > + /* > + * MDCR_EL2.TDRA > + * > + * ARMv7 (DDI 0406C.b): B1.14.15 > + * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 > + * > + * Unhandled: > + * DBGDRAR > + * DBGDSAR > + */ > + Why did you put the comment here? For AArch32, only DBGDRAR and DBGSAR are trapped with this bit. I think this should be moved above the label default. > case HSR_CPREG32(DBGDIDR): > /* > * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis > @@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) > * > * ARMv7 (DDI 0406C.b): B1.14.16 > * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 > + * > + * And all other unknown registers. > */ > default: > gdprintk(XENLOG_ERR, > @@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr) > * > * ARMv7 (DDI 0406C.b): B1.14.16 > * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 > + * > + * MDCR_EL2.TDRA > + * > + * ARMv7 (DDI 0406C.b): B1.14.15 > + * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 > + * > + * Unhandled: > + * DBGDRAR64 > + * DBGDSAR64 This is confusing. The real name of the register is DBGDRAR. I would say "DBGDRAR 64-bit". Furthermore, this is the only registers not handled on AArch32 for this bit. This is rather strange to list them while you didn't do it for the trace registers. > + * > + * And all other unknown registers. For consistency, I would have add this part of the comment in patch #10 (where the comment has been added). Anyway, the patch is already written so I'm fine with it. > */ > gdprintk(XENLOG_ERR, > "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n", > @@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs, > *x = v->arch.actlr; > break; > > + /* > + * MDCR_EL2.TDRA > + * > + * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 > + */ > + case HSR_SYSREG_MDRAR_EL1: > + return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 1); This change should be in a separate patch or mention in the commit message. Regards, -- Julien Grall