RFR: 8164652: c1 port, code patching

[Anton Kozlov]

Hi, Andrew, sorry for delay.

> With all of the icache flushing done in the correct order I think this
> is OK.
>
> Having said that, I want to sound a note of caution.
>
> There is a compatibility problem in that the architecture
> specification says
>
>    ...once the modified instructions are observable, each PE that is
>    executing the modified instructions *must* (my emphasis) issue the
>    following instruction to ensure execution of the modified
>    instructions:
>
>          ISB;         Synchronize fetched instruction stream
>

ARM ARM for v7 states: 

The ARMv7 architecture limits the set of instructions that can be executed by one thread of execution as they are
being modified by another thread of execution without requiring explicit synchronization.
Except for the instructions identified in this section, the effect of the concurrent modification and execution of an
instruction is UNPREDICTABLE .
For the following instructions only, the architecture guarantees that, after modification of the instruction, behavior
is consistent with execution of either:
• The instruction originally fetched.
• A fetch of the new instruction. That is, a fetch of the instruction that results from the modification.
The instructions to which this guarantee applies are:
In the ARM instruction set
The B , BL , NOP , BKPT , SVC , HVC , and SMC instructions.
For all other instructions, to avoid UNPREDICTABLE behavior, instruction modifications must be explicitly
synchronized before they are executed. The required synchronization is as follows:
1 ...
2 ..once the modified instructions are observable, ...

We are patching branches only, that aren't require explicit synchronization. You cited step 2 from explicit synchronization sequence.

> But we are not doing that on all PEs.  For strict correctness we
> should execute an ISB on every PE after a stub has been written and
> before it is executed.  In practice I don't think that it matters
> because the instruction cache on the other PEs probably won't already
> contain a stale copy of the stub.  But it is possible: with a long
> instruction cache it may do if the line has already been fetched for
> an adjacent stub.  Without an ISB the PE may not re-fetch its icache
> line and may then execute the stale stub code.

I'm a bit confused with this. Could you clarify your thought about icache problem?
ISB is pipeline flush. Let's suppose ISB is required after branch patch. 
>From my point of view, then, number of executed instructions from point of patching to point of execution of patched instructions matter.
ISB could be required by ARM, but in practice bad things will happen if second point is reached in less
than (pipeline-length) instructions. And we are patching deeply in runtime, there are a lot of branching, mutex release,..
But yes, in case ISB required it should be added.

Thanks,
Anton