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

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

[gang.chen.5i5j]

From: Chen Gang <gang.chen.5i5j@gmail.com>

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". 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

Until now, EX_CONTEXT_0_0 and EX_CONTEXT_0_1 are only used in mtspr, so
just skip them, at present. "jrp lr" in __longjmp is for historical
reasons, and might get removed in the future.

After this patch, busybox sh can run correctly.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 target-tilegx/translate.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
index 421766b..b7bb4f3 100644
--- a/target-tilegx/translate.c
+++ b/target-tilegx/translate.c
@@ -563,8 +563,14 @@ static TileExcp gen_rr_opcode(DisasContext *dc, unsigned opext,
         break;
     case OE_RR_X0(FSINGLE_PACK1):
     case OE_RR_Y0(FSINGLE_PACK1):
-    case OE_RR_X1(IRET):
         return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
+    case OE_RR_X1(IRET):
+        if (srca) {
+            return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
+        }
+        srca = TILEGX_R_LR;
+        mnemonic = "iret";
+        goto do_jr;
     case OE_RR_X1(LD1S):
         memop = MO_SB;
         mnemonic = "ld1s"; /* prefetch_l1_fault */
@@ -1823,6 +1829,8 @@ static const TileSPR *find_spr(unsigned spr)
       offsetof(CPUTLGState, spregs[TILEGX_SPR_CRITICAL_SEC]), 0, 0)
     D(SIM_CONTROL,
       offsetof(CPUTLGState, spregs[TILEGX_SPR_SIM_CONTROL]), 0, 0)
+    D(EX_CONTEXT_0_0, -1, 0, 0) /* Skip it */
+    D(EX_CONTEXT_0_1, -1, 0, 0) /* Skip it */
     }
 
 #undef D
@@ -1836,9 +1844,11 @@ static TileExcp gen_mtspr_x1(DisasContext *dc, unsigned spr, unsigned srca)
     const TileSPR *def = find_spr(spr);
     TCGv tsrca;
 
-    if (def == NULL) {
+    if (!def) {
         qemu_log_mask(CPU_LOG_TB_IN_ASM, "mtspr spr[%u], %s", spr, reg_names[srca]);
         return TILEGX_EXCP_OPCODE_UNKNOWN;
+    } else if (def->offset == -1) {
+        goto tail;
     }
 
     tsrca = load_gr(dc, srca);
@@ -1847,6 +1857,8 @@ static TileExcp gen_mtspr_x1(DisasContext *dc, unsigned spr, unsigned srca)
     } else {
         tcg_gen_st_tl(tsrca, cpu_env, def->offset);
     }
+
+tail:
     qemu_log_mask(CPU_LOG_TB_IN_ASM, "mtspr %s, %s", def->name, reg_names[srca]);
     return TILEGX_EXCP_NONE;
 }
@@ -1856,7 +1868,7 @@ static TileExcp gen_mfspr_x1(DisasContext *dc, unsigned dest, unsigned spr)
     const TileSPR *def = find_spr(spr);
     TCGv tdest;
 
-    if (def == NULL) {
+    if (!def || def->offset == -1) {
         qemu_log_mask(CPU_LOG_TB_IN_ASM, "mtspr %s, spr[%u]", reg_names[dest], spr);
         return TILEGX_EXCP_OPCODE_UNKNOWN;
     }
-- 
1.9.3

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

[Richard Henderson]

On 10/01/2015 10:37 PM, gang.chen.5i5j@gmail.com wrote:
>           {
>            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
>
> Until now, EX_CONTEXT_0_0 and EX_CONTEXT_0_1 are only used in mtspr, so
> just skip them, at present. "jrp lr" in __longjmp is for historical
> reasons, and might get removed in the future.

So, really, iret is supposed to branch to EX_CONTEXT_0_0, and (presumably) 
validate the privilege level in EX_CONTEXT_0_1 continues to be user-mode.

> +    case OE_RR_X1(IRET):
> +        if (srca) {
> +            return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
> +        }
> +        srca = TILEGX_R_LR;
> +        mnemonic = "iret";
> +        goto do_jr;

which means this is wrong, but just happens to work for __longjmp.

It appears that the entire point of this iret path is to atomically branch and 
set INTERRUPT_CRITICAL_SECTION at the same time.  So, this isn't complete.

What INTERRUPT_CRITICAL_SECTION is supposed to *do* at user mode, I don't know.


r~

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

[Chen Gang]

On 10/2/15 08:36, Richard Henderson wrote:
> On 10/01/2015 10:37 PM, gang.chen.5i5j@gmail.com wrote:
>> {
>> 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
>>
>> Until now, EX_CONTEXT_0_0 and EX_CONTEXT_0_1 are only used in mtspr, so
>> just skip them, at present. "jrp lr" in __longjmp is for historical
>> reasons, and might get removed in the future.
>
> So, really, iret is supposed to branch to EX_CONTEXT_0_0, and (presumably) validate the privilege level in EX_CONTEXT_0_1 continues to be user-mode.
>

Oh, really.

>> + case OE_RR_X1(IRET):
>> + if (srca) {
>> + return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>> + }
>> + srca = TILEGX_R_LR;
>> + mnemonic = "iret";
>> + goto do_jr;
>
> which means this is wrong, but just happens to work for __longjmp.
>
> It appears that the entire point of this iret path is to atomically branch and set INTERRUPT_CRITICAL_SECTION at the same time. So, this isn't complete.
>

OK, thanks.


> What INTERRUPT_CRITICAL_SECTION is supposed to *do* at user mode, I don't know.
>

Welcome any other members' ideas, suggestions and completions.


Thanks.
--
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  

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

[Chris Metcalf]

On 10/1/2015 8:36 PM, Richard Henderson wrote:
> On 10/01/2015 10:37 PM, gang.chen.5i5j@gmail.com wrote:
>>           {
>>            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
>>
>> Until now, EX_CONTEXT_0_0 and EX_CONTEXT_0_1 are only used in mtspr, so
>> just skip them, at present. "jrp lr" in __longjmp is for historical
>> reasons, and might get removed in the future.
>
> So, really, iret is supposed to branch to EX_CONTEXT_0_0, and (presumably) validate the privilege level in EX_CONTEXT_0_1 continues to be user-mode.

Yes, I gave the same feedback earlier today.  EX_CONTEXT_0_1 should be either 0 or 1 to set INTERRUPT_CRITICAL_SECTION appropriately, and raise GPV for any other value.  (Obviously it's more complex if you're really emulating system software, but for now that's out of scope, I think.)

>
>> +    case OE_RR_X1(IRET):
>> +        if (srca) {
>> +            return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>> +        }
>> +        srca = TILEGX_R_LR;
>> +        mnemonic = "iret";
>> +        goto do_jr;
>
> which means this is wrong, but just happens to work for __longjmp.
>
> It appears that the entire point of this iret path is to atomically branch and set INTERRUPT_CRITICAL_SECTION at the same time.  So, this isn't complete.
>
> What INTERRUPT_CRITICAL_SECTION is supposed to *do* at user mode, I don't know.

It disables interrupts from being delivered.  This means asynchronous interrupts get deferred until ICS is set back to zero, and synchronous interrupts (page fault, etc) cause a double-fault instead.  ICS is automatically set on entry to interrupt handlers, so the handler has time to acquire any information about the interrupt from SPRs, and it is expected that ICS is cleared as soon as possible.  ICS can also be used before returning from interrupts if you need to do something like adjust the interrupt mask prior to returning.

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

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

[Chen Gang]

OK, thanks. I shall try to send patch v4 for it within 2 days.

On 10/2/15 09:31, Chris Metcalf wrote:
> On 10/1/2015 8:36 PM, Richard Henderson wrote:
>> On 10/01/2015 10:37 PM, gang.chen.5i5j@gmail.com wrote:
>>>           {
>>>            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
>>>
>>> Until now, EX_CONTEXT_0_0 and EX_CONTEXT_0_1 are only used in mtspr, so
>>> just skip them, at present. "jrp lr" in __longjmp is for historical
>>> reasons, and might get removed in the future.
>>
>> So, really, iret is supposed to branch to EX_CONTEXT_0_0, and (presumably) validate the privilege level in EX_CONTEXT_0_1 continues to be user-mode.
> 
> Yes, I gave the same feedback earlier today.  EX_CONTEXT_0_1 should be either 0 or 1 to set INTERRUPT_CRITICAL_SECTION appropriately, and raise GPV for any other value.  (Obviously it's more complex if you're really emulating system software, but for now that's out of scope, I think.)
> 
>>
>>> +    case OE_RR_X1(IRET):
>>> +        if (srca) {
>>> +            return TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>>> +        }
>>> +        srca = TILEGX_R_LR;
>>> +        mnemonic = "iret";
>>> +        goto do_jr;
>>
>> which means this is wrong, but just happens to work for __longjmp.
>>
>> It appears that the entire point of this iret path is to atomically branch and set INTERRUPT_CRITICAL_SECTION at the same time.  So, this isn't complete.
>>
>> What INTERRUPT_CRITICAL_SECTION is supposed to *do* at user mode, I don't know.
> 
> It disables interrupts from being delivered.  This means asynchronous interrupts get deferred until ICS is set back to zero, and synchronous interrupts (page fault, etc) cause a double-fault instead.  ICS is automatically set on entry to interrupt handlers, so the handler has time to acquire any information about the interrupt from SPRs, and it is expected that ICS is cleared as soon as possible.  ICS can also be used before returning from interrupts if you need to do something like adjust the interrupt mask prior to returning.
> 

-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

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

[Richard Henderson]

On 10/02/2015 11:31 AM, Chris Metcalf wrote:
>
> It disables interrupts from being delivered.  This means asynchronous
> interrupts get deferred until ICS is set back to zero, and synchronous
> interrupts (page fault, etc) cause a double-fault instead. ICS is automatically
> set on entry to interrupt handlers, so the handler has time to acquire any
> information about the interrupt from SPRs, and it is expected that ICS is
> cleared as soon as possible.  ICS can also be used before returning from
> interrupts if you need to do something like adjust the interrupt mask prior to
> returning.

Which is all very well and good for supervisor mode... but what's it good for 
in user mode?

I was about to quote you from 2012 (https://lkml.org/lkml/2012/3/30/994):

> In general we want to avoid ever touching memory while within an
> interrupt critical section, since the page fault path goes through
> a different path from the hypervisor when in an interrupt critical
> section, and we carefully decided with tilegx that we didn't need
> to support this path in the kernel.

Which implies that tilegx userland does nothing at all with ICS, and is in fact 
unsupported?


r~

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

[Chris Metcalf]

On 10/1/2015 10:26 PM, Richard Henderson wrote:
> On 10/02/2015 11:31 AM, Chris Metcalf wrote:
>>
>> It disables interrupts from being delivered.  This means asynchronous
>> interrupts get deferred until ICS is set back to zero, and synchronous
>> interrupts (page fault, etc) cause a double-fault instead. ICS is automatically
>> set on entry to interrupt handlers, so the handler has time to acquire any
>> information about the interrupt from SPRs, and it is expected that ICS is
>> cleared as soon as possible.  ICS can also be used before returning from
>> interrupts if you need to do something like adjust the interrupt mask prior to
>> returning.
>
> Which is all very well and good for supervisor mode... but what's it good for in user mode?

In user-space it is primarily useful for userspace drivers that are receiving device interrupts.  You can also configure to receive performance counter interrupts in userspace.  The bottom line is that is indeed a rarely-used feature.

> I was about to quote you from 2012 (https://lkml.org/lkml/2012/3/30/994):
>
>> In general we want to avoid ever touching memory while within an
>> interrupt critical section, since the page fault path goes through
>> a different path from the hypervisor when in an interrupt critical
>> section, and we carefully decided with tilegx that we didn't need
>> to support this path in the kernel.
>
> Which implies that tilegx userland does nothing at all with ICS, and is in fact unsupported?

This refers to the kernel's own use of the interrupt critical section, i.e. having that SPR set while running at PL2.  Each protection level (PL) has its own ICS.  So the text you quoted is orthogonal to userspace use of ICS.

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