On 09.09.2011, at 15:27, Fabien Chouteau wrote:
> On 09/09/2011 12:55, Alexander Graf wrote:
>>
>> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>>>>>
>>>>>>
>>>>>>> + }
>>>>>>> +
>>>>>>> + qemu_mod_timer(timer, *next);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void booke_decr_cb(void *opaque)
>>>>>>> +{
>>>>>>> + CPUState *env;
>>>>>>> + ppc_tb_t *tb_env;
>>>>>>> + booke_timer_t *booke_timer;
>>>>>>> +
>>>>>>> + env = opaque;
>>>>>>> + tb_env = env->tb_env;
>>>>>>> + booke_timer = tb_env->opaque;
>>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>>> + }
>>>>>>
>>>>>> You will need this more often - also for the TCR write case - so please
>>>>>> put
>>>>>> the 3 lines above into a separate function and just call that here :)
>>>>>
>>>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>>>
>>>> Sure, we have 3 different tests for the 3 different bits we can
>>>> potentially set in TCR. The check always ends up being the same though:
>>>>
>>>> if (TSR & bit && TCR & bit)
>>>> set_irq(irq_for_bit);
>>>>
>>>> Most device emulators have a simple function for this called "update_irq"
>>>> that checks for all the bits and sets the respective irq lines.
>>>>
>>>
>>> I know but we have two cases:
>>>
>>> - Timer hit: we check DIE in TCR
>>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>>
>>> I don't think we can have a good generic function for this, and I don't
>>> forecast any improvement in code readability.
>>
>> update_decr_irq() {
>> if (TSR.DIS && TCR.DIE) {
>> set_irq(DECR);
>> } else {
>> unset_irq(DECR);
>> }
>> }
>>
>> Timer hit:
>>
>> TSR |= DIS;
>> update_decr_irq();
>>
>> Setting TCR:
>>
>> TCR |= DIE;
>> update_decr_irq();
>>
>> Or am I misunderstanding the conditions under which the irq actually
>> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
>> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
>> is what the timer sets when it hits. Unless I completely misread the spec, an
>> interrupt occurs when both of them are true. So all we need to do is have
>> that check and run it every time we change a value in TSR or TCR.
>
> Well OK, this can work to trigger the interrupts, not to clear them though.
> And it will call ppc_set_irq when it's not required.
Calling ppc_set_irq shouldn't be an issue - at the end of the day it only does
a simple OR operation. Unsetting should be pretty obvious too though, no? When
either TSR.DIS or TCR.DIE are not set, the interrupt line is low.
The benefit we get from making the interrupt logic implicit at a single place
is that we're getting rid of a full category of potential bugs where we behave
subtly differently depending on the conditions we were in before. This solution
is a big hammer, yes, but it's proven pretty useful in QEMU so far :).
>>
>> [...]
>>
>>>>>
>>>>>> Very nice patch however! Thanks a lot for sitting down and fixing the
>>>>>> timer
>>>>>> mess on booke that we currently have.
>>>>>
>>>>> You are welcome, It's my thank you gift for the MMU ;)
>>>>
>>>> Haha thanks :). So you're going to fix the MPIC mess for me implementing
>>>> SMP support? :D
>>>>
>>>
>>> I'll see, but I'm not sure you deserve it :)
>>
>> Probably not :). What else is on your todo list to get your awesome guest
>> running? :)
>>
>
> Actually it works pretty well with VxWorks already, I still have to clean up
> few things and play with this new memory API to implement the CCSR
> reallocation.
Ah, cool :). Great to hear!
> I've tried to look at Liu's patch, but I really don't know nothing about KVM
> so
> I won't be able to make any valuable comment...
Ah, too bad. Well, thanks for looking at it nevertheless! If you see something
obvious, please just reply :)
Alex
