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

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

[Hollis Blanchard]

On Fri, 2008-03-28 at 16:34 -0500, Jerone Young wrote: 
> # 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:

This code definitely does not belong here. Why didn't you put it in
kvmppc_deliver_interrupt()?

> @@ -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:

This code also definitely does not belong here. For starters, the mtmsr
emulation hasn't yet run, so vcpu->arch.msr hasn't been updated.

Why are you calling kvmppc_check_and_deliver_interrupts() inside the
loop?? I don't understand this code flow at all.

You don't seem to have included the patch that adds vcpu->wq.

Wouldn't you want to notify the waitqueue from
kvmppc_deliver_interrupt()?

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

[Jerone Young]

On Fri, 2008-03-28 at 16:50 -0500, Hollis Blanchard wrote:
> On Fri, 2008-03-28 at 16:34 -0500, Jerone Young wrote: 
> > # 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:
> 
> This code definitely does not belong here. Why didn't you put it in
> kvmppc_deliver_interrupt()?
> 
> > @@ -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:
> 
> This code also definitely does not belong here. For starters, the
> mtmsr
> emulation hasn't yet run, so vcpu->arch.msr hasn't been updated.

It needs to go lower in the function.

> 
> Why are you calling kvmppc_check_and_deliver_interrupts() inside the
> loop?? I don't understand this code flow at all.

So the thinking here is that we are waiting for the fact that there are
no pending interrupts. So the only way to deliver said interrupts is to
do it while we are waiting in our wait loop.


> 
> You don't seem to have included the patch that adds vcpu->wq.
> 
"wq" is already apart of the vcpu structure. They use it for in x86 kvm
to do about the same thing as I am trying to do here for us.

> Wouldn't you want to notify the waitqueue from
> kvmppc_deliver_interrupt()?

I can try this instead of having the queue call it.

> 


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

[Hollis Blanchard]

On Mon, 2008-03-31 at 07:57 -0500, 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;

No need to modify msr: we're about to blow it away anyways.

In fact, I don't think you even need to mask off WE at all. Leave it in
srr1, and the CPU will automatically go back to sleep when it executes
rfi. This probably explains why many existing power_save()
implementations never return at all...

> @@ -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++;

We already know exactly when we're setting MSR_WE, so there is no need
to poll for it on every exit.

You should call kvm_vcpu_block() instead of duplicating it.

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

[Jerone Young]

On Wed, 2008-04-02 at 11:17 +0200, Christian Ehrhardt wrote:
> 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.
> 

The reason for not using kvm_vcpu_block is that I couldn't figure out in
enough time how to get the premempt notify stuff to work. We really
don't even need it, but it's used in kvm_vcpu_block and I just don't
have the time to figure out what all is needed for it to work.

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


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

[Jerone Young]

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 ?

> 


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

[Jerone Young]

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.  

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. 

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.


On Tue, 2008-04-15 at 10:28 -0500, Hollis Blanchard wrote:
> 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.
> 


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

[Jerone Young]

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. 

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


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

[Jerone Young]

On Tue, 2008-04-15 at 13:56 -0500, Hollis Blanchard wrote:
> 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.

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.

> 


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