[PATCH v3 0/8] KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL

[PATCH v3 0/8] KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL

[Oliver Upton]

[v1] https://lore.kernel.org/r/20190828234134.132704-1-oupton@google.com
[v2] https://lore.kernel.org/r/20190903213044.168494-1-oupton@google.com

v1 => v2:
 - Add Krish's Co-developed-by and Signed-off-by tags.
 - Fix minor nit to kvm-unit-tests to use 'host' local variable
   throughout test_load_pgc()
 - Teach guest_state_test_main() to check guest state from within nested
   VM
 - Update proposed tests to use guest/host state checks, wherein the
   value is checked from MSR_CORE_PERF_GLOBAL_CTRL.
 - Changelog line wrapping

v2 => v3:
 - Remove the value unchanged condition from
   kvm_is_valid_perf_global_ctrl
 - Add line to changelog for patch 3/8

This patchset exposes the "load IA32_PERF_GLOBAL_CTRL" to guests for nested
VM-entry and VM-exit. There already was some existing code that supported
the VM-exit ctrl, though it had an issue and was not exposed to the guest
anyway. These patches are based on the original set that Krish Sadhukhan
sent out earlier this year.

Purpose of each patch:

(1) Change the existing code that implemented the VM-exit functionality
    to use kvm_set_msr() to avoid being overwritten by
    atomic_perf_switch_msrs().
(2) Update prepare_vmcs02() to implement the VM-entry functionality,
    again using kvm_set_msr().
(3) Create a helper function for checking the validity of an
    IA32_PERF_GLOBAL_CTRL value against pmu->global_ctrl_mask.
(4) Check guest state on VM-entry as described in the SDM.
(5) Check host state on VM-entry as described in the SDM.
(6) Expose the "load IA32_PERF_GLOBAL_CTRL" VM-entry and VM-exit
    controls if IA32_PERF_GLOBAL_CTRL is a valid MSR.
(7) Modify guest_state_test_main() to check guest state MSRs
(8) Tests in kvm-unit-tests to check the VM-entry and VM-exit controls
    work properly

Oliver Upton (6):
  KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit
  KVM: nVMX: Load GUEST_IA32_PERF_GLOBAL_CTRL MSR on vm-entry
  KVM: VMX: Add helper to check reserved bits in IA32_PERF_GLOBAL_CTRL
  KVM: nVMX: check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry
  KVM: nVMX: Check HOST_IA32_PERF_GLOBAL_CTRL on VM-entry
  KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL vm control if supported

 arch/x86/kvm/pmu.h           |  6 ++++++
 arch/x86/kvm/vmx/nested.c    | 37 +++++++++++++++++++++++++++++++++---
 arch/x86/kvm/vmx/pmu_intel.c |  5 ++++-
 arch/x86/kvm/vmx/vmx.c       | 21 ++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.h       |  1 +
 5 files changed, 66 insertions(+), 4 deletions(-)

-- 
2.23.0.187.g17f5b7556c-goog

[PATCH v3 1/8] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Oliver Upton]

The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
could cause this value to be overwritten. Instead, call kvm_set_msr()
which will allow atomic_switch_perf_msrs() to correctly set the values.

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ced9fba32598..b0ca34bf4d21 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 				   struct vmcs12 *vmcs12)
 {
 	struct kvm_segment seg;
+	struct msr_data msr_info;
 	u32 entry_failure_code;
 
 	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
@@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
 		vcpu->arch.pat = vmcs12->host_ia32_pat;
 	}
-	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
-		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
-			vmcs12->host_ia32_perf_global_ctrl);
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
+		msr_info.host_initiated = false;
+		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
+		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
+		if (kvm_set_msr(vcpu, &msr_info))
+			pr_debug_ratelimited(
+				"%s cannot write MSR (0x%x, 0x%llx)\n",
+				__func__, msr_info.index, msr_info.data);
+	}
 
 	/* Set L1 segment info according to Intel SDM
 	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH v3 2/8] KVM: nVMX: Load GUEST_IA32_PERF_GLOBAL_CTRL MSR on vm-entry

[Oliver Upton]

Add condition to prepare_vmcs02 which loads IA32_PERF_GLOBAL_CTRL on
VM-entry if the "load IA32_PERF_GLOBAL_CTRL" bit on the VM-entry control
is set. Use kvm_set_msr() rather than directly writing to the field to
avoid overwrite by atomic_switch_perf_msrs().

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b0ca34bf4d21..9ba90b38d74b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2281,6 +2281,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct hv_enlightened_vmcs *hv_evmcs = vmx->nested.hv_evmcs;
+	struct msr_data msr_info;
 	bool load_guest_pdptrs_vmcs12 = false;
 
 	if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
@@ -2404,6 +2405,16 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	if (!enable_ept)
 		vcpu->arch.walk_mmu->inject_page_fault = vmx_inject_page_fault_nested;
 
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) {
+		msr_info.host_initiated = false;
+		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
+		msr_info.data = vmcs12->guest_ia32_perf_global_ctrl;
+		if (kvm_set_msr(vcpu, &msr_info))
+			pr_debug_ratelimited(
+				"%s cannot write MSR (0x%x, 0x%llx)\n",
+				__func__, msr_info.index, msr_info.data);
+	}
+
 	kvm_rsp_write(vcpu, vmcs12->guest_rsp);
 	kvm_rip_write(vcpu, vmcs12->guest_rip);
 	return 0;
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH v3 3/8] KVM: VMX: Add helper to check reserved bits in IA32_PERF_GLOBAL_CTRL

[Oliver Upton]

Create a helper function to check the validity of a proposed value for
IA32_PERF_GLOBAL_CTRL from the existing check in intel_pmu_set_msr().

Per Intel's SDM, the reserved bits in IA32_PERF_GLOBAL_CTRL must be
cleared for the corresponding host/guest state fields.

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/pmu.h           | 6 ++++++
 arch/x86/kvm/vmx/pmu_intel.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 58265f761c3b..67a0f6da567c 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -79,6 +79,12 @@ static inline bool pmc_is_enabled(struct kvm_pmc *pmc)
 	return kvm_x86_ops->pmu_ops->pmc_is_enabled(pmc);
 }
 
+static inline bool kvm_is_valid_perf_global_ctrl(struct kvm_pmu *pmu,
+						 u64 data)
+{
+	return !(pmu->global_ctrl_mask & data);
+}
+
 /* returns general purpose PMC with the specified MSR. Note that it can be
  * used for both PERFCTRn and EVNTSELn; that is why it accepts base as a
  * paramenter to tell them apart.
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 4dea0e0e7e39..963766d631ad 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -223,7 +223,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (pmu->global_ctrl == data)
 			return 0;
-		if (!(data & pmu->global_ctrl_mask)) {
+		if (kvm_is_valid_perf_global_ctrl(pmu, data)) {
 			global_ctrl_changed(pmu, data);
 			return 0;
 		}
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH v3 4/8] KVM: nVMX: check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry

[Oliver Upton]

Add condition to nested_vmx_check_guest_state() to check the validity of
GUEST_IA32_PERF_GLOBAL_CTRL. Per Intel's SDM Vol 3 26.3.1.1:

  If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, bits
  reserved in the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the field for that
  register.

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 9ba90b38d74b..6c3aa3bcede3 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -10,6 +10,7 @@
 #include "hyperv.h"
 #include "mmu.h"
 #include "nested.h"
+#include "pmu.h"
 #include "trace.h"
 #include "x86.h"
 
@@ -2732,6 +2733,7 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 					u32 *exit_qual)
 {
 	bool ia32e;
+	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 
 	*exit_qual = ENTRY_FAIL_DEFAULT;
 
@@ -2748,6 +2750,11 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 	}
 
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL &&
+	    !kvm_is_valid_perf_global_ctrl(pmu,
+					   vmcs12->guest_ia32_perf_global_ctrl))
+		return -EINVAL;
+
 	/*
 	 * If the load IA32_EFER VM-entry control is 1, the following checks
 	 * are performed on the field for the IA32_EFER MSR:
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH v3 5/8] KVM: nVMX: Check HOST_IA32_PERF_GLOBAL_CTRL on VM-entry

[Oliver Upton]

Add a consistency check on nested vm-entry for host's
IA32_PERF_GLOBAL_CTRL from vmcs12. Per Intel's SDM Vol 3 26.2.2:

  If the "load IA32_PERF_GLOBAL_CTRL"
  VM-exit control is 1, bits reserved in the IA32_PERF_GLOBAL_CTRL
  MSR must be 0 in the field for that register"

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 6c3aa3bcede3..e2baa9ca562f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2636,6 +2636,7 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 				       struct vmcs12 *vmcs12)
 {
 	bool ia32e;
+	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 
 	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
 	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
@@ -2650,6 +2651,11 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    !kvm_pat_valid(vmcs12->host_ia32_pat))
 		return -EINVAL;
 
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
+	    !kvm_is_valid_perf_global_ctrl(pmu,
+					   vmcs12->host_ia32_perf_global_ctrl))
+		return -EINVAL;
+
 	ia32e = (vmcs12->vm_exit_controls &
 		 VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
 
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH v3 6/8] KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL vm control if supported

[Oliver Upton]

The "load IA32_PERF_GLOBAL_CTRL" bit for VM-entry and VM-exit should
only be exposed to the guest if IA32_PERF_GLOBAL_CTRL is a valid MSR.
Create a new helper to allow pmu_refresh() to update the VM-entry and
VM-exit controls to ensure PMU values are initialized when performing
the is_valid_msr() check.

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c |  3 +++
 arch/x86/kvm/vmx/vmx.c       | 21 +++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.h       |  1 +
 3 files changed, 25 insertions(+)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 963766d631ad..2dc7be724321 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -16,6 +16,7 @@
 #include "cpuid.h"
 #include "lapic.h"
 #include "pmu.h"
+#include "vmx.h"
 
 static struct kvm_event_hw_type_mapping intel_arch_events[] = {
 	/* Index must match CPUID 0x0A.EBX bit vector */
@@ -314,6 +315,8 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
 	    (boot_cpu_has(X86_FEATURE_HLE) || boot_cpu_has(X86_FEATURE_RTM)) &&
 	    (entry->ebx & (X86_FEATURE_HLE|X86_FEATURE_RTM)))
 		pmu->reserved_bits ^= HSW_IN_TX|HSW_IN_TX_CHECKPOINTED;
+
+	nested_vmx_pmu_entry_exit_ctls_update(vcpu);
 }
 
 static void intel_pmu_init(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 570a233e272b..5b0664bff23b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6417,6 +6417,27 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
 	}
 }
 
+void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx;
+
+	if (!nested_vmx_allowed(vcpu))
+		return;
+
+	vmx = to_vmx(vcpu);
+	if (intel_pmu_ops.is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL)) {
+		vmx->nested.msrs.entry_ctls_high |=
+				VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
+		vmx->nested.msrs.exit_ctls_high |=
+				VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
+	} else {
+		vmx->nested.msrs.entry_ctls_high &=
+				~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
+		vmx->nested.msrs.exit_ctls_high &=
+				~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
+	}
+}
+
 bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
 
 static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 82d0bc3a4d52..e06884cf88ad 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -331,6 +331,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
 struct shared_msr_entry *find_msr_entry(struct vcpu_vmx *vmx, u32 msr);
 void pt_update_intercept_for_msr(struct vcpu_vmx *vmx);
 void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
+void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu);
 
 #define POSTED_INTR_ON  0
 #define POSTED_INTR_SN  1
-- 
2.23.0.187.g17f5b7556c-goog

[kvm-unit-tests PATCH v3 7/8] x86: VMX: Make guest_state_test_main() check state from nested VM

[Oliver Upton]

The current tests for guest state do not yet check the validity of
loaded state from within the nested VM. Introduce the
load_state_test_data struct to share data with the nested VM.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 x86/vmx_tests.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index f035f24a771a..b72a27583793 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -5017,13 +5017,28 @@ static void test_entry_msr_load(void)
 	test_vmx_valid_controls(false);
 }
 
+static struct load_state_test_data {
+	u32 msr;
+	u64 exp;
+	bool enabled;
+} load_state_test_data;
+
 static void guest_state_test_main(void)
 {
+	u64 obs;
+	struct load_state_test_data *data = &load_state_test_data;
+
 	while (1) {
-		if (vmx_get_test_stage() != 2)
-			vmcall();
-		else
+		if (vmx_get_test_stage() == 2)
 			break;
+
+		if (data->enabled) {
+			obs = rdmsr(obs);
+			report("Guest state is 0x%lx (expected 0x%lx)",
+			       data->exp == obs, obs, data->exp);
+		}
+
+		vmcall();
 	}
 
 	asm volatile("fnop");
@@ -6854,7 +6869,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
 	u64 i, val;
 	u32 j;
 	int error;
+	struct load_state_test_data *data = &load_state_test_data;
 
+	data->enabled = false;
 	vmcs_clear_bits(ctrl_field, ctrl_bit);
 	if (field == GUEST_PAT) {
 		vmx_set_test_stage(1);
-- 
2.23.0.187.g17f5b7556c-goog

[kvm-unit-tests PATCH v3 8/8] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Oliver Upton]

Tests to verify that KVM performs the correct checks on Host/Guest state
at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
Control Registers and MSRs".

Test that KVM does the following:

    If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
    reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
    GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
    should fail with an exit reason of "VM-entry failure due to invalid
    guest state" (33). On a successful VM-entry, the correct value
    should be observed when the nested VM performs an RDMSR on
    IA32_PERF_GLOBAL_CTRL.

    If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
    reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
    HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
    should fail with a VM-instruction error of "VM entry with invalid
    host-state field(s)" (8). On a successful VM-exit, the correct value
    should be observed when L1 performs an RDMSR on
    IA32_PERF_GLOBAL_CTRL.

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 x86/vmx_tests.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 197 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index b72a27583793..73c46eba6be9 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -5033,7 +5033,7 @@ static void guest_state_test_main(void)
 			break;
 
 		if (data->enabled) {
-			obs = rdmsr(obs);
+			obs = rdmsr(data->msr);
 			report("Guest state is 0x%lx (expected 0x%lx)",
 			       data->exp == obs, obs, data->exp);
 		}
@@ -6854,6 +6854,200 @@ static void test_host_efer(void)
 	test_efer(HOST_EFER, "HOST_EFER", EXI_CONTROLS, EXI_LOAD_EFER);
 }
 
+union cpuid10_eax {
+	struct {
+		unsigned int version_id:8;
+		unsigned int num_counters:8;
+		unsigned int bit_width:8;
+		unsigned int mask_length:8;
+	} split;
+	unsigned int full;
+};
+
+union cpuid10_edx {
+	struct {
+		unsigned int num_counters_fixed:5;
+		unsigned int bit_width_fixed:8;
+		unsigned int reserved:19;
+	} split;
+	unsigned int full;
+};
+
+static bool valid_pgc(u64 val)
+{
+	struct cpuid id;
+	union cpuid10_eax eax;
+	union cpuid10_edx edx;
+	u64 mask;
+
+	id = cpuid(0xA);
+	eax.full = id.a;
+	edx.full = id.d;
+	mask = ~(((1ull << eax.split.num_counters) - 1) |
+		(((1ull << edx.split.num_counters_fixed) - 1) << 32));
+
+	return !(val & mask);
+}
+
+static void test_pgc_vmlaunch(u32 xerror, bool xfail, bool host)
+{
+	u32 inst_err;
+	u64 guest_rip, inst_len, obs;
+	bool success;
+	struct load_state_test_data *data = &load_state_test_data;
+
+	if (host) {
+		success = vmlaunch_succeeds();
+		obs = rdmsr(data->msr);
+		if (data->enabled && success)
+			report("Host state is 0x%lx (expected 0x%lx)",
+			       data->exp == obs, obs, data->exp);
+	} else {
+		if (xfail)
+			enter_guest_with_invalid_guest_state();
+		else
+			enter_guest();
+		success = VMX_VMCALL == (vmcs_read(EXI_REASON) & 0xff);
+		guest_rip = vmcs_read(GUEST_RIP);
+		inst_len = vmcs_read(EXI_INST_LEN);
+		if (success)
+			vmcs_write(GUEST_RIP, guest_rip + inst_len);
+	}
+	if (!success) {
+		inst_err = vmcs_read(VMX_INST_ERROR);
+		report("vmlaunch failed, VMX Inst Error is %d (expected %d)",
+		       xerror == inst_err, inst_err, xerror);
+	} else {
+		report("vmlaunch succeeded", success != xfail);
+	}
+}
+
+/*
+ * test_load_pgc is a generic function for testing the
+ * "load IA32_PERF_GLOBAL_CTRL" VM-{entry,exit} control. This test function
+ * will test the provided ctrl_val disabled and enabled.
+ *
+ * @nr - VMCS field number corresponding to the Host/Guest state field
+ * @name - Name of the above VMCS field for printing in test report
+ * @ctrl_nr - VMCS field number corresponding to the VM-{entry,exit} control
+ * @ctrl_val - Bit to set on the ctrl field.
+ */
+static void test_load_pgc(u32 nr, const char *name, u32 ctrl_nr,
+			  const char *ctrl_name, u64 ctrl_val)
+{
+	u64 ctrl_saved = vmcs_read(ctrl_nr);
+	u64 pgc_saved = vmcs_read(nr);
+	u64 i, val;
+	bool host = nr == HOST_PERF_GLOBAL_CTRL;
+	struct load_state_test_data *data = &load_state_test_data;
+
+	data->msr = MSR_CORE_PERF_GLOBAL_CTRL;
+	msr_bmp_init();
+	if (!host) {
+		vmx_set_test_stage(1);
+		test_set_guest(guest_state_test_main);
+	}
+	vmcs_write(ctrl_nr, ctrl_saved & ~ctrl_val);
+	data->enabled = false;
+	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=0 on %s",
+			    ctrl_name);
+	for (i = 0; i < 64; i++) {
+		val = 1ull << i;
+		vmcs_write(nr, val);
+		report_prefix_pushf("%s = 0x%lx", name, val);
+		/*
+		 * If the "load IA32_PERF_GLOBAL_CTRL" bit is 0 then
+		 * the {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL field is ignored,
+		 * thus setting reserved bits in this field does not cause
+		 * vmlaunch to fail.
+		 */
+		test_pgc_vmlaunch(0, false, host);
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	vmcs_write(ctrl_nr, ctrl_saved | ctrl_val);
+	data->enabled = true;
+	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=1 on %s",
+			    ctrl_name);
+	for (i = 0; i < 64; i++) {
+		val = 1ull << i;
+		data->exp = val;
+		vmcs_write(nr, val);
+		report_prefix_pushf("%s = 0x%lx", name, val);
+		if (valid_pgc(val)) {
+			test_pgc_vmlaunch(0, false, host);
+		} else {
+			/*
+			 * [SDM 30.4]
+			 *
+			 * Invalid host state fields result in an VM
+			 * instruction error with error number 8
+			 * (VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
+			 */
+			if (host) {
+				test_pgc_vmlaunch(
+					VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
+					true, host);
+			/*
+			 * [SDM 26.1]
+			 *
+			 * If a VM-Entry fails according to one of
+			 * the guest-state checks, the exit reason on the VMCS
+			 * will be set to reason number 33 (VMX_FAIL_STATE)
+			 */
+			} else {
+				test_pgc_vmlaunch(
+					0,
+					true, host);
+				TEST_ASSERT_EQ(
+					VMX_ENTRY_FAILURE | VMX_FAIL_STATE,
+					vmcs_read(EXI_REASON));
+			}
+		}
+		report_prefix_pop();
+	}
+
+	report_prefix_pop();
+
+	if (nr == GUEST_PERF_GLOBAL_CTRL) {
+		/*
+		 * Let the guest finish execution
+		 */
+		vmx_set_test_stage(2);
+		vmcs_write(ctrl_nr, ctrl_saved);
+		vmcs_write(nr, pgc_saved);
+		enter_guest();
+	}
+
+	vmcs_write(ctrl_nr, ctrl_saved);
+	vmcs_write(nr, pgc_saved);
+}
+
+static void test_load_host_pgc(void)
+{
+	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
+		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
+		       "exit control not supported\n");
+		return;
+	}
+
+	test_load_pgc(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
+		      EXI_CONTROLS, "EXI_CONTROLS", EXI_LOAD_PERF);
+}
+
+
+static void test_load_guest_pgc(void)
+{
+	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
+		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
+		       "entry control not supported\n");
+	}
+
+	test_load_pgc(GUEST_PERF_GLOBAL_CTRL, "GUEST_PERF_GLOBAL_CTRL",
+		      ENT_CONTROLS, "ENT_CONTROLS", ENT_LOAD_PERF);
+}
+
 /*
  * PAT values higher than 8 are uninteresting since they're likely lumped
  * in with "8". We only test values above 8 one bit at a time,
@@ -7147,6 +7341,7 @@ static void vmx_host_state_area_test(void)
 	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
 
 	test_host_efer();
+	test_load_host_pgc();
 	test_load_host_pat();
 	test_host_segment_regs();
 	test_host_desc_tables();
@@ -8587,7 +8782,6 @@ static int invalid_msr_entry_failure(struct vmentry_failure *failure)
 	return VMX_TEST_VMEXIT;
 }
 
-
 #define TEST(name) { #name, .v2 = name }
 
 /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
@@ -8637,6 +8831,7 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_host_state_area_test),
 	TEST(vmx_guest_state_area_test),
 	TEST(vmentry_movss_shadow_test),
+	TEST(test_load_guest_pgc),
 	/* APICv tests */
 	TEST(vmx_eoi_bitmap_ioapic_scan_test),
 	TEST(vmx_hlt_with_rvi_test),
-- 
2.23.0.187.g17f5b7556c-goog

Re: [kvm-unit-tests PATCH v3 8/8] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Krish Sadhukhan]

On 09/03/2019 02:58 PM, Oliver Upton wrote:
> Tests to verify that KVM performs the correct checks on Host/Guest state
> at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
> Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
> Control Registers and MSRs".
>
> Test that KVM does the following:
>
>      If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
>      reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>      GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>      should fail with an exit reason of "VM-entry failure due to invalid
>      guest state" (33). On a successful VM-entry, the correct value
>      should be observed when the nested VM performs an RDMSR on
>      IA32_PERF_GLOBAL_CTRL.
>
>      If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
>      reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>      HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>      should fail with a VM-instruction error of "VM entry with invalid
>      host-state field(s)" (8). On a successful VM-exit, the correct value
>      should be observed when L1 performs an RDMSR on
>      IA32_PERF_GLOBAL_CTRL.
>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>   x86/vmx_tests.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 197 insertions(+), 2 deletions(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index b72a27583793..73c46eba6be9 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -5033,7 +5033,7 @@ static void guest_state_test_main(void)
>   			break;
>   
>   		if (data->enabled) {
> -			obs = rdmsr(obs);
> +			obs = rdmsr(data->msr);
>   			report("Guest state is 0x%lx (expected 0x%lx)",
>   			       data->exp == obs, obs, data->exp);
>   		}
> @@ -6854,6 +6854,200 @@ static void test_host_efer(void)
>   	test_efer(HOST_EFER, "HOST_EFER", EXI_CONTROLS, EXI_LOAD_EFER);
>   }
>   
> +union cpuid10_eax {
> +	struct {
> +		unsigned int version_id:8;
> +		unsigned int num_counters:8;

It's better to name it num_gpc or num_counters_gp.

> +		unsigned int bit_width:8;
> +		unsigned int mask_length:8;
> +	} split;
> +	unsigned int full;
> +};
> +
> +union cpuid10_edx {
> +	struct {
> +		unsigned int num_counters_fixed:5;
> +		unsigned int bit_width_fixed:8;
> +		unsigned int reserved:19;
> +	} split;
> +	unsigned int full;
> +};
> +

In kvm-unit-test source,  the naming of variables, functions etc. seems 
to be based on the hex value of the CPUID leaf that they represent. For 
example, cpuid_7_ebx, check_cpuid_80000001_edx etc. Isn't it better to 
name these structures something like cpuidA_eax and cpuidA_edx ?

> +static bool valid_pgc(u64 val)
> +{
> +	struct cpuid id;
> +	union cpuid10_eax eax;
> +	union cpuid10_edx edx;
> +	u64 mask;
> +
> +	id = cpuid(0xA);
> +	eax.full = id.a;
> +	edx.full = id.d;
> +	mask = ~(((1ull << eax.split.num_counters) - 1) |
> +		(((1ull << edx.split.num_counters_fixed) - 1) << 32));
> +
> +	return !(val & mask);
> +}
> +
> +static void test_pgc_vmlaunch(u32 xerror, bool xfail, bool host)
> +{
> +	u32 inst_err;
> +	u64 guest_rip, inst_len, obs;
> +	bool success;
> +	struct load_state_test_data *data = &load_state_test_data;
> +
> +	if (host) {
> +		success = vmlaunch_succeeds();
> +		obs = rdmsr(data->msr);
> +		if (data->enabled && success)
> +			report("Host state is 0x%lx (expected 0x%lx)",
> +			       data->exp == obs, obs, data->exp);
> +	} else {
> +		if (xfail)
> +			enter_guest_with_invalid_guest_state();
> +		else
> +			enter_guest();
> +		success = VMX_VMCALL == (vmcs_read(EXI_REASON) & 0xff);
> +		guest_rip = vmcs_read(GUEST_RIP);
> +		inst_len = vmcs_read(EXI_INST_LEN);
> +		if (success)
> +			vmcs_write(GUEST_RIP, guest_rip + inst_len);

Is it possible to re-use the existing report_guest_state_test() here and 
the below code ?

> +	}
> +	if (!success) {
> +		inst_err = vmcs_read(VMX_INST_ERROR);
> +		report("vmlaunch failed, VMX Inst Error is %d (expected %d)",
> +		       xerror == inst_err, inst_err, xerror);
> +	} else {
> +		report("vmlaunch succeeded", success != xfail);
> +	}
> +}
> +
> +/*
> + * test_load_pgc is a generic function for testing the
> + * "load IA32_PERF_GLOBAL_CTRL" VM-{entry,exit} control. This test function
> + * will test the provided ctrl_val disabled and enabled.
> + *
> + * @nr - VMCS field number corresponding to the Host/Guest state field
> + * @name - Name of the above VMCS field for printing in test report
> + * @ctrl_nr - VMCS field number corresponding to the VM-{entry,exit} control
> + * @ctrl_val - Bit to set on the ctrl field.
> + */
> +static void test_load_pgc(u32 nr, const char *name, u32 ctrl_nr,
> +			  const char *ctrl_name, u64 ctrl_val)
> +{
> +	u64 ctrl_saved = vmcs_read(ctrl_nr);
> +	u64 pgc_saved = vmcs_read(nr);
> +	u64 i, val;
> +	bool host = nr == HOST_PERF_GLOBAL_CTRL;
> +	struct load_state_test_data *data = &load_state_test_data;
> +
> +	data->msr = MSR_CORE_PERF_GLOBAL_CTRL;
> +	msr_bmp_init();
> +	if (!host) {
> +		vmx_set_test_stage(1);
> +		test_set_guest(guest_state_test_main);
> +	}
> +	vmcs_write(ctrl_nr, ctrl_saved & ~ctrl_val);
> +	data->enabled = false;
> +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=0 on %s",
> +			    ctrl_name);
> +	for (i = 0; i < 64; i++) {
> +		val = 1ull << i;
> +		vmcs_write(nr, val);
> +		report_prefix_pushf("%s = 0x%lx", name, val);
> +		/*
> +		 * If the "load IA32_PERF_GLOBAL_CTRL" bit is 0 then
> +		 * the {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL field is ignored,
> +		 * thus setting reserved bits in this field does not cause
> +		 * vmlaunch to fail.
> +		 */

This comment is really not required as it's obvious that there's no 
effect on VM-entry if the control bit is not set.

> +		test_pgc_vmlaunch(0, false, host);
> +		report_prefix_pop();
> +	}
> +	report_prefix_pop();
> +
> +	vmcs_write(ctrl_nr, ctrl_saved | ctrl_val);
> +	data->enabled = true;
> +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=1 on %s",
> +			    ctrl_name);
> +	for (i = 0; i < 64; i++) {
> +		val = 1ull << i;
> +		data->exp = val;
> +		vmcs_write(nr, val);
> +		report_prefix_pushf("%s = 0x%lx", name, val);
> +		if (valid_pgc(val)) {
> +			test_pgc_vmlaunch(0, false, host);
> +		} else {
> +			/*
> +			 * [SDM 30.4]
> +			 *
> +			 * Invalid host state fields result in an VM
> +			 * instruction error with error number 8
> +			 * (VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
> +			 */
> +			if (host) {
> +				test_pgc_vmlaunch(
> +					VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
> +					true, host);
> +			/*
> +			 * [SDM 26.1]
> +			 *
> +			 * If a VM-Entry fails according to one of
> +			 * the guest-state checks, the exit reason on the VMCS
> +			 * will be set to reason number 33 (VMX_FAIL_STATE)

Again, these comments are probably not necessary.

> +			 */
> +			} else {
> +				test_pgc_vmlaunch(
> +					0,
> +					true, host);
> +				TEST_ASSERT_EQ(
> +					VMX_ENTRY_FAILURE | VMX_FAIL_STATE,
> +					vmcs_read(EXI_REASON));
> +			}
> +		}
> +		report_prefix_pop();
> +	}
> +
> +	report_prefix_pop();
> +
> +	if (nr == GUEST_PERF_GLOBAL_CTRL) {
> +		/*
> +		 * Let the guest finish execution
> +		 */
> +		vmx_set_test_stage(2);
> +		vmcs_write(ctrl_nr, ctrl_saved);
> +		vmcs_write(nr, pgc_saved);
> +		enter_guest();
> +	}
> +
> +	vmcs_write(ctrl_nr, ctrl_saved);
> +	vmcs_write(nr, pgc_saved);
> +}
> +
> +static void test_load_host_pgc(void)
> +{
> +	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
> +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
> +		       "exit control not supported\n");
> +		return;
> +	}
> +
> +	test_load_pgc(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
> +		      EXI_CONTROLS, "EXI_CONTROLS", EXI_LOAD_PERF);
> +}
> +
> +
> +static void test_load_guest_pgc(void)
> +{
> +	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
> +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
> +		       "entry control not supported\n");
> +	}
> +
> +	test_load_pgc(GUEST_PERF_GLOBAL_CTRL, "GUEST_PERF_GLOBAL_CTRL",
> +		      ENT_CONTROLS, "ENT_CONTROLS", ENT_LOAD_PERF);
> +}
> +
>   /*
>    * PAT values higher than 8 are uninteresting since they're likely lumped
>    * in with "8". We only test values above 8 one bit at a time,
> @@ -7147,6 +7341,7 @@ static void vmx_host_state_area_test(void)
>   	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
>   
>   	test_host_efer();
> +	test_load_host_pgc();

I would rename it to test_load_host_perf_global_ctrl to avoid confusion 
between "performance counter" and "VMCS control field" though the name 
is bit long.

>   	test_load_host_pat();
>   	test_host_segment_regs();
>   	test_host_desc_tables();
> @@ -8587,7 +8782,6 @@ static int invalid_msr_entry_failure(struct vmentry_failure *failure)
>   	return VMX_TEST_VMEXIT;
>   }
>   
> -
>   #define TEST(name) { #name, .v2 = name }
>   
>   /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
> @@ -8637,6 +8831,7 @@ struct vmx_test vmx_tests[] = {
>   	TEST(vmx_host_state_area_test),
>   	TEST(vmx_guest_state_area_test),
>   	TEST(vmentry_movss_shadow_test),
> +	TEST(test_load_guest_pgc),

Same comment as above.
The other comment I have is, why not put this inside of 
'vmx_guest_state_area_test' because this VMCS field belongs to Guest 
State Area only.

>   	/* APICv tests */
>   	TEST(vmx_eoi_bitmap_ioapic_scan_test),
>   	TEST(vmx_hlt_with_rvi_test),

Re: [kvm-unit-tests PATCH v3 7/8] x86: VMX: Make guest_state_test_main() check state from nested VM

[Krish Sadhukhan]

On 09/03/2019 02:58 PM, Oliver Upton wrote:
> The current tests for guest state do not yet check the validity of
> loaded state from within the nested VM. Introduce the
> load_state_test_data struct to share data with the nested VM.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>   x86/vmx_tests.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index f035f24a771a..b72a27583793 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -5017,13 +5017,28 @@ static void test_entry_msr_load(void)
>   	test_vmx_valid_controls(false);
>   }
>   
> +static struct load_state_test_data {
> +	u32 msr;
> +	u64 exp;
> +	bool enabled;
> +} load_state_test_data;

A better name is probably 'loaded_state_test_data'  as you are checking 
the validity of the loaded MSR in the guest.

> +
>   static void guest_state_test_main(void)
>   {
> +	u64 obs;
> +	struct load_state_test_data *data = &load_state_test_data;
> +
>   	while (1) {
> -		if (vmx_get_test_stage() != 2)
> -			vmcall();
> -		else
> +		if (vmx_get_test_stage() == 2)
>   			break;
> +
> +		if (data->enabled) {
> +			obs = rdmsr(obs);

Although you fixed it in the next patch, why not use  'data->msr' in 
place of 'obs' as the parameter to rdmsr() in this patch only ?

> +			report("Guest state is 0x%lx (expected 0x%lx)",
> +			       data->exp == obs, obs, data->exp);
> +		}
> +
> +		vmcall();
>   	}
>   
>   	asm volatile("fnop");
> @@ -6854,7 +6869,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>   	u64 i, val;
>   	u32 j;
>   	int error;
> +	struct load_state_test_data *data = &load_state_test_data;
>   
> +	data->enabled = false;
>   	vmcs_clear_bits(ctrl_field, ctrl_bit);
>   	if (field == GUEST_PAT) {
>   		vmx_set_test_stage(1);

Re: [kvm-unit-tests PATCH v3 8/8] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Oliver Upton]

On Wed, Sep 04, 2019 at 05:13:17PM -0700, Krish Sadhukhan wrote:
> 
> 
> On 09/03/2019 02:58 PM, Oliver Upton wrote:
> > Tests to verify that KVM performs the correct checks on Host/Guest state
> > at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
> > Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
> > Control Registers and MSRs".
> > 
> > Test that KVM does the following:
> > 
> >      If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
> >      reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
> >      GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
> >      should fail with an exit reason of "VM-entry failure due to invalid
> >      guest state" (33). On a successful VM-entry, the correct value
> >      should be observed when the nested VM performs an RDMSR on
> >      IA32_PERF_GLOBAL_CTRL.
> > 
> >      If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
> >      reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
> >      HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
> >      should fail with a VM-instruction error of "VM entry with invalid
> >      host-state field(s)" (8). On a successful VM-exit, the correct value
> >      should be observed when L1 performs an RDMSR on
> >      IA32_PERF_GLOBAL_CTRL.
> > 
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >   x86/vmx_tests.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 197 insertions(+), 2 deletions(-)
> > 
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > index b72a27583793..73c46eba6be9 100644
> > --- a/x86/vmx_tests.c
> > +++ b/x86/vmx_tests.c
> > @@ -5033,7 +5033,7 @@ static void guest_state_test_main(void)
> >   			break;
> >   		if (data->enabled) {
> > -			obs = rdmsr(obs);
> > +			obs = rdmsr(data->msr);
> >   			report("Guest state is 0x%lx (expected 0x%lx)",
> >   			       data->exp == obs, obs, data->exp);
> >   		}
> > @@ -6854,6 +6854,200 @@ static void test_host_efer(void)
> >   	test_efer(HOST_EFER, "HOST_EFER", EXI_CONTROLS, EXI_LOAD_EFER);
> >   }
> > +union cpuid10_eax {
> > +	struct {
> > +		unsigned int version_id:8;
> > +		unsigned int num_counters:8;
> 
> It's better to name it num_gpc or num_counters_gp.

Ack to all style comments that follow. I brought these structs over from
kvm, but I'll align the style with what is suggested for kvm-unit-tests.

> > +		unsigned int bit_width:8;
> > +		unsigned int mask_length:8;
> > +	} split;
> > +	unsigned int full;
> > +};
> > +
> > +union cpuid10_edx {
> > +	struct {
> > +		unsigned int num_counters_fixed:5;
> > +		unsigned int bit_width_fixed:8;
> > +		unsigned int reserved:19;
> > +	} split;
> > +	unsigned int full;
> > +};
> > +
> 
> In kvm-unit-test source,  the naming of variables, functions etc. seems to
> be based on the hex value of the CPUID leaf that they represent. For
> example, cpuid_7_ebx, check_cpuid_80000001_edx etc. Isn't it better to name
> these structures something like cpuidA_eax and cpuidA_edx ?
> 
> > +static bool valid_pgc(u64 val)
> > +{
> > +	struct cpuid id;
> > +	union cpuid10_eax eax;
> > +	union cpuid10_edx edx;
> > +	u64 mask;
> > +
> > +	id = cpuid(0xA);
> > +	eax.full = id.a;
> > +	edx.full = id.d;
> > +	mask = ~(((1ull << eax.split.num_counters) - 1) |
> > +		(((1ull << edx.split.num_counters_fixed) - 1) << 32));
> > +
> > +	return !(val & mask);
> > +}
> > +
> > +static void test_pgc_vmlaunch(u32 xerror, bool xfail, bool host)
> > +{
> > +	u32 inst_err;
> > +	u64 guest_rip, inst_len, obs;
> > +	bool success;
> > +	struct load_state_test_data *data = &load_state_test_data;
> > +
> > +	if (host) {
> > +		success = vmlaunch_succeeds();
> > +		obs = rdmsr(data->msr);
> > +		if (data->enabled && success)
> > +			report("Host state is 0x%lx (expected 0x%lx)",
> > +			       data->exp == obs, obs, data->exp);
> > +	} else {
> > +		if (xfail)
> > +			enter_guest_with_invalid_guest_state();
> > +		else
> > +			enter_guest();
> > +		success = VMX_VMCALL == (vmcs_read(EXI_REASON) & 0xff);
> > +		guest_rip = vmcs_read(GUEST_RIP);
> > +		inst_len = vmcs_read(EXI_INST_LEN);
> > +		if (success)
> > +			vmcs_write(GUEST_RIP, guest_rip + inst_len);
> 
> Is it possible to re-use the existing report_guest_state_test() here and the
> below code ?

Ah, I missed this in my cursory search. Better to reuse what's already
here :)

> > +	}
> > +	if (!success) {
> > +		inst_err = vmcs_read(VMX_INST_ERROR);
> > +		report("vmlaunch failed, VMX Inst Error is %d (expected %d)",
> > +		       xerror == inst_err, inst_err, xerror);
> > +	} else {
> > +		report("vmlaunch succeeded", success != xfail);
> > +	}
> > +}
> > +
> > +/*
> > + * test_load_pgc is a generic function for testing the
> > + * "load IA32_PERF_GLOBAL_CTRL" VM-{entry,exit} control. This test function
> > + * will test the provided ctrl_val disabled and enabled.
> > + *
> > + * @nr - VMCS field number corresponding to the Host/Guest state field
> > + * @name - Name of the above VMCS field for printing in test report
> > + * @ctrl_nr - VMCS field number corresponding to the VM-{entry,exit} control
> > + * @ctrl_val - Bit to set on the ctrl field.
> > + */
> > +static void test_load_pgc(u32 nr, const char *name, u32 ctrl_nr,
> > +			  const char *ctrl_name, u64 ctrl_val)
> > +{
> > +	u64 ctrl_saved = vmcs_read(ctrl_nr);
> > +	u64 pgc_saved = vmcs_read(nr);
> > +	u64 i, val;
> > +	bool host = nr == HOST_PERF_GLOBAL_CTRL;
> > +	struct load_state_test_data *data = &load_state_test_data;
> > +
> > +	data->msr = MSR_CORE_PERF_GLOBAL_CTRL;
> > +	msr_bmp_init();
> > +	if (!host) {
> > +		vmx_set_test_stage(1);
> > +		test_set_guest(guest_state_test_main);
> > +	}
> > +	vmcs_write(ctrl_nr, ctrl_saved & ~ctrl_val);
> > +	data->enabled = false;
> > +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=0 on %s",
> > +			    ctrl_name);
> > +	for (i = 0; i < 64; i++) {
> > +		val = 1ull << i;
> > +		vmcs_write(nr, val);
> > +		report_prefix_pushf("%s = 0x%lx", name, val);
> > +		/*
> > +		 * If the "load IA32_PERF_GLOBAL_CTRL" bit is 0 then
> > +		 * the {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL field is ignored,
> > +		 * thus setting reserved bits in this field does not cause
> > +		 * vmlaunch to fail.
> > +		 */
> 
> This comment is really not required as it's obvious that there's no effect
> on VM-entry if the control bit is not set.

I'll drop this.

> > +		test_pgc_vmlaunch(0, false, host);
> > +		report_prefix_pop();
> > +	}
> > +	report_prefix_pop();
> > +
> > +	vmcs_write(ctrl_nr, ctrl_saved | ctrl_val);
> > +	data->enabled = true;
> > +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=1 on %s",
> > +			    ctrl_name);
> > +	for (i = 0; i < 64; i++) {
> > +		val = 1ull << i;
> > +		data->exp = val;
> > +		vmcs_write(nr, val);
> > +		report_prefix_pushf("%s = 0x%lx", name, val);
> > +		if (valid_pgc(val)) {
> > +			test_pgc_vmlaunch(0, false, host);
> > +		} else {
> > +			/*
> > +			 * [SDM 30.4]
> > +			 *
> > +			 * Invalid host state fields result in an VM
> > +			 * instruction error with error number 8
> > +			 * (VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
> > +			 */
> > +			if (host) {
> > +				test_pgc_vmlaunch(
> > +					VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
> > +					true, host);
> > +			/*
> > +			 * [SDM 26.1]
> > +			 *
> > +			 * If a VM-Entry fails according to one of
> > +			 * the guest-state checks, the exit reason on the VMCS
> > +			 * will be set to reason number 33 (VMX_FAIL_STATE)
> 
> Again, these comments are probably not necessary.

I wanted to capture the nuance between host/guest, since the control
flow seems unnecessarily complicated on first glance. If they're
distracting, I'll drop these comments as well.

> > +			 */
> > +			} else {
> > +				test_pgc_vmlaunch(
> > +					0,
> > +					true, host);
> > +				TEST_ASSERT_EQ(
> > +					VMX_ENTRY_FAILURE | VMX_FAIL_STATE,
> > +					vmcs_read(EXI_REASON));
> > +			}
> > +		}
> > +		report_prefix_pop();
> > +	}
> > +
> > +	report_prefix_pop();
> > +
> > +	if (nr == GUEST_PERF_GLOBAL_CTRL) {
> > +		/*
> > +		 * Let the guest finish execution
> > +		 */
> > +		vmx_set_test_stage(2);
> > +		vmcs_write(ctrl_nr, ctrl_saved);
> > +		vmcs_write(nr, pgc_saved);
> > +		enter_guest();
> > +	}
> > +
> > +	vmcs_write(ctrl_nr, ctrl_saved);
> > +	vmcs_write(nr, pgc_saved);
> > +}
> > +
> > +static void test_load_host_pgc(void)
> > +{
> > +	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
> > +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
> > +		       "exit control not supported\n");
> > +		return;
> > +	}
> > +
> > +	test_load_pgc(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
> > +		      EXI_CONTROLS, "EXI_CONTROLS", EXI_LOAD_PERF);
> > +}
> > +
> > +
> > +static void test_load_guest_pgc(void)
> > +{
> > +	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
> > +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
> > +		       "entry control not supported\n");
> > +	}
> > +
> > +	test_load_pgc(GUEST_PERF_GLOBAL_CTRL, "GUEST_PERF_GLOBAL_CTRL",
> > +		      ENT_CONTROLS, "ENT_CONTROLS", ENT_LOAD_PERF);
> > +}
> > +
> >   /*
> >    * PAT values higher than 8 are uninteresting since they're likely lumped
> >    * in with "8". We only test values above 8 one bit at a time,
> > @@ -7147,6 +7341,7 @@ static void vmx_host_state_area_test(void)
> >   	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
> >   	test_host_efer();
> > +	test_load_host_pgc();
> 
> I would rename it to test_load_host_perf_global_ctrl to avoid confusion
> between "performance counter" and "VMCS control field" though the name is
> bit long.

Sure thing.

> >   	test_load_host_pat();
> >   	test_host_segment_regs();
> >   	test_host_desc_tables();
> > @@ -8587,7 +8782,6 @@ static int invalid_msr_entry_failure(struct vmentry_failure *failure)
> >   	return VMX_TEST_VMEXIT;
> >   }
> > -
> >   #define TEST(name) { #name, .v2 = name }
> >   /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
> > @@ -8637,6 +8831,7 @@ struct vmx_test vmx_tests[] = {
> >   	TEST(vmx_host_state_area_test),
> >   	TEST(vmx_guest_state_area_test),
> >   	TEST(vmentry_movss_shadow_test),
> > +	TEST(test_load_guest_pgc),
> 
> Same comment as above.
> The other comment I have is, why not put this inside of
> 'vmx_guest_state_area_test' because this VMCS field belongs to Guest State
> Area only.

This is actually a point of discussion. Are we going to want every guest
state test to set up its guest each time? As the code stands currently,
we can only set an L2 guest once, or the test will fail out.

This was less invasive, but I think the better solution would be to set
up the guest once from 'vmx_guest_state_area_test', let the test
functions run, then clean it up. This would require also moving the
guest setup tidbits out of test_pat(). Would you agree this is the
better route?

> >   	/* APICv tests */
> >   	TEST(vmx_eoi_bitmap_ioapic_scan_test),
> >   	TEST(vmx_hlt_with_rvi_test),
>

Thanks for the review, Krish. I'll address style fixes where noted, only
pending question is on the guest state test.

--
Thanks,
Oliver

Re: [kvm-unit-tests PATCH v3 7/8] x86: VMX: Make guest_state_test_main() check state from nested VM

[Oliver Upton]

On Wed, Sep 04, 2019 at 05:25:40PM -0700, Krish Sadhukhan wrote:
> 
> 
> On 09/03/2019 02:58 PM, Oliver Upton wrote:
> > The current tests for guest state do not yet check the validity of
> > loaded state from within the nested VM. Introduce the
> > load_state_test_data struct to share data with the nested VM.
> > 
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >   x86/vmx_tests.c | 23 ++++++++++++++++++++---
> >   1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > index f035f24a771a..b72a27583793 100644
> > --- a/x86/vmx_tests.c
> > +++ b/x86/vmx_tests.c
> > @@ -5017,13 +5017,28 @@ static void test_entry_msr_load(void)
> >   	test_vmx_valid_controls(false);
> >   }
> > +static struct load_state_test_data {
> > +	u32 msr;
> > +	u64 exp;
> > +	bool enabled;
> > +} load_state_test_data;
> 
> A better name is probably 'loaded_state_test_data'  as you are checking the
> validity of the loaded MSR in the guest.

Other usages of structs for data sharing follow the previous naming
convention, but I slightly missed the mark with that as well. Other
structs seem to use the same prefix that the associated tests have (e.g.
ept_access_test_data corresponds to ept_access_test_*). To best match
that pattern, I should instead name it "vmx_state_area_test_data" (since
its used for both guest/host test data anyway.

That isn't to say there is a better pattern we could follow for naming
this! Which do you think is better?

> > +
> >   static void guest_state_test_main(void)
> >   {
> > +	u64 obs;
> > +	struct load_state_test_data *data = &load_state_test_data;
> > +
> >   	while (1) {
> > -		if (vmx_get_test_stage() != 2)
> > -			vmcall();
> > -		else
> > +		if (vmx_get_test_stage() == 2)
> >   			break;
> > +
> > +		if (data->enabled) {
> > +			obs = rdmsr(obs);
> 
> Although you fixed it in the next patch, why not use  'data->msr' in place
> of 'obs' as the parameter to rdmsr() in this patch only ?

Ugh, I mucked this up when reworking before sending out. 'data->msr'
should have appeared in this patch. I'll fix this.

> > +			report("Guest state is 0x%lx (expected 0x%lx)",
> > +			       data->exp == obs, obs, data->exp);
> > +		}
> > +
> > +		vmcall();
> >   	}
> >   	asm volatile("fnop");
> > @@ -6854,7 +6869,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
> >   	u64 i, val;
> >   	u32 j;
> >   	int error;
> > +	struct load_state_test_data *data = &load_state_test_data;
> > +	data->enabled = false;
> >   	vmcs_clear_bits(ctrl_field, ctrl_bit);
> >   	if (field == GUEST_PAT) {
> >   		vmx_set_test_stage(1);
>

Thanks for the review, Krish. Looks like a typo I didn't rework into
this patch correctly, please let me know what you think on the other
comment.

--
Thanks,
Oliver

Re: [kvm-unit-tests PATCH v3 8/8] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Krish Sadhukhan]

On 09/04/2019 05:35 PM, Oliver Upton wrote:
> On Wed, Sep 04, 2019 at 05:13:17PM -0700, Krish Sadhukhan wrote:
>>
>> On 09/03/2019 02:58 PM, Oliver Upton wrote:
>>> Tests to verify that KVM performs the correct checks on Host/Guest state
>>> at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
>>> Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
>>> Control Registers and MSRs".
>>>
>>> Test that KVM does the following:
>>>
>>>       If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
>>>       reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>>>       GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>>>       should fail with an exit reason of "VM-entry failure due to invalid
>>>       guest state" (33). On a successful VM-entry, the correct value
>>>       should be observed when the nested VM performs an RDMSR on
>>>       IA32_PERF_GLOBAL_CTRL.
>>>
>>>       If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
>>>       reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>>>       HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>>>       should fail with a VM-instruction error of "VM entry with invalid
>>>       host-state field(s)" (8). On a successful VM-exit, the correct value
>>>       should be observed when L1 performs an RDMSR on
>>>       IA32_PERF_GLOBAL_CTRL.
>>>
>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>> Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> Signed-off-by: Oliver Upton <oupton@google.com>
>>> ---
>>>    x86/vmx_tests.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 197 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index b72a27583793..73c46eba6be9 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -5033,7 +5033,7 @@ static void guest_state_test_main(void)
>>>    			break;
>>>    		if (data->enabled) {
>>> -			obs = rdmsr(obs);
>>> +			obs = rdmsr(data->msr);
>>>    			report("Guest state is 0x%lx (expected 0x%lx)",
>>>    			       data->exp == obs, obs, data->exp);
>>>    		}
>>> @@ -6854,6 +6854,200 @@ static void test_host_efer(void)
>>>    	test_efer(HOST_EFER, "HOST_EFER", EXI_CONTROLS, EXI_LOAD_EFER);
>>>    }
>>> +union cpuid10_eax {
>>> +	struct {
>>> +		unsigned int version_id:8;
>>> +		unsigned int num_counters:8;
>> It's better to name it num_gpc or num_counters_gp.
> Ack to all style comments that follow. I brought these structs over from
> kvm, but I'll align the style with what is suggested for kvm-unit-tests.
>
>>> +		unsigned int bit_width:8;
>>> +		unsigned int mask_length:8;
>>> +	} split;
>>> +	unsigned int full;
>>> +};
>>> +
>>> +union cpuid10_edx {
>>> +	struct {
>>> +		unsigned int num_counters_fixed:5;
>>> +		unsigned int bit_width_fixed:8;
>>> +		unsigned int reserved:19;
>>> +	} split;
>>> +	unsigned int full;
>>> +};
>>> +
>> In kvm-unit-test source,  the naming of variables, functions etc. seems to
>> be based on the hex value of the CPUID leaf that they represent. For
>> example, cpuid_7_ebx, check_cpuid_80000001_edx etc. Isn't it better to name
>> these structures something like cpuidA_eax and cpuidA_edx ?
>>
>>> +static bool valid_pgc(u64 val)
>>> +{
>>> +	struct cpuid id;
>>> +	union cpuid10_eax eax;
>>> +	union cpuid10_edx edx;
>>> +	u64 mask;
>>> +
>>> +	id = cpuid(0xA);
>>> +	eax.full = id.a;
>>> +	edx.full = id.d;
>>> +	mask = ~(((1ull << eax.split.num_counters) - 1) |
>>> +		(((1ull << edx.split.num_counters_fixed) - 1) << 32));
>>> +
>>> +	return !(val & mask);
>>> +}
>>> +
>>> +static void test_pgc_vmlaunch(u32 xerror, bool xfail, bool host)
>>> +{
>>> +	u32 inst_err;
>>> +	u64 guest_rip, inst_len, obs;
>>> +	bool success;
>>> +	struct load_state_test_data *data = &load_state_test_data;
>>> +
>>> +	if (host) {
>>> +		success = vmlaunch_succeeds();
>>> +		obs = rdmsr(data->msr);
>>> +		if (data->enabled && success)
>>> +			report("Host state is 0x%lx (expected 0x%lx)",
>>> +			       data->exp == obs, obs, data->exp);
>>> +	} else {
>>> +		if (xfail)
>>> +			enter_guest_with_invalid_guest_state();
>>> +		else
>>> +			enter_guest();
>>> +		success = VMX_VMCALL == (vmcs_read(EXI_REASON) & 0xff);
>>> +		guest_rip = vmcs_read(GUEST_RIP);
>>> +		inst_len = vmcs_read(EXI_INST_LEN);
>>> +		if (success)
>>> +			vmcs_write(GUEST_RIP, guest_rip + inst_len);
>> Is it possible to re-use the existing report_guest_state_test() here and the
>> below code ?
> Ah, I missed this in my cursory search. Better to reuse what's already
> here :)
>
>>> +	}
>>> +	if (!success) {
>>> +		inst_err = vmcs_read(VMX_INST_ERROR);
>>> +		report("vmlaunch failed, VMX Inst Error is %d (expected %d)",
>>> +		       xerror == inst_err, inst_err, xerror);
>>> +	} else {
>>> +		report("vmlaunch succeeded", success != xfail);
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * test_load_pgc is a generic function for testing the
>>> + * "load IA32_PERF_GLOBAL_CTRL" VM-{entry,exit} control. This test function
>>> + * will test the provided ctrl_val disabled and enabled.
>>> + *
>>> + * @nr - VMCS field number corresponding to the Host/Guest state field
>>> + * @name - Name of the above VMCS field for printing in test report
>>> + * @ctrl_nr - VMCS field number corresponding to the VM-{entry,exit} control
>>> + * @ctrl_val - Bit to set on the ctrl field.
>>> + */
>>> +static void test_load_pgc(u32 nr, const char *name, u32 ctrl_nr,
>>> +			  const char *ctrl_name, u64 ctrl_val)
>>> +{
>>> +	u64 ctrl_saved = vmcs_read(ctrl_nr);
>>> +	u64 pgc_saved = vmcs_read(nr);
>>> +	u64 i, val;
>>> +	bool host = nr == HOST_PERF_GLOBAL_CTRL;
>>> +	struct load_state_test_data *data = &load_state_test_data;
>>> +
>>> +	data->msr = MSR_CORE_PERF_GLOBAL_CTRL;
>>> +	msr_bmp_init();
>>> +	if (!host) {
>>> +		vmx_set_test_stage(1);
>>> +		test_set_guest(guest_state_test_main);
>>> +	}
>>> +	vmcs_write(ctrl_nr, ctrl_saved & ~ctrl_val);
>>> +	data->enabled = false;
>>> +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=0 on %s",
>>> +			    ctrl_name);
>>> +	for (i = 0; i < 64; i++) {
>>> +		val = 1ull << i;
>>> +		vmcs_write(nr, val);
>>> +		report_prefix_pushf("%s = 0x%lx", name, val);
>>> +		/*
>>> +		 * If the "load IA32_PERF_GLOBAL_CTRL" bit is 0 then
>>> +		 * the {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL field is ignored,
>>> +		 * thus setting reserved bits in this field does not cause
>>> +		 * vmlaunch to fail.
>>> +		 */
>> This comment is really not required as it's obvious that there's no effect
>> on VM-entry if the control bit is not set.
> I'll drop this.
>
>>> +		test_pgc_vmlaunch(0, false, host);
>>> +		report_prefix_pop();
>>> +	}
>>> +	report_prefix_pop();
>>> +
>>> +	vmcs_write(ctrl_nr, ctrl_saved | ctrl_val);
>>> +	data->enabled = true;
>>> +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=1 on %s",
>>> +			    ctrl_name);
>>> +	for (i = 0; i < 64; i++) {
>>> +		val = 1ull << i;
>>> +		data->exp = val;
>>> +		vmcs_write(nr, val);
>>> +		report_prefix_pushf("%s = 0x%lx", name, val);
>>> +		if (valid_pgc(val)) {
>>> +			test_pgc_vmlaunch(0, false, host);
>>> +		} else {
>>> +			/*
>>> +			 * [SDM 30.4]
>>> +			 *
>>> +			 * Invalid host state fields result in an VM
>>> +			 * instruction error with error number 8
>>> +			 * (VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
>>> +			 */
>>> +			if (host) {
>>> +				test_pgc_vmlaunch(
>>> +					VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>>> +					true, host);
>>> +			/*
>>> +			 * [SDM 26.1]
>>> +			 *
>>> +			 * If a VM-Entry fails according to one of
>>> +			 * the guest-state checks, the exit reason on the VMCS
>>> +			 * will be set to reason number 33 (VMX_FAIL_STATE)
>> Again, these comments are probably not necessary.
> I wanted to capture the nuance between host/guest, since the control
> flow seems unnecessarily complicated on first glance. If they're
> distracting, I'll drop these comments as well.

The only reason for this comment is other guest state tests currently do 
not have such elaborate comments.
But I am fine either way.

>
>>> +			 */
>>> +			} else {
>>> +				test_pgc_vmlaunch(
>>> +					0,
>>> +					true, host);
>>> +				TEST_ASSERT_EQ(
>>> +					VMX_ENTRY_FAILURE | VMX_FAIL_STATE,
>>> +					vmcs_read(EXI_REASON));
>>> +			}
>>> +		}
>>> +		report_prefix_pop();
>>> +	}
>>> +
>>> +	report_prefix_pop();
>>> +
>>> +	if (nr == GUEST_PERF_GLOBAL_CTRL) {
>>> +		/*
>>> +		 * Let the guest finish execution
>>> +		 */
>>> +		vmx_set_test_stage(2);
>>> +		vmcs_write(ctrl_nr, ctrl_saved);
>>> +		vmcs_write(nr, pgc_saved);
>>> +		enter_guest();
>>> +	}
>>> +
>>> +	vmcs_write(ctrl_nr, ctrl_saved);
>>> +	vmcs_write(nr, pgc_saved);
>>> +}
>>> +
>>> +static void test_load_host_pgc(void)
>>> +{
>>> +	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
>>> +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
>>> +		       "exit control not supported\n");
>>> +		return;
>>> +	}
>>> +
>>> +	test_load_pgc(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
>>> +		      EXI_CONTROLS, "EXI_CONTROLS", EXI_LOAD_PERF);
>>> +}
>>> +
>>> +
>>> +static void test_load_guest_pgc(void)
>>> +{
>>> +	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
>>> +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
>>> +		       "entry control not supported\n");
>>> +	}
>>> +
>>> +	test_load_pgc(GUEST_PERF_GLOBAL_CTRL, "GUEST_PERF_GLOBAL_CTRL",
>>> +		      ENT_CONTROLS, "ENT_CONTROLS", ENT_LOAD_PERF);
>>> +}
>>> +
>>>    /*
>>>     * PAT values higher than 8 are uninteresting since they're likely lumped
>>>     * in with "8". We only test values above 8 one bit at a time,
>>> @@ -7147,6 +7341,7 @@ static void vmx_host_state_area_test(void)
>>>    	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
>>>    	test_host_efer();
>>> +	test_load_host_pgc();
>> I would rename it to test_load_host_perf_global_ctrl to avoid confusion
>> between "performance counter" and "VMCS control field" though the name is
>> bit long.
> Sure thing.
>
>>>    	test_load_host_pat();
>>>    	test_host_segment_regs();
>>>    	test_host_desc_tables();
>>> @@ -8587,7 +8782,6 @@ static int invalid_msr_entry_failure(struct vmentry_failure *failure)
>>>    	return VMX_TEST_VMEXIT;
>>>    }
>>> -
>>>    #define TEST(name) { #name, .v2 = name }
>>>    /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
>>> @@ -8637,6 +8831,7 @@ struct vmx_test vmx_tests[] = {
>>>    	TEST(vmx_host_state_area_test),
>>>    	TEST(vmx_guest_state_area_test),
>>>    	TEST(vmentry_movss_shadow_test),
>>> +	TEST(test_load_guest_pgc),
>> Same comment as above.
>> The other comment I have is, why not put this inside of
>> 'vmx_guest_state_area_test' because this VMCS field belongs to Guest State
>> Area only.
> This is actually a point of discussion. Are we going to want every guest
> state test to set up its guest each time? As the code stands currently,
> we can only set an L2 guest once, or the test will fail out.
>
> This was less invasive, but I think the better solution would be to set
> up the guest once from 'vmx_guest_state_area_test', let the test
> functions run, then clean it up. This would require also moving the
> guest setup tidbits out of test_pat(). Would you agree this is the
> better route?

As you said, the current implementation allows a guest executable to be 
set only once for each of the entries in 'vmx_tests' array. Hence 
sub-tests can not setup their own guest.

Now, the sub-tests (including future additions) within 
'vmx_guest_state_area_test' are complete and independent tests on their 
own. The only reason for grouping them under 'vmx_guest_state_area_test' 
is to organize them in a nice hierarchical fashion.

May be, a better solution is to remove the restriction in 
test_set_guest() and let each test/sub-test set it's own guest. This 
will also mean that test_set_guest() should set GUEST_RIP as well.

>
>>>    	/* APICv tests */
>>>    	TEST(vmx_eoi_bitmap_ioapic_scan_test),
>>>    	TEST(vmx_hlt_with_rvi_test),
> Thanks for the review, Krish. I'll address style fixes where noted, only
> pending question is on the guest state test.
>
> --
> Thanks,
> Oliver

Re: [kvm-unit-tests PATCH v3 7/8] x86: VMX: Make guest_state_test_main() check state from nested VM

[Krish Sadhukhan]

On 09/04/2019 05:49 PM, Oliver Upton wrote:
> On Wed, Sep 04, 2019 at 05:25:40PM -0700, Krish Sadhukhan wrote:
>>
>> On 09/03/2019 02:58 PM, Oliver Upton wrote:
>>> The current tests for guest state do not yet check the validity of
>>> loaded state from within the nested VM. Introduce the
>>> load_state_test_data struct to share data with the nested VM.
>>>
>>> Signed-off-by: Oliver Upton <oupton@google.com>
>>> ---
>>>    x86/vmx_tests.c | 23 ++++++++++++++++++++---
>>>    1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index f035f24a771a..b72a27583793 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -5017,13 +5017,28 @@ static void test_entry_msr_load(void)
>>>    	test_vmx_valid_controls(false);
>>>    }
>>> +static struct load_state_test_data {
>>> +	u32 msr;
>>> +	u64 exp;
>>> +	bool enabled;
>>> +} load_state_test_data;
>> A better name is probably 'loaded_state_test_data'  as you are checking the
>> validity of the loaded MSR in the guest.
> Other usages of structs for data sharing follow the previous naming
> convention, but I slightly missed the mark with that as well. Other
> structs seem to use the same prefix that the associated tests have (e.g.
> ept_access_test_data corresponds to ept_access_test_*). To best match
> that pattern, I should instead name it "vmx_state_area_test_data" (since
> its used for both guest/host test data anyway.
>
> That isn't to say there is a better pattern we could follow for naming
> this! Which do you think is better?

'vmx_state_area_test_data' sounds fine to me. Thanks !

>
>>> +
>>>    static void guest_state_test_main(void)
>>>    {
>>> +	u64 obs;
>>> +	struct load_state_test_data *data = &load_state_test_data;
>>> +
>>>    	while (1) {
>>> -		if (vmx_get_test_stage() != 2)
>>> -			vmcall();
>>> -		else
>>> +		if (vmx_get_test_stage() == 2)
>>>    			break;
>>> +
>>> +		if (data->enabled) {
>>> +			obs = rdmsr(obs);
>> Although you fixed it in the next patch, why not use  'data->msr' in place
>> of 'obs' as the parameter to rdmsr() in this patch only ?
> Ugh, I mucked this up when reworking before sending out. 'data->msr'
> should have appeared in this patch. I'll fix this.
>
>>> +			report("Guest state is 0x%lx (expected 0x%lx)",
>>> +			       data->exp == obs, obs, data->exp);
>>> +		}
>>> +
>>> +		vmcall();
>>>    	}
>>>    	asm volatile("fnop");
>>> @@ -6854,7 +6869,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>>>    	u64 i, val;
>>>    	u32 j;
>>>    	int error;
>>> +	struct load_state_test_data *data = &load_state_test_data;
>>> +	data->enabled = false;
>>>    	vmcs_clear_bits(ctrl_field, ctrl_bit);
>>>    	if (field == GUEST_PAT) {
>>>    		vmx_set_test_stage(1);
> Thanks for the review, Krish. Looks like a typo I didn't rework into
> this patch correctly, please let me know what you think on the other
> comment.
>
> --
> Thanks,
> Oliver