| Message ID | 20200317045238.30434-24-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: TLB flushing fixes and enhancements | expand |
On 17/03/20 05:52, Sean Christopherson wrote: > + nested_vmx_transition_tlb_flush(vcpu, vmcs12); > + > + /* > + * There is no direct mapping between vpid02 and vpid12, vpid02 is > + * per-vCPU and reused for all nested vCPUs. If vpid12 is changing > + * then the new "virtual" VPID will reuse the same "real" VPID, > + * vpid02, and so needs to be sync'd. Skip the sync if a TLB flush > + * has already been requested, but always update the last used VPID. > + */ > + if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu) && > + vmcs12->virtual_processor_id != vmx->nested.last_vpid) { > + vmx->nested.last_vpid = vmcs12->virtual_processor_id; > + if (!kvm_test_request(KVM_REQ_TLB_FLUSH, vcpu)) > + vpid_sync_context(nested_get_vpid02(vcpu)); > } Would it make sense to move nested_vmx_transition_tlb_flush into an "else" branch? And should this also test that KVM_REQ_TLB_FLUSH_CURRENT is not set? Paolo
On Tue, Mar 17, 2020 at 06:17:59PM +0100, Paolo Bonzini wrote: > On 17/03/20 05:52, Sean Christopherson wrote: > > + nested_vmx_transition_tlb_flush(vcpu, vmcs12); > > + > > + /* > > + * There is no direct mapping between vpid02 and vpid12, vpid02 is > > + * per-vCPU and reused for all nested vCPUs. If vpid12 is changing > > + * then the new "virtual" VPID will reuse the same "real" VPID, > > + * vpid02, and so needs to be sync'd. Skip the sync if a TLB flush > > + * has already been requested, but always update the last used VPID. > > + */ > > + if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu) && > > + vmcs12->virtual_processor_id != vmx->nested.last_vpid) { > > + vmx->nested.last_vpid = vmcs12->virtual_processor_id; > > + if (!kvm_test_request(KVM_REQ_TLB_FLUSH, vcpu)) > > + vpid_sync_context(nested_get_vpid02(vcpu)); > > } > > Would it make sense to move nested_vmx_transition_tlb_flush into an > "else" branch? Maybe? I tried that at one point, but didn't like making the call to nested_vmx_transition_tlb_flush() conditional. My intent is to have the ...tlb_flush() call be standalone, i.e. logic that is common to all nested transitions, so that someone can look at the code can easily (relatively speaking) understand the basic rules for TLB flushing on nested transitions. I also tried the oppositie, i.e. putting the above code in an else-branch, with nested_vmx_transition_tlb_flush() returning true if it requested a flush. But that required updating vmx->nested.last_vpid in a separate flow, which was quite awkward. > And should this also test that KVM_REQ_TLB_FLUSH_CURRENT is not set? Doh, yes.
On 17/03/20 19:18, Sean Christopherson wrote: > On Tue, Mar 17, 2020 at 06:17:59PM +0100, Paolo Bonzini wrote: >> On 17/03/20 05:52, Sean Christopherson wrote: >>> + nested_vmx_transition_tlb_flush(vcpu, vmcs12); >>> + >>> + /* >>> + * There is no direct mapping between vpid02 and vpid12, vpid02 is >>> + * per-vCPU and reused for all nested vCPUs. If vpid12 is changing >>> + * then the new "virtual" VPID will reuse the same "real" VPID, >>> + * vpid02, and so needs to be sync'd. Skip the sync if a TLB flush >>> + * has already been requested, but always update the last used VPID. >>> + */ >>> + if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu) && >>> + vmcs12->virtual_processor_id != vmx->nested.last_vpid) { >>> + vmx->nested.last_vpid = vmcs12->virtual_processor_id; >>> + if (!kvm_test_request(KVM_REQ_TLB_FLUSH, vcpu)) >>> + vpid_sync_context(nested_get_vpid02(vcpu)); >>> } >> >> Would it make sense to move nested_vmx_transition_tlb_flush into an >> "else" branch? > > Maybe? I tried that at one point, but didn't like making the call to > nested_vmx_transition_tlb_flush() conditional. My intent is to have > the ...tlb_flush() call be standalone, i.e. logic that is common to all > nested transitions, so that someone can look at the code can easily > (relatively speaking) understand the basic rules for TLB flushing on > nested transitions. I think it's clear from the above code that we're handling a TLB flush in a way that doesn't require nested_vmx_transition_tlb_flush. But perhaps I didn't understand what you mean by "logic that is common to all nested transitions" and why you named it nested_vmx_transition_tlb_flush. Perhaps nested_vmx_transition_tlb_flush could grow a vmentry/vmexit bool argument instead? > I also tried the oppositie, i.e. putting the above code in an else-branch, > with nested_vmx_transition_tlb_flush() returning true if it requested a > flush. But that required updating vmx->nested.last_vpid in a separate > flow, which was quite awkward. No, that's awkward indeed. Paolo >> And should this also test that KVM_REQ_TLB_FLUSH_CURRENT is not set? > > Doh, yes. >
On Wed, Mar 18, 2020 at 11:45:57AM +0100, Paolo Bonzini wrote: > Perhaps nested_vmx_transition_tlb_flush could grow a vmentry/vmexit bool > argument instead? I'll give that a shot.
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 960ecbab5ebe..a7cc41e69948 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -1154,6 +1154,31 @@ static bool nested_has_guest_tlb_tag(struct kvm_vcpu *vcpu) (nested_cpu_has_vpid(vmcs12) && to_vmx(vcpu)->nested.vpid02); } +static void nested_vmx_transition_tlb_flush(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + /* + * If VPID is disabled, linear and combined mappings are flushed on + * VM-Enter/VM-Exit, and guest-physical mappings are valid only for + * their associated EPTP. + * + * If vmcs12 doesn't use VPID, L1 expects linear and combined mappings + * for *all* contexts to be flushed on VM-Enter/VM-Exit. + * + * If VPID is enabled and used by vmc12, but L2 does not have a unique + * TLB tag (ASID), i.e. EPT is disabled and KVM was unable to allocate + * a VPID for L2, flush the TLB as the effective ASID is common to both + * L1 and L2. + * + * Defer the flush so that it runs after vmcs02.EPTP has been set by + * KVM_REQ_LOAD_MMU_PGD (if nested EPT is enabled) and to avoid + * redundant flushes further down the nested pipeline. + */ + if (enable_vpid && + (!nested_cpu_has_vpid(vmcs12) || !nested_has_guest_tlb_tag(vcpu))) + kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); +} + static bool is_bitwise_subset(u64 superset, u64 subset, u64 mask) { superset &= mask; @@ -2462,31 +2487,20 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, if (kvm_has_tsc_control) decache_tsc_multiplier(vmx); - if (enable_vpid) { - /* - * There is no direct mapping between vpid02 and vpid12, the - * vpid02 is per-vCPU for L0 and reused while the value of - * vpid12 is changed w/ one invvpid during nested vmentry. - * The vpid12 is allocated by L1 for L2, so it will not - * influence global bitmap(for vpid01 and vpid02 allocation) - * even if spawn a lot of nested vCPUs. - */ - 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; - vpid_sync_context(nested_get_vpid02(vcpu)); - } - } else { - /* - * If L1 use EPT, then L0 needs to execute INVEPT on - * EPTP02 instead of EPTP01. Therefore, delay TLB - * flush until vmcs02->eptp is fully updated by - * KVM_REQ_LOAD_MMU_PGD. Note that this assumes - * KVM_REQ_TLB_FLUSH is evaluated after - * KVM_REQ_LOAD_MMU_PGD in vcpu_enter_guest(). - */ - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); - } + nested_vmx_transition_tlb_flush(vcpu, vmcs12); + + /* + * There is no direct mapping between vpid02 and vpid12, vpid02 is + * per-vCPU and reused for all nested vCPUs. If vpid12 is changing + * then the new "virtual" VPID will reuse the same "real" VPID, + * vpid02, and so needs to be sync'd. Skip the sync if a TLB flush + * has already been requested, but always update the last used VPID. + */ + if (nested_cpu_has_vpid(vmcs12) && nested_has_guest_tlb_tag(vcpu) && + vmcs12->virtual_processor_id != vmx->nested.last_vpid) { + vmx->nested.last_vpid = vmcs12->virtual_processor_id; + if (!kvm_test_request(KVM_REQ_TLB_FLUSH, vcpu)) + vpid_sync_context(nested_get_vpid02(vcpu)); } if (nested_cpu_has_ept(vmcs12)) @@ -4054,24 +4068,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, if (!enable_ept) vcpu->arch.walk_mmu->inject_page_fault = kvm_inject_page_fault; - /* - * If vmcs01 doesn't use VPID, CPU flushes TLB on every - * VMEntry/VMExit. Thus, no need to flush TLB. - * - * If vmcs12 doesn't use VPID, L1 expects TLB to be - * flushed on every VMEntry/VMExit. - * - * Otherwise, we can preserve TLB entries as long as we are - * able to tag L1 TLB entries differently than L2 TLB entries. - * - * If vmcs12 uses EPT, we need to execute this flush on EPTP01 - * and therefore we request the TLB flush to happen only after VMCS EPTP - * has been set by KVM_REQ_LOAD_MMU_PGD. - */ - if (enable_vpid && - (!nested_cpu_has_vpid(vmcs12) || !nested_has_guest_tlb_tag(vcpu))) { - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); - } + nested_vmx_transition_tlb_flush(vcpu, vmcs12); vmcs_write32(GUEST_SYSENTER_CS, vmcs12->host_ia32_sysenter_cs); vmcs_writel(GUEST_SYSENTER_ESP, vmcs12->host_ia32_sysenter_esp);
Add a helper to determine whether or not a full TLB flush needs to be performed on nested VM-Enter/VM-Exit, as the logic is identical for both flows and needs a fairly beefy comment to boot. This also provides a common point to make future adjustments to the logic. Skip the INVVPID if a TLB flush is pending, which mostly preserves the existing logic, but also skips INVVPID in the unlikely event that a TLB flush was requested for some other reason. Remove the explicit enable_vpid from prepare_vmcs02() as its implied by nested_cpu_has_vpid(), which can return true if and only if VPID is enabled in KVM (L0). Cc: Liran Alon <liran.alon@oracle.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/nested.c | 83 +++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 43 deletions(-)