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

[Alexander Graf]

On 27.12.2012, at 12:38, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----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().

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