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

[Scott Wood <scottwood@freescale.com>]

On 07/17/2012 09:35 AM, Alexander Graf wrote:
> On 07/17/2012 04:13 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 7:31 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 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:
>>>>>> 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.
>> I am sorry Alex but I did not get you.
>>
>> What I meant was that you were having concern that dec is atomic while
>> watchdog final expiration (complex) is not atomic, but if we move the
>> TCR.WRC clearing to userspace on final expiration then are you ok with
>> current code?
> 
> If we move the final expiration TCR.WRC clearing out of that branch, you
> don't have any condition left to check if we are in the final expiration
> path.

I don't follow.

Determining whether we're on final expiration is based only on the
previous value of TSR[ENW,WIS] when the timer expires.  We look at TCR
to determine whether we need to actually exit to QEMU (as opposed to a
final expiration with no action, which still should result in the timer
being stopped), but I don't see any problematic races there as long as
we don't try to update TCR from that context.

> So you need to set some other bit somewhere (eventually
> pending_exceptions probably) that indicates that we need to return to
> user space.

We should do that to simplify other parts of the code, but I'm not sure
what it has to do with TCR[WRC].

-Scott