[PATCH KVM 1/3] nSVM: Fix PAT in VMCB02 and check PAT in VMCB12

From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: kvm@vger.kernel.org
Cc: pbonzini@redhat.com, jmattson@google.com, mlevitsk@redhat.com
Subject: [PATCH KVM 1/3] nSVM: Fix PAT in VMCB02 and check PAT in VMCB12
Date: Tue, 18 Jan 2022 14:14:47 -0500	[thread overview]
Message-ID: <20220118191449.38852-2-krish.sadhukhan@oracle.com> (raw)
In-Reply-To: <20220118191449.38852-1-krish.sadhukhan@oracle.com>

Currently, KVM uses PAT from VMCB01 when launching nested guests, as well
as, when nested guests write MSR_IA32_CR_PAT. But section "Nested Paging
and VMRUN/#VMEXIT" in APM vol 2 states the following:

    "When VMRUN is executed with nested paging enabled (NP_ENABLE = 1),
     the paging registers are affected as follows:
	• VMRUN loads the guest paging state from the guest VMCB into the
	  guest registers (i.e., VMRUN loads CR3 with the VMCB CR3 field,
	  etc.). The guest PAT register is loaded from G_PAT field in the
	  VMCB."

Therefore, if we are launching nested guests, the PAT value from VMCB12
needs to be use in VMCB02 if nested paging is enabled in VMCB12 whereas the
PAT value from VMCB01 needs to used in VMCB02 if nested paging is disabled
in VMCB12.

However, when nested guets write MSR_IA32_CR_PAT, that register needs to
be updated only if nested paging is disabled in nested guests and the PAT
to be used is the one from VMCB12.

According to the same section in APM vol 2, the following guest state is
illegal:

	• Any G_PAT.PA field has an unsupported type encoding or any
	  reserved field in G_PAT has a nonzero value.

So, add checks for the PAT fields in VMCB12.


Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/nested.c | 34 +++++++++++++++++++++++++++-------
 arch/x86/kvm/svm/svm.c    |  3 ++-
 arch/x86/kvm/svm/svm.h    |  3 ++-
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index f8b7bc04b3e7..3283a58d5b0f 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -326,6 +326,17 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+noinline bool nested_vmcb_check_save_area(struct kvm_vcpu *vcpu,
+                                       struct vmcb_control_area *control,
+                                       struct vmcb_save_area *save)
+{
+	if (CC((control->nested_ctl  & SVM_NESTED_CTL_NP_ENABLE) &&
+	    !kvm_pat_valid(save->g_pat)))
+		return false;
+
+	return nested_vmcb_valid_sregs(vcpu, save);
+}
+
 void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 				     struct vmcb_control_area *control)
 {
@@ -452,21 +463,28 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
 	return 0;
 }
 
-void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm)
+void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm, u64 g_pat,
+				 u64 nested_ctl, bool from_vmrun)
 {
 	if (!svm->nested.vmcb02.ptr)
 		return;
 
-	/* FIXME: merge g_pat from vmcb01 and vmcb12.  */
-	svm->nested.vmcb02.ptr->save.g_pat = svm->vmcb01.ptr->save.g_pat;
+	if (from_vmrun) {
+		if (nested_ctl & SVM_NESTED_CTL_NP_ENABLE)
+			svm->nested.vmcb02.ptr->save.g_pat = g_pat;
+		else
+			svm->nested.vmcb02.ptr->save.g_pat =
+			    svm->vmcb01.ptr->save.g_pat;
+	} else {
+	    if (!(nested_ctl & SVM_NESTED_CTL_NP_ENABLE))
+		svm->nested.vmcb02.ptr->save.g_pat = g_pat;
+	}
 }
 
 static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12)
 {
 	bool new_vmcb12 = false;
 
-	nested_vmcb02_compute_g_pat(svm);
-
 	/* Load the nested guest state */
 	if (svm->nested.vmcb12_gpa != svm->nested.last_vmcb12_gpa) {
 		new_vmcb12 = true;
@@ -679,8 +697,10 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 
 	nested_load_control_from_vmcb12(svm, &vmcb12->control);
+	nested_vmcb02_compute_g_pat(svm, vmcb12->save.g_pat,
+	    vmcb12->control.nested_ctl, true);
 
-	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
+	if (!nested_vmcb_check_save_area(vcpu, &vmcb12->control, &vmcb12->save) ||
 	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = 0;
@@ -1386,7 +1406,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	if (!(save->cr0 & X86_CR0_PG) ||
 	    !(save->cr0 & X86_CR0_PE) ||
 	    (save->rflags & X86_EFLAGS_VM) ||
-	    !nested_vmcb_valid_sregs(vcpu, save))
+	    !nested_vmcb_check_save_area(vcpu, ctl, save))
 		goto out_free;
 
 	/*
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 5151efa424ac..e08e55082e77 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2907,7 +2907,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		vcpu->arch.pat = data;
 		svm->vmcb01.ptr->save.g_pat = data;
 		if (is_guest_mode(vcpu))
-			nested_vmcb02_compute_g_pat(svm);
+			nested_vmcb02_compute_g_pat(svm, data,
+			    svm->vmcb->control.nested_ctl, false);
 		vmcb_mark_dirty(svm->vmcb, VMCB_NPT);
 		break;
 	case MSR_IA32_SPEC_CTRL:
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 1c7306c370fa..872d6c72d937 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -497,7 +497,8 @@ void svm_write_tsc_multiplier(struct kvm_vcpu *vcpu, u64 multiplier);
 void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 				     struct vmcb_control_area *control);
 void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
-void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
+void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm, u64 g_pat,
+				 u64 nested_ctl, bool from_vmrun);
 void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
 
 extern struct kvm_x86_nested_ops svm_nested_ops;
-- 
2.27.0

