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 23:43 +0100, Julien Grall wrote:
> On 23 July 2013 23:16, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > 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?
> 
> In THUMB set, a 32-bit instruction consisting of two consecutive
> halfwords (see section A6.1).
> So the instruction is in "big endian", but Xen will read the word as a
> "little endian" things.

Are you saying that the 16-bit sub-words are in the opposite order to
what I would have expected and what seems most natural from a decode
PoV?

Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
        1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12

(where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
and imm12 is the remainder of the second halfword).

I would have expected that the halfword with the "11111" pattern (which
identifies this as a 32-bit thumb instruction) would have to come first,
so the decode knows to look for the second. IOW the halfword with 11111
should be at PC and the Rt/imm12 halfword should be at PC+2. So if we
copy 4 bytes from guest PC we should end up with things in the order
given above (and in the manual) and swapping shouldn't be needed.
Perhaps I need to go have some coffee...

Have you tested and actually observed this case on real h/w?

> 
> But in ARM set, a 32-bit instruction is a single word. I will add a
> check to handle this case.
> 



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