Re: [PATCH 15/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDA

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

On Mon, 2015-04-06 at 15:41 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Gather the affected handlers in a single place per trap type.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   71 ++++++++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 60 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 518f047..0ab5e56 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
> >       case HSR_CPREG32(DBGOSDLR):
> >           return handle_raz_wi(regs, r, cp32.read, hsr, 1);
> >
> > +    /*
> > +     * MDCR_EL2.TDA
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B1.14.15
> > +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-59
> > +     *
> > +     * Unhandled:
> > +     *    DBGDCCINT
> > +     *    DBGDTRRXint
> > +     *    DBGDTRTXint
> > +     *    DBGWFAR
> > +     *    DBGDTRTXext
> > +     *    DBGDTRRXext,
> > +     *    DBGBXVR<n>
> > +     *    DBGCLAIMSET
> > +     *    DBGCLAIMCLR
> > +     *    DBGAUTHSTATUS
> > +     *    DBGDEVID
> > +     *    DBGDEVID1
> > +     *    DBGDEVID2
> > +     *    DBGOSECCR
> 
> Listing unhandled registers doesn't bring much information.

Actually it does and it was quite deliberate and part of the purpose of
the series. By listing these unhandled registers you can now look at the
docs (in particular the ARMv8 table is very good for this) and see
immediately that every register is either handled or listed as
unhandled.

Without that you need to remember why certain things aren't handled in
order to distinguish them from things which have been accidentally
forgotten.

> Also, it's very confusing that you are mixing AArch64 name with AArch32 
> name. The former is used for the traps register while the latter is used 
> for register name.

For the traps register where it is similar enough I chose to only list
the Aarch64 name, since that is what we use when we write it. I could
write both I suppose but it didn't seem especially useful.

For the registers themselves it is easier to correlate with the docs if
the name which is relevant to the context (e.g. an arm32 vs arm64
handler). Comparing two lists where one is Aarch64 names and the other
Aarch32 is hard, especially in the dbg registers where the names don't
always correlate especially obviously.

The only change I'd be willing to make here would be to list HDCR.TDA
after MDCR_EL2.TDA.

> > +     */
> >       case HSR_CPREG32(DBGDIDR):
> >           /*
> >            * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> > @@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
> >           return;
> >       }
> >
> > +    /*
> > +     * MDCR_EL2.TDOSA
> 
> Did you mean MDCR_EL2.TDA?
> 
> [..]

No, this hunk should have been in the previous patch. Moved.

> 
> >       case HSR_SYSREG_MDSCR_EL1:
> > -    /*  - Breakpoints */
> 
> [..]
> 
> > -    /*  - Watchpoints */
> 
> I think this 2 comments was useful in order to split the list of registers.

In the new code this is just:
    HSR_SYSREG_DBG_CASES(DBGBVR):
    HSR_SYSREG_DBG_CASES(DBGBCR):
    HSR_SYSREG_DBG_CASES(DBGWVR):
    HSR_SYSREG_DBG_CASES(DBGWCR):
        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);

I don't think the comments were adding much any more, especially given
we aren't actually doing anything with them.

Ian.