Re: [Qemu-devel] [PATCH 3/3] Enable kvm emulated watchdog

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Monday, December 17, 2012 8:09 PM
> To: Bhushan Bharat-R65777
> Cc: address@hidden; address@hidden; 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 <address@hidden>
> > ---
> > 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().

Thanks
-Bharat

> 
> > +    }
> > +}
> > +
> > +static int kvm_booke_watchdog_enable(CPUPPCState *env) {
> > +    int ret;
> > +    struct kvm_enable_cap encap = {};
> > +
> > +    if (!kvm_enabled()) {
> > +        return 0;
> 
> Why return 0?
> 
> > +    }
> > +
> > +    if (!cap_ppc_watchdog) {
> > +        printf("warning: KVM does not support watchdog");
> > +        return 0;
> 
> Why return 0?
> 
> > +    }
> > +
> > +    encap.cap = KVM_CAP_PPC_BOOKE_WATCHDOG;
> > +    ret = kvm_vcpu_ioctl(env, KVM_ENABLE_CAP, &encap);
> > +    if (ret < 0) {
> > +        fprintf(stderr, "%s: couldn't enable KVM_CAP_PPC_BOOKE_WATCHDOG:
> %s\n",
> > +                __func__, strerror(-ret));
> > +        return ret;
> > +    }
> 
> Please wrap this in a kvmppc_... function in kvm.c.
> 
> > +
> > +    return ret;
> > +}
> > +
> > void ppc_booke_timers_init(CPUPPCState *env, uint32_t freq, uint32_t
> > flags) {
> >     ppc_tb_t *tb_env;
> > @@ -262,5 +327,11 @@ void ppc_booke_timers_init(CPUPPCState *env, uint32_t
> freq, uint32_t flags)
> >     booke_timer->wdt_timer =
> >         qemu_new_timer_ns(vm_clock, &booke_wdt_cb, env);
> >
> > +    if (kvm_enabled()) {
> 
> Why double-check kvm_enabled()? And what happens on failure? Shouldn't failure
> be crucial if a watchdog action is set?
> 
> > +        kvm_booke_watchdog_enable(env);
> > +    }
> > +
> > +    qemu_add_vm_change_state_handler(cpu_state_change_handler, env);
> > +
> >     qemu_register_reset(ppc_booke_timer_reset_handle, env); } diff
> > --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 3f5df57..3d5f86a
> > 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -32,6 +32,7 @@
> > #include "device_tree.h"
> > #include "hw/sysbus.h"
> > #include "hw/spapr.h"
> > +#include "hw/watchdog.h"
> >
> > #include "hw/sysbus.h"
> > #include "hw/spapr.h"
> > @@ -56,11 +57,12 @@ const KVMCapabilityInfo
> > kvm_arch_required_capabilities[] = { static int cap_interrupt_unset =
> > false; static int cap_interrupt_level = false; static int
> > cap_segstate; -static int cap_booke_sregs;
> > +int cap_booke_sregs;
> 
> No.
> 
> > static int cap_ppc_smt;
> > static int cap_ppc_rma;
> > static int cap_spapr_tce;
> > static int cap_hior;
> > +int cap_ppc_watchdog;
> 
> static
> 
> >
> > /* XXX We have a race condition where we actually have a level triggered
> >  *     interrupt, but the infrastructure can't expose that yet, so the guest
> > @@ -90,6 +92,7 @@ int kvm_arch_init(KVMState *s)
> >     cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
> >     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> >     cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> > +    cap_ppc_watchdog = kvm_check_extension(s,
> > + KVM_CAP_PPC_BOOKE_WATCHDOG);
> >
> >     if (!cap_interrupt_level) {
> >         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect 
> > the "
> > @@ -823,6 +826,14 @@ int kvm_arch_handle_exit(CPUPPCState *env, struct 
> > kvm_run
> *run)
> >         ret = 0;
> >         break;
> > #endif
> > +#ifdef KVM_EXIT_WATCHDOG
> 
> Why an #ifdef here?
> 
> 
> Alex
> 
> > +    case KVM_EXIT_WATCHDOG:
> > +        dprintf("handle watchdog expiry\n");
> > +        watchdog_perform_action();
> > +        ret = 0;
> > +        break;
> > +#endif
> > +
> >     default:
> >         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> >         ret = -1;
> > --
> > 1.7.0.4
> >
> >
>