On Mon, Dec 30, 2024 at 04:27:21PM +0800, Xu Kuohai wrote:
> > > As explained above, RS and RT2 fields should be fixed to 1s.
> >
> > I'm already setting Rs and Rt2 to all 1's here, as AARCH64_INSN_REG_ZR
> > is defined as 31 (0b11111):
> >
> > AARCH64_INSN_REG_ZR = 31,
>
> I see, but the setting of fixed bits is smomewhat of a waste of jit time.
Fair point, I'll instead make load_acq/store_rel's MASK/VALUE include
those (1) bits.
> > On a related note, I simply grabbed {load,store}_ex's MASK and VALUE,
> > then set their 15th and 23rd bits to make them load-acquire and
> > store-release:
> >
> > +__AARCH64_INSN_FUNCS(load_acq, 0x3FC08000, 0x08C08000)
> > +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
> > __AARCH64_INSN_FUNCS(load_ex, 0x3F400000, 0x08400000)
> > __AARCH64_INSN_FUNCS(store_ex, 0x3F400000, 0x08000000)
> >
> > My question is, should we extend {load,store}_ex's MASK to make them
> > contain BIT(15) and BIT(23) as well? As-is, aarch64_insn_is_load_ex()
> > would return true for a load-acquire.
> >
> > The only user of aarch64_insn_is_load_ex() seems to be this
> > arm64-specific kprobe code in arch/arm64/kernel/probes/decode-insn.c:
> >
> > #ifdef CONFIG_KPROBES
> > static bool __kprobes
> > is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
> > {
> > while (scan_start >= scan_end) {
> > /*
> > * atomic region starts from exclusive load and ends with
> > * exclusive store.
> > */
> > if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
> > return false;
> > else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
> > return true;
> >
> > But I'm not sure yet if changing {load,store}_ex's MASK would affect the
> > above code. Do you happen to know the context?
>
> IIUC, this code prevents kprobe from interrupting the LL-SC loop constructed
> by LDXR/STXR pair, as the kprobe trap causes unexpected memory access that
> prevents the exclusive memory access loop from exiting.
>
> Since load-acquire/store-release instructions are not used to construct LL-SC
> loop, I think it is safe to exclude them from {load,store}_ex.
Ah, I see, thanks! I'll extend {load,store}_ex's MASK to prevent
aarch64_insn_is_{load,store}_ex() from returning false-positives for
load-acquire/store-release.
Thanks,
Peilin Ye
