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

[Fabien Chouteau]

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 <address@hidden 
>>>>> <mailto:address@hidden>>:
>>>>>
>>>>>> 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 <address@hidden <mailto:address@hidden>>
>>>>>> ---
>>>>>> 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