On 07/24/2012 02:45 AM, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Monday, July 23, 2012 10:00 PM >> To: Bhushan Bharat-R65777 >> Cc: Wood Scott-B07421; kvm-...@vger.kernel.org; kvm@vger.kernel.org; >> ag...@suse.de >> 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-...@vger.kernel.org; kvm@vger.kernel.org; >>>> ag...@suse.de >>>> 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-...@vger.kernel.org; >>>>>> kvm@vger.kernel.org; ag...@suse.de >>>>>> 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:
Sigh, I didn't realize this came from generic code. What, from generic code's perspective, is the difference between kvm_arch_vcpu_init() and kvm_arch_vcpu_setup() supposed to be? They're synonyms! And as always in Linux, there's no documentation (that I can find) on what's expected of functions that have multiple implementations. Inferring from what the ppc code does, apparently kvm_arch_vcpu_create() is supposed to call kvm_vcpu_init(), which then calls back into arch code with kvm_arch_vcpu_init(). Then, after the preempt notifier is initialized (but not actually activated), generic code goes back to arch code with kvm_arch_vcpu_setup(). So basically the "setup" version comes later. And PPC KVM arbitrarily decides to use one of these for PPC-wide init, and the other for subarch init. This could really use some untangling (or documentation if there really is a method to the madness). In any case, we can still put this init code in kvmppc_arch_vcpu_setup() (i.e. the subarch init), and add a kvmppc_subarch_vcpu_uninit() that we call from kvm_arch_vcpu_uninit(). Or just move kvm_arch_vcpu_uninit() into subarch code -- the only thing it does is call kvmppc_mmu_destroy, which is a subarch hook anyway. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
