> -----Original Message----- > From: Alexander Graf [mailto:agraf@xxxxxxx] > Sent: Tuesday, July 17, 2012 8:05 PM > To: Bhushan Bharat-R65777 > Cc: Wood Scott-B07421; <kvm-ppc@xxxxxxxxxxxxxxx>; <kvm@xxxxxxxxxxxxxxx>; > <bharatb.yadav@xxxxxxxxx>; Benjamin Herrenschmidt; Kumar Gala; Avi Kivity > Subject: Re: [PATCH 2/2 v2] KVM: PPC: booke: Add watchdog emulation > > On 07/17/2012 04:13 PM, Bhushan Bharat-R65777 wrote: > > > >> -----Original Message----- > >> From: kvm-ppc-owner@xxxxxxxxxxxxxxx > >> [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander Graf > >> Sent: Tuesday, July 17, 2012 7:31 PM > >> To: Bhushan Bharat-R65777 > >> Cc: Wood Scott-B07421; <kvm-ppc@xxxxxxxxxxxxxxx>; > >> <kvm@xxxxxxxxxxxxxxx>; <bharatb.yadav@xxxxxxxxx>; 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@xxxxxxxxxxxxxxx > >>>> [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander Graf > >>>> Sent: Tuesday, July 17, 2012 6:22 PM > >>>> To: Bhushan Bharat-R65777 > >>>> Cc: Wood Scott-B07421; <kvm-ppc@xxxxxxxxxxxxxxx>; > >>>> <kvm@xxxxxxxxxxxxxxx>; <bharatb.yadav@xxxxxxxxx>; 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@xxxxxxxxxxxxxxx > >>>>>> [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander > >>>>>> Graf > >>>>>> Sent: Tuesday, July 17, 2012 12:50 PM > >>>>>> To: Wood Scott-B07421 > >>>>>> Cc: Bhushan Bharat-R65777; <kvm-ppc@xxxxxxxxxxxxxxx>; > >>>>>> <kvm@xxxxxxxxxxxxxxx>; <bharatb.yadav@xxxxxxxxx>; 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@xxxxxxxxxxxxx> 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. > > 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. So you > need to set some other bit somewhere (eventually pending_exceptions probably) > that indicates that we need to return to user space. Thanks Alex... We still have TSR.WRS (TCR.WRC is different if you are getting confused with that), we can use that as a condition for final expiration (the way we are using currently in code). -Bharat > > > Alex > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
