* Re: [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible
@ 2020-02-21 6:35 linmiaohe
0 siblings, 0 replies; 5+ messages in thread
From: linmiaohe @ 2020-02-21 6:35 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Sean Christopherson <sean.j.christopherson@intel.com> writes:
>Use vpid_sync_context() directly for flows that run if and only if enable_vpid=1, or more specifically, nested VMX flows that are gated by
>vmx->nested.msrs.secondary_ctls_high.SECONDARY_EXEC_ENABLE_VPID being
>set, which is allowed if and only if enable_vpid=1. Because these flows call __vmx_flush_tlb() with @invalidate_gpa=false, the if-statement that decides between INVEPT and >INVVPID will always go down the INVVPID path, i.e. call vpid_sync_context() because "enable_ept && (invalidate_gpa || !enable_vpid)" always evaluates false.
>
>This helps pave the way toward removing @invalidate_gpa and @vpid from
>__vmx_flush_tlb() and its callers.
>
>No functional change intended.
>
>Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 00/10] KVM: x86: Clean up VMX's TLB flushing code
@ 2020-02-20 20:43 Sean Christopherson
2020-02-20 20:43 ` [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2020-02-20 20:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
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(-)
--
2.24.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible
2020-02-20 20:43 [PATCH 00/10] KVM: x86: Clean up VMX's TLB flushing code Sean Christopherson
@ 2020-02-20 20:43 ` Sean Christopherson
2020-02-21 13:17 ` Vitaly Kuznetsov
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2020-02-20 20:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Use vpid_sync_context() directly for flows that run if and only if
enable_vpid=1, or more specifically, nested VMX flows that are gated by
vmx->nested.msrs.secondary_ctls_high.SECONDARY_EXEC_ENABLE_VPID being
set, which is allowed if and only if enable_vpid=1. Because these flows
call __vmx_flush_tlb() with @invalidate_gpa=false, the if-statement that
decides between INVEPT and INVVPID will always go down the INVVPID path,
i.e. call vpid_sync_context() because
"enable_ept && (invalidate_gpa || !enable_vpid)" always evaluates false.
This helps pave the way toward removing @invalidate_gpa and @vpid from
__vmx_flush_tlb() and its callers.
No functional change intended.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 657c2eda357c..19ac4083667f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2466,7 +2466,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu)) {
if (vmcs12->virtual_processor_id != vmx->nested.last_vpid) {
vmx->nested.last_vpid = vmcs12->virtual_processor_id;
- __vmx_flush_tlb(vcpu, nested_get_vpid02(vcpu), false);
+ vpid_sync_context(nested_get_vpid02(vcpu));
}
} else {
/*
@@ -5154,17 +5154,17 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
__invvpid(VMX_VPID_EXTENT_INDIVIDUAL_ADDR,
vpid02, operand.gla);
} else
- __vmx_flush_tlb(vcpu, vpid02, false);
+ vpid_sync_context(vpid02);
break;
case VMX_VPID_EXTENT_SINGLE_CONTEXT:
case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
if (!operand.vpid)
return nested_vmx_failValid(vcpu,
VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
- __vmx_flush_tlb(vcpu, vpid02, false);
+ vpid_sync_context(vpid02);
break;
case VMX_VPID_EXTENT_ALL_CONTEXT:
- __vmx_flush_tlb(vcpu, vpid02, false);
+ vpid_sync_context(vpid02);
break;
default:
WARN_ON_ONCE(1);
--
2.24.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible
2020-02-20 20:43 ` [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible Sean Christopherson
@ 2020-02-21 13:17 ` Vitaly Kuznetsov
2020-02-21 15:36 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-21 13:17 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel
Sean Christopherson <sean.j.christopherson@intel.com> writes:
> Use vpid_sync_context() directly for flows that run if and only if
> enable_vpid=1, or more specifically, nested VMX flows that are gated by
> vmx->nested.msrs.secondary_ctls_high.SECONDARY_EXEC_ENABLE_VPID being
> set, which is allowed if and only if enable_vpid=1. Because these flows
> call __vmx_flush_tlb() with @invalidate_gpa=false, the if-statement that
> decides between INVEPT and INVVPID will always go down the INVVPID path,
> i.e. call vpid_sync_context() because
> "enable_ept && (invalidate_gpa || !enable_vpid)" always evaluates false.
>
> This helps pave the way toward removing @invalidate_gpa and @vpid from
> __vmx_flush_tlb() and its callers.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kvm/vmx/nested.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 657c2eda357c..19ac4083667f 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2466,7 +2466,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu)) {
> if (vmcs12->virtual_processor_id != vmx->nested.last_vpid) {
> vmx->nested.last_vpid = vmcs12->virtual_processor_id;
> - __vmx_flush_tlb(vcpu, nested_get_vpid02(vcpu), false);
> + vpid_sync_context(nested_get_vpid02(vcpu));
> }
> } else {
> /*
> @@ -5154,17 +5154,17 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
> __invvpid(VMX_VPID_EXTENT_INDIVIDUAL_ADDR,
> vpid02, operand.gla);
> } else
> - __vmx_flush_tlb(vcpu, vpid02, false);
> + vpid_sync_context(vpid02);
This is a pre-existing condition but coding style requires braces even
for single statements when they were used in another branch.
> break;
> case VMX_VPID_EXTENT_SINGLE_CONTEXT:
> case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
> if (!operand.vpid)
> return nested_vmx_failValid(vcpu,
> VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> - __vmx_flush_tlb(vcpu, vpid02, false);
> + vpid_sync_context(vpid02);
> break;
> case VMX_VPID_EXTENT_ALL_CONTEXT:
> - __vmx_flush_tlb(vcpu, vpid02, false);
> + vpid_sync_context(vpid02);
> break;
> default:
> WARN_ON_ONCE(1);
Seems to be no change indeed,
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible
2020-02-21 13:17 ` Vitaly Kuznetsov
@ 2020-02-21 15:36 ` Sean Christopherson
2020-02-21 17:26 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2020-02-21 15:36 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel
On Fri, Feb 21, 2020 at 02:17:46PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
>
> > Use vpid_sync_context() directly for flows that run if and only if
> > enable_vpid=1, or more specifically, nested VMX flows that are gated by
> > vmx->nested.msrs.secondary_ctls_high.SECONDARY_EXEC_ENABLE_VPID being
> > set, which is allowed if and only if enable_vpid=1. Because these flows
> > call __vmx_flush_tlb() with @invalidate_gpa=false, the if-statement that
> > decides between INVEPT and INVVPID will always go down the INVVPID path,
> > i.e. call vpid_sync_context() because
> > "enable_ept && (invalidate_gpa || !enable_vpid)" always evaluates false.
> >
> > This helps pave the way toward removing @invalidate_gpa and @vpid from
> > __vmx_flush_tlb() and its callers.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> > arch/x86/kvm/vmx/nested.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 657c2eda357c..19ac4083667f 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2466,7 +2466,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> > if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu)) {
> > if (vmcs12->virtual_processor_id != vmx->nested.last_vpid) {
> > vmx->nested.last_vpid = vmcs12->virtual_processor_id;
> > - __vmx_flush_tlb(vcpu, nested_get_vpid02(vcpu), false);
> > + vpid_sync_context(nested_get_vpid02(vcpu));
> > }
> > } else {
> > /*
> > @@ -5154,17 +5154,17 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
> > __invvpid(VMX_VPID_EXTENT_INDIVIDUAL_ADDR,
> > vpid02, operand.gla);
> > } else
> > - __vmx_flush_tlb(vcpu, vpid02, false);
> > + vpid_sync_context(vpid02);
>
> This is a pre-existing condition but coding style requires braces even
> for single statements when they were used in another branch.
I'll fix this in v2.
> > break;
> > case VMX_VPID_EXTENT_SINGLE_CONTEXT:
> > case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
> > if (!operand.vpid)
> > return nested_vmx_failValid(vcpu,
> > VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> > - __vmx_flush_tlb(vcpu, vpid02, false);
> > + vpid_sync_context(vpid02);
> > break;
> > case VMX_VPID_EXTENT_ALL_CONTEXT:
> > - __vmx_flush_tlb(vcpu, vpid02, false);
> > + vpid_sync_context(vpid02);
> > break;
> > default:
> > WARN_ON_ONCE(1);
>
> Seems to be no change indeed,
Heh, that's about the same level of confidence I had :-)
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> --
> Vitaly
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible
2020-02-21 15:36 ` Sean Christopherson
@ 2020-02-21 17:26 ` Paolo Bonzini
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2020-02-21 17:26 UTC (permalink / raw)
To: Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel
On 21/02/20 16:36, Sean Christopherson wrote:
>>> vmx->nested.last_vpid = vmcs12->virtual_processor_id;
>>> - __vmx_flush_tlb(vcpu, nested_get_vpid02(vcpu), false);
>>> + vpid_sync_context(nested_get_vpid02(vcpu));
>>> }
>>> } else {
>>> /*
>>> @@ -5154,17 +5154,17 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>>> __invvpid(VMX_VPID_EXTENT_INDIVIDUAL_ADDR,
>>> vpid02, operand.gla);
>>> } else
>>> - __vmx_flush_tlb(vcpu, vpid02, false);
>>> + vpid_sync_context(vpid02);
>> This is a pre-existing condition but coding style requires braces even
>> for single statements when they were used in another branch.
> I'll fix this in v2.
>
Can also remove the braces from the "then" branch.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-21 17:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-21 6:35 [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible linmiaohe
-- strict thread matches above, loose matches on Subject: below --
2020-02-20 20:43 [PATCH 00/10] KVM: x86: Clean up VMX's TLB flushing code Sean Christopherson
2020-02-20 20:43 ` [PATCH 01/10] KVM: VMX: Use vpid_sync_context() directly when possible Sean Christopherson
2020-02-21 13:17 ` Vitaly Kuznetsov
2020-02-21 15:36 ` Sean Christopherson
2020-02-21 17:26 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®