Re: [PATCH v2 6/9] xen: arm: Handle CP14 32-bit register accesses from userspace

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 10/02/15 04:45, Ian Campbell wrote:
> Accesses to these from 32-bit userspace would cause a hypervisor exception
> (host crash) when running a 64-bit kernel, which is worked around by the fix to
> XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
> incorrect but harmless.
> 
> Update as follows:
>  - DBGDSCRINT should be R/O.
>  - DBGDSCREXT should be EL1 only.
>  - DBGOSLAR is RO and EL1 only.
>  - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.
> 
> DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
> emulate that as RAZ/WI we allow access.
> 
> When we do not allow an access we now silently inject an undef even in debug
> mode since the debugging messages are not helpful (we have handled the access,
> by explicitly choosing not to).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/traps.c |   34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6045396..18f8332 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1721,10 +1721,12 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
>      switch ( hsr.bits & HSR_CP32_REGS_MASK )
>      {
>      case HSR_CPREG32(DBGDIDR):
> -
> -        /* Read-only register */
> +        /*
> +         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> +         * is set to 0, which we emulated below.
> +         */
>          if ( !cp32.read )
> -            goto bad_cp;
> +            goto undef_cp14_32;
>  
>          /* Implement the minimum requirements:
>           *  - Number of watchpoints: 1
> @@ -1737,15 +1739,24 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
>          break;
>  
>      case HSR_CPREG32(DBGDSCRINT):
> +        if ( !cp32.read )
> +            goto undef_cp14_32;
> +
> +        *r = 0;
> +        break;
> +

The comment explaining why this field is RAZ/WI is useful. I would add
it again here.

Regards,


-- 
Julien Grall