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

[Tim Deegan <tim@xxxxxxx>]

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@xxxxxxxxxx> 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.

> 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.

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?

Cheers,

Tim.

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