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

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

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.

>> uint16_t a[2];
>> rc = raw_copy_from_guest(instr, (void * __user)regs->pc, len);
> 
> You definitely should read exactly the correct number of bytes -- if we
> are decoding a 16-bit instruction at the end of a page we don't want to
> trigger a fault by reading 32 bits and crossing a page boundary.

I already read either 16 or 32 bits depending of the instruction len.
The array is bigger to fit to the both instruction.

> Oh, and one other comment that I've already lost the context for: can
> you please rename the instruction fetch-and-shuffle function to have
> 'thumb' in its name somewhere?

Actually, I have sent a version for this patch series
(http://www.gossamer-threads.com/lists/xen/devel/291808). It's also
support ARM instruction decoding.

Cheers,

-- 
Julien