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.
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
