Re: [PATCH v3 3/3] KVM: PPC: Use ppc_hard_irq_disable before guest entry

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

Hi,

On 12/11/26 6:39 PM, Shrikanth Hegde wrote:
> After applying preempt disable patch, i.e PATCH 2/3, Christian reported
> a subsequent warning stopping his board to boot properly.
> 
> WARNING: at .kvmppc_fix_ee_before_entry+0x10/0x28, CPU#0: qemu-system-ppc/4667
> CPU: 0 UID: 1000 PID: 4667 Comm: qemu-system-ppc Tainted: G        W           7.3.0-rc4-2-powerpc64-smp #1 PREEMPT
> Tainted: [W]=WARN
> Hardware name: varisys,CYRUS5040 e5500 0x80240012 CoreNet Generic
> NIP [c00000000005dc3c] .kvmppc_fix_ee_before_entry+0x10/0x28
> LR [c00000000005f298] .kvmppc_handle_exit+0x5b4/0x5e8
> Call Trace:
> [c000000086587780] [c00000000005ee10] .kvmppc_handle_exit+0x12c/0x5e8 (unreliable)
> [c000000086587830] [c000000000062068] .kvmppc_resume_host+0xb8/0x10c
> 
> It triggers below warning...
> 
> static inline void kvmppc_fix_ee_before_entry(void)
> {
>          trace_hardirqs_on();
> 
>          /*
>           * To avoid races, the caller must have gone directly from having
>           * interrupts fully-enabled to hard-disabled.
>           */
>          WARN_ON(local_paca->irq_happened != PACA_IRQ_HARD_DIS);
> 
> This happens since kvmppc_prepare_to_enter does first local_irq_disable
> followed by hard_irq_disable. This leaves a small window where interrupt
> may occur and it could set the irq pending bit in PACA.
> 
> That local_irq_disable was necessary for PPC32 to track the lockdep
> state etc. On PPC64 hard_irq_disable is good enough. There is no need to
> do both. Use the new irq api to do the necessary one.
> 
> Fixes: 12013e3d4695 ("KVM: powerpc: Use generic xfer to guest work function")
> Reported-by: Christian Zigotzky<chzigotzky@xenosoft.de>
> Closes:https://lore.kernel.org/all/b8f82519-9ae5-c247-e020- 
> e79c627e57df@xenosoft.de/
> Tested-by: Christian Zigotzky<chzigotzky@xenosoft.de>
> Reviewed-by: Mukesh Kumar Chaurasiya (IBM)<mkchauras@gmail.com>
> Signed-off-by: Shrikanth Hegde<sshegde@linux.ibm.com>
> ---
>   arch/powerpc/kvm/booke.c   |  3 +--
>   arch/powerpc/kvm/powerpc.c | 14 +++-----------
>   2 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index aed7b476b7eb..1ba28848e8bc 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -722,8 +722,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>   	if (vcpu->arch.shared->msr & MSR_WE) {
>   		local_irq_enable();
>   		kvm_vcpu_halt(vcpu);
> -		local_irq_disable();
> -		hard_irq_disable();
> +		ppc_hard_irq_disable();
>   
>   		kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS);
>   		r = 1;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 9194cf492d1c..4ee3d7bfe917 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -81,13 +81,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>   	int r;
>   
>   	WARN_ON(irqs_disabled());
> -	/*
> -	 * local_irq_disable() first: on 32-bit, hard_irq_disable() alone is a
> -	 * raw MSR[EE] clear that bypasses the lockdep/irq-tracing state, and
> -	 * the xfer_to_guest_mode helpers assert IRQs are seen as disabled.
> -	 */
> -	local_irq_disable();
> -	hard_irq_disable();
> +	ppc_hard_irq_disable();
>   
>   	while (true) {
>   		xfer_to_guest_mode_prepare();
> @@ -101,8 +95,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>   			 */
>   			local_irq_enable();
>   			r = kvm_xfer_to_guest_mode_handle_work(vcpu);
> -			local_irq_disable();
> -			hard_irq_disable();
> +			ppc_hard_irq_disable();
>   			if (r) {
>   				/*
>   				 * The generic helper does not set the exit
> @@ -134,8 +127,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>   			local_irq_enable();
>   			trace_kvm_check_requests(vcpu);
>   			r = kvmppc_core_check_requests(vcpu);
> -			local_irq_disable();
> -			hard_irq_disable();
> +			ppc_hard_irq_disable();
>   			if (r > 0)
>   				continue;
>   			break;
The changes look good to me.

Using ppc_hard_irq_disable() directly on PPC64 avoids the soft-disable 
to hard-disable window, while preserving the software IRQ mask and 
tracing state required by the guest-entry path. The BookE halt path and 
the guest-work retry paths are covered as well.

Reviewed-by: Narayana Murty N nnmlinux@linux.ibm.com