Re: [Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Tom Musta]

On 4/8/2014 2:58 PM, Alexander Graf wrote:
> On 04/08/2014 09:56 PM, Tom Musta wrote:
>> On 4/6/2014 3:55 PM, Alexander Graf wrote:
>> <snip>
>>
>>> @@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, 
>>> uint32_t freq)
>>>       tb_env = g_malloc0(sizeof(ppc_tb_t));
>>>       env->tb_env = tb_env;
>>>       tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
>>> +    if (env->insns_flags & PPC_SEGMENT_64B) {
>>> +        /* All Book3S 64bit CPUs implement level based DEC logic */
>>> +        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
>>> +    }
>>>       /* Create new timer */
>>>       tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
>>> &cpu_ppc_decr_cb, cpu);
>>>       if (0) {
>> Equating Book3S with PPC_SEGMENT_64B is clever ... is it too clever?  
>> Especially since
>> the SLB Bridge is in the phased-out category and consequently we should 
>> expect future
>> Book3S implementations to not support this instruction category.
> 
> Maybe it's too clever :). I'm very open to suggestions on how to figure this 
> out otherwise. Or maybe we should just rework the way timers get created and 
> make them be part of the core itself?
> 
> 
> Alex
> 

A somewhat more practical approach than redesigning timer init:  The phrasing 
introduced into Book3S
that corresponds to your UNDERFLOW_LEVEL flag has existed at least since ISA 
2.03.  And 2.03 introduced
some new features, like SPE and Altivec.  So ...

    if (env->insns_flags & (PPC_SEGMENT_64B | PPC_SPE | PPC_ALTIVEC)) {
        /* All Book3S 64bit CPUs implement level based DEC logic */
        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
    }

would catch a few more.  I'm not sure we get into this code for Book3E 
machines, but if you are worried
about that you could also ensure that insns_flags doesn't have PPC_BOOKE on.