On 27.12.2012, at 12:38, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:ag...@suse.de] >> Sent: Monday, December 17, 2012 8:09 PM >> To: Bhushan Bharat-R65777 >> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Bhushan Bharat-R65777 >> Subject: Re: [PATCH 3/3] Enable kvm emulated watchdog >> >> >> On 17.12.2012, at 07:08, Bharat Bhushan wrote: >> >>> Enable the KVM emulated watchdog if KVM supports (use the capability >>> enablement in watchdog handler). Also watchdog exit >>> (KVM_EXIT_WATCHDOG) handling is added. >>> Watchdog state machine is cleared whenever VM state changes to running. >>> This is to handle the cases like return from debug halt etc. >>> >>> Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com> >>> --- >>> hw/ppc.h | 2 + >>> hw/ppc_booke.c | 71 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> target-ppc/kvm.c | 13 +++++++++- >>> 3 files changed, 85 insertions(+), 1 deletions(-) >>> >>> diff --git a/hw/ppc.h b/hw/ppc.h >>> index 2f3ea27..3672fe8 100644 >>> --- a/hw/ppc.h >>> +++ b/hw/ppc.h >>> @@ -44,6 +44,8 @@ struct ppc_tb_t { >>> >>> 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 (CPUPPCState *env, uint32_t >>> freq); >>> +extern int cap_ppc_watchdog; >>> +extern int cap_booke_sregs; >> >> No. Never export cap_ variables. They are kvm internal. >> >>> /* Embedded PowerPC DCR management */ >>> typedef uint32_t (*dcr_read_cb)(void *opaque, int dcrn); typedef void >>> (*dcr_write_cb)(void *opaque, int dcrn, uint32_t val); diff --git >>> a/hw/ppc_booke.c b/hw/ppc_booke.c index 837a5b6..f18df74 100644 >>> --- a/hw/ppc_booke.c >>> +++ b/hw/ppc_booke.c >>> @@ -21,6 +21,8 @@ >>> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>> DEALINGS IN >>> * THE SOFTWARE. >>> */ >>> +#include "sysemu.h" >>> +#include "kvm.h" >>> #include "hw.h" >>> #include "ppc.h" >>> #include "qemu-timer.h" >>> @@ -203,6 +205,11 @@ static void booke_wdt_cb(void *opaque) >>> booke_timer->wdt_timer); } >>> >>> +static void ppc_booke_watchdog_clear_tsr(CPUPPCState *env, >>> +target_ulong tsr) { >>> + env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS | >>> +TSR_WRS_MASK); } >>> + >>> void store_booke_tsr(CPUPPCState *env, target_ulong val) { >>> env->spr[SPR_BOOKE_TSR] &= ~val; >>> @@ -241,6 +248,64 @@ static void ppc_booke_timer_reset_handle(void *opaque) >>> booke_update_irq(env); >>> } >>> >>> +static void cpu_state_change_handler(void *opaque, int running, >>> +RunState state) { >>> + CPUPPCState *env = opaque; >>> + >>> + struct kvm_sregs sregs; >>> + >>> + if (!running) { >>> + return; >>> + } >>> + >>> + /* >>> + * Clear watchdog interrupt condition by clearing TSR. >>> + * Similar logic needed to be implemented for watchdog >>> + * emulation in qemu. >>> + */ >>> + >>> + if (!kvm_enabled()) { >>> + /* FIXME: add handling for qemu emulated case */ >>> + return; >>> + } >>> + >>> + if (cap_booke_sregs && cap_ppc_watchdog) { >>> + kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs); >>> + >>> + /* Clear TSR.ENW, TSR.WIS and TSR.WRS */ >>> + ppc_booke_watchdog_clear_tsr(env, sregs.u.e.tsr); >> >> This should happen outside of the if (kvm_enabled()) block. >> >>> + sregs.u.e.tsr = env->spr[SPR_BOOKE_TSR]; >>> + sregs.u.e.update_special = KVM_SREGS_E_UPDATE_TSR; >>> + >>> + kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs); >> >> Please create a kvmppc_... wrapper for all this in target-ppc/kvm.c. Or maybe >> even better yet add a helper variable that tells the kvm register sync >> function >> to sync TSR as well and just use the normal cpu_synchronize_state() way of >> pushing register into the CPU. > > Not sure what type of helper variable you are talking about. > What came in my mine is we define a helper variable as per bitmap of SREGS > update feature KVM_SREGS_E_UPDATE_* (update_special) in env. Whenever any > code changes the env[spr] it will set the update_special. Env->update_special > will be checked in put_registers().
Yes, just that the bitmap shouldn't use KVM_SREGS bits but its own bit ids :). That way we can also support ONE_REG variables. Alex
