Re: [RFC PATCH 3/5] powerpc: sstep: Add instruction emulation selftests

On 11/02/19 6:17 AM, Daniel Axtens wrote:

Hi Sandipan,

I'm not really confident to review the asm, but I did have a couple of
questions about the C:

quoted

+#define MAX_INSNS	32

This doesn't seem to be used...

True. Thanks for pointing this out.

quoted

+int execute_instr(struct pt_regs *regs, unsigned int instr)
+{
+	extern unsigned int exec_instr_execute[];
+	extern int exec_instr(struct pt_regs *regs);

These externs sit inside the function scope. This feels less than ideal
to me - is there a reason not to have these at global scope?

Currently, execute_instr() is the only consumer. So, I thought I'd keep
them local for now.

quoted

+
+	if (!regs || !instr)
+		return -EINVAL;
+
+	/* Patch the NOP with the actual instruction */
+	patch_instruction(&exec_instr_execute[0], instr);
+	if (exec_instr(regs)) {
+		pr_info("execution failed, opcode = 0x%08x\n", instr);
+		return -EFAULT;
+	}
+
+	return 0;
+}

quoted

+late_initcall(run_sstep_tests);

A design question: is there a reason to run these as an initcall rather
than as a module that could either be built in or loaded separately? I'm
not saying you have to do this, but I was wondering if you had
considered it?

I did. As of now, there are some existing tests in test_emulate_step.c
which use the same approach. So, I thought I'd stick with that approach
to start off. This is anyway controlled by a Kconfig option.

Lastly, snowpatch reports some checkpatch issues for this and your
remaining patches: https://patchwork.ozlabs.org/patch/1035683/ (You are
allowed to violate checkpatch rules with justification, FWIW)

Will look into them.

Regards,
Daniel

quoted

-- 
2.19.2