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 <chout...@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 <chout...@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.
>
>
>>
>>> + } 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? :)
>
>
>>> + }
>>> }
>>>
>>> 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?
>
>>
>>> + 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? :)
>
>>
>>> + }
>>> +
>>> + 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.
>
>
>>> +
>>> + 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.
>
>>> +}
>>> +
>>> +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.
>
>>> +}
>>> +
>>> +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
>
>> Since you definitely know your way around booke timekeeping code, could you
>> please also take a look at the decr patches on kvm-ppc@vger 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!
Alex
