Re: [Qemu-devel] [PATCH v5] target-tilegx: Support iret instruction and related special registers

[Chris Metcalf <cmetcalf@ezchip.com>]

Comments just on the commit message:

On 10/06/2015 10:55 AM, Chen Gang wrote:
>  From fa0950e403bbb98989117f632215ae0e698457d7 Mon Sep 17 00:00:00 2001
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> Date: Sun, 4 Oct 2015 17:41:14 +0800
> Subject: [PATCH v5] target-tilegx: Support iret instruction and related special registers
>
> Acording to the __longjmp tilegx libc implementation, and reference from
> tilegx ISA document, and suggested by tilegx architecture member, we can
> treat iret instruction as "jrp lr".

You need to update this comment to reflect the actual code in the
commit.  You aren't treating iret as jrp lr anymore.

>   The related code is below:
>
>    ENTRY (__longjmp)
>           FEEDBACK_ENTER(__longjmp)
>
>    #define RESTORE(r) { LD r, r0 ; ADDI_PTR r0, r0, REGSIZE }
>           FOR_EACH_CALLEE_SAVED_REG(RESTORE)
>
>           {
>            LD r2, r0       ; retrieve ICS bit from jmp_buf
>            movei r3, 1
>            CMPEQI r0, r1, 0
>           }
>
>           {
>            mtspr INTERRUPT_CRITICAL_SECTION, r3
>            shli r2, r2, SPR_EX_CONTEXT_0_1__ICS_SHIFT
>           }
>
>           {
>            mtspr EX_CONTEXT_0_0, lr
>            ori r2, r2, RETURN_PL
>           }
>
>           {
>            or r0, r1, r0
>            mtspr EX_CONTEXT_0_1, r2
>           }
>
>           iret
>
>           jrp lr

I don't think this code snippet is helpful to the commit message.

> EX_CONTEXT_0_0 is used for jumping address, and EX_CONTEXT_0_1 is for
> INTERRUPT_CRITICAL_SECTION, which should only be 0 or 1 in user mode, or
> it will cause target SEGV (and the patch doesn't implement system mode).

You correctly modified the change to raise SIGILL, so you should
also update the commit message the same way.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com