1077346 - ARMv6 simulator support for atomics

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

On ARMv6 the barriers are implemented by means of CP15 instructions; we should support those so that all ARMv6 code can run on the simulator.  See the assembler for more, search for dmb_trap, dsb_trap, isb_trap.

dougc makes the point that it would be useful to enable alignment checking in the simulator to catch alignment problems.  This is particularly interesting since on ARMv6 load-exclusive/store-exclusive to byte and halfword data must be implemented with RMW and word-wide load/store.  So add proper alignment checks to the atomic instructions as part of this patch.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Implement minimal handling of the coprocessor traps

(The alignment stuff will be a separate patch once dougc's changes land.)

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8499558 [details] [diff] [review]
Implement minimal handling of the coprocessor traps

Cancelling review request because I missed a case.

Comment 3

[Lars T Hansen [:lth]]

Attachment: Implement minimal handling of the coprocessor traps, v2

Like the previous one but also handles cond=15, which takes a different decoding path.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8500315 [details] [diff] [review]
Implement minimal handling of the coprocessor traps, v2

Redirecting r?, marty appears to be absent.

Comment 5

[Douglas Crosher [:dougc]]

Comment on attachment 8500315 [details] [diff] [review]
Implement minimal handling of the coprocessor traps, v2

Review of attachment 8500315 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Simulator-arm.cpp
@@ +3240,5 @@
>  Simulator::decodeType7(SimInstruction *instr)
>  {
>      if (instr->bit(24) == 1)
>          softwareInterrupt(instr);
> +    else if (instr->bit(4) == 1 && instr->bits(11,9) != 5)

Could we take the decodeType7CoprocessorIns() path even if bits(11,9) == 5 and move this test into decodeType7CoprocessorIns()? Did you spot any VFP instructions that have bit(4)==1 and bits(11,9)==5?

@@ +3257,5 @@
> +            int opc2 = instr->bits(7,5);
> +            int CRn = instr->bits(19,16);
> +            int CRm = instr->bits(3,0);
> +            if (opc1 == 0 && opc2 == 4 && CRn == 7 && CRm == 10) {
> +                // ARMv6 DSB instruction - do nothing now, see comments above

Do you have a reference in which these ARMv6 MCR operations are described? Do DSB etc have to be unconditional with these operations like the ARMv7 encodings?

Comment 6

[Lars T Hansen [:lth]]

(In reply to Douglas Crosher [:dougc] from comment #5)
> Comment on attachment 8500315 [details] [diff] [review]
> Implement minimal handling of the coprocessor traps, v2
> 
> Review of attachment 8500315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm/Simulator-arm.cpp
> @@ +3240,5 @@
> >  Simulator::decodeType7(SimInstruction *instr)
> >  {
> >      if (instr->bit(24) == 1)
> >          softwareInterrupt(instr);
> > +    else if (instr->bit(4) == 1 && instr->bits(11,9) != 5)
> 
> Could we take the decodeType7CoprocessorIns() path even if bits(11,9) == 5
> and move this test into decodeType7CoprocessorIns()?

We could, though I'm not quite sure why we'd want to (yet).  The extra decoding here exactly matches the instructions recognized by decodeType7CoprocessorIns(), and everything else flows through to the VFP decoder as before.

> Did you spot any VFP instructions that have bit(4)==1 and bits(11,9)==5?

I did not look, since I saw it as my job to factor out what I wanted and leave everything else the same as before...

> @@ +3257,5 @@
> > +            int opc2 = instr->bits(7,5);
> > +            int CRn = instr->bits(19,16);
> > +            int CRm = instr->bits(3,0);
> > +            if (opc1 == 0 && opc2 == 4 && CRn == 7 && CRm == 10) {
> > +                // ARMv6 DSB instruction - do nothing now, see comments above
> 
> Do you have a reference in which these ARMv6 MCR operations are described?

See Assembler::as_{dsb,dmb,isb}_trap in Assembler-arm.cpp.  I did not add the links in the simulator source since, generally, the simulator does not explain its workings.

> Do DSB etc have to be unconditional with these operations like the ARMv7
> encodings?

I don't think I understand that question.  The patch contains two paths into decodeType7CoprocessorIns(), one from decodeType7() and one from decodeSpecialCondition(), because of the way the decoding tree is implemented.  On the latter path the condition is respected; on the former path only 0xF is handled and the instruction is unconditional.

Comment 7

[Douglas Crosher [:dougc]]

(In reply to Lars T Hansen [:lth] from comment #6)
> (In reply to Douglas Crosher [:dougc] from comment #5)
> > Comment on attachment 8500315 [details] [diff] [review]
> > Implement minimal handling of the coprocessor traps, v2
> > 
> > Review of attachment 8500315 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/arm/Simulator-arm.cpp
> > @@ +3240,5 @@
> > >  Simulator::decodeType7(SimInstruction *instr)
> > >  {
> > >      if (instr->bit(24) == 1)
> > >          softwareInterrupt(instr);
> > > +    else if (instr->bit(4) == 1 && instr->bits(11,9) != 5)
> > 
> > Could we take the decodeType7CoprocessorIns() path even if bits(11,9) == 5
> > and move this test into decodeType7CoprocessorIns()?
> 
> We could, though I'm not quite sure why we'd want to (yet).  The extra
> decoding here exactly matches the instructions recognized by
> decodeType7CoprocessorIns(), and everything else flows through to the VFP
> decoder as before.

For example, the ARM DDI0406C manual documents the MCR and MRC instructions as including encodings with bits(11,9)==5, but notes that these encodings are undefined.
 
> > Did you spot any VFP instructions that have bit(4)==1 and bits(11,9)==5?
> 
> I did not look, since I saw it as my job to factor out what I wanted and
> leave everything else the same as before...

Sounds ok. But there might also be some merit in organising the simulator decoding to follow the organisation in the ARM manual.
 
> > @@ +3257,5 @@
> > > +            int opc2 = instr->bits(7,5);
> > > +            int CRn = instr->bits(19,16);
> > > +            int CRm = instr->bits(3,0);
> > > +            if (opc1 == 0 && opc2 == 4 && CRn == 7 && CRm == 10) {
> > > +                // ARMv6 DSB instruction - do nothing now, see comments above
> > 
> > Do you have a reference in which these ARMv6 MCR operations are described?
> 
> See Assembler::as_{dsb,dmb,isb}_trap in Assembler-arm.cpp.  I did not add
> the links in the simulator source since, generally, the simulator does not
> explain its workings.

Thank you for the pointer. Found them now in the ARM manual I have on hand, DDI0406C Section B4.2.5 Data and instruction barrier operations, VMSA. Looks good.
 
> > Do DSB etc have to be unconditional with these operations like the ARMv7
> > encodings?
> 
> I don't think I understand that question.  The patch contains two paths into
> decodeType7CoprocessorIns(), one from decodeType7() and one from
> decodeSpecialCondition(), because of the way the decoding tree is
> implemented.  On the latter path the condition is respected; on the former
> path only 0xF is handled and the instruction is unconditional.

Looks good. It seems that a conditional barrier could be encoded using MCR.

Comment 8

[Douglas Crosher [:dougc]]

Comment on attachment 8500315 [details] [diff] [review]
Implement minimal handling of the coprocessor traps, v2

Review of attachment 8500315 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work.

Comment 9

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c07de0783e27

Comment 10

[Lars T Hansen [:lth]]

Work is complete: Alignment checking is already part of LDREX/STREX since they delegate to readW/writeW internally.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c07de0783e27