Re: [Qemu-devel] [RESEND][PATCH] booke timers

[Fabien Chouteau <chouteau@adacore.com>]

On 09/09/2011 12:55, Alexander Graf wrote:
> 
> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
> 
>> On 07/09/2011 21:59, Alexander Graf wrote:
>>>
>>> On 07.09.2011, at 16:41, Fabien Chouteau wrote:
>>>
>>>> On 06/09/2011 21:33, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> Am 01.09.2011 um 10:20 schrieb Fabien Chouteau <chouteau@adacore.com <mailto:chouteau@adacore.com>>:
>>>>>
>>>>>> While working on the emulation of the freescale p2010 (e500v2) I realized that
>>>>>> there's no implementation of booke's timers features. Currently mpc8544 uses
>>>>>> ppc_emb (ppc_emb_timers_init) which is close but not exactly like booke (for
>>>>>> example booke uses different SPR).
>>>>>>
>>>>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com <mailto:chouteau@adacore.com>>
>>>>>> ---
>>>>>> Makefile.target             |    2 +-
>>>>>> hw/ppc.c                    |  133 ++++++++--------------
>>>>>> hw/ppc.h                    |   25 ++++-
>>>>>> hw/ppc4xx_devs.c            |    2 +-
>>>>>> hw/ppc_booke.c              |  263 +++++++++++++++++++++++++++++++++++++++++++
>>>>>> hw/ppce500_mpc8544ds.c      |    4 +-
>>>>>> hw/virtex_ml507.c           |   11 +--
>>>>>> target-ppc/cpu.h            |   29 +++++
>>>>>> target-ppc/translate_init.c |   43 +++++++-
>>>>>> 9 files changed, 412 insertions(+), 100 deletions(-)
>>>>>> create mode 100644 hw/ppc_booke.c
>>>>>>
>>>>>> diff --git a/Makefile.target b/Makefile.target
>>>>>> index 07af4d4..c8704cd 100644
>>>>>> --- a/Makefile.target
>>>>>> +++ b/Makefile.target
>>>>>> @@ -234,7 +234,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>>>>>> obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>>>
>>>>>> # shared objects
>>>>>> -obj-ppc-y = ppc.o
>>>>>> +obj-ppc-y = ppc.o ppc_booke.o
>>>>>> obj-ppc-y += vga.o
>>>>>> # PREP target
>>>>>> obj-ppc-y += i8259.o mc146818rtc.o
>>>>>> diff --git a/hw/ppc.c b/hw/ppc.c
>>>>>> index 8870748..bcb1e91 100644
>>>>>> --- a/hw/ppc.c
>>>>>> +++ b/hw/ppc.c
>>>>>> @@ -50,7 +50,7 @@
>>>>>> static void cpu_ppc_tb_stop (CPUState *env);
>>>>>> static void cpu_ppc_tb_start (CPUState *env);
>>>>>>
>>>>>> -static void ppc_set_irq (CPUState *env, int n_IRQ, int level)
>>>>>> +void ppc_set_irq(CPUState *env, int n_IRQ, int level)
>>>>>> {
>>>>>>   unsigned int old_pending = env->pending_interrupts;
>>>>>>
>>>>>> @@ -423,25 +423,8 @@ void ppce500_irq_init (CPUState *env)
>>>>>> }
>>>>>> /*****************************************************************************/
>>>>>> /* PowerPC time base and decrementer emulation */
>>>>>> -struct ppc_tb_t {
>>>>>> -    /* Time base management */
>>>>>> -    int64_t  tb_offset;    /* Compensation                    */
>>>>>> -    int64_t  atb_offset;   /* Compensation                    */
>>>>>> -    uint32_t tb_freq;      /* TB frequency                    */
>>>>>> -    /* Decrementer management */
>>>>>> -    uint64_t decr_next;    /* Tick for next decr interrupt    */
>>>>>> -    uint32_t decr_freq;    /* decrementer frequency           */
>>>>>> -    struct QEMUTimer *decr_timer;
>>>>>> -    /* Hypervisor decrementer management */
>>>>>> -    uint64_t hdecr_next;    /* Tick for next hdecr interrupt  */
>>>>>> -    struct QEMUTimer *hdecr_timer;
>>>>>> -    uint64_t purr_load;
>>>>>> -    uint64_t purr_start;
>>>>>> -    void *opaque;
>>>>>> -};
>>>>>>
>>>>>> -static inline uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk,
>>>>>> -                                      int64_t tb_offset)
>>>>>> +uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset)
>>>>>> {
>>>>>>   /* TB time in tb periods */
>>>>>>   return muldiv64(vmclk, tb_env->tb_freq, get_ticks_per_sec()) + tb_offset;
>>>>>> @@ -611,10 +594,13 @@ static inline uint32_t _cpu_ppc_load_decr(CPUState *env, uint64_t next)
>>>>>>   int64_t diff;
>>>>>>
>>>>>>   diff = next - qemu_get_clock_ns(vm_clock);
>>>>>> -    if (diff >= 0)
>>>>>> +    if (diff >= 0) {
>>>>>>       decr = muldiv64(diff, tb_env->decr_freq, get_ticks_per_sec());
>>>>>> -    else
>>>>>> +    } else if (env->insns_flags & PPC_BOOKE) {
>>>>>> +        decr = 0;
>>>>>
>>>>> I don't think we should expose instruction interpreter details into the
>>>>> timing code. It'd be better to have a separate flag that gets set on the booke
>>>>> timer init function which would be stored in tb_env. Keeps things better
>>>>> separated :)
>>>>
>>>> Good idea, but how do we set the flags? ppc_booke_timers_init doesn't know
>>>> which processor is emulated.
>>>
>>> We could have two different init functions. One for normal booke and one for
>>> e500. Or we pass in timer flags to the init function.
>>
>> How can we handle the -cpu option? For example if I want to test a ppc604 on my
>> p2010 board? I'm not sure if it really makes sense...
> 
> I guess at the end of the day we need to have the CPU initialize its timers. They are tightly coupled with the CPU anyways. For now, we can leave the instantiation as is though.
> 
>>
>>
>>>>
>>>>
>>>>>
>>>>>> +    }  else {
>>>>>>       decr = -muldiv64(-diff, tb_env->decr_freq, get_ticks_per_sec());
>>>>>> +    }
>>>>>>   LOG_TB("%s: %08" PRIx32 "\n", __func__, decr);
>>>>>>
>>>>>>   return decr;
>>>>>> @@ -678,18 +664,23 @@ static void __cpu_ppc_store_decr (CPUState *env, uint64_t *nextp,
>>>>>>               decr, value);
>>>>>>   now = qemu_get_clock_ns(vm_clock);
>>>>>>   next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
>>>>>> -    if (is_excp)
>>>>>> +    if (is_excp) {
>>>>>>       next += *nextp - now;
>>>>>> -    if (next == now)
>>>>>> +    }
>>>>>> +    if (next == now) {
>>>>>>       next++;
>>>>>> +    }
>>>>>>   *nextp = next;
>>>>>>   /* Adjust timer */
>>>>>>   qemu_mod_timer(timer, next);
>>>>>>   /* If we set a negative value and the decrementer was positive,
>>>>>> -     * raise an exception.
>>>>>> +     * raise an exception (not for booke).
>>>>>>    */
>>>>>> -    if ((value & 0x80000000) && !(decr & 0x80000000))
>>>>>> +    if (!(env->insns_flags & PPC_BOOKE)
>>>>>> +        && (value & 0x80000000)
>>>>>> +        && !(decr & 0x80000000)) {
>>>>>>       (*raise_excp)(env);
>>>>>
>>>>> Please make this a separate flag too. IIRC this is not unified behavior on all ppc CPUs, not even all server type ones.
>>>>>
>>>>
>>>> This come from a comment by Scott (CC'd), I don't know much about it. Can you
>>>> help me to find a good name for this feature?
>>>
>>> PPC_DECR_TRIGGER_ON_NEGATIVE? :)
>>>
>>
>> After a quick check, PPC_DECR_UNDERFLOW_TRIGGERED seems more appropriate.
> 
> Good :).
> 
> [...]
> 
>>>>>> +/* Return the location of the bit of time base at which the FIT will raise an
>>>>>> +   interrupt */
>>>>>> +static uint8_t booke_get_fit_target(CPUState *env)
>>>>>> +{
>>>>>> +    uint8_t fp = (env->spr[SPR_BOOKE_TCR] & TCR_FP_MASK) >> TCR_FP_SHIFT;
>>>>>> +
>>>>>> +    /* Only for e500 */
>>>>>> +    if (env->insns_flags2 & PPC2_E500) {
>>>>>
>>>>> Same here again :). Do you have test cases for fit? IIRC Linux doesn't use it.
>>>>
>>>> VxWorks uses it.
>>>
>>> If even remotely possible, I'd really like to have something in my portfolio
>>> of guest code to test this case - especially given that KVM implements its
>>> own timers. I assume your binary is non-redistributable?
>>
>> No I can't give you the binary. Maybe the best solution is to write a simple
>> test case from scratch.
> 
> Yeah, I'll poke and see if someone already has something there.
> 
>>
>>
>>>>
>>>>>
>>>>>> +        uint32_t fpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_FPEXT_MASK)
>>>>>> +            >> TCR_E500_FPEXT_SHIFT;
>>>>>> +        fp = 63 - (fp | fpext << 2);
>>>>>> +    } else {
>>>>>> +        fp = env->fit_period[fp];
>>>>>> +    }
>>>>>> +
>>>>>> +    return fp;
>>>>>> +}
>>>>>> +
>>>>>> +/* Return the location of the bit of time base at which the WDT will raise an
>>>>>> +   interrupt */
>>>>>> +static uint8_t booke_get_wdt_target(CPUState *env)
>>>>>> +{
>>>>>> +    uint8_t wp = (env->spr[SPR_BOOKE_TCR] & TCR_WP_MASK) >> TCR_WP_SHIFT;
>>>>>> +
>>>>>> +    /* Only for e500 */
>>>>>> +    if (env->insns_flags2 & PPC2_E500) {
>>>>>> +        uint32_t wpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_WPEXT_MASK)
>>>>>> +            >> TCR_E500_WPEXT_SHIFT;
>>>>>> +        wp = 63 - (wp | wpext << 2);
>>>>>> +    } else {
>>>>>> +        wp = env->wdt_period[wp];
>>>>>> +    }
>>>>>> +
>>>>>> +    return wp;
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_update_fixed_timer(CPUState         *env,
>>>>>> +                                     uint8_t           target_bit,
>>>>>> +                                     uint64_t          *next,
>>>>>> +                                     struct QEMUTimer *timer)
>>>>>> +{
>>>>>> +    ppc_tb_t *tb_env = env->tb_env;
>>>>>> +    uint64_t lapse;
>>>>>> +    uint64_t tb;
>>>>>> +    uint64_t period = 1 << (target_bit + 1);
>>>>>> +    uint64_t now;
>>>>>> +
>>>>>> +    now = qemu_get_clock_ns(vm_clock);
>>>>>> +    tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
>>>>>> +
>>>>>> +    lapse = period - ((tb - (1 << target_bit)) & (period - 1));
>>>>>> +
>>>>>> +    *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq);
>>>>>> +
>>>>>> +    if (*next == now) {
>>>>>> +        (*next)++;
>>>>>
>>>>> Huh? If we hit the timer we don't fire it?
>>>>
>>>> Yes we do, but one nanosecond later :)
>>>
>>> Well, why trigger a timer when we can just as well call the callback immediately? :)
>>>
>>
>> This come from ppc.c, QEMUTimer is kind of a private type so we don't have
>> access to the callback.
> 
> Oh, I see. Please just add that as an XXX comment so we can resolve it when we get around to it.
> 
>>
>>>>
>>>>>
>>>>>> +    }
>>>>>> +
>>>>>> +    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.

>>
>>>>
>>>>
>>>>>> +
>>>>>> +    if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
>>>>>> +        /* Auto Reload */
>>>>>> +        cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
>>>>>> +    }
>>>>>
>>>>> Ah nice - never seen this one used. Do you have a test case?
>>>>>
>>>>
>>>> VxWorks :)
>>>>
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_fit_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_FIS;
>>>>>> +    if (env->spr[SPR_BOOKE_TCR] & TCR_FIE) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>>> +    }
>>>>>
>>>>> Same as above :)
>>>>>
>>>>>> +
>>>>>> +    booke_update_fixed_timer(env,
>>>>>> +                             booke_get_fit_target(env),
>>>>>> +                             &booke_timer->fit_next,
>>>>>> +                             booke_timer->fit_timer);
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_wdt_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;
>>>>>> +
>>>>>> +    /* TODO: There's lots of complicated stuff to do here */
>>>>>> +
>>>>>> +    booke_update_fixed_timer(env,
>>>>>> +                             booke_get_wdt_target(env),
>>>>>> +                             &booke_timer->wdt_next,
>>>>>> +                             booke_timer->wdt_timer);
>>>>>> +}
>>>>>> +
>>>>>> +void store_booke_tsr(CPUState *env, target_ulong val)
>>>>>> +{
>>>>>> +    ppc_tb_t *tb_env = env->tb_env;
>>>>>> +    booke_timer_t *booke_timer;
>>>>>> +
>>>>>> +    booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> +    env->spr[SPR_BOOKE_TSR] &= ~val;
>>>>>> +
>>>>>> +    if (val & TSR_DIS) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_DECR, 0);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (val & TSR_FIS) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_FIT, 0);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (val & TSR_WIS) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_WDT, 0);
>>>>>> +    }
>>>>>
>>>>> Why do you need the above? Shouldn't they still be active if the guest didn't
>>>>> reset the bit? This is probably hiding a bug in the interrupt delivery
>>>>> mechanism automatically unmasking an irq bit when it's delivered, right?
>>>>
>>>> They are active until the bit is cleared by user, and TSR is a write-1-to-clear
>>>> register.
>>>
>>> Yes, that's what I mean. There shouldn't be a need to set the irq again because it should still be active. These interrupts are level based.
>>>
>>
>> I feel like there's a misunderstanding here. I do not set the interrupts I
>> clear them.
> 
> Oh. Yes, I misread that. Sorry :). That indeed does make a lot of sense. I can however be folded in with the normal "is DECR active right now" check.
> 
>>
>>
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +void store_booke_tcr(CPUState *env, target_ulong val)
>>>>>> +{
>>>>>> +    ppc_tb_t *tb_env = env->tb_env;
>>>>>> +    booke_timer_t *booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> +    tb_env = env->tb_env;
>>>>>> +    env->spr[SPR_BOOKE_TCR] = val;
>>>>>> +
>>>>>> +    booke_update_fixed_timer(env,
>>>>>> +                             booke_get_fit_target(env),
>>>>>> +                             &booke_timer->fit_next,
>>>>>> +                             booke_timer->fit_timer);
>>>>>> +
>>>>>> +    booke_update_fixed_timer(env,
>>>>>> +                             booke_get_wdt_target(env),
>>>>>> +                             &booke_timer->wdt_next,
>>>>>> +                             booke_timer->wdt_timer);
>>>>>> +
>>>>>> +    if (val & TCR_DIE && env->spr[SPR_BOOKE_TSR] & TSR_DIS) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (val & TCR_FIE && env->spr[SPR_BOOKE_TSR] & TSR_FIS) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (val & TCR_WIE && env->spr[SPR_BOOKE_TSR] & TSR_WIS) {
>>>>>> +        ppc_set_irq(env, PPC_INTERRUPT_WDT, 1);
>>>>>> +    }
>>>>>
>>>>> Here is the second user of the checks. It really does make sense to only have
>>>>> them in a single spot, so that ever ppc_set_irq(DECR) always goes through the
>>>>> TSR and TCR checks for example :).
>>>>
>>>> ...here we test DIE in TCR and DIS in TSR.
>>>
>>> Yes, both of which are basically the prerequisites for actually triggering the internal DECR interrupt line.
>>>
>>
>> Not exactly, see above.
> 
> [...]
> 
>>>>
>>>>> 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.

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...

Regards,

-- 
Fabien Chouteau