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
