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 <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.
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 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!
>
Regards,
--
Fabien Chouteau
