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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
>   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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel