RE: [PATCH] KVM: x86: Fix the intel_pt PMI handling wrongly considered from guest

[Xu, Yanfei]

Hi Sean,
You are right, the change of kvm_arch_pmi_in_guest is incorrect, because it should cover two cases of PMI. 
For the PMI of intel pt, it certainly is the NMI PMI. So how about fixing it like below?

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 610355b9ccce..378036c1cf94 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7856,7 +7856,7 @@ static unsigned int vmx_handle_intel_pt_intr(void)
        struct kvm_vcpu *vcpu = kvm_get_running_vcpu();

        /* '0' on failure so that the !PT case can use a RET0 static call. */
-       if (!kvm_arch_pmi_in_guest(vcpu))
+       if (!kvm_handling_nmi_from_guest(vcpu))
                return 0;

        kvm_make_request(KVM_REQ_PMI, vcpu);

Thanks,
Yanfei

-----Original Message-----
From: Sean Christopherson <seanjc@xxxxxxxxxx> 
Sent: Monday, May 16, 2022 9:58 PM
To: Xu, Yanfei <yanfei.xu@xxxxxxxxx>
Cc: pbonzini@xxxxxxxxxx; vkuznets@xxxxxxxxxx; wanpengli@xxxxxxxxxxx; jmattson@xxxxxxxxxx; joro@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] KVM: x86: Fix the intel_pt PMI handling wrongly considered from guest

On Mon, May 16, 2022, Yanfei Xu wrote:
> When kernel handles the vm-exit caused by external interrupts and PMI, 
> it always set a type of kvm_intr_type to handling_intr_from_guest to 
> tell if it's dealing an IRQ or NMI.
> However, the further type judgment is missing in kvm_arch_pmi_in_guest().
> It could make the PMI of intel_pt wrongly considered it comes from a 
> guest once the PMI breaks the handling of vm-exit of external interrupts.
> 
> Fixes: db215756ae59 ("KVM: x86: More precisely identify NMI from guest 
> when handling PMI")
> Signed-off-by: Yanfei Xu <yanfei.xu@xxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h | 8 +++++++-
>  arch/x86/kvm/x86.h              | 6 ------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h 
> b/arch/x86/include/asm/kvm_host.h index 4ff36610af6a..308cf19f123d 
> 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1582,8 +1582,14 @@ static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm)
>  		return -ENOTSUPP;
>  }
>  
> +enum kvm_intr_type {
> +	/* Values are arbitrary, but must be non-zero. */
> +	KVM_HANDLING_IRQ = 1,
> +	KVM_HANDLING_NMI,
> +};
> +
>  #define kvm_arch_pmi_in_guest(vcpu) \
> -	((vcpu) && (vcpu)->arch.handling_intr_from_guest)
> +	((vcpu) && (vcpu)->arch.handling_intr_from_guest == 
> +KVM_HANDLING_NMI)

My understanding is that this isn't correct as a general change, as perf events can use regular IRQs in some cases.  See commit dd60d217062f4 ("KVM: x86: Fix perf timer mode IP reporting").

I assume there's got to be a way to know which mode perf is using, e.g. we should be able to make this look something like:

	((vcpu) && (vcpu)->arch.handling_intr_from_guest == kvm_pmi_vector)


>  void kvm_mmu_x86_module_init(void);
>  int kvm_mmu_vendor_module_init(void); diff --git a/arch/x86/kvm/x86.h 
> b/arch/x86/kvm/x86.h index 588792f00334..3bdf1bc76863 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -344,12 +344,6 @@ static inline bool kvm_cstate_in_guest(struct kvm *kvm)
>  	return kvm->arch.cstate_in_guest;
>  }
>  
> -enum kvm_intr_type {
> -	/* Values are arbitrary, but must be non-zero. */
> -	KVM_HANDLING_IRQ = 1,
> -	KVM_HANDLING_NMI,
> -};
> -
>  static inline void kvm_before_interrupt(struct kvm_vcpu *vcpu,
>  					enum kvm_intr_type intr)
>  {
> --
> 2.32.0
>