Re: [PATCH 1/2] KVM: PPC: BookE: Disable preemption before loading guest FP and Altivec

[Narayana Murty N <nnmlinux@linux.ibm.com>]

Hi Shrikanth,
Thanks for the fixes. I had one question on patch 1.

On 28/09/26 4:34 PM, Shrikanth Hegde wrote:
> Christian reported that booting preemptible kernel on FSL Cyrus+ board
> causes boot hang.
> 
> The logs pointed that system was busy in printing below warning.
> 
> WARNING: at .enable_kernel_fp+0x30/0x78, CPU#3: qemu-system-ppc/4884
> Modules linked in:
> CPU: 3 UID: 1000 PID: 4884 Comm: qemu-system-ppc Not tainted 7.3.0-rc1-powerpc64-smp-preempt #1 PREEMPT
> NIP [c000000000003338] .enable_kernel_fp+0x30/0x78
> LR [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
> Call Trace:
> [c000000085ca7700] [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
> [c000000085ca7780] [c00000000005f2a0] .kvmppc_handle_exit+0x5bc/0x5cc
> [c000000085ca7830] [c00000000006204c] .kvmppc_resume_host+0xb8/0x10c
> 
> Which is...
> 
> void enable_kernel_fp(void)
> {
>          unsigned long cpumsr;
>          WARN_ON(preemptible());
> 
> And...
> 
> Though irq's are hard disabled after kvmppc_prepare_to_enter, but
> kvmppc_fix_ee_before_entry enables the softmask's IRQ state.
> That causes the irqs_disabled to return false.
> Hence leading to the warnings.
> 
> Fix it by disabling the preemption using the preempt disable.
> Note, it is calling noresched variant of preempt enable, since hard
> irq are disabled. It is likely not a good idea to call schedule.
> 
> Fixes: 3efc7da61f6c ("KVM: PPC: Book3E: Increase FPU laziness")
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Closes: https://lore.kernel.org/all/33342fbf-eb7b-bde6-2c8c-254fe8bfb993@xenosoft.de/
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>   arch/powerpc/kvm/booke.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 13ad4cf5fa71..5b9118eefe1d 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1404,10 +1404,17 @@ int kvmppc_handle_exit(struct kvm_vcpu *vcpu, unsigned int exit_nr)
>   		if (s <= 0)
>   			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>   		else {
> -			/* interrupts now hard-disabled */
> +			/*
> +			 * kvmppc_fix_ee_before_entry() marks the software
> +			 * IRQ state enabled while interrupts are still
> +			 * hard-disabled. So disable preemption while loading
> +			 * guest FP and Altivec.
> +			 */
>   			kvmppc_fix_ee_before_entry();
> +			preempt_disable();
>   			kvmppc_load_guest_fp(vcpu);
>   			kvmppc_load_guest_altivec(vcpu);
> +			preempt_enable_no_resched();
Would it be simpler to move kvmppc_fix_ee_before_entry() after the
FP/Altivec loads instead?

The normal kvmppc_vcpu_run() entry path already loads the guest
FP/Altivec state while interrupts are still disabled and calls
kvmppc_fix_ee_before_entry() immediately before entering the guest.

So could this path follow the same ordering:

kvmppc_load_guest_fp(vcpu);
kvmppc_load_guest_altivec(vcpu);
kvmppc_fix_ee_before_entry();

That would avoid making the software IRQ state enabled before loading
the guest FP/Altivec state, and also avoid the additional
preempt_disable()/preempt_enable_no_resched() pair.

Thanks,
Narayana Murty.
>   		}
>   	}
>