Re: [Qemu-devel] [PATCH 1/5] target-tricore: Add trap handling

[Richard Henderson <rth@twiddle.net>]

On 02/12/2016 10:12 PM, Bastian Koppelmann wrote:
> On 02/12/2016 03:39 AM, Richard Henderson wrote:
>> On 02/12/2016 03:01 AM, Bastian Koppelmann wrote:
>>> +void tricore_cpu_do_interrupt(CPUState *cs)
>>> +{
>>> +    TriCoreCPU *cpu = TRICORE_CPU(cs);
>>> +    CPUTriCoreState *env = &cpu->env;
>>> +
>>> +    /* The stack pointer in A[10] is set to the Interrupt Stack
>>> Pointer (ISP)
>>> +       when the processor was not previously using the interrupt stack
>>> +       (in case of PSW.IS = 0). The stack pointer bit is set for
>>> using the
>>> +       interrupt stack: PSW.IS = 1. */
>>> +    if ((env->PSW & MASK_PSW_IS) == 0) {
>>> +        env->gpr_a[10] = env->ISP;
>>> +    }
>>
>> You appear to have forgotten to save pre-interrupt state here.
>
> What do you mean by "pre-interrupt state"? The register context is saved
> by generate_trap() calls.

Because do_interrupt is normally for handling asynchronous interrupts.  Stuff 
that isn't related at all to the instruction stream.  Which therefore could not 
have saved anything at all.

It does get (ab)used on some targets for synchronous events, because the method 
by which kernel entry is made is the same and the author wanted to share code.

But if you have no external hardware devices, no timer interrupt or anything, 
let's not start by passing synchronous events through this function.  It sounds 
like the code here in do_interrupt more properly belongs at the end of 
generate_trap.

Err.. the generate_trap in op_helper I mean.  It's confusing that you have two 
(static) functions of the same name in different files.

I do wonder if these two functions ought to share more code.  For instance, 
op_helper.c places TIN into a_15, whereas translate.c places TIN into d_15.


r~