Re: [Xen-devel] [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
On Tue, 2015-04-21 at 13:26 +0500, Julien Grall wrote: > Hi Ian, > > On 17/04/2015 16:51, Ian Campbell wrote: > >> 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. > > > > My intention was that every register trapped by a bit which we set be > > listed somewhere, to make it easier to cross reference with the docs and > > check we haven't accidentally forgotten something (as opposed to > > deliberately ignoring as indicated by these comments). > > > > You seem to be saying I've missed some trace registers, which ones? > > I meant that you didn't list the trace registers trapped but unhandled. > Although I wasn't able to find a list, is it trace module specific? If > so maybe a comment would be good? I think maybe you are talking about the things trapped by CPTR_EL2.TTA rather than MDCR_EL2.TDRA (the subject of this patch)? The table referenced for CPTR_EL2.TTA just says "All implemented trace registers", rather than listing anything specific. I could add a similar comment to the relevant patch. Looks like HCR_EL2.TIDCP is similarly lacking a comment for the unhandled ones. I'll add one. > > Regards, > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
Hi Ian, On 17/04/2015 16:51, Ian Campbell wrote: 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. My intention was that every register trapped by a bit which we set be listed somewhere, to make it easier to cross reference with the docs and check we haven't accidentally forgotten something (as opposed to deliberately ignoring as indicated by these comments). You seem to be saying I've missed some trace registers, which ones? I meant that you didn't list the trace registers trapped but unhandled. Although I wasn't able to find a list, is it trace module specific? If so maybe a comment would be good? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
On Mon, 2015-04-06 at 15:24 +0200, Julien Grall wrote:
> 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.
Yes, not sure how it got out of place here. Moved.
>
> > 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".
Yes, this was a tricky one because the register is actually two (a 32
and 64 version), hence in the #define I had to add a suffix and I
duplicated that here.
Instead I'll make both of them "DBGDRAR (NN-bit accesses)", similar to
what I did for the #defines
>
> 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.
My intention was that every register trapped by a bit which we set be
listed somewhere, to make it easier to cross reference with the docs and
check we haven't accidentally forgotten something (as opposed to
deliberately ignoring as indicated by these comments).
You seem to be saying I've missed some trace registers, which ones?
>
> > + *
> > + * 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.
I was rebasing any way so I did this.
> >*/
> > 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.
I just added a mention of it since it is the AArch64 version of DBGDRAR.
I can't remember why I made this raz instead of trapping like with
DBGDRAR, but since I already wrote it I left them as is.
Ian
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
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
+ */
+
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
+ *
+ * And all other unknown registers.
*/
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);
+
/* RAZ/WI registers: */
/* - Debug */
case HSR_SYSREG_MDSCR_EL1:
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index afe9148..9db8cfd 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -89,10 +89,14 @@
#define TEECR p14,6,c0,c0,0 /* ThumbEE Configuration Register */
/* CP14 CR1: */
+#define DBGDRAR64 p14,0,c1/* Debug ROM Address Register (64-bit
access) */
+#define DBGDRAR p14,0,c1,c0,0 /* Debug ROM Address Register (32-bit
access) */
#define TEEHBR p14,6,c1,c0,0 /* ThumbEE Handler Base Register */
#define JOSCR p14,7,c1,c0,0 /* Jazelle OS Control Register */
/* CP14 CR2: */
+#define DBGDSAR64 p14,0,c2/* Debug Self Address Offset Register
(64-bit access) */
+#define DBGDSAR p14,0,c2,c0,0 /* Debug Self Address Offset Register
(32-bit access) */
#define JMCRp14,7,c2,c0,0 /* Jazelle Main Configuration Register
*/
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index d75e154..55457fd 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -45,6 +45,7 @@
#define HSR_SYSREG_DCCISW HSR_SYSREG(1,0,c7,c14,2)
#define HSR_SYSREG_MDSCR_EL1 HSR_SYSREG(2,0,c0,c2,2)
+#define HSR_SYSREG_MDRAR_EL1 HSR_SYSREG(2,0,c1,c0,0)
#define HSR_SYSREG_OSLAR_EL1 HSR_SYSREG(2,0,c1,c0,4)
#define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4)
#define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)
--
1.7.10.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
