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

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

On 07/29/2013 04:00 PM, Ian Campbell wrote:
> On Mon, 2013-07-29 at 15:56 +0100, Julien Grall wrote:
>> On 07/29/2013 03:46 PM, Tim Deegan wrote:
>>> Hi,
>>>
>>> At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
>>>> 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@citrix.com> wrote:
>>>>>>> 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.
>>>
>>> Yes.  This seems to be pretty much explicit in section A6.1 -- you can
>>> read 16 bits and decide from those whether you need to read another 16.
>>>
>>> "If bits [15:11] of the halfword being decoded take any of the following
>>>  values, the halfword is the first halfword of a 32-bit instruction"
>>>
>>>>> 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.
>>>
>>> Sadly, no.  Instruction memory is always little-endian, but:
>>>
>>> "The Thumb instruction stream is a sequence of halfword-aligned
>>>  halfwords. Each Thumb instruction is either a single 16-bit halfword
>>>  in that stream, or a 32-bit instruction consisting of two consecutive
>>>  halfwords in that stream."
>>>
>>> So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
>>> with the magic 5-bit pattern) will be stored in memory as a mixed-endian
>>> monstrosity:
>>>
>>> PC:   B
>>> PC+1: A
>>> PC+2: D
>>> PC+3: C
>>>
>>> A little-endian halfword read of PC gives you AB; another halfword read
>>> at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.
>>>
>>> We don't necessarily have to do the bit-shuffling explicitly: we could
>>> do thumb32 decode with the shuffle implicit in the layout used for
>>> decoding.
>>
>> I think we need to keep the bit-shuffling explicitly. It's less
>> confusing than having "non-coherent" shift during the decoding.
> 
> Whatever we do it needs a big comment along the lines of what Tim wrote
> above.

I will add the comment in the third version of this patch series.

-- 
Julien