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 22:41 +0100, Julien Grall wrote:
> On 23 July 2013 19:18, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > 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?
> 
> I was thinking to provide a "high level" function to retrieve an
> instruction just in case we need it in the future.

I suppose that does sound potentially useful.

Were does this the idea of swapping them come from though? The ARM ARM
seems (see e.g. section A6.3) to describe things in the order they are
in memory -- is doing otherwise not going to end up confusing us?

[...]
> > 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.
> 
> Sounds good. I will rework the patch with this solution.

Thanks.

Ian.


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