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:
>
>> +#define MAX_INSNS 32
> This doesn't seem to be used...
>
True. Thanks for pointing this out.
>> +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.
>> +
>> +if (!regs || !instr)
>> +return -EINVAL;
>> +
>> +/* Patch the NOP with the actual instruction */
>> +patch_instruction(_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?
>
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
>> --
>> 2.19.2
>
