Re: [RFC PATCH] kvm/x86: Keep root hpa in prev_roots as much as possible

On Fri, Jul 30, 2021 at 2:06 AM Sean Christopherson [off-list ref] wrote:

Hello

I think it is too complicated.  And it is hard to accept to put "unsync"
out of struct kvm_mmu_root_info when they should be bound to each other.

How about this:
- KVM_MMU_NUM_PREV_ROOTS
+ KVM_MMU_NUM_CACHED_ROOTS
- mmu->prev_roots[KVM_MMU_NUM_PREV_ROOTS]
+ mmu->cached_roots[KVM_MMU_NUM_CACHED_ROOTS]
- mmu->root_hpa
+ mmu->cached_roots[0].hpa
- mmu->root_pgd
+ mmu->cached_roots[0].pgd

And using the bit63 in @pgd as the information that it is not requested
to sync since the last sync.

Thanks
Lai.

        /*

quoted

 arch/x86/include/asm/kvm_host.h |  3 ++-
 arch/x86/kvm/mmu/mmu.c          |  6 ++++++
 arch/x86/kvm/vmx/nested.c       | 12 ++++--------
 arch/x86/kvm/x86.c              |  9 +++++----
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 55efbacfc244..19a337cf7aa6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h

@@ -354,10 +354,11 @@ struct rsvd_bits_validate {
 struct kvm_mmu_root_info {
      gpa_t pgd;
      hpa_t hpa;
+     bool need_sync;

Hmm, use "unsync" instead of "need_sync", purely to match the existing terminology
in KVM's MMU for this sort of behavior.

quoted

 };

 #define KVM_MMU_ROOT_INFO_INVALID \
-     ((struct kvm_mmu_root_info) { .pgd = INVALID_PAGE, .hpa = INVALID_PAGE })
+     ((struct kvm_mmu_root_info) { .pgd = INVALID_PAGE, .hpa = INVALID_PAGE, .need_sync = true})

 #define KVM_MMU_NUM_PREV_ROOTS 3

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 5e60b00e8e50..147827135549 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c

@@ -3878,6 +3878,7 @@ static bool cached_root_available(struct kvm_vcpu *vcpu, gpa_t new_pgd,

      root.pgd = mmu->root_pgd;
      root.hpa = mmu->root_hpa;
+     root.need_sync = false;

      if (is_root_usable(&root, new_pgd, new_role))
              return true;

@@ -3892,6 +3893,11 @@ static bool cached_root_available(struct kvm_vcpu *vcpu, gpa_t new_pgd,
      mmu->root_hpa = root.hpa;
      mmu->root_pgd = root.pgd;

+     if (i < KVM_MMU_NUM_PREV_ROOTS && root.need_sync) {

Probably makes sense to write this as:

        if (i >= KVM_MMU_NUM_PREV_ROOTS)
                return false;

        if (root.need_sync) {
                kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
                kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
        }
        return true;

The "i < KVM_MMU_NUM_PREV_ROOTS == success" logic is just confusing enough that
it'd be nice to write it only once.

And that would also play nicely with deferring a sync for the "current" root
(see below), e.g.

        ...
        unsync = mmu->root_unsync;

        if (is_root_usable(&root, new_pgd, new_role))
                goto found_root;

        for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
                swap(root, mmu->prev_roots[i]);
                swap(unsync, mmu->unsync_roots[i]);

                if (is_root_usable(&root, new_pgd, new_role))
                        break;
        }

        if (i >= KVM_MMU_NUM_PREV_ROOTS)
                return false;

found_root:
        if (unsync) {
                kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
                kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
        }
        return true;

quoted

+             kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
+             kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
+     }
+
      return i < KVM_MMU_NUM_PREV_ROOTS;
 }

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 6058a65a6ede..ab7069ac6dc5 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c

@@ -5312,7 +5312,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 {
      struct vcpu_vmx *vmx = to_vmx(vcpu);
      u32 vmx_instruction_info, types;
-     unsigned long type, roots_to_free;
+     unsigned long type;
      struct kvm_mmu *mmu;
      gva_t gva;
      struct x86_exception e;

@@ -5361,29 +5361,25 @@ static int handle_invept(struct kvm_vcpu *vcpu)
                      return nested_vmx_fail(vcpu,
                              VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);

-             roots_to_free = 0;
              if (nested_ept_root_matches(mmu->root_hpa, mmu->root_pgd,
                                          operand.eptp))
-                     roots_to_free |= KVM_MMU_ROOT_CURRENT;
+                     kvm_mmu_free_roots(vcpu, mmu, KVM_MMU_ROOT_CURRENT);

For a non-RFC series, I think this should do two things:

  1. Separate INVEPT from INVPCID, i.e. do only INVPCID first.
  2. Enhance INVEPT to SYNC+FLUSH the current root instead of freeing it

As alluded to above, this can be done by deferring the sync+flush (which can't
be done right away because INVEPT runs in L1 context, whereas KVM needs to sync+flush
L2 EPT context).

quoted

              for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
                      if (nested_ept_root_matches(mmu->prev_roots[i].hpa,
                                                  mmu->prev_roots[i].pgd,
                                                  operand.eptp))
-                             roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
+                             mmu->prev_roots[i].need_sync = true;
              }
              break;
      case VMX_EPT_EXTENT_GLOBAL:
-             roots_to_free = KVM_MMU_ROOTS_ALL;
+             kvm_mmu_free_roots(vcpu, mmu, KVM_MMU_ROOTS_ALL);
              break;
      default:
              BUG();
              break;
      }

-     if (roots_to_free)
-             kvm_mmu_free_roots(vcpu, mmu, roots_to_free);
-
      return nested_vmx_succeed(vcpu);
 }