Re: [PATCH 04/15] arc: TCG and decoder glue code and helpers

[Cupertino Miranda]

>> +void QEMU_NORETURN helper_halt(CPUARCState *env, uint32_t npc)
>> +{
>> +    CPUState *cs = env_cpu(env);
>> +    if (env->stat.Uf) {
>> +        cs->exception_index = EXCP_PRIVILEGEV;
>> +        env->causecode = 0;
>> +        env->param = 0;
>> +         /* Restore PC such that we point at the faulty instruction.  */
>> +        env->eret = env->pc;
> 
> Any reason not to handle Uf at translate time?  Or at least create a single
> helper function for that here.  But it seems like translate will have to do a
> lot of priv checking anyway and will already have that handy.

Since we needed a helper anyway to deal with causecode and param, we 
thought it would be reasonable to do all in the helper.
We did not made a TCG access for causecode and param enviroment values.

> 
>> +void helper_enter(CPUARCState *env, uint32_t u6)
>> +{
>> +    /* nothing to do? then bye-bye! */
>> +    if (!u6) {
>> +        return;
>> +    }
>> +
>> +    uint8_t regs       = u6 & 0x0f; /* u[3:0] determines registers to save 
>> */
>> +    bool    save_fp    = u6 & 0x10; /* u[4] indicates if fp must be saved  
>> */
>> +    bool    save_blink = u6 & 0x20; /* u[5] indicates saving of blink      
>> */
>> +    uint8_t stack_size = 4 * (regs + save_fp + save_blink);
>> +
>> +    /* number of regs to be saved must be sane */
>> +    check_enter_leave_nr_regs(env, regs, GETPC());
> 
> Both of these checks could be translate time.
> 
>> +    /* this cannot be executed in a delay/execution slot */
>> +    check_delay_or_execution_slot(env, GETPC());
> 
> As could this.
> 
>> +    /* stack must be a multiple of 4 (32 bit aligned) */
>> +    check_addr_is_word_aligned(env, CPU_SP(env) - stack_size, GETPC());
>> +
>> +    uint32_t tmp_sp = CPU_SP(env);
>> +
>> +    if (save_fp) {
>> +        tmp_sp -= 4;
>> +        cpu_stl_data(env, tmp_sp, CPU_FP(env));
>> +    }
> 
> And what if these stores raise an exception?  I doubt you're going to get an
> exception at the correct pc.
> 
>> +void helper_leave(CPUARCState *env, uint32_t u7)
> 
> Similarly.  I think that both of these could be implemented entirely in
> translate, which is what
> 
>> +    bool restore_fp    = u7 & 0x10; /* u[4] indicates if fp must be saved  
>> */
>> +    bool restore_blink = u7 & 0x20; /* u[5] indicates saving of blink      
>> */
>> +    bool jump_to_blink = u7 & 0x40; /* u[6] should we jump to blink?       
>> */
> 
> these bits strongly imply.
> 

For lack of knowing better, it is unclear to me where to draw the line 
when choosing between a translate time (tcg) or helper implementation.
Your suggestions for carry/overflow computation are sharp and we should 
have never used an helper, however I wonder what would be the benefit of 
implementing enter and leave through TCG.

We have dealt with those exception issues by just changing SP in the end 
of the instruction implementation, when no exceptions can happen.

As far as I understand when an exception happens in the middle of the 
helper or even on a TCG implementation, it jumps out of that TB 
execution to deal with the exception. On rtie instead of it returning to 
the same tcg_ld or tcg_st where it actually triggered the exception it 
will re-decode the same instruction which triggered the exception, and 
re-attempts to execute it.
Is that the case in current TCG implementation, or did it improved and 
it is now able to return to previous execution flow (i.e translation 
block) ?