[PATCH] kvm: vmx: Sync all matching EPTPs when injecting nested EPT fault

[PATCH] kvm: vmx: Sync all matching EPTPs when injecting nested EPT fault

[Junaid Shahid]

When a nested EPT violation/misconfig is injected into the guest,
the shadow EPT PTEs associated with that address need to be synced.
This is done by kvm_inject_emulated_page_fault() before it calls
nested_ept_inject_page_fault(). However, that will only sync the
shadow EPT PTE associated with the current L1 EPTP. Since the ASID
is based on EP4TA rather than the full EPTP, so syncing the current
EPTP is not enough. The SPTEs associated with any other L1 EPTPs
in the prev_roots cache with the same EP4TA also need to be synced.

Signed-off-by: Junaid Shahid <junaids@google.com>
---
 arch/x86/kvm/vmx/nested.c | 53 ++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1a52134b0c42..cd506af1d1b6 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -330,6 +330,31 @@ void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	vcpu_put(vcpu);
 }
 
+#define EPTP_PA_MASK   GENMASK_ULL(51, 12)
+
+static bool nested_ept_root_matches(hpa_t root_hpa, u64 root_eptp, u64 eptp)
+{
+	return VALID_PAGE(root_hpa) &&
+	       ((root_eptp & EPTP_PA_MASK) == (eptp & EPTP_PA_MASK));
+}
+
+static void nested_ept_invalidate_addr(struct kvm_vcpu *vcpu, gpa_t eptp,
+				       gpa_t addr)
+{
+	uint i;
+	struct kvm_mmu_root_info *cached_root;
+
+	WARN_ON_ONCE(!mmu_is_nested(vcpu));
+
+	for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
+		cached_root = &vcpu->arch.mmu->prev_roots[i];
+
+		if (nested_ept_root_matches(cached_root->hpa, cached_root->pgd,
+					    eptp))
+			vcpu->arch.mmu->invlpg(vcpu, addr, cached_root->hpa);
+	}
+}
+
 static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
 		struct x86_exception *fault)
 {
@@ -342,10 +367,22 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
 		vm_exit_reason = EXIT_REASON_PML_FULL;
 		vmx->nested.pml_full = false;
 		exit_qualification &= INTR_INFO_UNBLOCK_NMI;
-	} else if (fault->error_code & PFERR_RSVD_MASK)
-		vm_exit_reason = EXIT_REASON_EPT_MISCONFIG;
-	else
-		vm_exit_reason = EXIT_REASON_EPT_VIOLATION;
+	} else {
+		if (fault->error_code & PFERR_RSVD_MASK)
+			vm_exit_reason = EXIT_REASON_EPT_MISCONFIG;
+		else
+			vm_exit_reason = EXIT_REASON_EPT_VIOLATION;
+
+		/*
+		 * Although the caller (kvm_inject_emulated_page_fault) would
+		 * have already synced the faulting address in the shadow EPT
+		 * tables for the current L1 EPTP, we also need to sync it for
+		 * any other cached L1 EPTPs that share the same EP4TA, since
+		 * the ASID is derived from the EP4TA rather than the full EPTP.
+		 */
+		nested_ept_invalidate_addr(vcpu, vmcs12->ept_pointer,
+					   fault->address);
+	}
 
 	nested_vmx_vmexit(vcpu, vm_exit_reason, 0, exit_qualification);
 	vmcs12->guest_physical_address = fault->address;
@@ -5325,14 +5362,6 @@ static int handle_vmptrst(struct kvm_vcpu *vcpu)
 	return nested_vmx_succeed(vcpu);
 }
 
-#define EPTP_PA_MASK   GENMASK_ULL(51, 12)
-
-static bool nested_ept_root_matches(hpa_t root_hpa, u64 root_eptp, u64 eptp)
-{
-	return VALID_PAGE(root_hpa) &&
-		((root_eptp & EPTP_PA_MASK) == (eptp & EPTP_PA_MASK));
-}
-
 /* Emulate the INVEPT instruction */
 static int handle_invept(struct kvm_vcpu *vcpu)
 {
-- 
2.32.0.605.g8dce9f2422-goog

Re: [PATCH] kvm: vmx: Sync all matching EPTPs when injecting nested EPT fault

[Sean Christopherson]

On Fri, Aug 06, 2021, Junaid Shahid wrote:
> When a nested EPT violation/misconfig is injected into the guest,
> the shadow EPT PTEs associated with that address need to be synced.
> This is done by kvm_inject_emulated_page_fault() before it calls
> nested_ept_inject_page_fault(). However, that will only sync the
> shadow EPT PTE associated with the current L1 EPTP. Since the ASID

For the changelog and the comment, IMO using "vmcs12 EPTP" instead of "L1 EPTP"
would add clarity.  I usually think of "L1 EPTP" as vmcs01->eptp and "L2 EPTP"
as vmcs02->EPTP.  There are enough EPTPs in play with nested that it'd help to
be very explicit.

> is based on EP4TA rather than the full EPTP, so syncing the current
> EPTP is not enough. The SPTEs associated with any other L1 EPTPs
> in the prev_roots cache with the same EP4TA also need to be synced.

No small part of me wonders if we should disallow duplicate vmcs12 EP4TAs in a
single vCPU's root cache, e.g. purge existing roots with the same pgd but
different role.  INVEPT does the right thing, but that seems more coincidental
than intentional.

Practically speaking, this only affects A/D bits.  Wouldn't a VMM need to flush
the EP4TA if it toggled A/D enabling in order to have deterministic behavior?
In other words, is there a real world use case for switching between EPTPs with
same EP4TAs but different properties that would see a performance hit if KVM
purged unusable cached roots with the same EP4TA?

Re: [PATCH] kvm: vmx: Sync all matching EPTPs when injecting nested EPT fault

[Paolo Bonzini]

On 10/08/21 01:33, Sean Christopherson wrote:
> On Fri, Aug 06, 2021, Junaid Shahid wrote:
>> When a nested EPT violation/misconfig is injected into the guest,
>> the shadow EPT PTEs associated with that address need to be synced.
>> This is done by kvm_inject_emulated_page_fault() before it calls
>> nested_ept_inject_page_fault(). However, that will only sync the
>> shadow EPT PTE associated with the current L1 EPTP. Since the ASID
> 
> For the changelog and the comment, IMO using "vmcs12 EPTP" instead of "L1 EPTP"
> would add clarity.  I usually think of "L1 EPTP" as vmcs01->eptp and "L2 EPTP"
> as vmcs02->EPTP.  There are enough EPTPs in play with nested that it'd help to
> be very explicit.

Or more briefly "EPT12".

>> is based on EP4TA rather than the full EPTP, so syncing the current
>> EPTP is not enough. The SPTEs associated with any other L1 EPTPs
>> in the prev_roots cache with the same EP4TA also need to be synced.
> 
> No small part of me wonders if we should disallow duplicate vmcs12 EP4TAs in a
> single vCPU's root cache, e.g. purge existing roots with the same pgd but
> different role.  INVEPT does the right thing, but that seems more coincidental
> than intentional.
> 
> Practically speaking, this only affects A/D bits.  Wouldn't a VMM need to flush
> the EP4TA if it toggled A/D enabling in order to have deterministic behavior?
> In other words, is there a real world use case for switching between EPTPs with
> same EP4TAs but different properties that would see a performance hit if KVM
> purged unusable cached roots with the same EP4TA?

Probably not, but the complexity wouldn't be much different.

Paolo

Re: [PATCH] kvm: vmx: Sync all matching EPTPs when injecting nested EPT fault

[Junaid Shahid]

On Tue, Aug 10, 2021 at 10:52 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 10/08/21 01:33, Sean Christopherson wrote:
> > On Fri, Aug 06, 2021, Junaid Shahid wrote:
> >> When a nested EPT violation/misconfig is injected into the guest,
> >> the shadow EPT PTEs associated with that address need to be synced.
> >> This is done by kvm_inject_emulated_page_fault() before it calls
> >> nested_ept_inject_page_fault(). However, that will only sync the
> >> shadow EPT PTE associated with the current L1 EPTP. Since the ASID
> >
> > For the changelog and the comment, IMO using "vmcs12 EPTP" instead of "L1 EPTP"
> > would add clarity.  I usually think of "L1 EPTP" as vmcs01->eptp and "L2 EPTP"
> > as vmcs02->EPTP.  There are enough EPTPs in play with nested that it'd help to
> > be very explicit.
>
> Or more briefly "EPT12".

Sounds good.

>
> >> is based on EP4TA rather than the full EPTP, so syncing the current
> >> EPTP is not enough. The SPTEs associated with any other L1 EPTPs
> >> in the prev_roots cache with the same EP4TA also need to be synced.
> >
> > No small part of me wonders if we should disallow duplicate vmcs12 EP4TAs in a
> > single vCPU's root cache, e.g. purge existing roots with the same pgd but
> > different role.  INVEPT does the right thing, but that seems more coincidental
> > than intentional.
> >
> > Practically speaking, this only affects A/D bits.  Wouldn't a VMM need to flush
> > the EP4TA if it toggled A/D enabling in order to have deterministic behavior?
> > In other words, is there a real world use case for switching between EPTPs with
> > same EP4TAs but different properties that would see a performance hit if KVM
> > purged unusable cached roots with the same EP4TA?
>
> Probably not, but the complexity wouldn't be much different.
>

I also don't know of a real world use case like that, so either way
would work. But I agree that it likely wouldn't be that much simpler.
So I guess I'll just send a v2 with the clearer terminology, unless
anyone really prefers disallowing duplicate EP4TAs in the root cache.

Thanks,
Junaid

[PATCH] kvm: vmx: Sync all matching EPTPs when injecting nested EPT fault

[Junaid Shahid]

When a nested EPT violation/misconfig is injected into the guest,
the shadow EPT PTEs associated with that address need to be synced.
This is done by kvm_inject_emulated_page_fault() before it calls
nested_ept_inject_page_fault(). However, that will only sync the
shadow EPT PTE associated with the current L1 EPTP. Since the ASID
is based on EP4TA rather than the full EPTP, so syncing the current
EPTP is not enough. The SPTEs associated with any other L1 EPTPs
in the prev_roots cache with the same EP4TA also need to be synced.

Signed-off-by: Junaid Shahid <junaids@google.com>
---
 arch/x86/kvm/vmx/nested.c | 53 ++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1a52134b0c42..cd506af1d1b6 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -330,6 +330,31 @@ void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	vcpu_put(vcpu);
 }
 
+#define EPTP_PA_MASK   GENMASK_ULL(51, 12)
+
+static bool nested_ept_root_matches(hpa_t root_hpa, u64 root_eptp, u64 eptp)
+{
+	return VALID_PAGE(root_hpa) &&
+	       ((root_eptp & EPTP_PA_MASK) == (eptp & EPTP_PA_MASK));
+}
+
+static void nested_ept_invalidate_addr(struct kvm_vcpu *vcpu, gpa_t eptp,
+				       gpa_t addr)
+{
+	uint i;
+	struct kvm_mmu_root_info *cached_root;
+
+	WARN_ON_ONCE(!mmu_is_nested(vcpu));
+
+	for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
+		cached_root = &vcpu->arch.mmu->prev_roots[i];
+
+		if (nested_ept_root_matches(cached_root->hpa, cached_root->pgd,
+					    eptp))
+			vcpu->arch.mmu->invlpg(vcpu, addr, cached_root->hpa);
+	}
+}
+
 static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
 		struct x86_exception *fault)
 {
@@ -342,10 +367,22 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
 		vm_exit_reason = EXIT_REASON_PML_FULL;
 		vmx->nested.pml_full = false;
 		exit_qualification &= INTR_INFO_UNBLOCK_NMI;
-	} else if (fault->error_code & PFERR_RSVD_MASK)
-		vm_exit_reason = EXIT_REASON_EPT_MISCONFIG;
-	else
-		vm_exit_reason = EXIT_REASON_EPT_VIOLATION;
+	} else {
+		if (fault->error_code & PFERR_RSVD_MASK)
+			vm_exit_reason = EXIT_REASON_EPT_MISCONFIG;
+		else
+			vm_exit_reason = EXIT_REASON_EPT_VIOLATION;
+
+		/*
+		 * Although the caller (kvm_inject_emulated_page_fault) would
+		 * have already synced the faulting address in the shadow EPT
+		 * tables for the current L1 EPTP, we also need to sync it for
+		 * any other cached L1 EPTPs that share the same EP4TA, since
+		 * the ASID is derived from the EP4TA rather than the full EPTP.
+		 */
+		nested_ept_invalidate_addr(vcpu, vmcs12->ept_pointer,
+					   fault->address);
+	}
 
 	nested_vmx_vmexit(vcpu, vm_exit_reason, 0, exit_qualification);
 	vmcs12->guest_physical_address = fault->address;
@@ -5325,14 +5362,6 @@ static int handle_vmptrst(struct kvm_vcpu *vcpu)
 	return nested_vmx_succeed(vcpu);
 }
 
-#define EPTP_PA_MASK   GENMASK_ULL(51, 12)
-
-static bool nested_ept_root_matches(hpa_t root_hpa, u64 root_eptp, u64 eptp)
-{
-	return VALID_PAGE(root_hpa) &&
-		((root_eptp & EPTP_PA_MASK) == (eptp & EPTP_PA_MASK));
-}
-
 /* Emulate the INVEPT instruction */
 static int handle_invept(struct kvm_vcpu *vcpu)
 {
-- 
2.32.0.605.g8dce9f2422-goog