On 12/10/2009 08:38 PM, [email protected] wrote:
From: Orit Wasserman<[email protected]>


Missing changelog entry.  Please use the format common to all kvm patches.


diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3de0b37..3f63cdd 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -121,9 +121,6 @@ static int npt = 1;

  module_param(npt, int, S_IRUGO);

-static int nested = 1;
-module_param(nested, int, S_IRUGO);
-

Separate moving 'nested' into a different patch.

+struct __attribute__ ((__packed__)) level_state {
+};
+
+struct nested_vmx {
+       /* Has the level1 guest done vmxon? */
+       bool vmxon;
+       /* Level 1 state for switching to level 2 and back */
+       struct level_state *l1_state;

If this doesn't grow too large, can keep it as a member instead of a pointer.

+};
+

  static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
@@ -201,6 +214,7 @@ static struct kvm_vmx_segment_field {
  static u64 host_efer;

  static void ept_save_pdptrs(struct kvm_vcpu *vcpu);
+static int create_l1_state(struct kvm_vcpu *vcpu);

  /*
   * Keep MSR_K6_STAR at the end, as setup_msrs() will try to optimize it
@@ -961,6 +975,95 @@ static void guest_write_tsc(u64 guest_tsc, u64 host_tsc)
  }

  /*
+ * Handles msr read for nested virtualization
+ */
+static int nested_vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index,
+                             u64 *pdata)
+{
+       u64 vmx_msr = 0;
+
+       switch (msr_index) {
+       case MSR_IA32_FEATURE_CONTROL:
+               *pdata = 0;
+               break;
+       case MSR_IA32_VMX_BASIC:
+               *pdata = 0;

Not needed.

+               rdmsrl(MSR_IA32_VMX_BASIC, vmx_msr);
+               *pdata = (vmx_msr&  0x00ffffcfffffffff);

Please use symbolic constants.

+               break;
+       case MSR_IA32_VMX_PINBASED_CTLS:
+               rdmsrl(MSR_IA32_VMX_PINBASED_CTLS, vmx_msr);
+               *pdata = (PIN_BASED_EXT_INTR_MASK&  
vmcs_config.pin_based_exec_ctrl) |
+                       (PIN_BASED_NMI_EXITING&  
vmcs_config.pin_based_exec_ctrl) |
+                       (PIN_BASED_VIRTUAL_NMIS&  
vmcs_config.pin_based_exec_ctrl);

Don't understand.  You read vmx_msr and then use vmcs_config?

+       case MSR_IA32_VMX_PROCBASED_CTLS:
+       {
+               u32 vmx_msr_high, vmx_msr_low;
+               u32 control = CPU_BASED_HLT_EXITING |
+#ifdef CONFIG_X86_64
+                       CPU_BASED_CR8_LOAD_EXITING |
+                       CPU_BASED_CR8_STORE_EXITING |
+#endif
+                       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_INVLPG_EXITING |
+                       CPU_BASED_TPR_SHADOW |
+                       CPU_BASED_USE_MSR_BITMAPS |
+                       CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
+
+               rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
+
+               control&= vmx_msr_high; /* bit == 0 in high word ==>  must be 
zero */
+               control |= vmx_msr_low;  /* bit == 1 in low word  ==>  must be 
one  */
+
+               *pdata = (CPU_BASED_HLT_EXITING&  control) |
+#ifdef CONFIG_X86_64
+                       (CPU_BASED_CR8_LOAD_EXITING&  control) |
+                       (CPU_BASED_CR8_STORE_EXITING&  control) |
+#endif
+                       (CPU_BASED_CR3_LOAD_EXITING&  control) |
+                       (CPU_BASED_CR3_STORE_EXITING&  control) |
+                       (CPU_BASED_USE_IO_BITMAPS&  control) |
+                       (CPU_BASED_MOV_DR_EXITING&  control) |
+                       (CPU_BASED_USE_TSC_OFFSETING&  control) |
+                       (CPU_BASED_INVLPG_EXITING&  control) ;

What about the high word of the msr?  Will it always allow 0?


  /*
+ * Writes msr value for nested virtualization
+ * Returns 0 on success, non-0 otherwise.
+ */
+static int nested_vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
+{
+       switch (msr_index) {
+       case MSR_IA32_FEATURE_CONTROL:
+               if ((data&  (FEATURE_CONTROL_LOCKED |
+                            FEATURE_CONTROL_VMXON_ENABLED))
+                   != (FEATURE_CONTROL_LOCKED |
+                       FEATURE_CONTROL_VMXON_ENABLED))
+                       return 1;
+               break;

Need to trap if unsupported bits are set.

Need a way for userspace to write these msrs, so that live migration to an older kvm can work. We do the same thing with cpuid - userspace sets cpuid to values that are common across the migration cluster.

+static void free_l1_state(struct kvm_vcpu *vcpu)
+{
+       struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+       if (!vmx->nested.l1_state)
+               return;

Check isn't needed, kfree() likes NULLs.

+
+       kfree(vmx->nested.l1_state);
+       vmx->nested.l1_state = NULL;
+}
+
+



  struct kvm_shared_msrs_global {
@@ -505,7 +509,7 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
                return;
        }

-       if (cr4&  X86_CR4_VMXE) {
+       if (cr4&  X86_CR4_VMXE&&  !nested) {
                printk(KERN_DEBUG "set_cr4: #GP, setting VMXE\n");
                kvm_inject_gp(vcpu, 0);
                return;

Some bits are required to be set when VMXE is enabled.

Please split the MSR changes into a separate patch. Even cr4 is better on its own.

--
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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to