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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 4/25/25 19:01, Sebastian Andrzej Siewior wrote:
> 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.
>>>

When i look back, i think below makes sense, since this is what even the 
generic framework will do and it would work for all preemption models.

Since this happens in a loop, it is necessary to check for reschedule.

if need_resched()
	schedule()

Ideal would be call xfer_to_guest_mode_handle_work in the loop, which 
would handle the above too.

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