lkml.org 
[lkml]   [2023]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] KVM: x86/intr: Explicitly check NMI from guest to eliminate false positives
    From
    Date
    On Wed, 2023-12-06 at 11:20 +0800, Like Xu wrote:
    > From: Like Xu <likexu@tencent.com>
    >
    > Explicitly checking the source of external interrupt is indeed NMI and not
    > other types in the kvm_arch_pmi_in_guest(), which prevents perf-kvm false
    > positive samples generated in perf/core NMI mode after vm-exit but before
    > kvm_before_interrupt() from being incorrectly labelled as guest samples:
    >
    > # test: perf-record + cpu-cycles:HP (which collects host-only precise samples)
    > # Symbol Overhead sys usr guest sys guest usr
    > # ....................................... ........ ........ ........ ......... .........
    > #
    > # Before:
    > [g] entry_SYSCALL_64 24.63% 0.00% 0.00% 24.63% 0.00%
    > [g] syscall_return_via_sysret 23.23% 0.00% 0.00% 23.23% 0.00%
    > [g] files_lookup_fd_raw 6.35% 0.00% 0.00% 6.35% 0.00%
    > # After:
    > [k] perf_adjust_freq_unthr_context 57.23% 57.23% 0.00% 0.00% 0.00%
    > [k] __vmx_vcpu_run 4.09% 4.09% 0.00% 0.00% 0.00%
    > [k] vmx_update_host_rsp 3.17% 3.17% 0.00% 0.00% 0.00%
    >
    > In the above case, perf records the samples labelled '[g]', the RIPs behind
    > the weird samples are actually being queried by perf_instruction_pointer()
    > after determining whether it's in GUEST state or not, and here's the issue:
    >
    > If vm-exit is caused by a non-NMI interrupt (such as hrtimer_interrupt) and
    > at least one PMU counter is enabled on host, the kvm_arch_pmi_in_guest()
    > will remain true (KVM_HANDLING_IRQ is set) until kvm_before_interrupt().
    >
    > During this window, if a PMI occurs on host (since the KVM instructions on
    > host are being executed), the control flow, with the help of the host NMI
    > context, will be transferred to perf/core to generate performance samples,
    > thus perf_instruction_pointer() and perf_guest_get_ip() is called.
    >
    > Since kvm_arch_pmi_in_guest() only checks if there is an interrupt, it may
    > cause perf/core to mistakenly assume that the source RIP of the host NMI
    > belongs to the guest world and use perf_guest_get_ip() to get the RIP of
    > a vCPU that has already exited by a non-NMI interrupt.
    >
    > Error samples are recorded and presented to the end-user via perf-report.
    > Such false positive samples could be eliminated by explicitly determining
    > if the exit reason is KVM_HANDLING_NMI.
    >
    > Note that when vm-exit is indeed triggered by PMI and before HANDLING_NMI
    > is cleared, it's also still possible that another PMI is generated on host.
    > Also for perf/core timer mode, the false positives are still possible since
    > that non-NMI sources of interrupts are not always being used by perf/core.
    > In both cases above, perf/core should correctly distinguish between real
    > RIP sources or even need to generate two samples, belonging to host and
    > guest separately, but that's perf/core's story for interested warriors.
    >
    > Fixes: dd60d217062f ("KVM: x86: Fix perf timer mode IP reporting")
    > Signed-off-by: Like Xu <likexu@tencent.com>
    > ---
    > V1 -> V2 Changelog:
    > - Refine commit message to cover both perf/core timer and NMI modes;
    > - Use in_nmi() to distinguish whether it's NMI mode or not; (Sean)
    > V1: https://lore.kernel.org/kvm/20231204074535.9567-1-likexu@tencent.com/
    > arch/x86/include/asm/kvm_host.h | 10 +++++++++-
    > arch/x86/kvm/x86.h | 6 ------
    > 2 files changed, 9 insertions(+), 7 deletions(-)
    >
    > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
    > index c8c7e2475a18..167d592e08d0 100644
    > --- a/arch/x86/include/asm/kvm_host.h
    > +++ b/arch/x86/include/asm/kvm_host.h
    > @@ -1868,8 +1868,16 @@ static inline int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm, gfn_t gfn,
    > }
    > #endif /* CONFIG_HYPERV */
    >
    > +enum kvm_intr_type {
    > + /* Values are arbitrary, but must be non-zero. */
    > + KVM_HANDLING_IRQ = 1,
    > + KVM_HANDLING_NMI,
    > +};
    > +
    > +/* Enable perf NMI and timer modes to work, and minimise false positives. */
    > #define kvm_arch_pmi_in_guest(vcpu) \
    > - ((vcpu) && (vcpu)->arch.handling_intr_from_guest)
    > + ((vcpu) && (vcpu)->arch.handling_intr_from_guest && \
    > + (in_nmi() == ((vcpu)->arch.handling_intr_from_guest == KVM_HANDLING_NMI)))
    >
    > void __init 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 2f7e19166658..4dc38092d599 100644
    > --- a/arch/x86/kvm/x86.h
    > +++ b/arch/x86/kvm/x86.h
    > @@ -431,12 +431,6 @@ static inline bool kvm_notify_vmexit_enabled(struct kvm *kvm)
    > return kvm->arch.notify_vmexit_flags & KVM_X86_NOTIFY_VMEXIT_ENABLED;
    > }
    >
    > -enum kvm_intr_type {
    > - /* Values are arbitrary, but must be non-zero. */
    > - KVM_HANDLING_IRQ = 1,
    > - KVM_HANDLING_NMI,
    > -};
    > -
    > static __always_inline void kvm_before_interrupt(struct kvm_vcpu *vcpu,
    > enum kvm_intr_type intr)
    > {

    Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

    Best regards,
    Maxim Levitsky


    >
    > base-commit: 1ab097653e4dd8d23272d028a61352c23486fd4a


    \
     
     \ /
      Last update: 2023-12-07 16:34    [W:2.940 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site