Re: [PATCH 05/29] nVMX: Implement reading and writing of VMX MSRs
On Sun, Jan 30, 2011, Avi Kivity wrote about Re: [PATCH 05/29] nVMX: Implement
reading and writing of VMX MSRs:
+case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
+case MSR_IA32_VMX_PINBASED_CTLS:
+vmx_msr_low = CORE2_PINBASED_CTLS_MUST_BE_ONE;
+vmx_msr_high = CORE2_PINBASED_CTLS_MUST_BE_ONE |
+PIN_BASED_EXT_INTR_MASK |
+PIN_BASED_NMI_EXITING |
+PIN_BASED_VIRTUAL_NMIS;
Can we actually support PIN_BASED_VIRTUAL_NMIs on hosts which don't
support them?
Maybe better to drop for the initial version.
Thanks, I'll look into this. You already found this problem in June, and
it's already in my bugzilla. Just wanted to let you know that I'm taking
all your previous comments seriously, and not forgetting any of them.
Since you mention this one again, I'm increasing its priority, so I'll fix
it before the next version of the patches.
+static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
+{
+if (!nested_vmx_allowed(vcpu))
+return 0;
+
+/*
+ * according to the spec, VMX capability MSRs are read-only; an
+ * attempt to write them (with WRMSR) produces a #GP(0).
+ */
+if (msr_index= MSR_IA32_VMX_BASIC
+msr_index= MSR_IA32_VMX_TRUE_ENTRY_CTLS) {
+kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
+return 1;
Can just drop this part, #GP is the default response.
Right, thanks, I see that now. I'll remove the extra code, but leave a
comment.
--
Nadav Har'El| Monday, Jan 31 2011, 26 Shevat 5771
n...@math.technion.ac.il |-
Phone +972-523-790466, ICQ 13349191 |If the universe is expanding, why can't I
http://nadav.harel.org.il |find a parking space?
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] nVMX: Implement reading and writing of VMX MSRs
On 01/31/2011 10:57 AM, Nadav Har'El wrote: On Sun, Jan 30, 2011, Avi Kivity wrote about Re: [PATCH 05/29] nVMX: Implement reading and writing of VMX MSRs: + case MSR_IA32_VMX_TRUE_PINBASED_CTLS: + case MSR_IA32_VMX_PINBASED_CTLS: + vmx_msr_low = CORE2_PINBASED_CTLS_MUST_BE_ONE; + vmx_msr_high = CORE2_PINBASED_CTLS_MUST_BE_ONE | + PIN_BASED_EXT_INTR_MASK | + PIN_BASED_NMI_EXITING | + PIN_BASED_VIRTUAL_NMIS; Can we actually support PIN_BASED_VIRTUAL_NMIs on hosts which don't support them? Maybe better to drop for the initial version. Thanks, I'll look into this. You already found this problem in June, and it's already in my bugzilla. Just wanted to let you know that I'm taking all your previous comments seriously, and not forgetting any of them. Since you mention this one again, I'm increasing its priority, so I'll fix it before the next version of the patches. Thanks. Since I see many other comments are still unaddressed, I'll stop reviewing until you let me know that you have a version that is ready for review. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] nVMX: Implement reading and writing of VMX MSRs
On 01/27/2011 10:32 AM, Nadav Har'El wrote:
When the guest can use VMX instructions (when the nested module option is
on), it should also be able to read and write VMX MSRs, e.g., to query about
VMX capabilities. This patch adds this support.
+#define CORE2_PINBASED_CTLS_MUST_BE_ONE0x0016
+ case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
+ case MSR_IA32_VMX_PINBASED_CTLS:
+ vmx_msr_low = CORE2_PINBASED_CTLS_MUST_BE_ONE;
+ vmx_msr_high = CORE2_PINBASED_CTLS_MUST_BE_ONE |
+ PIN_BASED_EXT_INTR_MASK |
+ PIN_BASED_NMI_EXITING |
+ PIN_BASED_VIRTUAL_NMIS;
Can we actually support PIN_BASED_VIRTUAL_NMIs on hosts which don't
support them?
Maybe better to drop for the initial version.
+ rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
+
+ vmx_msr_low = 0; /* allow disabling any feature */
+ vmx_msr_high= /* do not expose new untested features */
+ CPU_BASED_HLT_EXITING | CPU_BASED_CR3_LOAD_EXITING |
+ CPU_BASED_CR3_STORE_EXITING | CPU_BASED_USE_IO_BITMAPS |
+ CPU_BASED_MOV_DR_EXITING | CPU_BASED_USE_TSC_OFFSETING |
+ CPU_BASED_MWAIT_EXITING | CPU_BASED_MONITOR_EXITING |
+ CPU_BASED_INVLPG_EXITING | CPU_BASED_TPR_SHADOW |
+#ifdef CONFIG_X86_64
+ CPU_BASED_CR8_LOAD_EXITING |
+ CPU_BASED_CR8_STORE_EXITING |
+#endif
+ CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
Similarly, I think CPU_BASED_TPR_SHADOW and
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS are not available on all models.
+ case MSR_IA32_VMX_PROCBASED_CTLS2:
+ *pdata = 0;
+ if (vm_need_virtualize_apic_accesses(vcpu-kvm))
+ *pdata |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
+ break;
Here, at least you qualify support by checking the host.
+ case MSR_IA32_VMX_EPT_VPID_CAP:
+ /* Currently, no nested ept or nested vpid */
+ *pdata = 0;
+ break;
+ default:
+ return 0;
+ }
+
+ return 1;
+}
+
+static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
+{
+ if (!nested_vmx_allowed(vcpu))
+ return 0;
+
+ /*
+* according to the spec, VMX capability MSRs are read-only; an
+* attempt to write them (with WRMSR) produces a #GP(0).
+*/
+ if (msr_index= MSR_IA32_VMX_BASIC
+ msr_index= MSR_IA32_VMX_TRUE_ENTRY_CTLS) {
+ kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
+ return 1;
Can just drop this part, #GP is the default response.
+ } else if (msr_index == MSR_IA32_FEATURE_CONTROL)
+ /* TODO: the right thing. */
+ return 1;
+ else
+ return 0;
+}
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/29] nVMX: Implement reading and writing of VMX MSRs
When the guest can use VMX instructions (when the nested module option is
on), it should also be able to read and write VMX MSRs, e.g., to query about
VMX capabilities. This patch adds this support.
Signed-off-by: Nadav Har'El n...@il.ibm.com
---
arch/x86/include/asm/msr-index.h |9 ++
arch/x86/kvm/vmx.c | 126 +
2 files changed, 135 insertions(+)
--- .before/arch/x86/kvm/vmx.c 2011-01-26 18:06:03.0 +0200
+++ .after/arch/x86/kvm/vmx.c 2011-01-26 18:06:03.0 +0200
@@ -1258,6 +1258,128 @@ static inline bool nested_vmx_allowed(st
}
/*
+ * If we allow our guest to use VMX instructions (i.e., nested VMX), we should
+ * also let it use VMX-specific MSRs.
+ * vmx_get_vmx_msr() and vmx_set_vmx_msr() return 1 when we handled a
+ * VMX-specific MSR, or 0 when we haven't (and the caller should handle it
+ * like all other MSRs).
+ */
+static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
+{
+ u64 vmx_msr = 0;
+ u32 vmx_msr_high, vmx_msr_low;
+
+ if (!nested_vmx_allowed(vcpu) msr_index = MSR_IA32_VMX_BASIC
+ msr_index = MSR_IA32_VMX_TRUE_ENTRY_CTLS) {
+ /*
+* According to the spec, processors which do not support VMX
+* should throw a #GP(0) when VMX capability MSRs are read.
+*/
+ kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
+ return 1;
+ }
+
+ switch (msr_index) {
+ case MSR_IA32_FEATURE_CONTROL:
+ *pdata = 0;
+ break;
+ case MSR_IA32_VMX_BASIC:
+ /*
+* This MSR reports some information about VMX support of the
+* processor. We should return information about the VMX we
+* emulate for the guest, and the VMCS structure we give it -
+* not about the VMX support of the underlying hardware.
+* However, some capabilities of the underlying hardware are
+* used directly by our emulation (e.g., the physical address
+* width), so these are copied from what the hardware reports.
+*/
+ *pdata = VMCS12_REVISION | (((u64)sizeof(struct vmcs12)) 32);
+ rdmsrl(MSR_IA32_VMX_BASIC, vmx_msr);
+ *pdata |= vmx_msr
+ (VMX_BASIC_64 | VMX_BASIC_MEM_TYPE | VMX_BASIC_INOUT);
+ break;
+#define CORE2_PINBASED_CTLS_MUST_BE_ONE0x0016
+ case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
+ case MSR_IA32_VMX_PINBASED_CTLS:
+ vmx_msr_low = CORE2_PINBASED_CTLS_MUST_BE_ONE;
+ vmx_msr_high = CORE2_PINBASED_CTLS_MUST_BE_ONE |
+ PIN_BASED_EXT_INTR_MASK |
+ PIN_BASED_NMI_EXITING |
+ PIN_BASED_VIRTUAL_NMIS;
+ *pdata = vmx_msr_low | ((u64)vmx_msr_high 32);
+ break;
+ case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
+ case MSR_IA32_VMX_PROCBASED_CTLS:
+ /* This MSR determines which vm-execution controls the L1
+* hypervisor may ask, or may not ask, to enable. Normally we
+* can only allow enabling features which the hardware can
+* support, but we limit ourselves to allowing only known
+* features that were tested nested. We allow disabling any
+* feature (even if the hardware can't disable it).
+*/
+ rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
+
+ vmx_msr_low = 0; /* allow disabling any feature */
+ vmx_msr_high = /* do not expose new untested features */
+ CPU_BASED_HLT_EXITING | CPU_BASED_CR3_LOAD_EXITING |
+ CPU_BASED_CR3_STORE_EXITING | CPU_BASED_USE_IO_BITMAPS |
+ CPU_BASED_MOV_DR_EXITING | CPU_BASED_USE_TSC_OFFSETING |
+ CPU_BASED_MWAIT_EXITING | CPU_BASED_MONITOR_EXITING |
+ CPU_BASED_INVLPG_EXITING | CPU_BASED_TPR_SHADOW |
+#ifdef CONFIG_X86_64
+ CPU_BASED_CR8_LOAD_EXITING |
+ CPU_BASED_CR8_STORE_EXITING |
+#endif
+ CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
+ *pdata = vmx_msr_low | ((u64)vmx_msr_high 32);
+ break;
+ case MSR_IA32_VMX_TRUE_EXIT_CTLS:
+ case MSR_IA32_VMX_EXIT_CTLS:
+ *pdata = 0;
+#ifdef CONFIG_X86_64
+ *pdata |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
+#endif
+ break;
+ case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+ case MSR_IA32_VMX_ENTRY_CTLS:
+ *pdata = 0;
+ break;
+ case MSR_IA32_VMX_PROCBASED_CTLS2:
+ *pdata = 0;
+ if (vm_need_virtualize_apic_accesses(vcpu-kvm))
+ *pdata |=
