| Message ID | 1353559919-29439-5-git-send-email-dongxiao.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 22, 2012 at 12:51:59PM +0800, Dongxiao Xu wrote: > The launch state is not a member in the VMCS area, use a separate > variable (list) to store it instead. > > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com> 1. What is the problem with keeping launched state in the VMCS? Assuming there is a positive answer to the above: 2. Don't you have to change VMCS ID? 3. Can't it be kept somewhere else other than a list? Current scheme allows guest to allocate unlimited amounts of host memory. 4. What is the state of migration / nested vmx again? If vmcs12 is migrated, this means launched state is not migrated anymore. Patches 1-3 seem fine. -- 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
On Tue, Nov 27, 2012 at 10:29:08PM -0200, Marcelo Tosatti wrote: > On Thu, Nov 22, 2012 at 12:51:59PM +0800, Dongxiao Xu wrote: > > The launch state is not a member in the VMCS area, use a separate > > variable (list) to store it instead. > > > > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com> > > 1. What is the problem with keeping launched state in the VMCS? > Assuming there is a positive answer to the above: > > 2. Don't you have to change VMCS ID? > > 3. Can't it be kept somewhere else other than a list? Current scheme > allows guest to allocate unlimited amounts of host memory. > > 4. What is the state of migration / nested vmx again? If vmcs12 is > migrated, this means launched state is not migrated anymore. > > Patches 1-3 seem fine. According to Dongxiao they are slowing down nested guest by 4%. -- Gleb. -- 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
On 11/28/2012 02:29 AM, Marcelo Tosatti wrote: > On Thu, Nov 22, 2012 at 12:51:59PM +0800, Dongxiao Xu wrote: >> The launch state is not a member in the VMCS area, use a separate >> variable (list) to store it instead. >> >> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com> > > 1. What is the problem with keeping launched state in the VMCS? > Assuming there is a positive answer to the above: > > 2. Don't you have to change VMCS ID? > > 3. Can't it be kept somewhere else other than a list? Current scheme > allows guest to allocate unlimited amounts of host memory. I agree with Marcelo you have to limit the number of VMCS in the list otherwise it will be easy to attack a host with nested :) > > 4. What is the state of migration / nested vmx again? If vmcs12 is > migrated, this means launched state is not migrated anymore. > > Patches 1-3 seem fine. > > -- > 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 > -- 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
> -----Original Message----- > From: Gleb Natapov [mailto:gleb@redhat.com] > Sent: Wednesday, November 28, 2012 7:28 PM > To: Marcelo Tosatti > Cc: Xu, Dongxiao; kvm@vger.kernel.org > Subject: Re: [PATCH v2 4/4] nested vmx: use a list to store the launched vmcs12 > for L1 VMM > > On Tue, Nov 27, 2012 at 10:29:08PM -0200, Marcelo Tosatti wrote: > > On Thu, Nov 22, 2012 at 12:51:59PM +0800, Dongxiao Xu wrote: > > > The launch state is not a member in the VMCS area, use a separate > > > variable (list) to store it instead. > > > > > > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com> > > > > 1. What is the problem with keeping launched state in the VMCS? > > Assuming there is a positive answer to the above: > > > > 2. Don't you have to change VMCS ID? > > > > 3. Can't it be kept somewhere else other than a list? Current scheme > > allows guest to allocate unlimited amounts of host memory. > > > > 4. What is the state of migration / nested vmx again? If vmcs12 is > > migrated, this means launched state is not migrated anymore. > > > > Patches 1-3 seem fine. > According to Dongxiao they are slowing down nested guest by 4%. For this version, it will introduce certain performance downgrade. Actually in my new patch, I simplified the vmcs12_read() and vmcs12_write() functions and there is no obvious performance downgrade. Thanks, Dongxiao > > -- > Gleb. -- 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
> -----Original Message----- > From: Orit Wasserman [mailto:owasserm@redhat.com] > Sent: Wednesday, November 28, 2012 8:30 PM > To: Marcelo Tosatti > Cc: Xu, Dongxiao; kvm@vger.kernel.org; gleb@redhat.com > Subject: Re: [PATCH v2 4/4] nested vmx: use a list to store the launched vmcs12 > for L1 VMM > > On 11/28/2012 02:29 AM, Marcelo Tosatti wrote: > > On Thu, Nov 22, 2012 at 12:51:59PM +0800, Dongxiao Xu wrote: > >> The launch state is not a member in the VMCS area, use a separate > >> variable (list) to store it instead. > >> > >> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com> > > > > 1. What is the problem with keeping launched state in the VMCS? > > Assuming there is a positive answer to the above: > > > > 2. Don't you have to change VMCS ID? > > > > 3. Can't it be kept somewhere else other than a list? Current scheme > > allows guest to allocate unlimited amounts of host memory. > I agree with Marcelo you have to limit the number of VMCS in the list otherwise > it will be easy to attack a host with nested :) Yes it is a point. I will add a limitation of the VMCS number for the guest VMM. Thanks, Dongxiao > > > > 4. What is the state of migration / nested vmx again? If vmcs12 is > > migrated, this means launched state is not migrated anymore. > > > > Patches 1-3 seem fine. > > > > -- > > 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 > > -- 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
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 20de88b..3be9265 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -177,8 +177,7 @@ struct __packed vmcs12 { u32 revision_id; u32 abort; - u32 launch_state; /* set to 0 by VMCLEAR, to 1 by VMLAUNCH */ - u32 padding[7]; /* room for future expansion */ + u32 padding[8]; /* room for future expansion */ u64 io_bitmap_a; u64 io_bitmap_b; @@ -339,6 +338,11 @@ struct vmcs02_list { struct loaded_vmcs vmcs02; }; +struct vmcs12_list { + unsigned long vmcs12_pa; + struct list_head node; +}; + /* * The nested_vmx structure is part of vcpu_vmx, and holds information we need * for correct emulation of VMX (i.e., nested VMX) on this vcpu. @@ -364,6 +368,8 @@ struct nested_vmx { * we must keep them pinned while L2 runs. */ struct page *apic_access_page; + /* vmcs12_pool contains the launched vmcs12. */ + struct list_head vmcs12_pool; }; struct vcpu_vmx { @@ -619,6 +625,58 @@ static void nested_release_page_clean(struct page *page) kvm_release_page_clean(page); } +static int vmcs12_launched(struct list_head *vmcs12_pool, + unsigned long vmcs12_pa) +{ + struct vmcs12_list *iter; + struct list_head *pos; + int launched = 0; + + list_for_each(pos, vmcs12_pool) { + iter = list_entry(pos, struct vmcs12_list, node); + if (vmcs12_pa == iter->vmcs12_pa) { + launched = 1; + break; + } + } + + return launched; +} + +static int set_vmcs12_launched(struct list_head *vmcs12_pool, + unsigned long vmcs12_pa) +{ + struct vmcs12_list *vmcs12; + + if (vmcs12_launched(vmcs12_pool, vmcs12_pa)) + return 0; + + vmcs12 = kzalloc(sizeof(struct vmcs12_list), GFP_KERNEL); + if (!vmcs12) + return -ENOMEM; + + vmcs12->vmcs12_pa = vmcs12_pa; + list_add(&vmcs12->node, vmcs12_pool); + + return 0; +} + +static void clear_vmcs12_launched(struct list_head *vmcs12_pool, + unsigned long vmcs12_pa) +{ + struct vmcs12_list *iter; + struct list_head *pos; + + list_for_each(pos, vmcs12_pool) { + iter = list_entry(pos, struct vmcs12_list, node); + if (vmcs12_pa == iter->vmcs12_pa) { + list_del(&iter->node); + kfree(iter); + break; + } + } +} + static u64 construct_eptp(unsigned long root_hpa); static void kvm_cpu_vmxon(u64 addr); static void kvm_cpu_vmxoff(void); @@ -5116,6 +5174,18 @@ static void nested_free_all_saved_vmcss(struct vcpu_vmx *vmx) } /* + * Free the vmcs12 list. + */ +static void nested_free_vmcs12_list(struct vcpu_vmx *vmx) +{ + struct vmcs12_list *item, *n; + list_for_each_entry_safe(item, n, &vmx->nested.vmcs12_pool, node) { + list_del(&item->node); + kfree(item); + } +} + +/* * Emulate the VMXON instruction. * Currently, we just remember that VMX is active, and do not save or even * inspect the argument to VMXON (the so-called "VMXON pointer") because we @@ -5212,6 +5282,7 @@ static void free_nested(struct vcpu_vmx *vmx) } nested_free_all_saved_vmcss(vmx); + nested_free_vmcs12_list(vmx); } /* Emulate the VMXOFF instruction */ @@ -5364,7 +5435,7 @@ static int handle_vmclear(struct kvm_vcpu *vcpu) return 1; } vmcs12 = kmap(page); - vmcs12->launch_state = 0; + clear_vmcs12_launched(&vmx->nested.vmcs12_pool, __pa(vmcs12)); kunmap(page); nested_release_page(page); @@ -6460,6 +6531,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) vmx->nested.current_vmptr = -1ull; vmx->nested.current_vmcs12 = NULL; + INIT_LIST_HEAD(&vmx->nested.vmcs12_pool); return &vmx->vcpu; @@ -6839,6 +6911,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) struct vcpu_vmx *vmx = to_vmx(vcpu); int cpu; struct loaded_vmcs *vmcs02; + int is_launched; if (!nested_vmx_check_permission(vcpu) || !nested_vmx_check_vmcs12(vcpu)) @@ -6857,7 +6930,9 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) * for misconfigurations which will anyway be caught by the processor * when using the merged vmcs02. */ - if (vmcs12->launch_state == launch) { + is_launched = + vmcs12_launched(&vmx->nested.vmcs12_pool, __pa(vmcs12)); + if (is_launched == launch) { nested_vmx_failValid(vcpu, launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS : VMXERR_VMRESUME_NONLAUNCHED_VMCS); @@ -6946,7 +7021,8 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) vcpu->cpu = cpu; put_cpu(); - vmcs12->launch_state = 1; + if (set_vmcs12_launched(&vmx->nested.vmcs12_pool, __pa(vmcs12)) < 0) + return -ENOMEM; prepare_vmcs02(vcpu);
The launch state is not a member in the VMCS area, use a separate variable (list) to store it instead. Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com> --- arch/x86/kvm/vmx.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 81 insertions(+), 5 deletions(-)