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

[Julien Grall <julien.grall@linaro.org>]

On 07/31/2013 09:47 AM, Ian Campbell wrote:
> On Tue, 2013-07-30 at 18:37 +0100, Julien Grall wrote:
>> On 07/29/2013 04:15 PM, Ian Campbell wrote:
>>> On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> 
>>>> +        /* 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;
>>>
>>> Encoding T2 (store via imm8 offset from sp) has it in 8..10.
>>
>> Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if
>> it "is not using the PC as its destination register". So this
>> instruction is consider as invalid and will go to "bad_data_abort".
> 
> I'm not sure what this has to do with the encoding I pointed to.
> 
> A8.8.203 STR (immediate, Thumb), Encoding T2:
>         STR<c> <Rt>, [SP, #<imm>]
> is:
>         1001 0ttt mmmm mmmm (ttt=Rt, mmmm mmmm=imm)
>

Oh right, I read 'pc' instead of 'sp'.

> So Rt is in bits 8..10 which != 0..3 which is all you handle above. I
> can't see any reason why you wouldn't need to handle this case, it is
> certainly a valid instruction.
> 
> I think it would be safest to explicitly check for known opcode patterns
> and handle those while logging any which we don't recognise. This might
> be doable with a lookup table but it may be too sparse. If we were doing
> a more full featured instruction decoder then it might be worth it, not
> sure about this very specific case.

As discussed with Ian, I will create a function decode_instruction. It
will decode and fill the HSR.

-- 
Julien