Re: [PATCH V3 3/4] arch/powerpc: Implement Optprobes

Hi Michael,
Thanks for the review! I'll defer to Anju on most of the aspects, but...

On 2017/02/01 09:53PM, Michael Ellerman wrote:

Anju T Sudhakar [off-list ref] writes:

quoted

+static void optimized_callback(struct optimized_kprobe *op,
+			       struct pt_regs *regs)
+{
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	unsigned long flags;
+
+	/* This is possible if op is under delayed unoptimizing */
+	if (kprobe_disabled(&op->kp))
+		return;
+
+	local_irq_save(flags);

What is that protecting against? Because on powerpc it doesn't actually
disable interrupts, it just masks some of them, the perf interrupt for
example can still run.

That's an excellent catch, as always! :)

This is meant to prevent us from missing kprobe hits while processing 
interrupts that arrive when this optprobe is being handled. And you are 
totally right -- we would miss kprobe hits during PMI handling with the 
current approach. We need a hard_irq_disable() there.

quoted

+	/*
+	 * Optprobe template:
+	 * This template gets copied into one of the slots in optinsn_slot
+	 * and gets fixed up with real optprobe structures et al.
+	 */
+	.global optprobe_template_entry
+optprobe_template_entry:
+	/* Create an in-memory pt_regs */
+	stdu	r1,-INT_FRAME_SIZE(r1)
+	SAVE_GPR(0,r1)
+	/* Save the previous SP into stack */
+	addi	r0,r1,INT_FRAME_SIZE
+	std	r0,GPR1(r1)
+	SAVE_10GPRS(2,r1)
+	SAVE_10GPRS(12,r1)
+	SAVE_10GPRS(22,r1)
+	/* Save SPRS */
+	mfmsr	r5
+	std	r5,_MSR(r1)
+	li	r5,0x700
+	std	r5,_TRAP(r1)
+	li	r5,0
+	std	r5,ORIG_GPR3(r1)
+	std	r5,RESULT(r1)
+	mfctr	r5
+	std	r5,_CTR(r1)
+	mflr	r5
+	std	r5,_LINK(r1)
+	mfspr	r5,SPRN_XER
+	std	r5,_XER(r1)
+	mfcr	r5
+	std	r5,_CCR(r1)
+	lbz     r5,PACASOFTIRQEN(r13)
+	std     r5,SOFTE(r1)
+	mfdar	r5
+	std	r5,_DAR(r1)
+	mfdsisr	r5
+	std	r5,_DSISR(r1)

So this is what made me originally reply to this patch. This
save/restore sequence.

I'm not clear on why this is what we need to save/restore.

Aren't we essentially just interposing a function call? If so do we need
to save/restore all of these? eg. MSR/DAR/DSISR. Non-volatile GPRs? And
why are we pretending there was a 0x700 trap?

Is it because we're going to end up emulating the instruction and so we
need everything in pt_regs ?

Yes, that and also for the kprobe pre_handler() which takes pt_regs.


Regards,
- Naveen