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

[Fabien Chouteau]

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


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

>>
>>
>>>> +    }
>>>> }
>>>>
>>>> static inline void _cpu_ppc_store_decr(CPUState *env, uint32_t decr,
>>>> @@ -806,11 +797,11 @@ uint32_t cpu_ppc601_load_rtcl (CPUState *env)
>>>> }
>>>>
>>>> /*****************************************************************************/
>>>> -/* Embedded PowerPC timers */
>>>> +/* PowerPC 40x timers */
>>>>
>>>> /* PIT, FIT & WDT */
>>>> -typedef struct ppcemb_timer_t ppcemb_timer_t;
>>>> -struct ppcemb_timer_t {
>>>> +typedef struct ppc40x_timer_t ppc40x_timer_t;
>>>> +struct ppc40x_timer_t {
>>>>    uint64_t pit_reload;  /* PIT auto-reload value        */
>>>>    uint64_t fit_next;    /* Tick for next FIT interrupt  */
>>>>    struct QEMUTimer *fit_timer;
>>>> @@ -826,12 +817,12 @@ static void cpu_4xx_fit_cb (void *opaque)
>>>> {
>>>>    CPUState *env;
>>>>    ppc_tb_t *tb_env;
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> +    ppc40x_timer_t *ppc40x_timer;
>>>>    uint64_t now, next;
>>>>
>>>>    env = opaque;
>>>>    tb_env = env->tb_env;
>>>> -    ppcemb_timer = tb_env->opaque;
>>>> +    ppc40x_timer = tb_env->opaque;
>>>>    now = qemu_get_clock_ns(vm_clock);
>>>>    switch ((env->spr[SPR_40x_TCR] >> 24) & 0x3) {
>>>>    case 0:
>>>> @@ -853,7 +844,7 @@ static void cpu_4xx_fit_cb (void *opaque)
>>>>    next = now + muldiv64(next, get_ticks_per_sec(), tb_env->tb_freq);
>>>>    if (next == now)
>>>>        next++;
>>>> -    qemu_mod_timer(ppcemb_timer->fit_timer, next);
>>>> +    qemu_mod_timer(ppc40x_timer->fit_timer, next);
>>>>    env->spr[SPR_40x_TSR] |= 1 << 26;
>>>>    if ((env->spr[SPR_40x_TCR] >> 23) & 0x1)
>>>>        ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>> @@ -865,11 +856,11 @@ static void cpu_4xx_fit_cb (void *opaque)
>>>> /* Programmable interval timer */
>>>> static void start_stop_pit (CPUState *env, ppc_tb_t *tb_env, int is_excp)
>>>> {
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> +    ppc40x_timer_t *ppc40x_timer;
>>>>    uint64_t now, next;
>>>>
>>>> -    ppcemb_timer = tb_env->opaque;
>>>> -    if (ppcemb_timer->pit_reload <= 1 ||
>>>> +    ppc40x_timer = tb_env->opaque;
>>>> +    if (ppc40x_timer->pit_reload <= 1 ||
>>>>        !((env->spr[SPR_40x_TCR] >> 26) & 0x1) ||
>>>>        (is_excp && !((env->spr[SPR_40x_TCR] >> 22) & 0x1))) {
>>>>        /* Stop PIT */
>>>> @@ -877,9 +868,9 @@ static void start_stop_pit (CPUState *env, ppc_tb_t 
>>>> *tb_env, int is_excp)
>>>>        qemu_del_timer(tb_env->decr_timer);
>>>>    } else {
>>>>        LOG_TB("%s: start PIT %016" PRIx64 "\n",
>>>> -                    __func__, ppcemb_timer->pit_reload);
>>>> +                    __func__, ppc40x_timer->pit_reload);
>>>>        now = qemu_get_clock_ns(vm_clock);
>>>> -        next = now + muldiv64(ppcemb_timer->pit_reload,
>>>> +        next = now + muldiv64(ppc40x_timer->pit_reload,
>>>>                              get_ticks_per_sec(), tb_env->decr_freq);
>>>>        if (is_excp)
>>>>            next += tb_env->decr_next - now;
>>>> @@ -894,21 +885,21 @@ static void cpu_4xx_pit_cb (void *opaque)
>>>> {
>>>>    CPUState *env;
>>>>    ppc_tb_t *tb_env;
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> +    ppc40x_timer_t *ppc40x_timer;
>>>>
>>>>    env = opaque;
>>>>    tb_env = env->tb_env;
>>>> -    ppcemb_timer = tb_env->opaque;
>>>> +    ppc40x_timer = tb_env->opaque;
>>>>    env->spr[SPR_40x_TSR] |= 1 << 27;
>>>>    if ((env->spr[SPR_40x_TCR] >> 26) & 0x1)
>>>> -        ppc_set_irq(env, ppcemb_timer->decr_excp, 1);
>>>> +        ppc_set_irq(env, ppc40x_timer->decr_excp, 1);
>>>>    start_stop_pit(env, tb_env, 1);
>>>>    LOG_TB("%s: ar %d ir %d TCR " TARGET_FMT_lx " TSR " TARGET_FMT_lx " "
>>>>           "%016" PRIx64 "\n", __func__,
>>>>           (int)((env->spr[SPR_40x_TCR] >> 22) & 0x1),
>>>>           (int)((env->spr[SPR_40x_TCR] >> 26) & 0x1),
>>>>           env->spr[SPR_40x_TCR], env->spr[SPR_40x_TSR],
>>>> -           ppcemb_timer->pit_reload);
>>>> +           ppc40x_timer->pit_reload);
>>>> }
>>>>
>>>> /* Watchdog timer */
>>>> @@ -916,12 +907,12 @@ static void cpu_4xx_wdt_cb (void *opaque)
>>>> {
>>>>    CPUState *env;
>>>>    ppc_tb_t *tb_env;
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> +    ppc40x_timer_t *ppc40x_timer;
>>>>    uint64_t now, next;
>>>>
>>>>    env = opaque;
>>>>    tb_env = env->tb_env;
>>>> -    ppcemb_timer = tb_env->opaque;
>>>> +    ppc40x_timer = tb_env->opaque;
>>>>    now = qemu_get_clock_ns(vm_clock);
>>>>    switch ((env->spr[SPR_40x_TCR] >> 30) & 0x3) {
>>>>    case 0:
>>>> @@ -948,13 +939,13 @@ static void cpu_4xx_wdt_cb (void *opaque)
>>>>    switch ((env->spr[SPR_40x_TSR] >> 30) & 0x3) {
>>>>    case 0x0:
>>>>    case 0x1:
>>>> -        qemu_mod_timer(ppcemb_timer->wdt_timer, next);
>>>> -        ppcemb_timer->wdt_next = next;
>>>> +        qemu_mod_timer(ppc40x_timer->wdt_timer, next);
>>>> +        ppc40x_timer->wdt_next = next;
>>>>        env->spr[SPR_40x_TSR] |= 1 << 31;
>>>>        break;
>>>>    case 0x2:
>>>> -        qemu_mod_timer(ppcemb_timer->wdt_timer, next);
>>>> -        ppcemb_timer->wdt_next = next;
>>>> +        qemu_mod_timer(ppc40x_timer->wdt_timer, next);
>>>> +        ppc40x_timer->wdt_next = next;
>>>>        env->spr[SPR_40x_TSR] |= 1 << 30;
>>>>        if ((env->spr[SPR_40x_TCR] >> 27) & 0x1)
>>>>            ppc_set_irq(env, PPC_INTERRUPT_WDT, 1);
>>>> @@ -982,12 +973,12 @@ static void cpu_4xx_wdt_cb (void *opaque)
>>>> void store_40x_pit (CPUState *env, target_ulong val)
>>>> {
>>>>    ppc_tb_t *tb_env;
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> +    ppc40x_timer_t *ppc40x_timer;
>>>>
>>>>    tb_env = env->tb_env;
>>>> -    ppcemb_timer = tb_env->opaque;
>>>> +    ppc40x_timer = tb_env->opaque;
>>>>    LOG_TB("%s val" TARGET_FMT_lx "\n", __func__, val);
>>>> -    ppcemb_timer->pit_reload = val;
>>>> +    ppc40x_timer->pit_reload = val;
>>>>    start_stop_pit(env, tb_env, 0);
>>>> }
>>>>
>>>> @@ -996,31 +987,7 @@ target_ulong load_40x_pit (CPUState *env)
>>>>    return cpu_ppc_load_decr(env);
>>>> }
>>>>
>>>> -void store_booke_tsr (CPUState *env, target_ulong val)
>>>> -{
>>>> -    ppc_tb_t *tb_env = env->tb_env;
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> -
>>>> -    ppcemb_timer = tb_env->opaque;
>>>> -
>>>> -    LOG_TB("%s: val " TARGET_FMT_lx "\n", __func__, val);
>>>> -    env->spr[SPR_40x_TSR] &= ~(val & 0xFC000000);
>>>> -    if (val & 0x80000000)
>>>> -        ppc_set_irq(env, ppcemb_timer->decr_excp, 0);
>>>> -}
>>>> -
>>>> -void store_booke_tcr (CPUState *env, target_ulong val)
>>>> -{
>>>> -    ppc_tb_t *tb_env;
>>>> -
>>>> -    tb_env = env->tb_env;
>>>> -    LOG_TB("%s: val " TARGET_FMT_lx "\n", __func__, val);
>>>> -    env->spr[SPR_40x_TCR] = val & 0xFFC00000;
>>>> -    start_stop_pit(env, tb_env, 1);
>>>> -    cpu_4xx_wdt_cb(env);
>>>> -}
>>>> -
>>>> -static void ppc_emb_set_tb_clk (void *opaque, uint32_t freq)
>>>> +static void ppc_40x_set_tb_clk (void *opaque, uint32_t freq)
>>>> {
>>>>    CPUState *env = opaque;
>>>>    ppc_tb_t *tb_env = env->tb_env;
>>>> @@ -1032,30 +999,30 @@ static void ppc_emb_set_tb_clk (void *opaque, 
>>>> uint32_t freq)
>>>>    /* XXX: we should also update all timers */
>>>> }
>>>>
>>>> -clk_setup_cb ppc_emb_timers_init (CPUState *env, uint32_t freq,
>>>> +clk_setup_cb ppc_40x_timers_init (CPUState *env, uint32_t freq,
>>>>                                  unsigned int decr_excp)
>>>> {
>>>>    ppc_tb_t *tb_env;
>>>> -    ppcemb_timer_t *ppcemb_timer;
>>>> +    ppc40x_timer_t *ppc40x_timer;
>>>>
>>>>    tb_env = g_malloc0(sizeof(ppc_tb_t));
>>>>    env->tb_env = tb_env;
>>>> -    ppcemb_timer = g_malloc0(sizeof(ppcemb_timer_t));
>>>> +    ppc40x_timer = g_malloc0(sizeof(ppc40x_timer_t));
>>>>    tb_env->tb_freq = freq;
>>>>    tb_env->decr_freq = freq;
>>>> -    tb_env->opaque = ppcemb_timer;
>>>> +    tb_env->opaque = ppc40x_timer;
>>>>    LOG_TB("%s freq %" PRIu32 "\n", __func__, freq);
>>>> -    if (ppcemb_timer != NULL) {
>>>> +    if (ppc40x_timer != NULL) {
>>>>        /* We use decr timer for PIT */
>>>>        tb_env->decr_timer = qemu_new_timer_ns(vm_clock, &cpu_4xx_pit_cb, 
>>>> env);
>>>> -        ppcemb_timer->fit_timer =
>>>> +        ppc40x_timer->fit_timer =
>>>>            qemu_new_timer_ns(vm_clock, &cpu_4xx_fit_cb, env);
>>>> -        ppcemb_timer->wdt_timer =
>>>> +        ppc40x_timer->wdt_timer =
>>>>            qemu_new_timer_ns(vm_clock, &cpu_4xx_wdt_cb, env);
>>>> -        ppcemb_timer->decr_excp = decr_excp;
>>>> +        ppc40x_timer->decr_excp = decr_excp;
>>>>    }
>>>>
>>>> -    return &ppc_emb_set_tb_clk;
>>>> +    return &ppc_40x_set_tb_clk;
>>>> }
>>>>
>>>> /*****************************************************************************/
>>>> diff --git a/hw/ppc.h b/hw/ppc.h
>>>> index 3ccf134..16df16a 100644
>>>> --- a/hw/ppc.h
>>>> +++ b/hw/ppc.h
>>>> @@ -1,3 +1,5 @@
>>>> +void ppc_set_irq (CPUState *env, int n_IRQ, int level);
>>>> +
>>>> /* PowerPC hardware exceptions management helpers */
>>>> typedef void (*clk_setup_cb)(void *opaque, uint32_t freq);
>>>> typedef struct clk_setup_t clk_setup_t;
>>>> @@ -11,6 +13,24 @@ static inline void clk_setup (clk_setup_t *clk, 
>>>> uint32_t freq)
>>>>        (*clk->cb)(clk->opaque, freq);
>>>> }
>>>>
>>>> +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;
>>>> +};
>>>> +
>>>> +uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t 
>>>> tb_offset);
>>>> clk_setup_cb cpu_ppc_tb_init (CPUState *env, uint32_t freq);
>>>> /* Embedded PowerPC DCR management */
>>>> typedef uint32_t (*dcr_read_cb)(void *opaque, int dcrn);
>>>> @@ -19,7 +39,7 @@ int ppc_dcr_init (CPUState *env, int 
>>>> (*dcr_read_error)(int dcrn),
>>>>                  int (*dcr_write_error)(int dcrn));
>>>> int ppc_dcr_register (CPUState *env, int dcrn, void *opaque,
>>>>                      dcr_read_cb drc_read, dcr_write_cb dcr_write);
>>>> -clk_setup_cb ppc_emb_timers_init (CPUState *env, uint32_t freq,
>>>> +clk_setup_cb ppc_40x_timers_init (CPUState *env, uint32_t freq,
>>>>                                  unsigned int decr_excp);
>>>>
>>>> /* Embedded PowerPC reset */
>>>> @@ -55,3 +75,6 @@ enum {
>>>> #define FW_CFG_PPC_KVM_PID      (FW_CFG_ARCH_LOCAL + 0x07)
>>>>
>>>> #define PPC_SERIAL_MM_BAUDBASE 399193
>>>> +
>>>> +/* ppc_booke.c */
>>>> +void ppc_booke_timers_init (CPUState *env, uint32_t freq);
>>>> diff --git a/hw/ppc4xx_devs.c b/hw/ppc4xx_devs.c
>>>> index 349f046..d18caa4 100644
>>>> --- a/hw/ppc4xx_devs.c
>>>> +++ b/hw/ppc4xx_devs.c
>>>> @@ -56,7 +56,7 @@ CPUState *ppc4xx_init (const char *cpu_model,
>>>>    cpu_clk->cb = NULL; /* We don't care about CPU clock frequency changes 
>>>> */
>>>>    cpu_clk->opaque = env;
>>>>    /* Set time-base frequency to sysclk */
>>>> -    tb_clk->cb = ppc_emb_timers_init(env, sysclk, PPC_INTERRUPT_PIT);
>>>> +    tb_clk->cb = ppc_40x_timers_init(env, sysclk, PPC_INTERRUPT_PIT);
>>>>    tb_clk->opaque = env;
>>>>    ppc_dcr_init(env, NULL, NULL);
>>>>    /* Register qemu callbacks */
>>>> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
>>>> new file mode 100644
>>>> index 0000000..35f11ca
>>>> --- /dev/null
>>>> +++ b/hw/ppc_booke.c
>>>> @@ -0,0 +1,263 @@
>>>> +/*
>>>> + * QEMU PowerPC Booke hardware System Emulator
>>>> + *
>>>> + * Copyright (c) 2011 AdaCore
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining 
>>>> a copy
>>>> + * of this software and associated documentation files (the "Software"), 
>>>> to deal
>>>> + * in the Software without restriction, including without limitation the 
>>>> rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
>>>> sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be 
>>>> included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>>>> OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>>>> ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
>>>> IN
>>>> + * THE SOFTWARE.
>>>> + */
>>>> +#include "hw.h"
>>>> +#include "ppc.h"
>>>> +#include "qemu-timer.h"
>>>> +#include "sysemu.h"
>>>> +#include "nvram.h"
>>>> +#include "qemu-log.h"
>>>> +#include "loader.h"
>>>> +
>>>> +
>>>> +/* Timer Control Register */
>>>> +
>>>> +#define TCR_WP_SHIFT  30        /* Watchdog Timer Period */
>>>> +#define TCR_WP_MASK   (0x3 << TCR_WP_SHIFT)
>>>> +#define TCR_WRC_SHIFT 28        /* Watchdog Timer Reset Control */
>>>> +#define TCR_WRC_MASK  (0x3 << TCR_WRC_SHIFT)
>>>> +#define TCR_WIE       (1 << 27) /* Watchdog Timer Interrupt Enable */
>>>> +#define TCR_DIE       (1 << 26) /* Decrementer Interrupt Enable */
>>>> +#define TCR_FP_SHIFT  24        /* Fixed-Interval Timer Period */
>>>> +#define TCR_FP_MASK   (0x3 << TCR_FP_SHIFT)
>>>> +#define TCR_FIE       (1 << 23) /* Fixed-Interval Timer Interrupt Enable 
>>>> */
>>>> +#define TCR_ARE       (1 << 22) /* Auto-Reload Enable */
>>>> +
>>>> +/* Timer Control Register (e500 specific fields) */
>>>> +
>>>> +#define TCR_E500_FPEXT_SHIFT 13 /* Fixed-Interval Timer Period Extension 
>>>> */
>>>> +#define TCR_E500_FPEXT_MASK  (0xf << TCR_E500_FPEXT_SHIFT)
>>>> +#define TCR_E500_WPEXT_SHIFT 17 /* Watchdog Timer Period Extension */
>>>> +#define TCR_E500_WPEXT_MASK  (0xf << TCR_E500_WPEXT_SHIFT)
>>>> +
>>>> +/* Timer Status Register  */
>>>> +
>>>> +#define TSR_FIS       (1 << 26) /* Fixed-Interval Timer Interrupt Status 
>>>> */
>>>> +#define TSR_DIS       (1 << 27) /* Decrementer Interrupt Status */
>>>> +#define TSR_WRS_SHIFT 28        /* Watchdog Timer Reset Status */
>>>> +#define TSR_WRS_MASK  (0x3 << TSR_WRS_SHIFT)
>>>> +#define TSR_WIS       (1 << 30) /* Watchdog Timer Interrupt Status */
>>>> +#define TSR_ENW       (1 << 31) /* Enable Next Watchdog Timer */
>>>> +
>>>> +typedef struct booke_timer_t booke_timer_t;
>>>> +struct booke_timer_t {
>>>> +
>>>> +    uint64_t fit_next;
>>>> +    struct QEMUTimer *fit_timer;
>>>> +
>>>> +    uint64_t wdt_next;
>>>> +    struct QEMUTimer *wdt_timer;
>>>> +};
>>>> +
>>>> +/* 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.


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

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

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


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

>>
>>>> +}
>>>> +
>>>> +void ppc_booke_timers_init(CPUState *env, uint32_t freq)
>>>> +{
>>>> +    ppc_tb_t *tb_env;
>>>> +    booke_timer_t *booke_timer;
>>>> +
>>>> +    tb_env      = g_malloc0(sizeof(ppc_tb_t));
>>>> +    booke_timer = g_malloc0(sizeof(booke_timer_t));
>>>> +
>>>> +    env->tb_env = tb_env;
>>>> +
>>>> +    tb_env->tb_freq    = freq;
>>>> +    tb_env->decr_freq  = freq;
>>>> +    tb_env->opaque     = booke_timer;
>>>> +    tb_env->decr_timer = qemu_new_timer_ns(vm_clock, &booke_decr_cb, env);
>>>> +
>>>> +    booke_timer->fit_timer =
>>>> +        qemu_new_timer_ns(vm_clock, &booke_fit_cb, env);
>>>> +    booke_timer->wdt_timer =
>>>> +        qemu_new_timer_ns(vm_clock, &booke_wdt_cb, env);
>>>> +}
>>>> diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
>>>> index 1274a3e..b8aa0d0 100644
>>>> --- a/hw/ppce500_mpc8544ds.c
>>>> +++ b/hw/ppce500_mpc8544ds.c
>>>> @@ -252,9 +252,7 @@ static void mpc8544ds_init(ram_addr_t ram_size,
>>>>        exit(1);
>>>>    }
>>>>
>>>> -    /* XXX register timer? */
>>>> -    ppc_emb_timers_init(env, 400000000, PPC_INTERRUPT_DECR);
>>>> -    ppc_dcr_init(env, NULL, NULL);
>>>> +    ppc_booke_timers_init(env, 400000000);
>>>>
>>>>    /* Register reset handler */
>>>>    qemu_register_reset(mpc8544ds_cpu_reset, env);
>>>> diff --git a/hw/virtex_ml507.c b/hw/virtex_ml507.c
>>>> index 333050c..dccaea3 100644
>>>> --- a/hw/virtex_ml507.c
>>>> +++ b/hw/virtex_ml507.c
>>>> @@ -81,7 +81,6 @@ static void mmubooke_create_initial_mapping(CPUState 
>>>> *env,
>>>> static CPUState *ppc440_init_xilinx(ram_addr_t *ram_size,
>>>>                                    int do_init,
>>>>                                    const char *cpu_model,
>>>> -                                    clk_setup_t *cpu_clk, clk_setup_t 
>>>> *tb_clk,
>>>>                                    uint32_t sysclk)
>>>> {
>>>>    CPUState *env;
>>>> @@ -93,11 +92,7 @@ static CPUState *ppc440_init_xilinx(ram_addr_t 
>>>> *ram_size,
>>>>        exit(1);
>>>>    }
>>>>
>>>> -    cpu_clk->cb = NULL; /* We don't care about CPU clock frequency 
>>>> changes */
>>>> -    cpu_clk->opaque = env;
>>>> -    /* Set time-base frequency to sysclk */
>>>> -    tb_clk->cb = ppc_emb_timers_init(env, sysclk, PPC_INTERRUPT_DECR);
>>>> -    tb_clk->opaque = env;
>>>> +    ppc_booke_timers_init(env, sysclk);
>>>>
>>>>    ppc_dcr_init(env, NULL, NULL);
>>>>
>>>> @@ -198,7 +193,6 @@ static void virtex_init(ram_addr_t ram_size,
>>>>    ram_addr_t phys_ram;
>>>>    ram_addr_t phys_flash;
>>>>    qemu_irq irq[32], *cpu_irq;
>>>> -    clk_setup_t clk_setup[7];
>>>>    int kernel_size;
>>>>    int i;
>>>>
>>>> @@ -208,8 +202,7 @@ static void virtex_init(ram_addr_t ram_size,
>>>>    }
>>>>
>>>>    memset(clk_setup, 0, sizeof(clk_setup));
>>>> -    env = ppc440_init_xilinx(&ram_size, 1, cpu_model, &clk_setup[0],
>>>> -                             &clk_setup[1], 400000000);
>>>> +    env = ppc440_init_xilinx(&ram_size, 1, cpu_model, 400000000);
>>>>    qemu_register_reset(main_cpu_reset, env);
>>>>
>>>>    phys_ram = qemu_ram_alloc(NULL, "ram", ram_size);
>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>> index b8d42e0..be0d79c 100644
>>>> --- a/target-ppc/cpu.h
>>>> +++ b/target-ppc/cpu.h
>>>> @@ -1010,8 +1010,35 @@ struct CPUPPCState {
>>>> #if !defined(CONFIG_USER_ONLY)
>>>>    void *load_info;    /* Holds boot loading state.  */
>>>> #endif
>>>> +
>>>> +    /* booke timers */
>>>> +
>>>> +    /* Specifies bit locations of the Time Base used to signal a fixed 
>>>> timer
>>>> +     * exception on a transition from 0 to 1. (watchdog or fixed-interval 
>>>> timer)
>>>> +     *
>>>> +     * 0 selects the least significant bit.
>>>> +     * 63 selects the most significant bit.
>>>> +     */
>>>> +    uint8_t fit_period[4];
>>>> +    uint8_t wdt_period[4];
>>>> };
>>>>
>>>> +#define SET_FIT_PERIOD(a_, b_, c_, d_)          \
>>>> +do {                                            \
>>>> +    env->fit_period[0] = (a_);                  \
>>>> +    env->fit_period[1] = (b_);                  \
>>>> +    env->fit_period[2] = (c_);                  \
>>>> +    env->fit_period[3] = (d_);                  \
>>>> + } while (0)
>>>> +
>>>> +#define SET_WDT_PERIOD(a_, b_, c_, d_)          \
>>>> +do {                                            \
>>>> +    env->wdt_period[0] = (a_);                  \
>>>> +    env->wdt_period[1] = (b_);                  \
>>>> +    env->wdt_period[2] = (c_);                  \
>>>> +    env->wdt_period[3] = (d_);                  \
>>>> + } while (0)
>>>> +
>>>> #if !defined(CONFIG_USER_ONLY)
>>>> /* Context used internally during MMU translations */
>>>> typedef struct mmu_ctx_t mmu_ctx_t;
>>>> @@ -1806,6 +1833,8 @@ enum {
>>>>
>>>>    /* BookE 2.06 PowerPC specification                                     
>>>>  */
>>>>    PPC2_BOOKE206      = 0x0000000000000001ULL,
>>>> +    /* e500 support                                                       
>>>>    */
>>>> +    PPC2_E500          = 0x0000000000000002ULL,
>>>
>>> I don't think we should have an e500 inst target. It should be enough to 
>>> have
>>> an SPE inst target and specific SPR init functions for e500, no? Keep in 
>>> mind
>>> that these flags are only meant to be used by the instruction interpreter.
>>
>> Yes sure, it was the easy way to implement e500 features in the timers, but 
>> now
>> I will remove it.
>>
>>> 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 :)

>>
>>> Since you definitely know your way around booke timekeeping code, could you
>>> please also take a look at the decr patches on address@hidden that Liu 
>>> posted
>>> recently?
>>
>> I don't know anything about kvm but I can take a look. Do you have the name 
>> of
>> this patch?
>
> Sure, it's "KVM: PPC: booke: Improve timer register emulation". Thanks a lot 
> for looking at it!
>

Regards,

-- 
Fabien Chouteau