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

[Sandipan Das <sandipan@linux.ibm.com>]

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(&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?
> 

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
>