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

[Alexander Graf <agraf@suse.de>]

On 07/17/2012 06:27 PM, Scott Wood wrote:
> 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.

Yeah, what I was trying to say is that I would like to keep the TSR 
state unmodified for the final expiry case. If user space decides to not 
act upon it, it should be able to call into VCPU_RUN and have the same 
behavior as if the watchdog never expired.

So there needs to be a check whether we return to user space based on 
TCR, yes. But we shouldn't modify TSR or TCR in that branch. We only 
trigger an event saying "return to user space with a watchdog expired exit".

Then user space can decide whether to act on it. If it decides to ignore 
it, we don't have an oddball TSR.WRS state lingering around that'd need 
clearing.

That way we can put a single if (watchdog_enabled) on the qemu bubble-up 
event injection. If it's not enabled, it'd be as if TCR.WRC is 0 and the 
watchdog expired without action. If it's enabled, user space can decide 
to fire it up again if it deems it necessary.


Alex

>
>> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html