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

[Jerone Y. <jy...@us...>]

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 <jy...@us...>
> > # 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 <jy...@us...>
> >
> > 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.
>