Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA

[Ian Campbell <ian.campbell@citrix.com>]

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 <ian.campbell@citrix.com>
> > ---
> >   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