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, address@hidden 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