Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

[Scott Wood <scottwood@freescale.com>]

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-ppc@vger.kernel.org; kvm@vger.kernel.org;
>> agraf@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-ppc@vger.kernel.org; kvm@vger.kernel.org;
>>>> agraf@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-ppc@vger.kernel.org;
>>>>>> kvm@vger.kernel.org; agraf@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