RE: Re: Do `NativeInstruction::set_*_at()` and `BarrierSetNMethod::disarm()` miss icache invalidation operations on AArch64 platform?

[Xiaolin Zheng]

Hi Erik and Andrew,

Thank you for the detailed explanations that are very reasonable to me - seems they could solve my confusion. The elaborately designed code really has lots of details hidden inside.
Next time I will post it to the aarch64-port-dev :-).

P.S. I have changed my name shown outside to the English one and hope it doesn't make confusions - it is not important though.

Thanks again for taking the time to look into this!

Best Regards,Xiaolin


------------------------------------------------------------------
On Tue, Feb 22, 2022 at 21:18 Andrew Haley <aph-open at littlepinkcloud.com> wrote:
On 2/22/22 03:28, 郑孝林(云矅) wrote:
> 
> Hi experts on AArch64,
> 
> As the topic, I was wondering if these methods lack icache invalidation operations when patching data saved in a code region, though it seems not harmful.
> 1. https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp#L95-L101
> 2. https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/gc/shared/barrierSetNMethod_aarch64.cpp#L49-L51
> 
> When patching a trampoline address, we will go
> ```
> NativeCall::set_destination_mt_safe()
>    -> NativeCallTrampolineStub::set_destination()
>      -> NativeInstruction::set_ptr_at()
> ```
> 
> bl(1) -> ldr
>              br
>              <int64: the real call target (2)>
> 
> When a call target is getting updated, (1) is flushed but (2) is not on AArch64. So I was wondering if there are more possibilities for other cores to see the new patching behavior with a theoretically longer latency when code is running at full speed on every core?

That's definitely true, but it doesn't matter because they'll observe
either the old call target in the dcache or the new one: both are still
valid. If the old one is observed, the processor will hit a trap and retry.

> Also `BarrierSetNMethod::disarm()` will patch the guard address as well, without flushing the related icache address on AArch64.

There's no need to touch the icache, and again this is by design.

> Maybe something like http://cr.openjdk.java.net/~xlinzheng/1.diff ?
> 
> Previously I wanted to send this to the aarch64-port-dev, but seems that list is not so active - I see only Duke's there syncing changesets.
> I feel sorry in advance if this isn't worth a discussion, and am very glad to receive any input.

aarch64-port-dev is the right place, but posts by non-members are
rejected. I guess we should start diverting the changeset messages
elsewhere.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671