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

[Daniel Axtens <dja@axtens.net>]

Hi Sandipan,

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

> +#define MAX_INSNS	32
This doesn't seem to be used...

> +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?

> +
> +	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;
> +}

> +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?

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)

Regards,
Daniel
> -- 
> 2.19.2