Re: [PATCH 1/2] powerpc: kvm: use generic transfer to guest mode work

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-04-25 16:49:19 [+0530], Shrikanth Hegde wrote:
> On 4/25/25 00:08, Sebastian Andrzej Siewior wrote:
> > On 2025-04-24 21:27:59 [+0530], Shrikanth Hegde wrote:
> > > > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > > > > index 19f4d298d..123539642 100644
> > > > > --- a/arch/powerpc/kvm/book3s_hv.c
> > > > > +++ b/arch/powerpc/kvm/book3s_hv.c
> > > > > @@ -4901,7 +4901,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
> > > > >    	}
> > > > >    	if (need_resched())
> > > > > -		cond_resched();
> > > > > +		schedule();
> > > > 
> > > 
> > > 
> > > > This looks unrelated and odd. I don't why but this should be a
> > > > cond_resched() so it can be optimized away on PREEMPT kernels.
> > > 
> > > This is needed, otherwise KVM on powerVM setup gets stuck on preempt=full/lazy.
> > 
> > But this makes no sense. On preempt=full the cond_resched() gets patched
> > out while schedule() doesn't. Okay, this explains the stuck.
> 
> cond_resched works. What you said is right about schedule and preemption models.
> Initially I had some other code changes and they were causing it get stuck. i retested it.

so it is unrelated then ;)

> But looking at the semantics of usage of xfer_to_guest_mode_work
> I think using schedule is probably right over here.
> Correct me if i got it all wrong.

No, if you do xfer_to_guest_mode_work() then it will invoke schedule()
when appropriate. It just the thing in kvmhv_run_single_vcpu() looks odd
and might have been duct tape or an accident and could probably be
removed.

> on x86:
> kvm_arch_vcpu_ioctl_run
> 	vcpu_run
> 		for () {
> 			.. run guest..
> 			xfer_to_guest_mode_handle_work
> 				schedule
> 		}
> 
> 
> on Powerpc:  ( taking book3s_hv flavour):
> kvm_arch_vcpu_ioctl_run
> kvmppc_vcpu_run_hv  *1
> 	do while() {
> 		kvmhv_run_single_vcpu or kvmppc_run_vcpu
> 			-- checking for need_resched and signals and bails out *2
> 	}
>
> 
> *1 - checks for need resched and signals before entering guest
I don't see the need_resched() check here.

> *2 - checks for need resched and signals while running the guest
> 
> 
> This patch is addressing only *1 but it needs to address *2 as well using generic framework.
> I think it is doable for books3s_hv atleast. (though might need rewrite)
> 
> __kvmppc_vcpu_run is a block box to me yet. I think it first makes sense
> to move it C and then try use the xfer_to_guest_mode_handle_work.
> nick, vaibhav, any idea on __kvmppc_vcpu_run on how is it handling signal pending, and need_resched.
> 
> 
> So this is going to need more work specially on *2 and doing that is also key for preempt=lazy/full to work
> for kvm on powepc. will try to figure out.

Okay.

Sebastian