Instruction concurrent modification issue of direct jump in AArch64

Instruction concurrent modification issue of direct jump in AArch64

[hev]

Hello,

I talked with Hu Qi about the risk of instruction concurrent
modification in TCG direct jump for LoongArch, and the conclusion is
that the implementation is correct.

Similarly, the AArch64 implementation doesn't seem to be quite
correct. IIUC, multiple instructions paired with an atomic write does
not guarantee atomic effects on the execution side.

For example, the issue in AArch64 is:

Instruction concurrent modification:

* Before:
  adrp
  addi
  br

* After
  b
  nop
  br

* May actually execution:
  adrp
  nop
  br

That will cause the jump to an unexpected address to execute, What do you think?

Regards,
Ray

Re: Instruction concurrent modification issue of direct jump in AArch64

[Richard Henderson]

On 11/19/22 00:19, hev wrote:
> Hello,
> 
> I talked with Hu Qi about the risk of instruction concurrent
> modification in TCG direct jump for LoongArch, and the conclusion is
> that the implementation is correct.
> 
> Similarly, the AArch64 implementation doesn't seem to be quite
> correct. IIUC, multiple instructions paired with an atomic write does
> not guarantee atomic effects on the execution side.
> 
> For example, the issue in AArch64 is:
> 
> Instruction concurrent modification:
> 
> * Before:
>    adrp
>    addi
>    br
> 
> * After
>    b
>    nop
>    br
> 
> * May actually execution:
>    adrp
>    nop
>    br
> 
> That will cause the jump to an unexpected address to execute, What do you think?

Yes, I agree this is a possible execution that I hadn't considered.  I *think* that it 
requires that the thread be interrupted after the adrp, to resume with the refreshed 
cacheline.  But an interrupt is certainly a valid sequence of events.

Perhaps a better construction would be

Before:
	ldr	x30, [pc, -XXX]
	br	x30

After:
	br	YYY
	br	x30

so that we only update 1 insn, and it goes between either a direct branch, or a 
pc-relative load of the branch address from the TranslationBlock structure (which sits 
right before the code, and we have a 1MB range on LDR (literal)).

Although at the moment the backend hook doesn't have enough information to recreate the 
LDR offset, so the quick fix would have to go between BR and NOP, and leave the LDR to follow.


r~