Re: [Xen-devel] [PATCH v2 3/3] xen/arm: errata 766422: decode thumb store during data abort

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> @@ -996,6 +997,28 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> +static int read_instruction(struct cpu_user_regs *regs, unsigned len,
> +                            uint32_t *instr)
> +{
> +    int rc;
> +
> +    rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ? 4 : 2));
> +
> +    if ( rc )
> +        return rc;
> +
> +    if ( !len ) /* 16-bit instruction */
> +        *instr &= 0xffff;
> +    else /* 32-bit instruction */
> +    {
> +        /* THUMB 32-bit instruction consisting of 2 consecutive halfwords */

Please could you incorporate something like Tim's description from
<20130729144626.GI37169@xxxxxxxxxxxxxxxxxxxxx> to make it totally
obvious what is going on here.

> +        if ( regs->cpsr & PSR_THUMB )
> +            *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;

Please can you add a comment like
          /* else: already in correct order for an ARM instruction */

> +    }
> +
> +    return 0;
> +}
> +
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                                       struct hsr_dabt dabt)
>  {
> @@ -1021,6 +1044,27 @@ static void do_trap_data_abort_guest(struct 
> cpu_user_regs *regs,
>      if ( !dabt.valid )
>          goto bad_data_abort;
>  
> +    /*
> +     * Errata 766422: Thumb store translation fault to Hypervisor may
> +     * not have correct HSR Rt value.
> +     */
> +    if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
> +    {
> +        uint32_t instr = 0;
> +
> +        rc = read_instruction(regs, dabt.len, &instr);
> +        if ( rc )
> +            goto bad_data_abort;
> +
> +        /* Retrieve the transfer register from the instruction */
> +        if ( dabt.len )
> +            /* With 32-bit store instruction, the register is in [12..15] */
> +            info.dabt.reg = (instr & 0xf000) >> 12;
> +        else
> +            /* With 16-bit store instruction, the register is in [0..3] */
> +            info.dabt.reg = instr & 0x7;

Encoding T2 (store via imm8 offset from sp) has it in 8..10.

Also for clarity I think you should write "With a NN-bit Thumb store
instruction".

> +    }
> +
>      if (handle_mmio(&info))
>      {
>          regs->pc += dabt.len ? 4 : 2;
> diff --git a/xen/include/asm-arm/arm32/processor.h 
> b/xen/include/asm-arm/arm32/processor.h
> index b266252..bc82fbc 100644
> --- a/xen/include/asm-arm/arm32/processor.h
> +++ b/xen/include/asm-arm/arm32/processor.h
> @@ -111,6 +111,9 @@ struct cpu_user_regs
>  #define READ_SYSREG(R...)       READ_SYSREG32(R)
>  #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
>  
> +/* Errata 766422: only Cortex A15 r0p4 is affected */
> +#define cpu_has_errata_766422() (current_cpu_data.midr.bits == 0x410fc0f4)

Do we have unlikely() in Xen? If yes then I think this is a good place
to use it.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel