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

[Tom Musta <tommusta@gmail.com>]

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.