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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
> From the errata document:
> 
> When a non-secure non-hypervisor memory operation instruction generates a
> stage2 page table translation fault, a trap to the hypervisor will be 
> triggered.
> For an architecturally defined subset of instructions, the Hypervisor Syndrome
> Register (HSR) will have the Instruction Syndrome Valid (ISV) bit set to 1âb1,
> and the Rt field should reflect the source register (for stores) or 
> destination
> register for loads.
> On Cortex-A15, for Thumb and ThumbEE stores, the Rt value may be incorrect
> and should not be used, even if the ISV bit is set. All loads, and all ARM
> instruction set loads and stores, will have the correct Rt value if the ISV
> bit is set.
> 
> To avoid this issue, Xen needs to decode thumb store instruction and update
> the transfer register.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index d6dc37d..da2bef6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -35,6 +35,7 @@
>  #include <asm/regs.h>
>  #include <asm/cpregs.h>
>  #include <asm/psci.h>
> +#include <asm/guest_access.h>
>  
>  #include "io.h"
>  #include "vtimer.h"
> @@ -996,6 +997,31 @@ 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);
> +
> +    if ( rc )
> +        return rc;
> +
> +    switch ( len )
> +    {
> +    /* 16-bit instruction */
> +    case 2:
> +        *instr &= 0xffff;
> +        break;
> +    /* 32-bit instruction */
> +    case 4:
> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;

Since you only ever consult bits 12..15 or 0..3 of the result couldn't
you just read two bytes from pc+2 instead of reading 4 bytes and
swapping etc?

> +        break;
> +    }
> +
> +    return 0;
> +}
> +
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                                       struct hsr_dabt dabt)
>  {
> @@ -1021,6 +1047,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 ( (regs->cpsr & PSR_THUMB) && dabt.write )

Is there some way to more precisely identify the processors with this
errata? It'd be better to avoid this rigmarole when we can...

I'd think about implementing this as a pseudo-cpuinfo thing setup either
via identify_cpu or perhaps via a processor callback in proc-v7.S and
friends. Then you can define and use something like cpu_has_errata766422
in the conditional and force it to zero for arm64 builds.


> +    {
> +        uint32_t instr = 0;
> +
> +        rc = read_instruction(regs, dabt.len ? 4 : 2, &instr);

You might as well just pass dabt.len directly I think, since you just
decode it again with a switch.

> +        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;
> +    }
> +
>      if (handle_mmio(&info))
>      {
>          regs->pc += dabt.len ? 4 : 2;



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