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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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.

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

-- 
Julien Grall

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