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

[Alexander Graf <agraf@suse.de>]

On 07/17/2012 03:15 PM, Bhushan Bharat-R65777 wrote:
>
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
>> Behalf Of Alexander Graf
>> Sent: Tuesday, July 17, 2012 6:22 PM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; <kvm-ppc@vger.kernel.org>; <kvm@vger.kernel.org>;
>> <bharatb.yadav@gmail.com>; Benjamin Herrenschmidt; Kumar Gala; Avi Kivity
>> Subject: Re: [PATCH 2/2 v2] KVM: PPC: booke: Add watchdog emulation
>>
>> On 07/17/2012 11:57 AM, Bhushan Bharat-R65777 wrote:
>>>> -----Original Message-----
>>>> From: kvm-ppc-owner@vger.kernel.org
>>>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
>>>> Sent: Tuesday, July 17, 2012 12:50 PM
>>>> To: Wood Scott-B07421
>>>> Cc: Bhushan Bharat-R65777; <kvm-ppc@vger.kernel.org>;
>>>> <kvm@vger.kernel.org>; <bharatb.yadav@gmail.com>; Bhushan
>>>> Bharat-R65777; Benjamin Herrenschmidt; Kumar Gala
>>>> Subject: Re: [PATCH 2/2 v2] KVM: PPC: booke: Add watchdog emulation
>>>>
>>>>
>>>>
>>>> On 17.07.2012, at 03:02, Scott Wood <scottwood@freescale.com> wrote:
>>>>
>>>>> On 07/16/2012 12:18 PM, Alexander Graf wrote:
>>>>>>> +/*
>>>>>>> + * Return the number of jiffies until the next timeout.  If the
>>>>>> timeout is
>>>>>>> + * longer than the NEXT_TIMER_MAX_DELTA, that
>>>>>> then?
>>>>>>
>>>>>>> return NEXT_TIMER_MAX_DELTA
>>>>>>> + * instead.
>>>>>> I can read code.
>>>>> Come on, it's not exactly x++; /* add one to x */
>>>>>
>>>>> It's faster to read code (as well as know the constraints within
>>>>> which you can modify it without having to spend a lot of time
>>>>> digesting all the callers' use cases) when you have a high level
>>>>> description of its interface contract, and can be selective about
>>>>> when to zoom in to the details.  Linux kernel code tends to be bad about
>> this.
>>>> Yeah, not opposed to leave that part in :).
>>>>
>>>>>> The important piece of information in the comment is
>>>>>> missing: The reason.
>>>>> The reason for what?  Why you want to know the next timeout?  That's
>>>>> the caller's business.  Or why we use NEXT_TIMER_MAX_DELTA as the limit?
>>>> Why we use the limit. IIRC it was explained in the last thread, just
>>>> didn't make its way into the comment.
>>> Earlier we have a comment on the #define MAX_TIMEOUT (new define added for a
>> purpose, so the comment described the puspose).
>>> Now we uses the generic #define NEX_TIMER_MAX_DELTA (include/linux/timer.h),
>> so removed the comment.
>>
>> Ah, ok. Just saying, if you comment on some mechanism, like you did here, please
>> also include the reasoning behind it. For example
>>
>>     Do foo if x is true.
>>
>> isn't particularly helpful. However
>>
>>     Do foo if x is true because the bar API will break with high values
>>
>> is very helpful. It includes the action and reason of the code :).
>> Alternatively, to me the same as above would be
>>
>>     /* bar API will break with high values */
>>     if (x)
>>       do(foo)
>>
>> because in this case the code is the action description. Either variant works
>> fine for me.
> Ok :)
>
>>>>>>> +void kvmppc_watchdog_func(unsigned long data) {
>>>>>>> +    struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;
>>>>>>> +    u32 tsr, new_tsr;
>>>>>>> +    int final;
>>>>>>> +
>>>>>>> +    do {
>>>>>>> +        new_tsr = tsr = vcpu->arch.tsr;
>>>>>>> +        final = 0;
>>>>>>> +
>>>>>>> +        /* Time out event */
>>>>>>> +        if (tsr & TSR_ENW) {
>>>>>>> +            if (tsr & TSR_WIS) {
>>>>>>> +                new_tsr = (tsr & ~TCR_WRC_MASK) |
>>>>>>> +                      (vcpu->arch.tcr & TCR_WRC_MASK);
>>>>>>> +                vcpu->arch.tcr &= ~TCR_WRC_MASK;
>>>>>> Can't we just poke the vcpu to exit the VM and do the above on its own?
>>>>> We've discussed this before.  TSR updates are done via atomics, and
>>>>> we send a request for the vcpu to act on the result.  This is how
>>>>> the decrementer works.
>>>>>
>>>>> http://www.spinics.net/lists/kvm-ppc/msg03169.html
>>>> Yeah, the major difference to the dec is the atomicity of the whole
>>>> thing. Dec changes one bit to enable the interrupt line. The final
>>>> expiration is more complex.
>>> Is not setting the TSR.WRS atomic here (cmpxchg() will handle this)?
>> Final expiration sets TCR. TSR should be ok.
> It is clearing some TCR bits :)
>
> Let us move the TCR clearing to userspace (please see below response ^^). Then it is just setting TSR. Right?

Then TSR is still set, right? So we don't set it, it just keeps on being 
1. We need to trigger another expired event in that if branch now.


Alex