qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alexander Graf
Subject: Re: [Qemu-devel] [RESEND][PATCH] booke timers
Date: Tue, 6 Sep 2011 21:33:38 +0200


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

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

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

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

> +    }
> +
> +    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 :)

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

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

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

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

Very nice patch however! Thanks a lot for sitting down and fixing the timer 
mess on booke that we currently have. 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?


Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]