On 01/21/2015 01:59 AM, Maciej W. Rozycki wrote:
> On Fri, 16 Jan 2015, Markos Chandras wrote:
>
>> MIPS R6 removed the BLTZL instruction so do not try to emulate it
>> if the R2-to-R6 emulator is not present.
>>
>> Signed-off-by: Markos Chandras <markos.chandras@xxxxxxxxxx>
>> ---
>
> I appreciate your effort in splitting 45-52/70 into separate patches, but
> frankly I think removing individual branch instructions one by one seems
> an overkill to me, and actually makes a review more difficult. They
> comprise a single functional entity and can really be treated as one
> feature and splitting it into pieces makes it easy to miss the big
> picture.
>
Ok will do
>> diff --git a/arch/mips/kernel/branch.c b/arch/mips/kernel/branch.c
>> index 9b622ca391d8..502bf2aeb834 100644
>> --- a/arch/mips/kernel/branch.c
>> +++ b/arch/mips/kernel/branch.c
>> @@ -436,6 +436,11 @@ int __compute_return_epc_for_insn(struct pt_regs *regs,
>> switch (insn.i_format.rt) {
>> case bltz_op:
>> case bltzl_op:
>> + if (NO_R6EMU && (insn.i_format.rt == bltzl_op)) {
>> + ret = -SIGILL;
>> + /* not emulating the branch likely for R6 */
>> + break;
>> + }
>> if ((long)regs->regs[insn.i_format.rs] < 0) {
>> epc = epc + 4 + (insn.i_format.simmediate << 2);
>> if (insn.i_format.rt == bltzl_op)
>
> For example here it is obvious that there is no need to repeat the
> condition and:
>
> switch (insn.i_format.rt) {
> case bltzl_op:
> if (NO_R6EMU) {
> ret = -SIGILL;
> /* not emulating the branch likely for R6 */
> break;
> }
> /* Fall through */
> case bltz_op:
>
> will do.
>
yes but i was trying to avoid touching more things than necessary. I
don't think the current code is unreadable, but I will change it as
suggested.
> However the way how the return value is set and the error return path
> handled here is inconsistent with how BPOSGE32 does these things here.
> From how `__compute_return_epc' and `evaluate_branch_instruction' handle
> error returns it looks to me it's BPOSGE32 that is right. So you do need
> to return -EFAULT instead and send a signal, probably SIGILL.
>
> So it looks to me you want to use `goto' instead, to a similar exit path
> (a poor man's plain C exception handler) like BPOSGE32 uses, with a
> similar message produced to the kernel log and SIGILL thrown, maybe
> calling them `sigill_dsp' and `sigill_r6' respectively. Why BPOSGE32 uses
> SIGBUS instead escapes me, perhaps someone just copied and pasted the
> `unaligned' case from `__compute_return_epc' without thinking much. I
> think it should be fixed too, or otherwise an explanatory comment added.
>
Ok I will do that.
--
markos
