Re: [PATCH v3] KVM: PPC: Book3S HV: Avoid spurious interrupts caused by LPCR_MER bit

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

Hi Gautam,

On 21/09/26 4:40 PM, Gautam Menghani wrote:
> A huge number of spurious interrupts can be seen immediately after a KVM
> on PowerNV guest boots up in XIVE mode.
> 
> $ cat /proc/interrupts  | grep SPU
> SPU:     223705     192439     273526     147623   Spurious interrupts
> 
> This bug was introduced by commit ecd10702baae5 ("KVM: PPC: Book3S HV:
> Handle pending exceptions on guest entry with MSR_EE"). The root cause
> is that once LPCR_MER bit is set, it is supposed to be reset by
> software. But when a vCPU starts running with LPCR_MER set, the vCPU does
> not exit back to the host until the decrementer expires or there is an
> hcall, etc. This is because KVM on PowerNV guests have support for
> native XIVE, so they are not dependent on host for interrupt emulation.
> Due to this behaviour, a huge number of spurious interrupts are seen
> since LPCR_MER continues to be set and LPCR_MER cannot be reset until the
> vCPU exits to the host.
> 
> Fix this behaviour by not using the LPCR_MER bit whenever native XIVE is
> available (currently in case of KVM on PowerNV only), as the XIVE hardware
> can present interrupts to the KVM guest vCPU directly. So the LPCR_MER
> functionality is not required. This reduces the number of spurious
> interrupts drastically.
> 
> Fixes: ecd10702baae5 ("KVM: PPC: Book3S HV: Handle pending exceptions on guest entry with MSR_EE")
> Cc: stable@vger.kernel.org # 6.8+
> Reported-by: Timothy Pearson <tpearson@raptorengineering.com>
> Closes: https://lore.kernel.org/linuxppc-dev/582904882.11159.1786719390349.JavaMail.zimbra@raptorengineeringinc.com
> Signed-off-by: Gautam Menghani <gautam@linux.ibm.com>
> ---
> v3:
> 1. Continue the use of LPCR_MER when kernel-irqchip=off (Sashiko)
> 
> v2:
> 1. Handle the case where xive_interrupt_pending() is true and also the
> external exception bit is set. (Narayana)
> 
>   arch/powerpc/include/asm/kvm_ppc.h | 7 +++++++
>   arch/powerpc/kvm/book3s_hv.c       | 2 +-
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 169ea6a7fbad..580ad2548c2b 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -747,6 +747,11 @@ static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
>   	return vcpu->arch.irq_type == KVMPPC_IRQ_XIVE;
>   }
>   
> +static inline bool kvmppc_xive_native_enabled(struct kvm *kvm)
> +{
> +	return kvm->arch.xive_devices.native;
> +}
Also, should 'kvmppc_xive_native_enabled()' check the active XIVE
device rather than just whether a native device has been created?

'kvmppc_xive_native_release()' clears 'kvm->arch.xive', but explicitly
keeps the 'kvmppc_xive' pointer under 'xive_devices' for reuse. The
comment in 'book3s_xive.c' also says that when switching between XICS
and native XIVE, the previous KVM device is released before the new one
is created.

So 'xive_devices.native != NULL' seems to mean that a native XIVE
device has been allocated/cached, rather than that it is currently
active.

Would this be more appropriate?

return kvm->arch.xive_devices.native &&
        kvm->arch.xive == kvm->arch.xive_devices.native;

> +
>   extern int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>   					   struct kvm_vcpu *vcpu, u32 cpu);
>   extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
> @@ -782,6 +787,8 @@ static inline bool kvmppc_xive_rearm_escalation(struct kvm_vcpu *vcpu) { return
>   
>   static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
>   	{ return 0; }
> +static inline bool kvmppc_xive_native_enabled(struct kvm *kvm) { return false; }
> +
>   static inline int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>   			  struct kvm_vcpu *vcpu, u32 cpu) { return -EBUSY; }
>   static inline void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu) { }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index dbac3573b2c8..cb2bb29451a7 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -4980,7 +4980,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>   			if (!kvmhv_on_pseries() && (__kvmppc_get_msr_hv(vcpu) & MSR_EE))
>   				kvmppc_inject_interrupt_hv(vcpu,
>   							   BOOK3S_INTERRUPT_EXTERNAL, 0);
> -			else
> +			else if (!kvmppc_xive_native_enabled(vcpu->kvm))
>   				lpcr |= LPCR_MER;
Is the intent here to suppress LPCR_MER only for interrupts that
native XIVE can deliver directly?

BOOK3S_IRQPRIO_EXTERNAL is a separate software-pending exception. If
MSR_EE is clear it remains pending, so it seems this path should still
set MER even when native XIVE is active.

IOW, should the behavior be:

else if (!kvmppc_xive_native_enabled(vcpu->kvm) ||
	 test_bit(BOOK3S_IRQPRIO_EXTERNAL,
		  &vcpu->arch.pending_exceptions))
	lpcr |= LPCR_MER;

so that only the native-XIVE-pending case suppresses MER?

Thanks,
narayana Murty N
>   		} else {
>   			/*