* [Qemu-devel] [RESEND][PATCH] booke timers
@ 2011-09-01 8:20 Fabien Chouteau
2011-09-06 19:33 ` Alexander Graf
0 siblings, 1 reply; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-01 8:20 UTC (permalink / raw)
To: qemu-devel; +Cc: scottwood, agraf
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 <chouteau@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;
+ } 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);
+ }
}
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) {
+ 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)++;
+ }
+
+ 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);
+ }
+
+ if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
+ /* Auto Reload */
+ cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
+ }
+}
+
+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);
+ }
+
+ 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);
+ }
+}
+
+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);
+ }
+}
+
+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,
};
/*****************************************************************************/
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 9ea193d..ca47569 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -3253,6 +3253,9 @@ static void init_proc_401 (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* PowerPC 401x2 */
@@ -3291,6 +3294,9 @@ static void init_proc_401x2 (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* PowerPC 401x3 */
@@ -3324,6 +3330,9 @@ static void init_proc_401x3 (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* IOP480 */
@@ -3362,6 +3371,9 @@ static void init_proc_IOP480 (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(8, 12, 16, 20);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* PowerPC 403 */
@@ -3392,6 +3404,9 @@ static void init_proc_403 (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(8, 12, 16, 20);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* PowerPC 403 GCX */
@@ -3442,6 +3457,9 @@ static void init_proc_403GCX (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(8, 12, 16, 20);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* PowerPC 405 */
@@ -3491,6 +3509,9 @@ static void init_proc_405 (CPUPPCState *env)
env->icache_line_size = 32;
/* Allocate hardware IRQ controller */
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(8, 12, 16, 20);
+ SET_WDT_PERIOD(16, 20, 24, 28);
}
/* PowerPC 440 EP */
@@ -3573,6 +3594,9 @@ static void init_proc_440EP (CPUPPCState *env)
env->dcache_line_size = 32;
env->icache_line_size = 32;
/* XXX: TODO: allocate internal IRQ controller */
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(20, 24, 28, 32);
}
/* PowerPC 440 GP */
@@ -3637,6 +3661,9 @@ static void init_proc_440GP (CPUPPCState *env)
env->dcache_line_size = 32;
env->icache_line_size = 32;
/* XXX: TODO: allocate internal IRQ controller */
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(20, 24, 28, 32);
}
/* PowerPC 440x4 */
@@ -3701,6 +3728,9 @@ static void init_proc_440x4 (CPUPPCState *env)
env->dcache_line_size = 32;
env->icache_line_size = 32;
/* XXX: TODO: allocate internal IRQ controller */
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(20, 24, 28, 32);
}
/* PowerPC 440x5 */
@@ -3782,6 +3812,9 @@ static void init_proc_440x5 (CPUPPCState *env)
env->dcache_line_size = 32;
env->icache_line_size = 32;
ppc40x_irq_init(env);
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(20, 24, 28, 32);
}
/* PowerPC 460 (guessed) */
@@ -3870,6 +3903,9 @@ static void init_proc_460 (CPUPPCState *env)
env->dcache_line_size = 32;
env->icache_line_size = 32;
/* XXX: TODO: allocate internal IRQ controller */
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(20, 24, 28, 32);
}
/* PowerPC 460F (guessed) */
@@ -3961,6 +3997,9 @@ static void init_proc_460F (CPUPPCState *env)
env->dcache_line_size = 32;
env->icache_line_size = 32;
/* XXX: TODO: allocate internal IRQ controller */
+
+ SET_FIT_PERIOD(12, 16, 20, 24);
+ SET_WDT_PERIOD(20, 24, 28, 32);
}
/* Freescale 5xx cores (aka RCPU) */
@@ -4330,7 +4369,7 @@ static void init_proc_e300 (CPUPPCState *env)
PPC_CACHE | PPC_CACHE_LOCK | PPC_CACHE_ICBI | \
PPC_CACHE_DCBZ | PPC_CACHE_DCBA | \
PPC_MEM_TLBSYNC | PPC_TLBIVAX)
-#define POWERPC_INSNS2_e500v1 (PPC2_BOOKE206)
+#define POWERPC_INSNS2_e500v1 (PPC2_BOOKE206 | PPC2_E500)
#define POWERPC_MSRM_e500v1 (0x000000000606FF30ULL)
#define POWERPC_MMU_e500v1 (POWERPC_MMU_BOOKE206)
#define POWERPC_EXCP_e500v1 (POWERPC_EXCP_BOOKE)
@@ -4349,7 +4388,7 @@ static void init_proc_e300 (CPUPPCState *env)
PPC_CACHE | PPC_CACHE_LOCK | PPC_CACHE_ICBI | \
PPC_CACHE_DCBZ | PPC_CACHE_DCBA | \
PPC_MEM_TLBSYNC | PPC_TLBIVAX)
-#define POWERPC_INSNS2_e500v2 (PPC2_BOOKE206)
+#define POWERPC_INSNS2_e500v2 (PPC2_BOOKE206 | PPC2_E500)
#define POWERPC_MSRM_e500v2 (0x000000000606FF30ULL)
#define POWERPC_MMU_e500v2 (POWERPC_MMU_BOOKE206)
#define POWERPC_EXCP_e500v2 (POWERPC_EXCP_BOOKE)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-01 8:20 [Qemu-devel] [RESEND][PATCH] booke timers Fabien Chouteau
@ 2011-09-06 19:33 ` Alexander Graf
2011-09-07 14:41 ` Fabien Chouteau
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Graf @ 2011-09-06 19:33 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: scottwood, qemu-devel
Am 01.09.2011 um 10:20 schrieb Fabien Chouteau <chouteau@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 <chouteau@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 :)
> + } 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 kvm-ppc@vger that Liu posted recently?
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-06 19:33 ` Alexander Graf
@ 2011-09-07 14:41 ` Fabien Chouteau
2011-09-07 19:59 ` Alexander Graf
0 siblings, 1 reply; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-07 14:41 UTC (permalink / raw)
To: Alexander Graf; +Cc: scottwood, qemu-devel
On 06/09/2011 21:33, Alexander Graf wrote:
>
>
> Am 01.09.2011 um 10:20 schrieb Fabien Chouteau <chouteau@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 <chouteau@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.
>
>> + } 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?
>> + }
>> }
>>
>> 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.
>
>> + 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 :)
>
>> + }
>> +
>> + 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...
>> +
>> + 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.
>> +}
>> +
>> +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.
>> +}
>> +
>> +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 ;)
> 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?
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-07 14:41 ` Fabien Chouteau
@ 2011-09-07 19:59 ` Alexander Graf
2011-09-09 10:36 ` Fabien Chouteau
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Graf @ 2011-09-07 19:59 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
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 <chouteau@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 <chouteau@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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-07 19:59 ` Alexander Graf
@ 2011-09-09 10:36 ` Fabien Chouteau
2011-09-09 10:55 ` Alexander Graf
0 siblings, 1 reply; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-09 10:36 UTC (permalink / raw)
To: Alexander Graf; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
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 <chouteau@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 <chouteau@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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-09 10:36 ` Fabien Chouteau
@ 2011-09-09 10:55 ` Alexander Graf
2011-09-09 13:27 ` Fabien Chouteau
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Graf @ 2011-09-09 10:55 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
[-- Attachment #1: Type: text/plain, Size: 16744 bytes --]
On 09.09.2011, at 12:36, Fabien Chouteau wrote:
> On 07/09/2011 21:59, Alexander Graf wrote:
>>
>> On 07.09.2011, at 16:41, Fabien Chouteau wrote:
>>
>>> On 06/09/2011 21:33, Alexander Graf wrote:
>>>>
>>>>
>>>> Am 01.09.2011 um 10:20 schrieb Fabien Chouteau <chouteau@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 <chouteau@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...
I guess at the end of the day we need to have the CPU initialize its timers. They are tightly coupled with the CPU anyways. For now, we can leave the instantiation as is though.
>
>
>>>
>>>
>>>>
>>>>> + } else {
>>>>> decr = -muldiv64(-diff, tb_env->decr_freq, get_ticks_per_sec());
>>>>> + }
>>>>> LOG_TB("%s: %08" PRIx32 "\n", __func__, decr);
>>>>>
>>>>> return decr;
>>>>> @@ -678,18 +664,23 @@ static void __cpu_ppc_store_decr (CPUState *env, uint64_t *nextp,
>>>>> decr, value);
>>>>> now = qemu_get_clock_ns(vm_clock);
>>>>> next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
>>>>> - if (is_excp)
>>>>> + if (is_excp) {
>>>>> next += *nextp - now;
>>>>> - if (next == now)
>>>>> + }
>>>>> + if (next == now) {
>>>>> next++;
>>>>> + }
>>>>> *nextp = next;
>>>>> /* Adjust timer */
>>>>> qemu_mod_timer(timer, next);
>>>>> /* If we set a negative value and the decrementer was positive,
>>>>> - * raise an exception.
>>>>> + * raise an exception (not for booke).
>>>>> */
>>>>> - if ((value & 0x80000000) && !(decr & 0x80000000))
>>>>> + if (!(env->insns_flags & PPC_BOOKE)
>>>>> + && (value & 0x80000000)
>>>>> + && !(decr & 0x80000000)) {
>>>>> (*raise_excp)(env);
>>>>
>>>> Please make this a separate flag too. IIRC this is not unified behavior on all ppc CPUs, not even all server type ones.
>>>>
>>>
>>> This come from a comment by Scott (CC'd), I don't know much about it. Can you
>>> help me to find a good name for this feature?
>>
>> PPC_DECR_TRIGGER_ON_NEGATIVE? :)
>>
>
> After a quick check, PPC_DECR_UNDERFLOW_TRIGGERED seems more appropriate.
Good :).
[...]
>>>>> +/* Return the location of the bit of time base at which the FIT will raise an
>>>>> + interrupt */
>>>>> +static uint8_t booke_get_fit_target(CPUState *env)
>>>>> +{
>>>>> + uint8_t fp = (env->spr[SPR_BOOKE_TCR] & TCR_FP_MASK) >> TCR_FP_SHIFT;
>>>>> +
>>>>> + /* Only for e500 */
>>>>> + if (env->insns_flags2 & PPC2_E500) {
>>>>
>>>> Same here again :). Do you have test cases for fit? IIRC Linux doesn't use it.
>>>
>>> VxWorks uses it.
>>
>> If even remotely possible, I'd really like to have something in my portfolio
>> of guest code to test this case - especially given that KVM implements its
>> own timers. I assume your binary is non-redistributable?
>
> No I can't give you the binary. Maybe the best solution is to write a simple
> test case from scratch.
Yeah, I'll poke and see if someone already has something there.
>
>
>>>
>>>>
>>>>> + uint32_t fpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_FPEXT_MASK)
>>>>> + >> TCR_E500_FPEXT_SHIFT;
>>>>> + fp = 63 - (fp | fpext << 2);
>>>>> + } else {
>>>>> + fp = env->fit_period[fp];
>>>>> + }
>>>>> +
>>>>> + return fp;
>>>>> +}
>>>>> +
>>>>> +/* Return the location of the bit of time base at which the WDT will raise an
>>>>> + interrupt */
>>>>> +static uint8_t booke_get_wdt_target(CPUState *env)
>>>>> +{
>>>>> + uint8_t wp = (env->spr[SPR_BOOKE_TCR] & TCR_WP_MASK) >> TCR_WP_SHIFT;
>>>>> +
>>>>> + /* Only for e500 */
>>>>> + if (env->insns_flags2 & PPC2_E500) {
>>>>> + uint32_t wpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_WPEXT_MASK)
>>>>> + >> TCR_E500_WPEXT_SHIFT;
>>>>> + wp = 63 - (wp | wpext << 2);
>>>>> + } else {
>>>>> + wp = env->wdt_period[wp];
>>>>> + }
>>>>> +
>>>>> + return wp;
>>>>> +}
>>>>> +
>>>>> +static void booke_update_fixed_timer(CPUState *env,
>>>>> + uint8_t target_bit,
>>>>> + uint64_t *next,
>>>>> + struct QEMUTimer *timer)
>>>>> +{
>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>> + uint64_t lapse;
>>>>> + uint64_t tb;
>>>>> + uint64_t period = 1 << (target_bit + 1);
>>>>> + uint64_t now;
>>>>> +
>>>>> + now = qemu_get_clock_ns(vm_clock);
>>>>> + tb = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
>>>>> +
>>>>> + lapse = period - ((tb - (1 << target_bit)) & (period - 1));
>>>>> +
>>>>> + *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq);
>>>>> +
>>>>> + if (*next == now) {
>>>>> + (*next)++;
>>>>
>>>> Huh? If we hit the timer we don't fire it?
>>>
>>> Yes we do, but one nanosecond later :)
>>
>> Well, why trigger a timer when we can just as well call the callback immediately? :)
>>
>
> This come from ppc.c, QEMUTimer is kind of a private type so we don't have
> access to the callback.
Oh, I see. Please just add that as an XXX comment so we can resolve it when we get around to it.
>
>>>
>>>>
>>>>> + }
>>>>> +
>>>>> + qemu_mod_timer(timer, *next);
>>>>> +}
>>>>> +
>>>>> +static void booke_decr_cb(void *opaque)
>>>>> +{
>>>>> + CPUState *env;
>>>>> + ppc_tb_t *tb_env;
>>>>> + booke_timer_t *booke_timer;
>>>>> +
>>>>> + env = opaque;
>>>>> + tb_env = env->tb_env;
>>>>> + booke_timer = tb_env->opaque;
>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>> + }
>>>>
>>>> You will need this more often - also for the TCR write case - so please put
>>>> the 3 lines above into a separate function and just call that here :)
>>>
>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>
>> Sure, we have 3 different tests for the 3 different bits we can potentially set in TCR. The check always ends up being the same though:
>>
>> if (TSR & bit && TCR & bit)
>> set_irq(irq_for_bit);
>>
>> Most device emulators have a simple function for this called "update_irq" that checks for all the bits and sets the respective irq lines.
>>
>
> I know but we have two cases:
>
> - Timer hit: we check DIE in TCR
> - Rising edge of DIE in TCR (from user): check if DIS is set
>
> I don't think we can have a good generic function for this, and I don't
> forecast any improvement in code readability.
update_decr_irq() {
if (TSR.DIS && TCR.DIE) {
set_irq(DECR);
} else {
unset_irq(DECR);
}
}
Timer hit:
TSR |= DIS;
update_decr_irq();
Setting TCR:
TCR |= DIE;
update_decr_irq();
Or am I misunderstanding the conditions under which the irq actually triggers? TCR.DIE is only the interrupt enabled flag - the timer can still hit nevertheless. The level of the interrupt is determined by TSR.DIR which is what the timer sets when it hits. Unless I completely misread the spec, an interrupt occurs when both of them are true. So all we need to do is have that check and run it every time we change a value in TSR or TCR.
>
>>>
>>>
>>>>> +
>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
>>>>> + /* Auto Reload */
>>>>> + cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
>>>>> + }
>>>>
>>>> Ah nice - never seen this one used. Do you have a test case?
>>>>
>>>
>>> VxWorks :)
>>>
>>>
>>>>> +}
>>>>> +
>>>>> +static void booke_fit_cb(void *opaque)
>>>>> +{
>>>>> + CPUState *env;
>>>>> + ppc_tb_t *tb_env;
>>>>> + booke_timer_t *booke_timer;
>>>>> +
>>>>> + env = opaque;
>>>>> + tb_env = env->tb_env;
>>>>> + booke_timer = tb_env->opaque;
>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_FIS;
>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_FIE) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>> + }
>>>>
>>>> Same as above :)
>>>>
>>>>> +
>>>>> + booke_update_fixed_timer(env,
>>>>> + booke_get_fit_target(env),
>>>>> + &booke_timer->fit_next,
>>>>> + booke_timer->fit_timer);
>>>>> +}
>>>>> +
>>>>> +static void booke_wdt_cb(void *opaque)
>>>>> +{
>>>>> + CPUState *env;
>>>>> + ppc_tb_t *tb_env;
>>>>> + booke_timer_t *booke_timer;
>>>>> +
>>>>> + env = opaque;
>>>>> + tb_env = env->tb_env;
>>>>> + booke_timer = tb_env->opaque;
>>>>> +
>>>>> + /* TODO: There's lots of complicated stuff to do here */
>>>>> +
>>>>> + booke_update_fixed_timer(env,
>>>>> + booke_get_wdt_target(env),
>>>>> + &booke_timer->wdt_next,
>>>>> + booke_timer->wdt_timer);
>>>>> +}
>>>>> +
>>>>> +void store_booke_tsr(CPUState *env, target_ulong val)
>>>>> +{
>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>> + booke_timer_t *booke_timer;
>>>>> +
>>>>> + booke_timer = tb_env->opaque;
>>>>> +
>>>>> + env->spr[SPR_BOOKE_TSR] &= ~val;
>>>>> +
>>>>> + if (val & TSR_DIS) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 0);
>>>>> + }
>>>>> +
>>>>> + if (val & TSR_FIS) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 0);
>>>>> + }
>>>>> +
>>>>> + if (val & TSR_WIS) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_WDT, 0);
>>>>> + }
>>>>
>>>> Why do you need the above? Shouldn't they still be active if the guest didn't
>>>> reset the bit? This is probably hiding a bug in the interrupt delivery
>>>> mechanism automatically unmasking an irq bit when it's delivered, right?
>>>
>>> They are active until the bit is cleared by user, and TSR is a write-1-to-clear
>>> register.
>>
>> Yes, that's what I mean. There shouldn't be a need to set the irq again because it should still be active. These interrupts are level based.
>>
>
> I feel like there's a misunderstanding here. I do not set the interrupts I
> clear them.
Oh. Yes, I misread that. Sorry :). That indeed does make a lot of sense. I can however be folded in with the normal "is DECR active right now" check.
>
>
>>>
>>>>> +}
>>>>> +
>>>>> +void store_booke_tcr(CPUState *env, target_ulong val)
>>>>> +{
>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>> + booke_timer_t *booke_timer = tb_env->opaque;
>>>>> +
>>>>> + tb_env = env->tb_env;
>>>>> + env->spr[SPR_BOOKE_TCR] = val;
>>>>> +
>>>>> + booke_update_fixed_timer(env,
>>>>> + booke_get_fit_target(env),
>>>>> + &booke_timer->fit_next,
>>>>> + booke_timer->fit_timer);
>>>>> +
>>>>> + booke_update_fixed_timer(env,
>>>>> + booke_get_wdt_target(env),
>>>>> + &booke_timer->wdt_next,
>>>>> + booke_timer->wdt_timer);
>>>>> +
>>>>> + if (val & TCR_DIE && env->spr[SPR_BOOKE_TSR] & TSR_DIS) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>> + }
>>>>> +
>>>>> + if (val & TCR_FIE && env->spr[SPR_BOOKE_TSR] & TSR_FIS) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>> + }
>>>>> +
>>>>> + if (val & TCR_WIE && env->spr[SPR_BOOKE_TSR] & TSR_WIS) {
>>>>> + ppc_set_irq(env, PPC_INTERRUPT_WDT, 1);
>>>>> + }
>>>>
>>>> Here is the second user of the checks. It really does make sense to only have
>>>> them in a single spot, so that ever ppc_set_irq(DECR) always goes through the
>>>> TSR and TCR checks for example :).
>>>
>>> ...here we test DIE in TCR and DIS in TSR.
>>
>> Yes, both of which are basically the prerequisites for actually triggering the internal DECR interrupt line.
>>
>
> Not exactly, see above.
[...]
>>>
>>>> Very nice patch however! Thanks a lot for sitting down and fixing the timer
>>>> mess on booke that we currently have.
>>>
>>> You are welcome, It's my thank you gift for the MMU ;)
>>
>> Haha thanks :). So you're going to fix the MPIC mess for me implementing SMP support? :D
>>
>
> I'll see, but I'm not sure you deserve it :)
Probably not :). What else is on your todo list to get your awesome guest running? :)
Alex
[-- Attachment #2: Type: text/html, Size: 72992 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-09 10:55 ` Alexander Graf
@ 2011-09-09 13:27 ` Fabien Chouteau
2011-09-09 13:46 ` Alexander Graf
0 siblings, 1 reply; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-09 13:27 UTC (permalink / raw)
To: Alexander Graf; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
On 09/09/2011 12:55, Alexander Graf wrote:
>
> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>
>> On 07/09/2011 21:59, Alexander Graf wrote:
>>>
>>> On 07.09.2011, at 16:41, Fabien Chouteau wrote:
>>>
>>>> On 06/09/2011 21:33, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> Am 01.09.2011 um 10:20 schrieb Fabien Chouteau <chouteau@adacore.com <mailto:chouteau@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 <chouteau@adacore.com <mailto:chouteau@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...
>
> I guess at the end of the day we need to have the CPU initialize its timers. They are tightly coupled with the CPU anyways. For now, we can leave the instantiation as is though.
>
>>
>>
>>>>
>>>>
>>>>>
>>>>>> + } else {
>>>>>> decr = -muldiv64(-diff, tb_env->decr_freq, get_ticks_per_sec());
>>>>>> + }
>>>>>> LOG_TB("%s: %08" PRIx32 "\n", __func__, decr);
>>>>>>
>>>>>> return decr;
>>>>>> @@ -678,18 +664,23 @@ static void __cpu_ppc_store_decr (CPUState *env, uint64_t *nextp,
>>>>>> decr, value);
>>>>>> now = qemu_get_clock_ns(vm_clock);
>>>>>> next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
>>>>>> - if (is_excp)
>>>>>> + if (is_excp) {
>>>>>> next += *nextp - now;
>>>>>> - if (next == now)
>>>>>> + }
>>>>>> + if (next == now) {
>>>>>> next++;
>>>>>> + }
>>>>>> *nextp = next;
>>>>>> /* Adjust timer */
>>>>>> qemu_mod_timer(timer, next);
>>>>>> /* If we set a negative value and the decrementer was positive,
>>>>>> - * raise an exception.
>>>>>> + * raise an exception (not for booke).
>>>>>> */
>>>>>> - if ((value & 0x80000000) && !(decr & 0x80000000))
>>>>>> + if (!(env->insns_flags & PPC_BOOKE)
>>>>>> + && (value & 0x80000000)
>>>>>> + && !(decr & 0x80000000)) {
>>>>>> (*raise_excp)(env);
>>>>>
>>>>> Please make this a separate flag too. IIRC this is not unified behavior on all ppc CPUs, not even all server type ones.
>>>>>
>>>>
>>>> This come from a comment by Scott (CC'd), I don't know much about it. Can you
>>>> help me to find a good name for this feature?
>>>
>>> PPC_DECR_TRIGGER_ON_NEGATIVE? :)
>>>
>>
>> After a quick check, PPC_DECR_UNDERFLOW_TRIGGERED seems more appropriate.
>
> Good :).
>
> [...]
>
>>>>>> +/* Return the location of the bit of time base at which the FIT will raise an
>>>>>> + interrupt */
>>>>>> +static uint8_t booke_get_fit_target(CPUState *env)
>>>>>> +{
>>>>>> + uint8_t fp = (env->spr[SPR_BOOKE_TCR] & TCR_FP_MASK) >> TCR_FP_SHIFT;
>>>>>> +
>>>>>> + /* Only for e500 */
>>>>>> + if (env->insns_flags2 & PPC2_E500) {
>>>>>
>>>>> Same here again :). Do you have test cases for fit? IIRC Linux doesn't use it.
>>>>
>>>> VxWorks uses it.
>>>
>>> If even remotely possible, I'd really like to have something in my portfolio
>>> of guest code to test this case - especially given that KVM implements its
>>> own timers. I assume your binary is non-redistributable?
>>
>> No I can't give you the binary. Maybe the best solution is to write a simple
>> test case from scratch.
>
> Yeah, I'll poke and see if someone already has something there.
>
>>
>>
>>>>
>>>>>
>>>>>> + uint32_t fpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_FPEXT_MASK)
>>>>>> + >> TCR_E500_FPEXT_SHIFT;
>>>>>> + fp = 63 - (fp | fpext << 2);
>>>>>> + } else {
>>>>>> + fp = env->fit_period[fp];
>>>>>> + }
>>>>>> +
>>>>>> + return fp;
>>>>>> +}
>>>>>> +
>>>>>> +/* Return the location of the bit of time base at which the WDT will raise an
>>>>>> + interrupt */
>>>>>> +static uint8_t booke_get_wdt_target(CPUState *env)
>>>>>> +{
>>>>>> + uint8_t wp = (env->spr[SPR_BOOKE_TCR] & TCR_WP_MASK) >> TCR_WP_SHIFT;
>>>>>> +
>>>>>> + /* Only for e500 */
>>>>>> + if (env->insns_flags2 & PPC2_E500) {
>>>>>> + uint32_t wpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_WPEXT_MASK)
>>>>>> + >> TCR_E500_WPEXT_SHIFT;
>>>>>> + wp = 63 - (wp | wpext << 2);
>>>>>> + } else {
>>>>>> + wp = env->wdt_period[wp];
>>>>>> + }
>>>>>> +
>>>>>> + return wp;
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_update_fixed_timer(CPUState *env,
>>>>>> + uint8_t target_bit,
>>>>>> + uint64_t *next,
>>>>>> + struct QEMUTimer *timer)
>>>>>> +{
>>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>>> + uint64_t lapse;
>>>>>> + uint64_t tb;
>>>>>> + uint64_t period = 1 << (target_bit + 1);
>>>>>> + uint64_t now;
>>>>>> +
>>>>>> + now = qemu_get_clock_ns(vm_clock);
>>>>>> + tb = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
>>>>>> +
>>>>>> + lapse = period - ((tb - (1 << target_bit)) & (period - 1));
>>>>>> +
>>>>>> + *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq);
>>>>>> +
>>>>>> + if (*next == now) {
>>>>>> + (*next)++;
>>>>>
>>>>> Huh? If we hit the timer we don't fire it?
>>>>
>>>> Yes we do, but one nanosecond later :)
>>>
>>> Well, why trigger a timer when we can just as well call the callback immediately? :)
>>>
>>
>> This come from ppc.c, QEMUTimer is kind of a private type so we don't have
>> access to the callback.
>
> Oh, I see. Please just add that as an XXX comment so we can resolve it when we get around to it.
>
>>
>>>>
>>>>>
>>>>>> + }
>>>>>> +
>>>>>> + qemu_mod_timer(timer, *next);
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_decr_cb(void *opaque)
>>>>>> +{
>>>>>> + CPUState *env;
>>>>>> + ppc_tb_t *tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + env = opaque;
>>>>>> + tb_env = env->tb_env;
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>> + }
>>>>>
>>>>> You will need this more often - also for the TCR write case - so please put
>>>>> the 3 lines above into a separate function and just call that here :)
>>>>
>>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>>
>>> Sure, we have 3 different tests for the 3 different bits we can potentially set in TCR. The check always ends up being the same though:
>>>
>>> if (TSR & bit && TCR & bit)
>>> set_irq(irq_for_bit);
>>>
>>> Most device emulators have a simple function for this called "update_irq" that checks for all the bits and sets the respective irq lines.
>>>
>>
>> I know but we have two cases:
>>
>> - Timer hit: we check DIE in TCR
>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>
>> I don't think we can have a good generic function for this, and I don't
>> forecast any improvement in code readability.
>
> update_decr_irq() {
> if (TSR.DIS && TCR.DIE) {
> set_irq(DECR);
> } else {
> unset_irq(DECR);
> }
> }
>
> Timer hit:
>
> TSR |= DIS;
> update_decr_irq();
>
> Setting TCR:
>
> TCR |= DIE;
> update_decr_irq();
>
> Or am I misunderstanding the conditions under which the irq actually
> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
> is what the timer sets when it hits. Unless I completely misread the spec, an
> interrupt occurs when both of them are true. So all we need to do is have
> that check and run it every time we change a value in TSR or TCR.
Well OK, this can work to trigger the interrupts, not to clear them though.
And it will call ppc_set_irq when it's not required.
>>
>>>>
>>>>
>>>>>> +
>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
>>>>>> + /* Auto Reload */
>>>>>> + cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
>>>>>> + }
>>>>>
>>>>> Ah nice - never seen this one used. Do you have a test case?
>>>>>
>>>>
>>>> VxWorks :)
>>>>
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_fit_cb(void *opaque)
>>>>>> +{
>>>>>> + CPUState *env;
>>>>>> + ppc_tb_t *tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + env = opaque;
>>>>>> + tb_env = env->tb_env;
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_FIS;
>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_FIE) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>>> + }
>>>>>
>>>>> Same as above :)
>>>>>
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_fit_target(env),
>>>>>> + &booke_timer->fit_next,
>>>>>> + booke_timer->fit_timer);
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_wdt_cb(void *opaque)
>>>>>> +{
>>>>>> + CPUState *env;
>>>>>> + ppc_tb_t *tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + env = opaque;
>>>>>> + tb_env = env->tb_env;
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> + /* TODO: There's lots of complicated stuff to do here */
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_wdt_target(env),
>>>>>> + &booke_timer->wdt_next,
>>>>>> + booke_timer->wdt_timer);
>>>>>> +}
>>>>>> +
>>>>>> +void store_booke_tsr(CPUState *env, target_ulong val)
>>>>>> +{
>>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> + env->spr[SPR_BOOKE_TSR] &= ~val;
>>>>>> +
>>>>>> + if (val & TSR_DIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 0);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TSR_FIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 0);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TSR_WIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_WDT, 0);
>>>>>> + }
>>>>>
>>>>> Why do you need the above? Shouldn't they still be active if the guest didn't
>>>>> reset the bit? This is probably hiding a bug in the interrupt delivery
>>>>> mechanism automatically unmasking an irq bit when it's delivered, right?
>>>>
>>>> They are active until the bit is cleared by user, and TSR is a write-1-to-clear
>>>> register.
>>>
>>> Yes, that's what I mean. There shouldn't be a need to set the irq again because it should still be active. These interrupts are level based.
>>>
>>
>> I feel like there's a misunderstanding here. I do not set the interrupts I
>> clear them.
>
> Oh. Yes, I misread that. Sorry :). That indeed does make a lot of sense. I can however be folded in with the normal "is DECR active right now" check.
>
>>
>>
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +void store_booke_tcr(CPUState *env, target_ulong val)
>>>>>> +{
>>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>>> + booke_timer_t *booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> + tb_env = env->tb_env;
>>>>>> + env->spr[SPR_BOOKE_TCR] = val;
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_fit_target(env),
>>>>>> + &booke_timer->fit_next,
>>>>>> + booke_timer->fit_timer);
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_wdt_target(env),
>>>>>> + &booke_timer->wdt_next,
>>>>>> + booke_timer->wdt_timer);
>>>>>> +
>>>>>> + if (val & TCR_DIE && env->spr[SPR_BOOKE_TSR] & TSR_DIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TCR_FIE && env->spr[SPR_BOOKE_TSR] & TSR_FIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TCR_WIE && env->spr[SPR_BOOKE_TSR] & TSR_WIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_WDT, 1);
>>>>>> + }
>>>>>
>>>>> Here is the second user of the checks. It really does make sense to only have
>>>>> them in a single spot, so that ever ppc_set_irq(DECR) always goes through the
>>>>> TSR and TCR checks for example :).
>>>>
>>>> ...here we test DIE in TCR and DIS in TSR.
>>>
>>> Yes, both of which are basically the prerequisites for actually triggering the internal DECR interrupt line.
>>>
>>
>> Not exactly, see above.
>
> [...]
>
>>>>
>>>>> Very nice patch however! Thanks a lot for sitting down and fixing the timer
>>>>> mess on booke that we currently have.
>>>>
>>>> You are welcome, It's my thank you gift for the MMU ;)
>>>
>>> Haha thanks :). So you're going to fix the MPIC mess for me implementing SMP support? :D
>>>
>>
>> I'll see, but I'm not sure you deserve it :)
>
> Probably not :). What else is on your todo list to get your awesome guest running? :)
>
Actually it works pretty well with VxWorks already, I still have to clean up
few things and play with this new memory API to implement the CCSR
reallocation.
I've tried to look at Liu's patch, but I really don't know nothing about KVM so
I won't be able to make any valuable comment...
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-09 13:27 ` Fabien Chouteau
@ 2011-09-09 13:46 ` Alexander Graf
2011-09-09 14:22 ` Fabien Chouteau
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Graf @ 2011-09-09 13:46 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
[-- Attachment #1: Type: text/plain, Size: 4023 bytes --]
On 09.09.2011, at 15:27, Fabien Chouteau wrote:
> On 09/09/2011 12:55, Alexander Graf wrote:
>>
>> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>>>>>
>>>>>>
>>>>>>> + }
>>>>>>> +
>>>>>>> + qemu_mod_timer(timer, *next);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void booke_decr_cb(void *opaque)
>>>>>>> +{
>>>>>>> + CPUState *env;
>>>>>>> + ppc_tb_t *tb_env;
>>>>>>> + booke_timer_t *booke_timer;
>>>>>>> +
>>>>>>> + env = opaque;
>>>>>>> + tb_env = env->tb_env;
>>>>>>> + booke_timer = tb_env->opaque;
>>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>>> + }
>>>>>>
>>>>>> You will need this more often - also for the TCR write case - so please put
>>>>>> the 3 lines above into a separate function and just call that here :)
>>>>>
>>>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>>>
>>>> Sure, we have 3 different tests for the 3 different bits we can potentially set in TCR. The check always ends up being the same though:
>>>>
>>>> if (TSR & bit && TCR & bit)
>>>> set_irq(irq_for_bit);
>>>>
>>>> Most device emulators have a simple function for this called "update_irq" that checks for all the bits and sets the respective irq lines.
>>>>
>>>
>>> I know but we have two cases:
>>>
>>> - Timer hit: we check DIE in TCR
>>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>>
>>> I don't think we can have a good generic function for this, and I don't
>>> forecast any improvement in code readability.
>>
>> update_decr_irq() {
>> if (TSR.DIS && TCR.DIE) {
>> set_irq(DECR);
>> } else {
>> unset_irq(DECR);
>> }
>> }
>>
>> Timer hit:
>>
>> TSR |= DIS;
>> update_decr_irq();
>>
>> Setting TCR:
>>
>> TCR |= DIE;
>> update_decr_irq();
>>
>> Or am I misunderstanding the conditions under which the irq actually
>> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
>> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
>> is what the timer sets when it hits. Unless I completely misread the spec, an
>> interrupt occurs when both of them are true. So all we need to do is have
>> that check and run it every time we change a value in TSR or TCR.
>
> Well OK, this can work to trigger the interrupts, not to clear them though.
> And it will call ppc_set_irq when it's not required.
Calling ppc_set_irq shouldn't be an issue - at the end of the day it only does a simple OR operation. Unsetting should be pretty obvious too though, no? When either TSR.DIS or TCR.DIE are not set, the interrupt line is low.
The benefit we get from making the interrupt logic implicit at a single place is that we're getting rid of a full category of potential bugs where we behave subtly differently depending on the conditions we were in before. This solution is a big hammer, yes, but it's proven pretty useful in QEMU so far :).
>>
>> [...]
>>
>>>>>
>>>>>> Very nice patch however! Thanks a lot for sitting down and fixing the timer
>>>>>> mess on booke that we currently have.
>>>>>
>>>>> You are welcome, It's my thank you gift for the MMU ;)
>>>>
>>>> Haha thanks :). So you're going to fix the MPIC mess for me implementing SMP support? :D
>>>>
>>>
>>> I'll see, but I'm not sure you deserve it :)
>>
>> Probably not :). What else is on your todo list to get your awesome guest running? :)
>>
>
> Actually it works pretty well with VxWorks already, I still have to clean up
> few things and play with this new memory API to implement the CCSR
> reallocation.
Ah, cool :). Great to hear!
> I've tried to look at Liu's patch, but I really don't know nothing about KVM so
> I won't be able to make any valuable comment...
Ah, too bad. Well, thanks for looking at it nevertheless! If you see something obvious, please just reply :)
Alex
[-- Attachment #2: Type: text/html, Size: 13864 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-09 13:46 ` Alexander Graf
@ 2011-09-09 14:22 ` Fabien Chouteau
2011-09-09 14:58 ` Alexander Graf
0 siblings, 1 reply; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-09 14:22 UTC (permalink / raw)
To: Alexander Graf; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
On 09/09/2011 15:46, Alexander Graf wrote:
>
> On 09.09.2011, at 15:27, Fabien Chouteau wrote:
>
>> On 09/09/2011 12:55, Alexander Graf wrote:
>>>
>>> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>>>>>>
>>>>>>>
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + qemu_mod_timer(timer, *next);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void booke_decr_cb(void *opaque)
>>>>>>>> +{
>>>>>>>> + CPUState *env;
>>>>>>>> + ppc_tb_t *tb_env;
>>>>>>>> + booke_timer_t *booke_timer;
>>>>>>>> +
>>>>>>>> + env = opaque;
>>>>>>>> + tb_env = env->tb_env;
>>>>>>>> + booke_timer = tb_env->opaque;
>>>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>>>> + }
>>>>>>>
>>>>>>> You will need this more often - also for the TCR write case - so please put
>>>>>>> the 3 lines above into a separate function and just call that here :)
>>>>>>
>>>>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>>>>
>>>>> Sure, we have 3 different tests for the 3 different bits we can potentially set in TCR. The check always ends up being the same though:
>>>>>
>>>>> if (TSR & bit && TCR & bit)
>>>>> set_irq(irq_for_bit);
>>>>>
>>>>> Most device emulators have a simple function for this called "update_irq" that checks for all the bits and sets the respective irq lines.
>>>>>
>>>>
>>>> I know but we have two cases:
>>>>
>>>> - Timer hit: we check DIE in TCR
>>>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>>>
>>>> I don't think we can have a good generic function for this, and I don't
>>>> forecast any improvement in code readability.
>>>
>>> update_decr_irq() {
>>> if (TSR.DIS && TCR.DIE) {
>>> set_irq(DECR);
>>> } else {
>>> unset_irq(DECR);
>>> }
>>> }
>>>
>>> Timer hit:
>>>
>>> TSR |= DIS;
>>> update_decr_irq();
>>>
>>> Setting TCR:
>>>
>>> TCR |= DIE;
>>> update_decr_irq();
>>>
>>> Or am I misunderstanding the conditions under which the irq actually
>>> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
>>> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
>>> is what the timer sets when it hits. Unless I completely misread the spec, an
>>> interrupt occurs when both of them are true. So all we need to do is have
>>> that check and run it every time we change a value in TSR or TCR.
>>
>> Well OK, this can work to trigger the interrupts, not to clear them though.
>> And it will call ppc_set_irq when it's not required.
>
> Calling ppc_set_irq shouldn't be an issue - at the end of the day it only
> does a simple OR operation. Unsetting should be pretty obvious too though,
> no? When either TSR.DIS or TCR.DIE are not set, the interrupt line is low.
>
if the interrupt is already set and you clear TCR.DIE, the interrupt has to
remain set. The only way to unset an interrupt is to clear the corresponding
bit in TSR (currently in store_booke_tsr).
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [RESEND][PATCH] booke timers
2011-09-09 14:22 ` Fabien Chouteau
@ 2011-09-09 14:58 ` Alexander Graf
2011-09-12 17:23 ` [Qemu-devel] [Qemu-ppc] " Scott Wood
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Graf @ 2011-09-09 14:58 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
On 09.09.2011, at 16:22, Fabien Chouteau wrote:
> On 09/09/2011 15:46, Alexander Graf wrote:
>>
>> On 09.09.2011, at 15:27, Fabien Chouteau wrote:
>>
>>> On 09/09/2011 12:55, Alexander Graf wrote:
>>>>
>>>> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + qemu_mod_timer(timer, *next);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void booke_decr_cb(void *opaque)
>>>>>>>>> +{
>>>>>>>>> + CPUState *env;
>>>>>>>>> + ppc_tb_t *tb_env;
>>>>>>>>> + booke_timer_t *booke_timer;
>>>>>>>>> +
>>>>>>>>> + env = opaque;
>>>>>>>>> + tb_env = env->tb_env;
>>>>>>>>> + booke_timer = tb_env->opaque;
>>>>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> You will need this more often - also for the TCR write case - so please put
>>>>>>>> the 3 lines above into a separate function and just call that here :)
>>>>>>>
>>>>>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>>>>>
>>>>>> Sure, we have 3 different tests for the 3 different bits we can potentially set in TCR. The check always ends up being the same though:
>>>>>>
>>>>>> if (TSR & bit && TCR & bit)
>>>>>> set_irq(irq_for_bit);
>>>>>>
>>>>>> Most device emulators have a simple function for this called "update_irq" that checks for all the bits and sets the respective irq lines.
>>>>>>
>>>>>
>>>>> I know but we have two cases:
>>>>>
>>>>> - Timer hit: we check DIE in TCR
>>>>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>>>>
>>>>> I don't think we can have a good generic function for this, and I don't
>>>>> forecast any improvement in code readability.
>>>>
>>>> update_decr_irq() {
>>>> if (TSR.DIS && TCR.DIE) {
>>>> set_irq(DECR);
>>>> } else {
>>>> unset_irq(DECR);
>>>> }
>>>> }
>>>>
>>>> Timer hit:
>>>>
>>>> TSR |= DIS;
>>>> update_decr_irq();
>>>>
>>>> Setting TCR:
>>>>
>>>> TCR |= DIE;
>>>> update_decr_irq();
>>>>
>>>> Or am I misunderstanding the conditions under which the irq actually
>>>> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
>>>> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
>>>> is what the timer sets when it hits. Unless I completely misread the spec, an
>>>> interrupt occurs when both of them are true. So all we need to do is have
>>>> that check and run it every time we change a value in TSR or TCR.
>>>
>>> Well OK, this can work to trigger the interrupts, not to clear them though.
>>> And it will call ppc_set_irq when it's not required.
>>
>> Calling ppc_set_irq shouldn't be an issue - at the end of the day it only
>> does a simple OR operation. Unsetting should be pretty obvious too though,
>> no? When either TSR.DIS or TCR.DIE are not set, the interrupt line is low.
>>
>
> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
> remain set. The only way to unset an interrupt is to clear the corresponding
> bit in TSR (currently in store_booke_tsr).
Are you sure? I see several things in the 2.06 spec:
TSR.DIS:
0 A Decrementer event has not occurred.
1 A Decrementer event has occurred.
when MSR.EE=1 and TCR.DIE=1, a Decrementer interrupt is taken.
TCR.DIE:
0 Disable Decrementer interrupt
1 Enable Decrementer interrupt
Decrement to one and stop on zero
If TCR.ARE=0, TSR.DIS is set to 1, the value 0x0000_0000 is then placed into the DEC, and the Decrementer stops decrementing.
A Decrementer interrupt occurs when no higher priority interrupt exists, a Decrementer exception exists, and the exception is enabled.
The interrupt is enabled by TCR.DIE=1 and MSR.EE=1.
See Section 7.6.12, “Decrementer Interrupt” on page 1021 for details of register behavior caused by the Decrementer inter- rupt.
7.6.12 Decrementer Interrupt
A Decrementer interrupt occurs when no higher priority interrupt exists (see Section 7.9 on page 1037), a Decrementer exception exists (TSR.DIS=1), and the exception is enabled. The interrupt is enabled by TCR.DIE=1 and MSR.EE=1. See Section 9.3 on page 1047.
Software is responsible for clearing the Decre- menter exception status prior to re-enabling the MSR.EE bit in order to avoid another redundant Decrementer interrupt. To clear the Decrementer exception, the interrupt handling routine must clear TSR.DIS. Clearing is done by writing a word to TSR using mtspr with a 1 in any bit position that is to be cleared and 0 in all other bit positions. The write-data to the TSR is not direct data, but a mask. A 1 causes the bit to be cleared, and a 0 has no effect.
To me that sounds as if the decrementer interrupt gets injected only when TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits stops the interrupt from being delivered.
Scott, can you please check up with the hardware guys if this is correct?
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-09 14:58 ` Alexander Graf
@ 2011-09-12 17:23 ` Scott Wood
2011-09-13 13:06 ` Fabien Chouteau
0 siblings, 1 reply; 17+ messages in thread
From: Scott Wood @ 2011-09-12 17:23 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc, qemu-devel@nongnu.org Developers, Fabien Chouteau
On 09/09/2011 09:58 AM, Alexander Graf wrote:
> On 09.09.2011, at 16:22, Fabien Chouteau wrote:
>> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
>> remain set. The only way to unset an interrupt is to clear the corresponding
>> bit in TSR (currently in store_booke_tsr).
>
> Are you sure? I see several things in the 2.06 spec:
[snip]
> To me that sounds as if the decrementer interrupt gets injected only
> when TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits
> stops the interrupt from being delivered.
>
> Scott, can you please check up with the hardware guys if this is correct?
This is how I've always understood it to work (assuming the interrupt
hasn't already been delivered, of course). Fabien, do you have real
hardware that you see behave the way you describe?
-Scott
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-12 17:23 ` [Qemu-devel] [Qemu-ppc] " Scott Wood
@ 2011-09-13 13:06 ` Fabien Chouteau
2011-09-13 13:08 ` Alexander Graf
0 siblings, 1 reply; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-13 13:06 UTC (permalink / raw)
To: Scott Wood; +Cc: qemu-ppc, Alexander Graf, qemu-devel@nongnu.org Developers
On 12/09/2011 19:23, Scott Wood wrote:
> On 09/09/2011 09:58 AM, Alexander Graf wrote:
>> On 09.09.2011, at 16:22, Fabien Chouteau wrote:
>>> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
>>> remain set. The only way to unset an interrupt is to clear the corresponding
>>> bit in TSR (currently in store_booke_tsr).
>>
>> Are you sure? I see several things in the 2.06 spec:
> [snip]
>> To me that sounds as if the decrementer interrupt gets injected only
>> when TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits
>> stops the interrupt from being delivered.
>>
>> Scott, can you please check up with the hardware guys if this is correct?
>
> This is how I've always understood it to work (assuming the interrupt
> hasn't already been delivered, of course). Fabien, do you have real
> hardware that you see behave the way you describe?
>
No I don't, it was just my understanding of Book-E documentation. I've tried
your solution (below) with VxWorks, and it works like a charm.
static void booke_update_irq(CPUState *env)
{
ppc_set_irq(env, PPC_INTERRUPT_DECR,
(env->spr[SPR_BOOKE_TSR] & TSR_DIS
&& env->spr[SPR_BOOKE_TCR] & TCR_DIE));
ppc_set_irq(env, PPC_INTERRUPT_WDT,
(env->spr[SPR_BOOKE_TSR] & TSR_WIS
&& env->spr[SPR_BOOKE_TCR] & TCR_WIE));
ppc_set_irq(env, PPC_INTERRUPT_FIT,
(env->spr[SPR_BOOKE_TSR] & TSR_FIS
&& env->spr[SPR_BOOKE_TCR] & TCR_FIE));
}
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-13 13:06 ` Fabien Chouteau
@ 2011-09-13 13:08 ` Alexander Graf
2011-09-13 13:13 ` Alexander Graf
2011-09-13 16:44 ` Scott Wood
0 siblings, 2 replies; 17+ messages in thread
From: Alexander Graf @ 2011-09-13 13:08 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
Fabien Chouteau wrote:
> On 12/09/2011 19:23, Scott Wood wrote:
>
>> On 09/09/2011 09:58 AM, Alexander Graf wrote:
>>
>>> On 09.09.2011, at 16:22, Fabien Chouteau wrote:
>>>
>>>> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
>>>> remain set. The only way to unset an interrupt is to clear the corresponding
>>>> bit in TSR (currently in store_booke_tsr).
>>>>
>>> Are you sure? I see several things in the 2.06 spec:
>>>
>> [snip]
>>
>>> To me that sounds as if the decrementer interrupt gets injected only
>>> when TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits
>>> stops the interrupt from being delivered.
>>>
>>> Scott, can you please check up with the hardware guys if this is correct?
>>>
>> This is how I've always understood it to work (assuming the interrupt
>> hasn't already been delivered, of course). Fabien, do you have real
>> hardware that you see behave the way you describe?
>>
>>
>
> No I don't, it was just my understanding of Book-E documentation. I've tried
> your solution (below) with VxWorks, and it works like a charm.
>
> static void booke_update_irq(CPUState *env)
> {
> ppc_set_irq(env, PPC_INTERRUPT_DECR,
> (env->spr[SPR_BOOKE_TSR] & TSR_DIS
> && env->spr[SPR_BOOKE_TCR] & TCR_DIE));
>
> ppc_set_irq(env, PPC_INTERRUPT_WDT,
> (env->spr[SPR_BOOKE_TSR] & TSR_WIS
> && env->spr[SPR_BOOKE_TCR] & TCR_WIE));
>
> ppc_set_irq(env, PPC_INTERRUPT_FIT,
> (env->spr[SPR_BOOKE_TSR] & TSR_FIS
> && env->spr[SPR_BOOKE_TCR] & TCR_FIE));
> }
>
Awesome! Please also check on MSR.EE and send a new patch then :)
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-13 13:08 ` Alexander Graf
@ 2011-09-13 13:13 ` Alexander Graf
2011-09-13 13:28 ` Fabien Chouteau
2011-09-13 16:44 ` Scott Wood
1 sibling, 1 reply; 17+ messages in thread
From: Alexander Graf @ 2011-09-13 13:13 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
Alexander Graf wrote:
> Fabien Chouteau wrote:
>
>> On 12/09/2011 19:23, Scott Wood wrote:
>>
>>
>>> On 09/09/2011 09:58 AM, Alexander Graf wrote:
>>>
>>>
>>>> On 09.09.2011, at 16:22, Fabien Chouteau wrote:
>>>>
>>>>
>>>>> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
>>>>> remain set. The only way to unset an interrupt is to clear the corresponding
>>>>> bit in TSR (currently in store_booke_tsr).
>>>>>
>>>>>
>>>> Are you sure? I see several things in the 2.06 spec:
>>>>
>>>>
>>> [snip]
>>>
>>>
>>>> To me that sounds as if the decrementer interrupt gets injected only
>>>> when TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits
>>>> stops the interrupt from being delivered.
>>>>
>>>> Scott, can you please check up with the hardware guys if this is correct?
>>>>
>>>>
>>> This is how I've always understood it to work (assuming the interrupt
>>> hasn't already been delivered, of course). Fabien, do you have real
>>> hardware that you see behave the way you describe?
>>>
>>>
>>>
>> No I don't, it was just my understanding of Book-E documentation. I've tried
>> your solution (below) with VxWorks, and it works like a charm.
>>
>> static void booke_update_irq(CPUState *env)
>> {
>> ppc_set_irq(env, PPC_INTERRUPT_DECR,
>> (env->spr[SPR_BOOKE_TSR] & TSR_DIS
>> && env->spr[SPR_BOOKE_TCR] & TCR_DIE));
>>
>> ppc_set_irq(env, PPC_INTERRUPT_WDT,
>> (env->spr[SPR_BOOKE_TSR] & TSR_WIS
>> && env->spr[SPR_BOOKE_TCR] & TCR_WIE));
>>
>> ppc_set_irq(env, PPC_INTERRUPT_FIT,
>> (env->spr[SPR_BOOKE_TSR] & TSR_FIS
>> && env->spr[SPR_BOOKE_TCR] & TCR_FIE));
>> }
>>
>>
>
> Awesome! Please also check on MSR.EE and send a new patch then :)
>
Ah, the EE check is in target-ppc/helper.c:ppc_hw_interrupt. Very
confusing (and probably wrong because it could generate spurious
interrupts), but it should be enough for now.
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-13 13:13 ` Alexander Graf
@ 2011-09-13 13:28 ` Fabien Chouteau
0 siblings, 0 replies; 17+ messages in thread
From: Fabien Chouteau @ 2011-09-13 13:28 UTC (permalink / raw)
To: Alexander Graf; +Cc: Scott Wood, qemu-ppc, qemu-devel@nongnu.org Developers
On 13/09/2011 15:13, Alexander Graf wrote:
> Alexander Graf wrote:
>> Fabien Chouteau wrote:
>>
>>> On 12/09/2011 19:23, Scott Wood wrote:
>>>
>>>
>>>> On 09/09/2011 09:58 AM, Alexander Graf wrote:
>>>>
>>>>
>>>>> On 09.09.2011, at 16:22, Fabien Chouteau wrote:
>>>>>
>>>>>
>>>>>> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
>>>>>> remain set. The only way to unset an interrupt is to clear the corresponding
>>>>>> bit in TSR (currently in store_booke_tsr).
>>>>>>
>>>>>>
>>>>> Are you sure? I see several things in the 2.06 spec:
>>>>>
>>>>>
>>>> [snip]
>>>>
>>>>
>>>>> To me that sounds as if the decrementer interrupt gets injected only
>>>>> when TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits
>>>>> stops the interrupt from being delivered.
>>>>>
>>>>> Scott, can you please check up with the hardware guys if this is correct?
>>>>>
>>>>>
>>>> This is how I've always understood it to work (assuming the interrupt
>>>> hasn't already been delivered, of course). Fabien, do you have real
>>>> hardware that you see behave the way you describe?
>>>>
>>>>
>>>>
>>> No I don't, it was just my understanding of Book-E documentation. I've tried
>>> your solution (below) with VxWorks, and it works like a charm.
>>>
>>> static void booke_update_irq(CPUState *env)
>>> {
>>> ppc_set_irq(env, PPC_INTERRUPT_DECR,
>>> (env->spr[SPR_BOOKE_TSR] & TSR_DIS
>>> && env->spr[SPR_BOOKE_TCR] & TCR_DIE));
>>>
>>> ppc_set_irq(env, PPC_INTERRUPT_WDT,
>>> (env->spr[SPR_BOOKE_TSR] & TSR_WIS
>>> && env->spr[SPR_BOOKE_TCR] & TCR_WIE));
>>>
>>> ppc_set_irq(env, PPC_INTERRUPT_FIT,
>>> (env->spr[SPR_BOOKE_TSR] & TSR_FIS
>>> && env->spr[SPR_BOOKE_TCR] & TCR_FIE));
>>> }
>>>
>>>
>>
>> Awesome! Please also check on MSR.EE and send a new patch then :)
>>
>
> Ah, the EE check is in target-ppc/helper.c:ppc_hw_interrupt. Very
> confusing (and probably wrong because it could generate spurious
> interrupts), but it should be enough for now.
>
That's what I was looking for...
So we are good.
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-13 13:08 ` Alexander Graf
2011-09-13 13:13 ` Alexander Graf
@ 2011-09-13 16:44 ` Scott Wood
2011-09-13 17:28 ` Alexander Graf
1 sibling, 1 reply; 17+ messages in thread
From: Scott Wood @ 2011-09-13 16:44 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc, qemu-devel@nongnu.org Developers, Fabien Chouteau
On 09/13/2011 08:08 AM, Alexander Graf wrote:
> Fabien Chouteau wrote:
>> static void booke_update_irq(CPUState *env)
>> {
>> ppc_set_irq(env, PPC_INTERRUPT_DECR,
>> (env->spr[SPR_BOOKE_TSR] & TSR_DIS
>> && env->spr[SPR_BOOKE_TCR] & TCR_DIE));
>>
>> ppc_set_irq(env, PPC_INTERRUPT_WDT,
>> (env->spr[SPR_BOOKE_TSR] & TSR_WIS
>> && env->spr[SPR_BOOKE_TCR] & TCR_WIE));
>>
>> ppc_set_irq(env, PPC_INTERRUPT_FIT,
>> (env->spr[SPR_BOOKE_TSR] & TSR_FIS
>> && env->spr[SPR_BOOKE_TCR] & TCR_FIE));
>> }
>>
>
> Awesome! Please also check on MSR.EE and send a new patch then :)
If you check on EE here, then you'll need to call booke_update_irq()
when EE changes (not sure whether that's the plan). Another option
would be to unset the irq if the condition is not valid. This would
also be better in that you could have all three set (DIS, DIE, EE) and
not deliver the interrupt because there's a higher priority exception.
-Scott
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RESEND][PATCH] booke timers
2011-09-13 16:44 ` Scott Wood
@ 2011-09-13 17:28 ` Alexander Graf
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Graf @ 2011-09-13 17:28 UTC (permalink / raw)
To: Scott Wood
Cc: <qemu-ppc@nongnu.org>,
qemu-devel@nongnu.org Developers, Fabien Chouteau
Am 13.09.2011 um 18:44 schrieb Scott Wood <scottwood@freescale.com>:
> On 09/13/2011 08:08 AM, Alexander Graf wrote:
>> Fabien Chouteau wrote:
>>> static void booke_update_irq(CPUState *env)
>>> {
>>> ppc_set_irq(env, PPC_INTERRUPT_DECR,
>>> (env->spr[SPR_BOOKE_TSR] & TSR_DIS
>>> && env->spr[SPR_BOOKE_TCR] & TCR_DIE));
>>>
>>> ppc_set_irq(env, PPC_INTERRUPT_WDT,
>>> (env->spr[SPR_BOOKE_TSR] & TSR_WIS
>>> && env->spr[SPR_BOOKE_TCR] & TCR_WIE));
>>>
>>> ppc_set_irq(env, PPC_INTERRUPT_FIT,
>>> (env->spr[SPR_BOOKE_TSR] & TSR_FIS
>>> && env->spr[SPR_BOOKE_TCR] & TCR_FIE));
>>> }
>>>
>>
>> Awesome! Please also check on MSR.EE and send a new patch then :)
>
> If you check on EE here, then you'll need to call booke_update_irq()
> when EE changes (not sure whether that's the plan). Another option
> would be to unset the irq if the condition is not valid. This would
> also be better in that you could have all three set (DIS, DIE, EE) and
> not deliver the interrupt because there's a higher priority exception.
Yup, which is what the patch actually does, so sorry for the fuss :). The subtile parts are in a different function and by lowering the irq line when TSR or TCR get set, we're good.
Alex
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-09-13 17:28 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-01 8:20 [Qemu-devel] [RESEND][PATCH] booke timers Fabien Chouteau
2011-09-06 19:33 ` Alexander Graf
2011-09-07 14:41 ` Fabien Chouteau
2011-09-07 19:59 ` Alexander Graf
2011-09-09 10:36 ` Fabien Chouteau
2011-09-09 10:55 ` Alexander Graf
2011-09-09 13:27 ` Fabien Chouteau
2011-09-09 13:46 ` Alexander Graf
2011-09-09 14:22 ` Fabien Chouteau
2011-09-09 14:58 ` Alexander Graf
2011-09-12 17:23 ` [Qemu-devel] [Qemu-ppc] " Scott Wood
2011-09-13 13:06 ` Fabien Chouteau
2011-09-13 13:08 ` Alexander Graf
2011-09-13 13:13 ` Alexander Graf
2011-09-13 13:28 ` Fabien Chouteau
2011-09-13 16:44 ` Scott Wood
2011-09-13 17:28 ` Alexander Graf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.