Re: [PATCH 2/2] KVM: PPC: Replay pending interrupts before entering the guest

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Hi Shrikanth,

Shrikanth Hegde <sshegde@linux.ibm.com> writes:

> After applying preempt disable patch, i.e PATCH 1/2, 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. When that
> happens replay that interrupt before entering the guest.
>
> 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/
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  arch/powerpc/kvm/powerpc.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 9194cf492d1c..847ff07c364b 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -147,6 +147,19 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>  			continue;
>  		}
>  
> +#ifdef CONFIG_PPC64
> +		/*
> +		 * Interrupt arrived between the soft and hard
> +		 * disable. Replay it and retry guest entry.
> +		 */
> +		if (unlikely(local_paca->irq_happened != PACA_IRQ_HARD_DIS)) {
> +			local_irq_enable();
> +			local_irq_disable();
> +			hard_irq_disable();
> +			continue;
> +		}
> +#endif
> +

I looked at the commit which causes this regression, I see there are few
more call sites which can hit the same WARN in kvmppc_fix_ee_before_entry().

So what I was thinking was - while we are at it - do you think we could
use the following helper instead of {local|hard}_irq_{disable|enable}()
pair and ...


/*
 * Hard disable interrupts before entering the guest.
 *
 * On 64-bit don't use local_irq_disable() here. It only soft-masks, so
 * an interrupt can still come in before we hard disable. That interrupt
 * gets marked pending in irq_happened and kvmppc_fix_ee_before_entry()
 * then throws it away. hard_irq_disable() clears MSR[EE] first and does
 * the irq tracing itself, so there's no such window.
 *
 * On 32-bit there is no soft mask and hard_irq_disable() doesn't do the
 * irq tracing, so use local_irq_disable().
 */
static inline void kvmppc_irq_disable_for_entry(void)
{
#ifdef CONFIG_PPC64
        hard_irq_disable();
#else
        local_irq_disable();
#endif
}

... and then update the other call sites too which the following commit
added - which can cause the WARN.
   Fixes: 12013e3d4695 ("KVM: powerpc: Use generic xfer to guest work function")

Thoughts?

-ritesh

>  		guest_enter_irqoff();
>  		return 1;
>  	}
> -- 
> 2.52.0