[kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling

[kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1206739218 18000
# Node ID 21179d0ab8a62ecc24d18be2da1600694aeca71a
# Parent  e48cf2ad6c85c457ff64c04b83960fc305420842
Add PowerPC KVM guest wait handling support

This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -177,6 +177,11 @@ static int kvmppc_can_deliver_interrupt(
 static int kvmppc_can_deliver_interrupt(struct kvm_vcpu *vcpu, int interrupt)
 {
 	int r;
+
+	/* clear guest wait state */
+	if (vcpu->arch.msr & MSR_WE) {
+		vcpu->arch.msr &= ~MSR_WE;
+	}
 
 	switch (interrupt) {
 	case BOOKE_INTERRUPT_CRITICAL:
@@ -294,6 +299,21 @@ int kvmppc_handle_exit(struct kvm_run *r
 
 	run->exit_reason = KVM_EXIT_UNKNOWN;
 	run->ready_for_interrupt_injection = 1;
+
+	/* handle guest vcpu that is in wait state */
+	if (vcpu->arch.msr & MSR_WE) {
+		DECLARE_WAITQUEUE(wait, current);
+		add_wait_queue(&vcpu->wq, &wait);
+
+		while(vcpu->arch.pending_exceptions) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+			kvmppc_check_and_deliver_interrupts(vcpu);
+		}
+
+		set_current_state(TASK_RUNNING);
+		remove_wait_queue(&vcpu->wq, &wait);
+	}
 
 	switch (exit_nr) {
 	case BOOKE_INTERRUPT_MACHINE_CHECK:

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

[kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1206968186 18000
# Node ID 529f5a653d1e86b81270eeb7598dafcdba9abc1d
# Parent  15675e59e019c4800a834ae79090d92b07f1d0ce
Add PowerPC KVM guest wait handling support

This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -214,6 +214,10 @@ static void kvmppc_deliver_interrupt(str
 		vcpu->arch.tsr |= TSR_DIS;
 		break;
 	}
+
+	/* clear guest wait state */
+	if (vcpu->arch.msr & MSR_WE)
+		vcpu->arch.msr &= ~MSR_WE;
 
 	vcpu->arch.srr0 = vcpu->arch.pc;
 	vcpu->arch.srr1 = vcpu->arch.msr;
@@ -452,6 +456,20 @@ int kvmppc_handle_exit(struct kvm_run *r
 	local_irq_disable();
 
 	kvmppc_check_and_deliver_interrupts(vcpu);
+
+	/* handle guest vcpu that is in wait state */
+	if (vcpu->arch.msr & MSR_WE) {
+		DECLARE_WAITQUEUE(wait, current);
+		add_wait_queue(&vcpu->wq, &wait);
+
+		while (!signal_pending(current)) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+		}
+
+		set_current_state(TASK_RUNNING);
+		remove_wait_queue(&vcpu->wq, &wait);
+	}
 
 	/* Do some exit accounting. */
 	vcpu->stat.exits++;

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Christian Ehrhardt]

Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1206968186 18000
> # Node ID 529f5a653d1e86b81270eeb7598dafcdba9abc1d
> # Parent  15675e59e019c4800a834ae79090d92b07f1d0ce
> Add PowerPC KVM guest wait handling support
> 
> This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -214,6 +214,10 @@ static void kvmppc_deliver_interrupt(str
>  		vcpu->arch.tsr |= TSR_DIS;
>  		break;
>  	}
> +
> +	/* clear guest wait state */
> +	if (vcpu->arch.msr & MSR_WE)
> +		vcpu->arch.msr &= ~MSR_WE;
> 
>  	vcpu->arch.srr0 = vcpu->arch.pc;
>  	vcpu->arch.srr1 = vcpu->arch.msr;
> @@ -452,6 +456,20 @@ int kvmppc_handle_exit(struct kvm_run *r
>  	local_irq_disable();
> 
>  	kvmppc_check_and_deliver_interrupts(vcpu);
> +
> +	/* handle guest vcpu that is in wait state */
> +	if (vcpu->arch.msr & MSR_WE) {
##> +		DECLARE_WAITQUEUE(wait, current);
##> +		add_wait_queue(&vcpu->wq, &wait);
> +
> +		while (!signal_pending(current)) {
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule();
> +		}
> +
> +		set_current_state(TASK_RUNNING);
##> +		remove_wait_queue(&vcpu->wq, &wait);

I might missing something but I tihnk this implementation is not yet complete.
The wait queue is useless until you have a caller for it. If that patch would be all you could just remove the lines I marked with "##".

We might need the waitqueue if we implement an explicit think that calls wake_up() e.g. from interrupt handling. In that case we will addtitionally need some locks to prevent a race between entering the sleep and these delivery/wake_up, but let's start without that waitqueue for now and remove it until we see the need for it - I have enough comments without racy wakeups ;-)

Atm only signal_pending is checked to exit that sleep loop, that might sleep forever. I know we have that sigalarm handler qemu/kvm sets up in userspace but by design the MSR[WE] sleep should not sleep until that signal right?
At the moment we have here in code:
 - exit MSRWE] sleep on a timer base (userspace SIG_ALRM)
In the specs I found the definition:
 - "... sleeps until an interrupt is taken, a reset occurs or an external debug tool clears WE"
 
let me describe some scenarios while the userspace alarm timer would not exist (we ignore it because that is not the defined exit of the MSR[WE] pause):
1. MSR[WE] is set to 1 by the guest and we enter this sleep loop here
2. an interrupt e.g. DEC occurs sets the pending exception via kvmppc_queue_exception and exits
-> the dec or any other interrupt occurs, but this is not causing a signal_pending we still sleep and never wake up
=> I think we need to check more than signal_pending here in this case vcpu->arch.pending_exceptions

1. MSR[WE] is set to 1 by the guest and we enter this sleep loop here
2. some day we might provide an interface that works like "external debug tools" and so we change MSR[WE] in memory
-> no signal occurs we still hang in that loop
=> so I guess we should check the condition of MSR[WE] again in the loop

When we are going to implement checks to pending_exceptions and MSR[WE] we will need to check kvmppc_check_and_deliver_interrupts(vcpu) on exit of that sleep e.g. where you set the task running atm.

So what I suggest (this will still need discussions and e.g. extension with waitqueue, a wake_up caller and locks if we need to wake it from somewhere else) would be something like:

+	/* handle guest vcpu that is in wait state */
+	if (vcpu->arch.msr & MSR_WE) {
+
+		while (!signal_pending(current) 
+			&& !pending_exceptions
+			&& (vcpu->arch.msr & MSR_WE))
+		{
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+		}
+
+		set_current_state(TASK_RUNNING);
+		kvmppc_check_and_deliver_interrupts(vcpu);

 
> +	}
> 
>  	/* Do some exit accounting. */
>  	vcpu->stat.exits++;
> 
> -------------------------------------------------------------------------
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
> _______________________________________________
> kvm-ppc-devel mailing list
> kvm-ppc-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel


-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

[kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1207122433 18000
# Node ID 37ac8b0137fecb4207bac52881b64a3ec5374a03
# Parent  6e9ae9e1e5992339ca1f3efa93f8200cf24cbee5
Add PowerPC KVM guest wait handling support

This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
 
 int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 {
-	/* XXX implement me */
-	return 0;
+	return v->arch.pending_exceptions ? 1 : 0;
 }
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
-	return 1;
+	return (v->arch.msr & MSR_WE) ? 0 : 1;
 }
 
 /* Check if we are ready to deliver the interrupt */
@@ -214,6 +213,10 @@ static void kvmppc_deliver_interrupt(str
 		vcpu->arch.tsr |= TSR_DIS;
 		break;
 	}
+
+	/* clear guest wait state */
+	if (vcpu->arch.msr & MSR_WE)
+		vcpu->arch.msr &= ~MSR_WE;
 
 	vcpu->arch.srr0 = vcpu->arch.pc;
 	vcpu->arch.srr1 = vcpu->arch.msr;
@@ -452,6 +455,25 @@ int kvmppc_handle_exit(struct kvm_run *r
 	local_irq_disable();
 
 	kvmppc_check_and_deliver_interrupts(vcpu);
+
+	/* handle guest vcpu that is in wait state */
+	/* XXX eventually replace with kvm_vcpu_block() */
+	if (vcpu->arch.msr & MSR_WE) {
+		DECLARE_WAITQUEUE(wait, current);
+
+		add_wait_queue(&vcpu->wq, &wait);
+
+		while (!kvm_cpu_has_interrupt(vcpu)
+			&& !signal_pending(current)
+			&& !kvm_arch_vcpu_runnable(vcpu)) {
+			
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+		}
+		
+		__set_current_state(TASK_RUNNING);
+		remove_wait_queue(&vcpu->wq, &wait);
+	}
 
 	/* Do some exit accounting. */
 	vcpu->stat.exits++;

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Christian Ehrhardt]

Jerone Young wrote:
> Add PowerPC KVM guest wait handling support
> 
> This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
[...]

> +	/* handle guest vcpu that is in wait state */
> +	/* XXX eventually replace with kvm_vcpu_block() */
> +	if (vcpu->arch.msr & MSR_WE) {

###
> +		DECLARE_WAITQUEUE(wait, current);
> +
> +		add_wait_queue(&vcpu->wq, &wait);
> +
> +		while (!kvm_cpu_has_interrupt(vcpu)
> +			&& !signal_pending(current)
> +			&& !kvm_arch_vcpu_runnable(vcpu)) {
> +			
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule();
> +		}
> +		
> +		__set_current_state(TASK_RUNNING);
> +		remove_wait_queue(&vcpu->wq, &wait);
###

We talked about calling kvm_vcpu_block here which does the same. What has become of it?
I mean the check is the same now and the waitqueues too. The only difference is vcpu_load/put which calls kvm_arch_vcpu_load/put which are noops for us.
I the issues are only caused by the prempt/lockign stuff there - ?is it? - this could either changed there in kvm_vcpu_block e.g. with a ifdef, a runtime if or by moving that to the architectures which support these stuff.

> +	}
> 
>  	/* Do some exit accounting. */


-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

You seem to have ignored some of my previous comments, so please go back and 
address those.

On Wednesday 02 April 2008 02:52:02 Jerone Young wrote:
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
>
>  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>  {
> -	/* XXX implement me */
> -	return 0;
> +	return v->arch.pending_exceptions ? 1 : 0;
>  }
>
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>  {
> -	return 1;
> +	return (v->arch.msr & MSR_WE) ? 0 : 1;
>  }

I hate ternary operators. Since the return values from these functions are 
just logical true/false, you can drop the ternary stuff completely.

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

[kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1207159658 18000
# Node ID f3f6c082021e9a3a515f140763b6ba2b677e8b34
# Parent  007c135cb5f14f5eb025c103623c2ebfcc76ab11
Add PowerPC KVM guest wait handling support

This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
 
 int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 {
-	/* XXX implement me */
-	return 0;
+	return (v->arch.pending_exceptions);
 }
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
-	return 1;
+	return !(v->arch.msr & MSR_WE);
 }
 
 /* Check if we are ready to deliver the interrupt */
@@ -342,6 +341,25 @@ int kvmppc_handle_exit(struct kvm_run *r
 		default:
 			BUG();
 		}
+
+		/* handle guest vcpu that is in wait state */
+		/* XXX eventually replace with kvm_vcpu_block() */
+		if (vcpu->arch.msr & MSR_WE) {
+			DECLARE_WAITQUEUE(wait, current);
+
+			add_wait_queue(&vcpu->wq, &wait);
+
+			while (!kvm_cpu_has_interrupt(vcpu)
+				&& !signal_pending(current)
+				&& !kvm_arch_vcpu_runnable(vcpu)) {
+				set_current_state(TASK_INTERRUPTIBLE);
+				schedule();
+			}
+
+			__set_current_state(TASK_RUNNING);
+			remove_wait_queue(&vcpu->wq, &wait);
+		}
+
 		break;
 
 	case BOOKE_INTERRUPT_DATA_STORAGE:

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

On Wednesday 02 April 2008 13:07:55 Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1207159658 18000
> # Node ID f3f6c082021e9a3a515f140763b6ba2b677e8b34
> # Parent  007c135cb5f14f5eb025c103623c2ebfcc76ab11
> Add PowerPC KVM guest wait handling support
>
> This patch handles a guest that is in a wait state. This ensures that the
> guest is not allways eating up 100% cpu when it is idle.
>
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
>
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
>
>  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>  {
> -	/* XXX implement me */
> -	return 0;
> +	return (v->arch.pending_exceptions);
>  }
>
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>  {
> -	return 1;
> +	return !(v->arch.msr & MSR_WE);
>  }
>
>  /* Check if we are ready to deliver the interrupt */
> @@ -342,6 +341,25 @@ int kvmppc_handle_exit(struct kvm_run *r
>  		default:
>  			BUG();
>  		}
> +
> +		/* handle guest vcpu that is in wait state */
> +		/* XXX eventually replace with kvm_vcpu_block() */
> +		if (vcpu->arch.msr & MSR_WE) {
> +			DECLARE_WAITQUEUE(wait, current);
> +
> +			add_wait_queue(&vcpu->wq, &wait);
> +
> +			while (!kvm_cpu_has_interrupt(vcpu)
> +				&& !signal_pending(current)
> +				&& !kvm_arch_vcpu_runnable(vcpu)) {
> +				set_current_state(TASK_INTERRUPTIBLE);
> +				schedule();
> +			}
> +
> +			__set_current_state(TASK_RUNNING);
> +			remove_wait_queue(&vcpu->wq, &wait);
> +		}
> +
>  		break;
>
>  	case BOOKE_INTERRUPT_DATA_STORAGE:

I've commented on this several times, by email and in person, and you told me 
that you would move this code inside emulate_instruction().

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

On Wednesday 02 April 2008 14:26:45 Jerone Young wrote:
> On Wed, 2008-04-02 at 14:23 -0500, Hollis Blanchard wrote:
> > On Wednesday 02 April 2008 13:07:55 Jerone Young wrote:
> > > # HG changeset patch
> > > # User Jerone Young <jyoung5@us.ibm.com>
> > > # Date 1207159658 18000
> > > # Node ID f3f6c082021e9a3a515f140763b6ba2b677e8b34
> > > # Parent  007c135cb5f14f5eb025c103623c2ebfcc76ab11
> > > Add PowerPC KVM guest wait handling support
> > >
> > > This patch handles a guest that is in a wait state. This ensures that
> > > the guest is not allways eating up 100% cpu when it is idle.
> > >
> > > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> > >
> > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > > --- a/arch/powerpc/kvm/powerpc.c
> > > +++ b/arch/powerpc/kvm/powerpc.c
> > > @@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
> > >
> > >  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
> > >  {
> > > -	/* XXX implement me */
> > > -	return 0;
> > > +	return (v->arch.pending_exceptions);
> > >  }
> > >
> > >  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> > >  {
> > > -	return 1;
> > > +	return !(v->arch.msr & MSR_WE);
> > >  }
> > >
> > >  /* Check if we are ready to deliver the interrupt */
> > > @@ -342,6 +341,25 @@ int kvmppc_handle_exit(struct kvm_run *r
> > >  		default:
> > >  			BUG();
> > >  		}
> > > +
> > > +		/* handle guest vcpu that is in wait state */
> > > +		/* XXX eventually replace with kvm_vcpu_block() */
> > > +		if (vcpu->arch.msr & MSR_WE) {
> > > +			DECLARE_WAITQUEUE(wait, current);
> > > +
> > > +			add_wait_queue(&vcpu->wq, &wait);
> > > +
> > > +			while (!kvm_cpu_has_interrupt(vcpu)
> > > +				&& !signal_pending(current)
> > > +				&& !kvm_arch_vcpu_runnable(vcpu)) {
> > > +				set_current_state(TASK_INTERRUPTIBLE);
> > > +				schedule();
> > > +			}
> > > +
> > > +			__set_current_state(TASK_RUNNING);
> > > +			remove_wait_queue(&vcpu->wq, &wait);
> > > +		}
> > > +
> > >  		break;
> > >
> > >  	case BOOKE_INTERRUPT_DATA_STORAGE:
> >
> > I've commented on this several times, by email and in person, and you
> > told me that you would move this code inside emulate_instruction().
>
> No I was saying that the code was going to go after emulate_instructions
> was run. Not in the emulate_instruction function. That is where the code
> has been placed just after the function is run.
>
> Did you want in in the emulate_instruction function ?

You're polling the MSR in every exit.

Could a decrementer interrupt alter vcpu->arch.msr & MSR_WE?
What about an ITLB miss?
What about a DSI?

How could the WE bit be changed, and what does that tell us about where this 
code belongs?

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Jimi Xenidis]

On Apr 2, 2008, at 12:27 PM, Hollis Blanchard wrote:

> You seem to have ignored some of my previous comments, so please go  
> back and
> address those.
>
> On Wednesday 02 April 2008 02:52:02 Jerone Young wrote:
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
>>
>>  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>>  {
>> -	/* XXX implement me */
>> -	return 0;
>> +	return v->arch.pending_exceptions ? 1 : 0;
>>  }
>>
>>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>>  {
>> -	return 1;
>> +	return (v->arch.msr & MSR_WE) ? 0 : 1;
>>  }
>
> I hate ternary operators. Since the return values from these  
> functions are
> just logical true/false, you can drop the ternary stuff completely.

Not if if the quantity does not fit in an int.. but returning a bit  
pattern sets a bad precedence.
You could if () it out or use !!(), I know you _love_ the latter.
-JX

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

[kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1207250241 18000
# Node ID 3e781009d2f28c4691ccbb999bf679716f66f349
# Parent  b7794c1fa50b531c9b84382c3f3d9a5466d86c0d
Add PowerPC KVM guest wait handling support

This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -269,6 +269,25 @@ int kvmppc_emulate_instruction(struct kv
 		case 146:                                       /* mtmsr */
 			rs = get_rs(inst);
 			kvmppc_set_msr(vcpu, vcpu->arch.gpr[rs]);
+			
+			/* handle guest vcpu that is in wait state */
+			if (vcpu->arch.msr & MSR_WE) {
+			/* XXX eventually replace with kvm_vcpu_block() */
+				DECLARE_WAITQUEUE(wait, current);
+
+				add_wait_queue(&vcpu->wq, &wait);
+
+				while (!kvm_cpu_has_interrupt(vcpu)
+					&& !signal_pending(current)
+					&& !kvm_arch_vcpu_runnable(vcpu)) {
+					set_current_state(TASK_INTERRUPTIBLE);
+					schedule();
+				}
+
+				__set_current_state(TASK_RUNNING);
+				remove_wait_queue(&vcpu->wq, &wait);
+			}
+
 			break;
 
 		case 163:                                       /* wrteei */
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
 
 int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 {
-	/* XXX implement me */
-	return 0;
+	return !!(v->arch.pending_exceptions);
 }
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
-	return 1;
+	return !(v->arch.msr & MSR_WE);
 }
 
 /* Check if we are ready to deliver the interrupt */

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

On Thursday 03 April 2008 14:35:59 Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1207250241 18000
> # Node ID 3e781009d2f28c4691ccbb999bf679716f66f349
> # Parent  b7794c1fa50b531c9b84382c3f3d9a5466d86c0d
> Add PowerPC KVM guest wait handling support
>
> This patch handles a guest that is in a wait state. This ensures that the
> guest is not allways eating up 100% cpu when it is idle.
>
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
>
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -269,6 +269,25 @@ int kvmppc_emulate_instruction(struct kv
>  		case 146:                                       /* mtmsr */
>  			rs = get_rs(inst);
>  			kvmppc_set_msr(vcpu, vcpu->arch.gpr[rs]);
> +
> +			/* handle guest vcpu that is in wait state */
> +			if (vcpu->arch.msr & MSR_WE) {
> +			/* XXX eventually replace with kvm_vcpu_block() */
> +				DECLARE_WAITQUEUE(wait, current);
> +
> +				add_wait_queue(&vcpu->wq, &wait);
> +
> +				while (!kvm_cpu_has_interrupt(vcpu)
> +					&& !signal_pending(current)
> +					&& !kvm_arch_vcpu_runnable(vcpu)) {
> +					set_current_state(TASK_INTERRUPTIBLE);
> +					schedule();
> +				}
> +
> +				__set_current_state(TASK_RUNNING);
> +				remove_wait_queue(&vcpu->wq, &wait);
> +			}
> +
>  			break;
>
>  		case 163:                                       /* wrteei */
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -164,13 +164,12 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *v
>
>  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>  {
> -	/* XXX implement me */
> -	return 0;
> +	return !!(v->arch.pending_exceptions);
>  }
>
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>  {
> -	return 1;
> +	return !(v->arch.msr & MSR_WE);
>  }
>
>  /* Check if we are ready to deliver the interrupt */

Hmm, did you ever revise this patch? I don't see the corrected version.

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

On Tuesday 15 April 2008 11:14:52 Jerone Young wrote:
> Actually there appears to be a real problem with preempt notify in 44x.
> I had no gotten a chance to get back with you about it. But I did some
> investigation into it last week. We are following the same code paths
> (common code) as x86 for preempt initalization. But I ran some tests
> using preempt_disable() & preempt_enable() around some places where it
> would make since (places where we disable interrupts), but just using
> these functions whould cause the kernel to dump sig #11.

preempt_disable() and preempt_enable() are completely different, and I've 
called those in the past and had no problems.

> The issue we have using function kvm_vcpu_block() that it is identical
> to the code below, BUT it calls vcpu_put which then calls
> preempt_notify_unregister() if it is called it will also sig #11.

If preempt_notify_unregister() dereferences a bad pointer, that shouldn't be 
too difficult to track down.

> I'm not sure if what is going on honestly. Based on what I found it
> should "just work" as we are initializing everything like x86 (we are
> calling preempt_notify_init() in the same place). But for 44x any
> preempt notfication calls blow up. So it appears calling anything
> preempt notify related just blows up.
>
> This is a much bigger issue. I'm not sure that we honest want to be
> stuck on this for long periods of time just to have a function call in a
> place where we honestly do not absolutely need to have it at this time.
> Plus I'm no expert with these scheduling frameworks. But givin what have
> read around the net what we have now "should" work. It just doesn't.
>
> Something can come back to later. But for now we should just roll with
> the working code.

No, I don't want to commit the hack. Sorry if I didn't make that clear before.

Accessing a bad pointer doesn't seem like a "much bigger issue" to me, so if 
it's more complicated than that please elaborate.

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

On Tuesday 15 April 2008 13:29:19 Jerone Young wrote:
> On Tue, 2008-04-15 at 13:19 -0500, Hollis Blanchard wrote:
> > On Tuesday 15 April 2008 11:14:52 Jerone Young wrote:
> > > Actually there appears to be a real problem with preempt notify in 44x.
> > > I had no gotten a chance to get back with you about it. But I did some
> > > investigation into it last week. We are following the same code paths
> > > (common code) as x86 for preempt initalization. But I ran some tests
> > > using preempt_disable() & preempt_enable() around some places where it
> > > would make since (places where we disable interrupts), but just using
> > > these functions whould cause the kernel to dump sig #11.
> >
> > preempt_disable() and preempt_enable() are completely different, and I've
> > called those in the past and had no problems.
>
> But it's not used anywhere in our code. And places where it can go ..
> blows up big time.

We don't explicitly use preempt_disable() now, because we use 
local_irq_disable() instead.

However, as I said before, when we *did* use preempt_disable() explicitly, it 
didn't even blow up a little bit.

preempt_disable() IS NOT THE SAME as preempt_notify_unregister(). Please read 
that again, because I think you're skimming over the function names. These 
are very different things.

> > > The issue we have using function kvm_vcpu_block() that it is identical
> > > to the code below, BUT it calls vcpu_put which then calls
> > > preempt_notify_unregister() if it is called it will also sig #11.
> >
> > If preempt_notify_unregister() dereferences a bad pointer, that shouldn't
> > be too difficult to track down.
> >
> > > I'm not sure if what is going on honestly. Based on what I found it
> > > should "just work" as we are initializing everything like x86 (we are
> > > calling preempt_notify_init() in the same place). But for 44x any
> > > preempt notfication calls blow up. So it appears calling anything
> > > preempt notify related just blows up.
> > >
> > > This is a much bigger issue. I'm not sure that we honest want to be
> > > stuck on this for long periods of time just to have a function call in
> > > a place where we honestly do not absolutely need to have it at this
> > > time. Plus I'm no expert with these scheduling frameworks. But givin
> > > what have read around the net what we have now "should" work. It just
> > > doesn't.
> > >
> > > Something can come back to later. But for now we should just roll with
> > > the working code.
> >
> > No, I don't want to commit the hack. Sorry if I didn't make that clear
> > before.
>
> But this isn't a hack. You just want to use the common function. The
> issue is there are a lot place we do not use the common function.

We use common code wherever it makes sense, and it makes a lot of sense here. 
If you can point out other areas we could or should share code, I would be 
interested in taking a look.

> Since 
> we really do need to get functional code up and going this is something
> that can be postponed to be fixed. I will (and have been) take a look at
> this to change this. But if we put a ton of emphasis on something that
> isn't needed to get us going, then we will never move forward to solve
> the real issues left to get this stuff usable.
>
> > Accessing a bad pointer doesn't seem like a "much bigger issue" to me, so
> > if it's more complicated than that please elaborate.
>
> The problem is the pointer is assigned by the frame work. I'm not
> passing in any pointer to preempt. So something more is going on.

Obviously you're not passing a pointer in, yet sig11 means the code is 
dereferencing a bad pointer. The backtrace probably indicates exactly what 
pointer is causing the problem, and it's 100% reproducible, so it doesn't 
sound like anything too subtle.

> I will 
> have to look into this more after I finish the next task. I don't
> honestly know all that much about the preempt frame work. But I have
> spent time trying to figure it out.

Great, I will look forward to the corrected patch.

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

[Hollis Blanchard]

On Tuesday 15 April 2008 14:09:22 Jerone Young wrote:
> On Tue, 2008-04-15 at 13:56 -0500, Hollis Blanchard wrote:
> > On Tuesday 15 April 2008 13:29:19 Jerone Young wrote:
> > > I will
> > > have to look into this more after I finish the next task. I don't
> > > honestly know all that much about the preempt frame work. But I have
> > > spent time trying to figure it out.
> >
> > Great, I will look forward to the corrected patch.
>
> I would suggest that you include the current patch so that other can use
> the functionality and it can be changed to use the common code function
> once I figure out the preempt issue. Otherwise anyone trying to use our
> source is still going to suffer from guest eating 100% cpu when idle.

Look, I won't commit this. Just fix it. I don't know how else to say it.

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel