linux-kernel - Re: [PATCH 00/10] KVM: x86: Clean up VMX's TLB flushing code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <efb07c80-58ab-c3ce-1fed-832475190add@redhat.com>
Date:   Fri, 21 Feb 2020 14:20:42 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/10] KVM: x86: Clean up VMX's TLB flushing code

On 20/02/20 21:43, Sean Christopherson wrote:
> This series is technically x86 wide, but it only superficially affects
> SVM, the motivation and primary touchpoints are all about VMX.
> 
> The goal of this series to ultimately clean up __vmx_flush_tlb(), which,
> for me, manages to be extremely confusing despite being only ten lines of
> code.
> 
> The most confusing aspect of __vmx_flush_tlb() is that it is overloaded
> for multiple uses:
> 
>  1) TLB flushes in response to a change in KVM's MMU
> 
>  2) TLB flushes during nested VM-Enter/VM-Exit when VPID is enabled
> 
>  3) Guest-scoped TLB flushes for paravirt TLB flushing
> 
> Handling (2) and (3) in the same flow as (1) is kludgy, because the rules
> for (1) are quite different than the rules for (2) and (3).  They're all
> squeezed into __vmx_flush_tlb() via the @invalidate_gpa param, which means
> "invalidate gpa mappings", not "invalidate a specific gpa"; it took me
> forever and a day to realize that.
> 
> To clean things up, handle (2) by directly calling vpid_sync_context()
> instead of bouncing through __vmx_flush_tlb(), and handle (3) via a
> dedicated kvm_x86_ops hook.  This allows for a less tricky implementation
> of vmx_flush_tlb() for (1), and (hopefully) clarifies the rules for what
> mappings must be invalidated when.
> 
> Sean Christopherson (10):
>   KVM: VMX: Use vpid_sync_context() directly when possible
>   KVM: VMX: Move vpid_sync_vcpu_addr() down a few lines
>   KVM: VMX: Handle INVVPID fallback logic in vpid_sync_vcpu_addr()
>   KVM: VMX: Fold vpid_sync_vcpu_{single,global}() into
>     vpid_sync_context()
>   KVM: nVMX: Use vpid_sync_vcpu_addr() to emulate INVVPID with address
>   KVM: x86: Move "flush guest's TLB" logic to separate kvm_x86_ops hook
>   KVM: VMX: Clean up vmx_flush_tlb_gva()
>   KVM: x86: Drop @invalidate_gpa param from kvm_x86_ops' tlb_flush()
>   KVM: VMX: Drop @invalidate_gpa from __vmx_flush_tlb()
>   KVM: VMX: Fold __vmx_flush_tlb() into vmx_flush_tlb()
> 
>  arch/x86/include/asm/kvm_host.h |  8 +++++++-
>  arch/x86/kvm/mmu/mmu.c          |  2 +-
>  arch/x86/kvm/svm.c              | 14 ++++++++++----
>  arch/x86/kvm/vmx/nested.c       | 12 ++++--------
>  arch/x86/kvm/vmx/ops.h          | 32 +++++++++-----------------------
>  arch/x86/kvm/vmx/vmx.c          | 26 +++++++++++++++++---------
>  arch/x86/kvm/vmx/vmx.h          | 19 ++++++++++---------
>  arch/x86/kvm/x86.c              |  8 ++++----
>  8 files changed, 62 insertions(+), 59 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@...hat.com>