[PATCH v2 0/5] KVM: nVMX: Small fixes improving emulation accuracy

[PATCH v2 0/5] KVM: nVMX: Small fixes improving emulation accuracy

[Jan Kiszka]

Changes in v2:
- reordering to avoid breaking the disabling of CR3 access interception
- express VMX_BASIC_TRUE_CTLS via bit-shift
- rename host_debugctl -> vmcs01_debugctl

Jan Kiszka (5):
  KVM: nVMX: Advertise support for MSR_IA32_VMX_TRUE_*_CTLS
  KVM: nVMX: Allow to disable CR3 access interception
  KVM: nVMX: Fix returned value of MSR_IA32_VMX_PROCBASED_CTLS
  KVM: nVMX: Allow to disable VM_{ENTRY_LOAD,EXIT_SAVE}_DEBUG_CONTROLS
  KVM: nVMX: Fix returned value of MSR_IA32_VMX_VMCS_ENUM

 arch/x86/include/asm/vmx.h            |  3 ++
 arch/x86/include/uapi/asm/msr-index.h |  1 +
 arch/x86/kvm/vmx.c                    | 75 +++++++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 21 deletions(-)

-- 
1.8.1.1.298.ge7eed54

[PATCH v2 1/5] KVM: nVMX: Advertise support for MSR_IA32_VMX_TRUE_*_CTLS

[Jan Kiszka]

We already implemented them but failed to advertise them. Currently they
all return the identical values to the capability MSRs they are
augmenting. So there is no change in exposed features yet.

Drop related comments at this chance that are partially incorrect and
redundant anyway.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/include/uapi/asm/msr-index.h |  1 +
 arch/x86/kvm/vmx.c                    | 13 ++-----------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index fcf2b3a..eaefcc6 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -558,6 +558,7 @@
 
 /* VMX_BASIC bits and bitmasks */
 #define VMX_BASIC_VMCS_SIZE_SHIFT	32
+#define VMX_BASIC_TRUE_CTLS		(1ULL << 55)
 #define VMX_BASIC_64		0x0001000000000000LLU
 #define VMX_BASIC_MEM_TYPE_SHIFT	50
 #define VMX_BASIC_MEM_TYPE_MASK	0x003c000000000000LLU
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 801332e..536f341 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2265,21 +2265,13 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	/* pin-based controls */
 	rdmsr(MSR_IA32_VMX_PINBASED_CTLS,
 	      nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high);
-	/*
-	 * According to the Intel spec, if bit 55 of VMX_BASIC is off (as it is
-	 * in our case), bits 1, 2 and 4 (i.e., 0x16) must be 1 in this MSR.
-	 */
 	nested_vmx_pinbased_ctls_low |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR;
 	nested_vmx_pinbased_ctls_high &= PIN_BASED_EXT_INTR_MASK |
 		PIN_BASED_NMI_EXITING | PIN_BASED_VIRTUAL_NMIS;
 	nested_vmx_pinbased_ctls_high |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR |
 		PIN_BASED_VMX_PREEMPTION_TIMER;
 
-	/*
-	 * Exit controls
-	 * If bit 55 of VMX_BASIC is off, bits 0-8 and 10, 11, 13, 14, 16 and
-	 * 17 must be 1.
-	 */
+	/* exit controls */
 	rdmsr(MSR_IA32_VMX_EXIT_CTLS,
 		nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high);
 	nested_vmx_exit_ctls_low = VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR;
@@ -2299,7 +2291,6 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	/* entry controls */
 	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
 		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
-	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
 	nested_vmx_entry_ctls_low = VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR;
 	nested_vmx_entry_ctls_high &=
 #ifdef CONFIG_X86_64
@@ -2394,7 +2385,7 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 		 * guest, and the VMCS structure we give it - not about the
 		 * VMX support of the underlying hardware.
 		 */
-		*pdata = VMCS12_REVISION |
+		*pdata = VMCS12_REVISION | VMX_BASIC_TRUE_CTLS |
 			   ((u64)VMCS12_SIZE << VMX_BASIC_VMCS_SIZE_SHIFT) |
 			   (VMX_BASIC_MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
 		break;
-- 
1.8.1.1.298.ge7eed54

[PATCH v2 2/5] KVM: nVMX: Allow to disable CR3 access interception

[Jan Kiszka]

We already have this control enabled by exposing a broken
MSR_IA32_VMX_PROCBASED_CTLS value. This will properly advertise our
capability once the value is fixed by clearing the right bits in
MSR_IA32_VMX_TRUE_PROCBASED_CTLS. We also have to ensure to test the
right value on L2 entry.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 536f341..7568679 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2239,6 +2239,7 @@ static inline bool nested_vmx_allowed(struct kvm_vcpu *vcpu)
  * or other means.
  */
 static u32 nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high;
+static u32 nested_vmx_true_procbased_ctls_low;
 static u32 nested_vmx_secondary_ctls_low, nested_vmx_secondary_ctls_high;
 static u32 nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high;
 static u32 nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high;
@@ -2328,6 +2329,10 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	 */
 	nested_vmx_procbased_ctls_high |= CPU_BASED_USE_MSR_BITMAPS;
 
+	/* We support free control of CR3 access interception. */
+	nested_vmx_true_procbased_ctls_low = nested_vmx_procbased_ctls_low &
+		~(CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING);
+
 	/* secondary cpu-based controls */
 	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2,
 		nested_vmx_secondary_ctls_low, nested_vmx_secondary_ctls_high);
@@ -2395,6 +2400,9 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 					nested_vmx_pinbased_ctls_high);
 		break;
 	case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
+		*pdata = vmx_control_msr(nested_vmx_true_procbased_ctls_low,
+					nested_vmx_procbased_ctls_high);
+		break;
 	case MSR_IA32_VMX_PROCBASED_CTLS:
 		*pdata = vmx_control_msr(nested_vmx_procbased_ctls_low,
 					nested_vmx_procbased_ctls_high);
@@ -8127,7 +8135,8 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 	}
 
 	if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
-	      nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high) ||
+				nested_vmx_true_procbased_ctls_low,
+				nested_vmx_procbased_ctls_high) ||
 	    !vmx_control_verify(vmcs12->secondary_vm_exec_control,
 	      nested_vmx_secondary_ctls_low, nested_vmx_secondary_ctls_high) ||
 	    !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
-- 
1.8.1.1.298.ge7eed54

[PATCH v2 3/5] KVM: nVMX: Fix returned value of MSR_IA32_VMX_PROCBASED_CTLS

[Jan Kiszka]

SDM says bits 1, 4-6, 8, 13-16, and 26 have to be set.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/include/asm/vmx.h | 3 +++
 arch/x86/kvm/vmx.c         | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index d989829..bcbfade 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -51,6 +51,9 @@
 #define CPU_BASED_MONITOR_EXITING               0x20000000
 #define CPU_BASED_PAUSE_EXITING                 0x40000000
 #define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS   0x80000000
+
+#define CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR	0x0401e172
+
 /*
  * Definitions of Secondary Processor-Based VM-Execution Controls.
  */
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7568679..475f2dc 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2306,7 +2306,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	/* cpu-based controls */
 	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
 		nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high);
-	nested_vmx_procbased_ctls_low = 0;
+	nested_vmx_procbased_ctls_low = CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR;
 	nested_vmx_procbased_ctls_high &=
 		CPU_BASED_VIRTUAL_INTR_PENDING |
 		CPU_BASED_VIRTUAL_NMI_PENDING | CPU_BASED_USE_TSC_OFFSETING |
@@ -2327,7 +2327,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	 * can use it to avoid exits to L1 - even when L0 runs L2
 	 * without MSR bitmaps.
 	 */
-	nested_vmx_procbased_ctls_high |= CPU_BASED_USE_MSR_BITMAPS;
+	nested_vmx_procbased_ctls_high |= CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR |
+		CPU_BASED_USE_MSR_BITMAPS;
 
 	/* We support free control of CR3 access interception. */
 	nested_vmx_true_procbased_ctls_low = nested_vmx_procbased_ctls_low &
-- 
1.8.1.1.298.ge7eed54

[PATCH v2 4/5] KVM: nVMX: Allow to disable VM_{ENTRY_LOAD,EXIT_SAVE}_DEBUG_CONTROLS

[Jan Kiszka]

Allow L1 to "leak" its debug controls into L2, i.e. permit cleared
VM_{ENTRY_LOAD,EXIT_SAVE}_DEBUG_CONTROLS. This requires to manually
transfer the state of DR7 and IA32_DEBUGCTLMSR from L1 into L2 as both
run on different VMCS.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 475f2dc..f20a5ee 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -383,6 +383,9 @@ struct nested_vmx {
 
 	struct hrtimer preemption_timer;
 	bool preemption_timer_expired;
+
+	/* to migrate it to L2 if VM_ENTRY_LOAD_DEBUG_CONTROLS is off */
+	u64 vmcs01_debugctl;
 };
 
 #define POSTED_INTR_ON  0
@@ -2243,7 +2246,9 @@ static u32 nested_vmx_true_procbased_ctls_low;
 static u32 nested_vmx_secondary_ctls_low, nested_vmx_secondary_ctls_high;
 static u32 nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high;
 static u32 nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high;
+static u32 nested_vmx_true_exit_ctls_low;
 static u32 nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high;
+static u32 nested_vmx_true_entry_ctls_low;
 static u32 nested_vmx_misc_low, nested_vmx_misc_high;
 static u32 nested_vmx_ept_caps;
 static __init void nested_vmx_setup_ctls_msrs(void)
@@ -2289,6 +2294,10 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	if (vmx_mpx_supported())
 		nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
 
+	/* We support free control of debug control saving. */
+	nested_vmx_true_exit_ctls_low = nested_vmx_exit_ctls_low &
+		~VM_EXIT_SAVE_DEBUG_CONTROLS;
+
 	/* entry controls */
 	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
 		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
@@ -2303,6 +2312,10 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	if (vmx_mpx_supported())
 		nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
 
+	/* We support free control of debug control loading. */
+	nested_vmx_true_entry_ctls_low = nested_vmx_entry_ctls_low &
+		~VM_ENTRY_LOAD_DEBUG_CONTROLS;
+
 	/* cpu-based controls */
 	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
 		nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high);
@@ -2409,11 +2422,17 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 					nested_vmx_procbased_ctls_high);
 		break;
 	case MSR_IA32_VMX_TRUE_EXIT_CTLS:
+		*pdata = vmx_control_msr(nested_vmx_true_exit_ctls_low,
+					nested_vmx_exit_ctls_high);
+		break;
 	case MSR_IA32_VMX_EXIT_CTLS:
 		*pdata = vmx_control_msr(nested_vmx_exit_ctls_low,
 					nested_vmx_exit_ctls_high);
 		break;
 	case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+		*pdata = vmx_control_msr(nested_vmx_true_entry_ctls_low,
+					nested_vmx_entry_ctls_high);
+		break;
 	case MSR_IA32_VMX_ENTRY_CTLS:
 		*pdata = vmx_control_msr(nested_vmx_entry_ctls_low,
 					nested_vmx_entry_ctls_high);
@@ -7836,7 +7855,13 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
 	vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
 
-	vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) {
+		kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
+		vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
+	} else {
+		kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
+		vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.vmcs01_debugctl);
+	}
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
 		vmcs12->vm_entry_intr_info_field);
 	vmcs_write32(VM_ENTRY_EXCEPTION_ERROR_CODE,
@@ -7846,7 +7871,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
 		vmcs12->guest_interruptibility_info);
 	vmcs_write32(GUEST_SYSENTER_CS, vmcs12->guest_sysenter_cs);
-	kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
 	vmx_set_rflags(vcpu, vmcs12->guest_rflags);
 	vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS,
 		vmcs12->guest_pending_dbg_exceptions);
@@ -8143,9 +8167,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 	    !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
 	      nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high) ||
 	    !vmx_control_verify(vmcs12->vm_exit_controls,
-	      nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high) ||
+				nested_vmx_true_exit_ctls_low,
+				nested_vmx_exit_ctls_high) ||
 	    !vmx_control_verify(vmcs12->vm_entry_controls,
-	      nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high))
+				nested_vmx_true_entry_ctls_low,
+				nested_vmx_entry_ctls_high))
 	{
 		nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD);
 		return 1;
@@ -8222,6 +8248,9 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 
 	vmx->nested.vmcs01_tsc_offset = vmcs_read64(TSC_OFFSET);
 
+	if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS))
+		vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
+
 	cpu = get_cpu();
 	vmx->loaded_vmcs = vmcs02;
 	vmx_vcpu_put(vcpu);
@@ -8399,7 +8428,6 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	vmcs12->guest_cr0 = vmcs12_guest_cr0(vcpu, vmcs12);
 	vmcs12->guest_cr4 = vmcs12_guest_cr4(vcpu, vmcs12);
 
-	kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
 	vmcs12->guest_rsp = kvm_register_read(vcpu, VCPU_REGS_RSP);
 	vmcs12->guest_rip = kvm_register_read(vcpu, VCPU_REGS_RIP);
 	vmcs12->guest_rflags = vmcs_readl(GUEST_RFLAGS);
@@ -8478,9 +8506,13 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
 		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
 
+	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) {
+		kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
+		vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
+	}
+
 	/* TODO: These cannot have changed unless we have MSR bitmaps and
 	 * the relevant bit asks not to trap the change */
-	vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
 	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
 		vmcs12->guest_ia32_pat = vmcs_read64(GUEST_IA32_PAT);
 	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)
-- 
1.8.1.1.298.ge7eed54

[PATCH v2 5/5] KVM: nVMX: Fix returned value of MSR_IA32_VMX_VMCS_ENUM

[Jan Kiszka]

Many real CPUs get this wrong as well, but ours is totally off: bits 9:1
define the highest index value.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f20a5ee..eabd8d8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2461,7 +2461,7 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 		*pdata = -1ULL;
 		break;
 	case MSR_IA32_VMX_VMCS_ENUM:
-		*pdata = 0x1f;
+		*pdata = 0x2e; /* highest index: VMX_PREEMPTION_TIMER_VALUE */
 		break;
 	case MSR_IA32_VMX_PROCBASED_CTLS2:
 		*pdata = vmx_control_msr(nested_vmx_secondary_ctls_low,
-- 
1.8.1.1.298.ge7eed54

Re: [PATCH v2 0/5] KVM: nVMX: Small fixes improving emulation accuracy

[Paolo Bonzini]

Il 16/06/2014 13:59, Jan Kiszka ha scritto:
> Changes in v2:
> - reordering to avoid breaking the disabling of CR3 access interception
> - express VMX_BASIC_TRUE_CTLS via bit-shift
> - rename host_debugctl -> vmcs01_debugctl
>
> Jan Kiszka (5):
>   KVM: nVMX: Advertise support for MSR_IA32_VMX_TRUE_*_CTLS
>   KVM: nVMX: Allow to disable CR3 access interception
>   KVM: nVMX: Fix returned value of MSR_IA32_VMX_PROCBASED_CTLS
>   KVM: nVMX: Allow to disable VM_{ENTRY_LOAD,EXIT_SAVE}_DEBUG_CONTROLS
>   KVM: nVMX: Fix returned value of MSR_IA32_VMX_VMCS_ENUM
>
>  arch/x86/include/asm/vmx.h            |  3 ++
>  arch/x86/include/uapi/asm/msr-index.h |  1 +
>  arch/x86/kvm/vmx.c                    | 75 +++++++++++++++++++++++++----------
>  3 files changed, 58 insertions(+), 21 deletions(-)
>

All applied -- locally for now.

Paolo

Re: [PATCH v2 1/5] KVM: nVMX: Advertise support for MSR_IA32_VMX_TRUE_*_CTLS

[Bandan Das]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> We already implemented them but failed to advertise them. Currently they
> all return the identical values to the capability MSRs they are
> augmenting. So there is no change in exposed features yet.
>
> Drop related comments at this chance that are partially incorrect and
> redundant anyway.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/include/uapi/asm/msr-index.h |  1 +
>  arch/x86/kvm/vmx.c                    | 13 ++-----------
>  2 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> index fcf2b3a..eaefcc6 100644
> --- a/arch/x86/include/uapi/asm/msr-index.h
> +++ b/arch/x86/include/uapi/asm/msr-index.h
> @@ -558,6 +558,7 @@
>  
>  /* VMX_BASIC bits and bitmasks */
>  #define VMX_BASIC_VMCS_SIZE_SHIFT	32
> +#define VMX_BASIC_TRUE_CTLS		(1ULL << 55)

Curiosity question - know of any case that failed since we didn't
set this bit ?

>  #define VMX_BASIC_64		0x0001000000000000LLU
>  #define VMX_BASIC_MEM_TYPE_SHIFT	50
>  #define VMX_BASIC_MEM_TYPE_MASK	0x003c000000000000LLU
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 801332e..536f341 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2265,21 +2265,13 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  	/* pin-based controls */
>  	rdmsr(MSR_IA32_VMX_PINBASED_CTLS,
>  	      nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high);
> -	/*
> -	 * According to the Intel spec, if bit 55 of VMX_BASIC is off (as it is
> -	 * in our case), bits 1, 2 and 4 (i.e., 0x16) must be 1 in this MSR.
> -	 */
>  	nested_vmx_pinbased_ctls_low |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR;
>  	nested_vmx_pinbased_ctls_high &= PIN_BASED_EXT_INTR_MASK |
>  		PIN_BASED_NMI_EXITING | PIN_BASED_VIRTUAL_NMIS;
>  	nested_vmx_pinbased_ctls_high |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR |
>  		PIN_BASED_VMX_PREEMPTION_TIMER;
>  
> -	/*
> -	 * Exit controls
> -	 * If bit 55 of VMX_BASIC is off, bits 0-8 and 10, 11, 13, 14, 16 and
> -	 * 17 must be 1.
> -	 */
> +	/* exit controls */
>  	rdmsr(MSR_IA32_VMX_EXIT_CTLS,
>  		nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high);
>  	nested_vmx_exit_ctls_low = VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR;
> @@ -2299,7 +2291,6 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  	/* entry controls */
>  	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>  		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
> -	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
>  	nested_vmx_entry_ctls_low = VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR;
>  	nested_vmx_entry_ctls_high &=
>  #ifdef CONFIG_X86_64
> @@ -2394,7 +2385,7 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>  		 * guest, and the VMCS structure we give it - not about the
>  		 * VMX support of the underlying hardware.
>  		 */
> -		*pdata = VMCS12_REVISION |
> +		*pdata = VMCS12_REVISION | VMX_BASIC_TRUE_CTLS |
>  			   ((u64)VMCS12_SIZE << VMX_BASIC_VMCS_SIZE_SHIFT) |
>  			   (VMX_BASIC_MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
>  		break;

Re: [PATCH v2 3/5] KVM: nVMX: Fix returned value of MSR_IA32_VMX_PROCBASED_CTLS

[Bandan Das]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> SDM says bits 1, 4-6, 8, 13-16, and 26 have to be set.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/include/asm/vmx.h | 3 +++
>  arch/x86/kvm/vmx.c         | 5 +++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index d989829..bcbfade 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -51,6 +51,9 @@
>  #define CPU_BASED_MONITOR_EXITING               0x20000000
>  #define CPU_BASED_PAUSE_EXITING                 0x40000000
>  #define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS   0x80000000
> +
> +#define CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR	0x0401e172
> +

Nit: Do we want to keep this consistent ? The previous defines have spaces,
and you have introduced a tab

>  /*
>   * Definitions of Secondary Processor-Based VM-Execution Controls.
>   */
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7568679..475f2dc 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2306,7 +2306,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  	/* cpu-based controls */
>  	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>  		nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high);
> -	nested_vmx_procbased_ctls_low = 0;
> +	nested_vmx_procbased_ctls_low = CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR;
>  	nested_vmx_procbased_ctls_high &=
>  		CPU_BASED_VIRTUAL_INTR_PENDING |
>  		CPU_BASED_VIRTUAL_NMI_PENDING | CPU_BASED_USE_TSC_OFFSETING |
> @@ -2327,7 +2327,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  	 * can use it to avoid exits to L1 - even when L0 runs L2
>  	 * without MSR bitmaps.
>  	 */
> -	nested_vmx_procbased_ctls_high |= CPU_BASED_USE_MSR_BITMAPS;
> +	nested_vmx_procbased_ctls_high |= CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR |
> +		CPU_BASED_USE_MSR_BITMAPS;
>  
>  	/* We support free control of CR3 access interception. */
>  	nested_vmx_true_procbased_ctls_low = nested_vmx_procbased_ctls_low &

Re: [PATCH v2 4/5] KVM: nVMX: Allow to disable VM_{ENTRY_LOAD,EXIT_SAVE}_DEBUG_CONTROLS

[Bandan Das]

Jan Kiszka <jan.kiszka@siemens.com> writes:

...
>  	/* cpu-based controls */
>  	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>  		nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high);
> @@ -2409,11 +2422,17 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>  					nested_vmx_procbased_ctls_high);
>  		break;
>  	case MSR_IA32_VMX_TRUE_EXIT_CTLS:
> +		*pdata = vmx_control_msr(nested_vmx_true_exit_ctls_low,
> +					nested_vmx_exit_ctls_high);
> +		break;
>  	case MSR_IA32_VMX_EXIT_CTLS:
>  		*pdata = vmx_control_msr(nested_vmx_exit_ctls_low,
>  					nested_vmx_exit_ctls_high);
>  		break;
>  	case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> +		*pdata = vmx_control_msr(nested_vmx_true_entry_ctls_low,
> +					nested_vmx_entry_ctls_high);
> +		break;
>  	case MSR_IA32_VMX_ENTRY_CTLS:
>  		*pdata = vmx_control_msr(nested_vmx_entry_ctls_low,
>  					nested_vmx_entry_ctls_high);
> @@ -7836,7 +7855,13 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
>  	vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
>  
> -	vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
> +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) {
> +		kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
> +		vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
> +	} else {
> +		kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
> +		vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.vmcs01_debugctl);
> +	}

(I guess I don't understand DEBUGCTLS enough) vmcs01_debugctl is used by
L0 to run L1, and if L1 hasn't set VM_ENTRY_LOAD_DEBUG_CONTROLS for L2,
why do we need the GUEST_IA32_DEBUGCTL vmwrite in the "else" case ?

 
>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
>  		vmcs12->vm_entry_intr_info_field);
>  	vmcs_write32(VM_ENTRY_EXCEPTION_ERROR_CODE,
> @@ -7846,7 +7871,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
>  		vmcs12->guest_interruptibility_info);
>  	vmcs_write32(GUEST_SYSENTER_CS, vmcs12->guest_sysenter_cs);
> -	kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
>  	vmx_set_rflags(vcpu, vmcs12->guest_rflags);
>  	vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS,
>  		vmcs12->guest_pending_dbg_exceptions);
> @@ -8143,9 +8167,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>  	    !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
>  	      nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high) ||
>  	    !vmx_control_verify(vmcs12->vm_exit_controls,
> -	      nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high) ||
> +				nested_vmx_true_exit_ctls_low,
> +				nested_vmx_exit_ctls_high) ||
>  	    !vmx_control_verify(vmcs12->vm_entry_controls,
> -	      nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high))
> +				nested_vmx_true_entry_ctls_low,
> +				nested_vmx_entry_ctls_high))
>  	{
>  		nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD);
>  		return 1;
> @@ -8222,6 +8248,9 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>  
>  	vmx->nested.vmcs01_tsc_offset = vmcs_read64(TSC_OFFSET);
>  
> +	if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS))
> +		vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
> +
>  	cpu = get_cpu();
>  	vmx->loaded_vmcs = vmcs02;
>  	vmx_vcpu_put(vcpu);
> @@ -8399,7 +8428,6 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  	vmcs12->guest_cr0 = vmcs12_guest_cr0(vcpu, vmcs12);
>  	vmcs12->guest_cr4 = vmcs12_guest_cr4(vcpu, vmcs12);
>  
> -	kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
>  	vmcs12->guest_rsp = kvm_register_read(vcpu, VCPU_REGS_RSP);
>  	vmcs12->guest_rip = kvm_register_read(vcpu, VCPU_REGS_RIP);
>  	vmcs12->guest_rflags = vmcs_readl(GUEST_RFLAGS);
> @@ -8478,9 +8506,13 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
>  		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
>  
> +	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) {
> +		kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
> +		vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
> +	}
> +
>  	/* TODO: These cannot have changed unless we have MSR bitmaps and
>  	 * the relevant bit asks not to trap the change */
> -	vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
>  	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
>  		vmcs12->guest_ia32_pat = vmcs_read64(GUEST_IA32_PAT);
>  	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)

Re: [PATCH v2 4/5] KVM: nVMX: Allow to disable VM_{ENTRY_LOAD,EXIT_SAVE}_DEBUG_CONTROLS

[Paolo Bonzini]

Il 16/06/2014 19:02, Bandan Das ha scritto:
> > -	vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
> > +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) {
> > +		kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
> > +		vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
> > +	} else {
> > +		kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
> > +		vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.vmcs01_debugctl);
> > +	}
>
> (I guess I don't understand DEBUGCTLS enough) vmcs01_debugctl is used by
> L0 to run L1, and if L1 hasn't set VM_ENTRY_LOAD_DEBUG_CONTROLS for L2,
> why do we need the GUEST_IA32_DEBUGCTL vmwrite in the "else" case ?

Beause we always set VM_ENTRY_LOAD_DEBUG_CONTROLS in the VMCS02, so we 
must always fill in GUEST_IA32_DEBUGCTL of the VMCS02.  Depending on the 
VMCS12's VM_ENTRY_LOAD_DEBUG_CONTROLS, the field comes from either 
VMCS01 or VMCS12.

Paolo

Re: [PATCH v2 1/5] KVM: nVMX: Advertise support for MSR_IA32_VMX_TRUE_*_CTLS

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3634 bytes --]

On 2014-06-16 18:54, Bandan Das wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> We already implemented them but failed to advertise them. Currently they
>> all return the identical values to the capability MSRs they are
>> augmenting. So there is no change in exposed features yet.
>>
>> Drop related comments at this chance that are partially incorrect and
>> redundant anyway.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/include/uapi/asm/msr-index.h |  1 +
>>  arch/x86/kvm/vmx.c                    | 13 ++-----------
>>  2 files changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
>> index fcf2b3a..eaefcc6 100644
>> --- a/arch/x86/include/uapi/asm/msr-index.h
>> +++ b/arch/x86/include/uapi/asm/msr-index.h
>> @@ -558,6 +558,7 @@
>>  
>>  /* VMX_BASIC bits and bitmasks */
>>  #define VMX_BASIC_VMCS_SIZE_SHIFT	32
>> +#define VMX_BASIC_TRUE_CTLS		(1ULL << 55)
> 
> Curiosity question - know of any case that failed since we didn't
> set this bit ?

None. But that is likely due to the fact that we exposed a broken
PROCBASED_CTLS value.

Jan

> 
>>  #define VMX_BASIC_64		0x0001000000000000LLU
>>  #define VMX_BASIC_MEM_TYPE_SHIFT	50
>>  #define VMX_BASIC_MEM_TYPE_MASK	0x003c000000000000LLU
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 801332e..536f341 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2265,21 +2265,13 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>>  	/* pin-based controls */
>>  	rdmsr(MSR_IA32_VMX_PINBASED_CTLS,
>>  	      nested_vmx_pinbased_ctls_low, nested_vmx_pinbased_ctls_high);
>> -	/*
>> -	 * According to the Intel spec, if bit 55 of VMX_BASIC is off (as it is
>> -	 * in our case), bits 1, 2 and 4 (i.e., 0x16) must be 1 in this MSR.
>> -	 */
>>  	nested_vmx_pinbased_ctls_low |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR;
>>  	nested_vmx_pinbased_ctls_high &= PIN_BASED_EXT_INTR_MASK |
>>  		PIN_BASED_NMI_EXITING | PIN_BASED_VIRTUAL_NMIS;
>>  	nested_vmx_pinbased_ctls_high |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR |
>>  		PIN_BASED_VMX_PREEMPTION_TIMER;
>>  
>> -	/*
>> -	 * Exit controls
>> -	 * If bit 55 of VMX_BASIC is off, bits 0-8 and 10, 11, 13, 14, 16 and
>> -	 * 17 must be 1.
>> -	 */
>> +	/* exit controls */
>>  	rdmsr(MSR_IA32_VMX_EXIT_CTLS,
>>  		nested_vmx_exit_ctls_low, nested_vmx_exit_ctls_high);
>>  	nested_vmx_exit_ctls_low = VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR;
>> @@ -2299,7 +2291,6 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>>  	/* entry controls */
>>  	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>>  		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
>> -	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
>>  	nested_vmx_entry_ctls_low = VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR;
>>  	nested_vmx_entry_ctls_high &=
>>  #ifdef CONFIG_X86_64
>> @@ -2394,7 +2385,7 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>>  		 * guest, and the VMCS structure we give it - not about the
>>  		 * VMX support of the underlying hardware.
>>  		 */
>> -		*pdata = VMCS12_REVISION |
>> +		*pdata = VMCS12_REVISION | VMX_BASIC_TRUE_CTLS |
>>  			   ((u64)VMCS12_SIZE << VMX_BASIC_VMCS_SIZE_SHIFT) |
>>  			   (VMX_BASIC_MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
>>  		break;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 3/5] KVM: nVMX: Fix returned value of MSR_IA32_VMX_PROCBASED_CTLS

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 2014-06-16 18:56, Bandan Das wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> SDM says bits 1, 4-6, 8, 13-16, and 26 have to be set.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/include/asm/vmx.h | 3 +++
>>  arch/x86/kvm/vmx.c         | 5 +++--
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
>> index d989829..bcbfade 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -51,6 +51,9 @@
>>  #define CPU_BASED_MONITOR_EXITING               0x20000000
>>  #define CPU_BASED_PAUSE_EXITING                 0x40000000
>>  #define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS   0x80000000
>> +
>> +#define CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR	0x0401e172
>> +
> 
> Nit: Do we want to keep this consistent ? The previous defines have spaces,
> and you have introduced a tab

Old code used spaces, recent additions tabs - well...

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]