Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity

[Guo Ren]

On Tue, Jan 31, 2023 at 3:03 PM Björn Töpel <bjorn@xxxxxxxxxx> wrote:
>
> Guo Ren <guoren@xxxxxxxxxx> writes:
>
> >> > >> >  static void __kprobes arch_prepare_simulate(struct kprobe *p)
> >> > >> > @@ -114,16 +120,23 @@ void *alloc_insn_page(void)
> >> > >> >  /* install breakpoint in text */
> >> > >> >  void __kprobes arch_arm_kprobe(struct kprobe *p)
> >> > >> >  {
> >> > >> > -     if ((p->opcode & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
> >> > >> > -             patch_text(p->addr, __BUG_INSN_32);
> >> > >> > -     else
> >> > >> > -             patch_text(p->addr, __BUG_INSN_16);
> >> > >> > +#ifdef CONFIG_RISCV_ISA_C
> >> > >> > +     u32 opcode = __BUG_INSN_16;
> >> > >> > +#else
> >> > >> > +     u32 opcode = __BUG_INSN_32;
> >> > >> > +#endif
> >> > >> > +     patch_text_nosync(p->addr, &opcode, GET_INSN_LENGTH(opcode));
> >> > >>
> >> > >> Sounds good, but it will leave some RVI instruction truncated in kernel text,
> >> > >> i doubt kernel behavior depends on the rest of the truncated instruction, well,
> >> > >> it needs more strict testing to prove my concern :)
> >> > > I do this on purpose, and it doesn't cause any problems. Don't worry;
> >> > > IFU hw must enforce the fetch sequence, and there is no way to execute
> >> > > broken instructions even in the speculative execution path.
> >> >
> >> > This is stretching reality a bit much. ARMv8, e.g., has a chapter in the
> >> > Arm ARM [2] Appendix B "Concurrent modification and execution of
> >> > instructions" (CMODX). *Some* instructions can be replaced concurrently,
> >> > and others cannot without caution. Assuming that that all RISC-V
> >> > implementations can, is a stretch. RISC-V hasn't even specified the
> >> > behavior of CMODX (which is problematic).
> >> Here we only use one sw/sh instruction to store a 32bit/16bit aligned element:
> >>
> >> INSN_0 <- ebreak (16bit/32bit aligned)
> >> INSN_1
> >> INSN_2
> >>
> >> The ebreak would cause an exception which implies a huge fence here.
> >> No machine could give a speculative execution for the ebreak path.
> >
> > For ARMv7, ebreak is also safe:
> >
> > ---
> > Concurrent modification and execution of instructions
> >
> > 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.
> > ...
> > The instructions to which this guarantee applies are:
> > In the Thumb instruction set
> > The 16-bit encodings of the B, NOP, BKPT, and SVC instructions.
> > ...
> > In the ARM instruction set
> > The B, BL, NOP, BKPT, SVC, HVC, and SMC instructions.
> > ---
>
> Right, and "B7.7 Concurrent modification and execution of instructions"
> Armv8-M ARM (https://developer.arm.com/documentation/ddi0553/latest),
> also defines that certain instructions can be concurrently modified.
>
> This is beside the point. We don't have a spec for RISC-V, yet. We're
> not even sure we can (in general) replace the lower 16b of an 32b
> instruction concurrently. "It's in the Armv8-M spec" is not enough.
>
> I'd love to have a spec defining that, and Derek et al has started
> [1]. Slide #99 has CMODX details.
For the c.ebreak, CMODX is unnecessary. CMODX would give a wider definition.

>
> Your patch might be great for some HW (which?), but not enough for
> general RISC-V Linux (yet). Until then, the existing stop_machine() way
> is unfortunately the way to go.
It's generic for c.ebreak, not some HW. It's natural to support. No
machine would break it.

Please let me clarify our argued premise of "ebreak" injection atomicity:
 - c.ebreak on the head of 32b insn
 - ebreak on an aligned 32b insn

>
>
> Björn
>
> [1] https://github.com/riscv/riscv-j-extension/blob/master/id-consistency-proposal.pdf



-- 
Best Regards
 Guo Ren