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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 07/24/2013 03:55 PM, Ian Campbell wrote:
> 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...

The ARM ARM specification describes a THUMB 32 bit instruction with
   HW1 HW2

HW1 => [31:16] => most significant
HW2 => [15:0] => less significant

raw_copy_from_copy will directly read 4 bytes and store in an uint32_t.
As Xen is running in little endian, it ends up in:
HW2 => [31:16]
HW1 => [15:0]

Which is false. If it's more clear, I can do something like this

uint16_t a[2];
rc = raw_copy_from_guest(instr, (void * __user)regs->pc, len);

...

switch ( len )
{
...
case 4:
   instr = a[0] << 16 | a[1];
...
}

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

I tried on the arndale board.

-- 
Julien


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