> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, July 23, 2012 10:00 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> agraf@xxxxxxx
> Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
>
> On 07/23/2012 11:04 AM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Monday, July 23, 2012 9:31 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: Wood Scott-B07421; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> >> agraf@xxxxxxx
> >> Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
> >>
> >> On 07/23/2012 10:43 AM, Bhushan Bharat-R65777 wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Wood Scott-B07421
> >>>> Sent: Monday, July 23, 2012 9:02 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: Wood Scott-B07421; kvm-ppc@xxxxxxxxxxxxxxx;
> >>>> kvm@xxxxxxxxxxxxxxx; agraf@xxxxxxx
> >>>> Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
> >>>>
> >>>> On 07/22/2012 11:10 PM, Bhushan Bharat-R65777 wrote:
> >>>>>>> @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu
> >>>>>>> *vcpu) #ifdef CONFIG_KVM_EXIT_TIMING
> >>>>>>> mutex_init(&vcpu->arch.exit_timing_lock);
> >>>>>>> #endif
> >>>>>>> -
> >>>>>>> +#ifdef CONFIG_BOOKE
> >>>>>>> + spin_lock_init(&vcpu->arch.wdt_lock);
> >>>>>>> + /* setup watchdog timer once */
> >>>>>>> + setup_timer(&vcpu->arch.wdt_timer, kvmppc_watchdog_func,
> >>>>>>> + (unsigned long)vcpu);
> >>>>>>> +#endif
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>
> >>>>>> Can you do this in kvmppc_booke_init()?
> >>>>>
> >>>>> I do not think we can do this in kvmppc_booke_init(). Watchdog
> >>>>> have association with vcpu, while there is no vcpu at kvmppc_booke_init().
> >>>>
> >>>> Sorry, I meant kvm_arch_vcpu_setup() in booke.c.
> >>>
> >>> Any specific reason to move this in above mentioned function? Want
> >>> to avoid
> >> #ifdef config_booke ?
> >>
> >> Yes, to avoid the ifdef. We already have a (poorly named) place for
> >> booke- specific vcpu init.
> >
> > Where we want to delete watchdog timer?
> >
> > Kvm_arch_vcpu_setup() is in flow of CREATE_VCPU ioctl, I do not see
> DESTROY_VCPU type of code?
> >
> > So is it ok to let del_timer_sync() as is with #ifdef config_booke ?
>
> You could add such a function. I suggest correcting the naming issue at the
> same time -- have kvm_subarch_vcpu_init() and kvm_subarch_vcpu_free().
I am sorry Scott but I did not get which functions you want to rename. What I understood from current code for powerpc is:
kvm_vm_ioctl_create_vcpu()
|
|-> kvm_arch_vcpu_create()
|-> kvm_arch_vcpu_setup() - This is where you want me to call watchdog timer setup. We cannot (require more efforts) rename this as this is called from virt/kvm/kvm_main.c. Say if we add, now let us figure out where to uninit what is done in this function:
1)
kvm_arch_vcpu_destroy()
|
|-> kvm_arch_vcpu_free() -> this undo what is done by 1) kvm_arch_vcpu_init() 2) kvm_arch_vcpu_create().
This also does not undo what is done in kvm_arch_vcpu_setup()
Also kvm_arch_vcpu_destroy() is called as error fallback of kvm_vm_ioctl_create_vcpu(), so this is not normal cleaup function call.
2)
The other function to undo is kvm_arch_destroy_vm() (which is called from kvm_vm_release(), kvm_vcpu_release() and error fallback in kvm_dev_ioctl_create_vm()/kvm_vm_ioctl_create_vcpu(). So this function is normal cleanup function and what it does is:
kvm_arch_destroy_vm()
|
|-> kvm_for_each_vcpu()
| |
| |->kvm_arch_vcpu_free() -> this undo what is done by 1) kvm_arch_vcpu_init() 2) kvm_arch_vcpu_create().
| This also does not undo what is done in kvm_arch_vcpu_setup()
|
|-> kvmppc_core_destroy_vm() -> this undo what was done by kvm_arch_init_vm() in flow of kvm_create_vm()
So what I understood from this is:
1) Move the kvm_vcpu_arch_setup() into powerpc.c
2) define kvm_subarch_vcpu_init() in (booke/book3s) to do subarch specific work and call this from kvm_vcpu_arch_setup()
3) define kvm_subarch_vcpu_free() to be called from kvm_arch_vcpu_free(). This will undo what is done by kvm_subarch_vcpu_init().
4) Also kvm_arch_vcpu_free() should do what is done by kvm_vcpu_srch_setup() in powerpc.c
Thanks
-Bharat
>
> -Scott
��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
