Nested Virtualization: svm specific implementation
PATCH 10/14 Nested Virtualization: svm specific implementation
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
# HG changeset patch
# User cegger
# Date 1281004712 -7200
Implement SVM specific part for Nested Virtualization
xen/arch/x86/hvm/svm/Makefile +1 -0
@@ -3,5 +3,6 @@ obj-y += emulate.o obj-y += entry.o obj-y += intr.o obj-y += svm.o +obj-y += svmdebug.o obj-y += vmcb.o obj-y += vpmu.o
xen/arch/x86/hvm/svm/emulate.c +11 -1
@@ -100,6 +100,11 @@ MAKE_INSTR(VMCALL, 3, 0x0f, 0x01, 0xd9);MAKE_INSTR (HLT ,1 ,0xf4 );MAKE_INSTR (INT3 ,1 ,0xcc );MAKE_INSTR (RDTSC ,2 ,0x0f ,0x31 ); +MAKE_INSTR (VMRUN ,3 ,0x0f ,0x01 ,0xd8 ); +MAKE_INSTR (VMLOAD ,3 ,0x0f ,0x01 ,0xda ); +MAKE_INSTR (VMSAVE ,3 ,0x0f ,0x01 ,0xdb ); +MAKE_INSTR (STGI ,3 ,0x0f ,0x01 ,0xdc ); +MAKE_INSTR (CLGI ,3 ,0x0f ,0x01 ,0xdd );static const u8 *opc_bytes[INSTR_MAX_COUNT ] = {
@@ -111,7 +116,12 @@ static const u8 *opc_bytes[INSTR_MAX_COU [INSTR_VMCALL] = OPCODE_VMCALL, [INSTR_HLT] = OPCODE_HLT, [INSTR_INT3] = OPCODE_INT3, - [INSTR_RDTSC] = OPCODE_RDTSC + [INSTR_RDTSC] = OPCODE_RDTSC, + [INSTR_VMRUN] = OPCODE_VMRUN, + [INSTR_VMLOAD] = OPCODE_VMLOAD, + [INSTR_VMSAVE] = OPCODE_VMSAVE, + [INSTR_STGI] = OPCODE_STGI, + [INSTR_CLGI] = OPCODE_CLGI, };static int fetch (struct vcpu *v,u8 *buf,unsigned long addr,int len)
xen/arch/x86/hvm/svm/svm.c +1317 -26
1,503 lines not on this page · raw message
xen/arch/x86/hvm/svm/svmdebug.c new file +185 -0
185 lines not on this page · raw message
xen/arch/x86/hvm/svm/vmcb.c +2 -102
128 lines not on this page · raw message
xen/include/asm-x86/hvm/svm/emulate.h +5 -0
@@ -31,6 +31,11 @@ enum instruction_index {INSTR_HLT ,INSTR_INT3 ,INSTR_RDTSC , +INSTR_VMRUN , +INSTR_VMLOAD , +INSTR_VMSAVE , +INSTR_STGI , +INSTR_CLGI ,INSTR_MAX_COUNT /* Must be last - Number of instructions supported */ };
xen/include/asm-x86/hvm/svm/svm.h +0 -2
@@ -29,8 +29,6 @@#include <asm/i387.h> #include <asm/hvm/vpmu.h> -void svm_dump_vmcb (const char *from,struct vmcb_struct *vmcb); -#define SVM_REG_EAX (0)#define SVM_REG_ECX (1)#define SVM_REG_EDX (2)
xen/include/asm-x86/hvm/svm/svmdebug.h new file +30 -0
@@ -0,0 +1,30 @@ +/* +* svmdebug.h: SVM related debug defintions +* Copyright (c) 2010, AMD Corporation. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License along with +* this program; if not, write to the Free Software Foundation, Inc., 59 Temple +* Place - Suite 330, Boston, MA 02111-1307 USA. +* +*/ + +#ifndef __ASM_X86_HVM_SVM_SVMDEBUG_H__ +#define __ASM_X86_HVM_SVM_SVMDEBUG_H__ + +#include <asm/types.h> +#include <asm/hvm/svm/vmcb.h> + +void svm_vmcb_dump (const char *from,struct vmcb_struct *vmcb); +bool_t svm_vmcb_isvalid (const char *from,struct vmcb_struct *vmcb, +bool_t verbose); + +#endif /* __ASM_X86_HVM_SVM_SVMDEBUG_H__ */
xen/include/asm-x86/hvm/svm/vmcb.h +14 -0
@@ -474,6 +474,20 @@ struct arch_svm_struct {uint64_t guest_sysenter_eip; }; +struct nestedsvm { +uint64_t ns_msr_hsavepa;/* MSR HSAVE_PA value */ + +/* Cached real intercepts of the nested guest */ +uint32_t ns_cr_intercepts; +uint32_t ns_dr_intercepts; +uint32_t ns_exception_intercepts; +uint32_t ns_general1_intercepts; +uint32_t ns_general2_intercepts; + +/* Cached real lbr of the nested guest */ +lbrctrl_t ns_lbr_control; +}; +struct vmcb_struct *alloc_vmcb (void );struct host_save_area *alloc_host_save_area (void );void free_vmcb (struct vmcb_struct *vmcb);
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 20 lines
xen/arch/x86/hvm/svm/svm.c +4
-5@@ -692,8 +732,10 @@ static void svm_ctxt_switch_to(struct vcstatic void svm_do_resume (struct vcpu *v) {bool_t debug_state = v->domain->debugger_attached; - -if (unlikely (v->arch.hvm_vcpu.debug_state_latch != debug_state) ) +bool_t guestmode =nestedhvm_vcpu_in_guestmode (v); + +if ( !guestmode && +unlikely (v->arch.hvm_vcpu.debug_state_latch != debug_state) ) {uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3); v->arch.hvm_vcpu.debug_state_latch = debug_state;@@ -712,11 +754,14 @@ static void svm_do_resume(struct vcpu *vhvm_asid_flush_vcpu (v); } -/* Reflect the vlapic's TPR in the hardware vtpr */ - v->arch.hvm_svm.vmcb->vintr.fields.tpr = - (vlapic_get_reg (vcpu_vlapic (v),APIC_TASKPRI ) &0xFF ) >>4 ;
xen/arch/x86/hvm/svm/svm.c static void svm_do_resume(struct vcpu *v · line 764
Quoted 10 lines
xen/arch/x86/hvm/svm/svm.c +8
-2- -hvm_do_resume (v); +if ( !guestmode ) + { +/* Reflect the vlapic's TPR in the hardware vtpr */ + v->arch.hvm_svm.vmcb->vintr.fields.tpr = + (vlapic_get_reg (vcpu_vlapic (v),APIC_TASKPRI ) &0xFF ) >>4 ; + +hvm_do_resume (v); + }
Can you explain why we shouldn't sync the vTPR and the vlapic state when the guest is in nested mode?
[...]
Quoted 5 lines
xen/arch/x86/hvm/svm/svm.c +5
-0+ }else { +/* host shadow paging + guest shadow paging. */ + host_vmcb->np_enable =0 ; + host_vmcb->h_cr3 =0x0 ; +
xen/arch/x86/hvm/svm/svm.c static void svm_init_erratum_383(struct · line 1612
Quoted 10 lines
xen/arch/x86/hvm/svm/svm.c +10
-0+#if 0 + host_vmcb->cr3 = v->shadow_shadow_table; + +/* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */ + rc =hvm_set_cr3 (ns_vmcb->cr3); +if (rc !=X86EMUL_OKAY ) +gdprintk (XENLOG_ERR ,"hvm_set_cr3 failed, rc: %u\n" , rc); +#endif + } +
Please remove this.
[...]
Quoted 5 lines
xen/arch/x86/hvm/svm/svm.c +5
-0+static void +svm_vmexit_do_vmload (struct vmcb_struct *vmcb, +struct cpu_user_regs *regs, +struct vcpu *v,uint64_t vmcbaddr) +{
xen/arch/x86/hvm/svm/svm.c static void svm_vmexit_do_rdtsc(struct c · line 2337
Quoted 10 lines
xen/arch/x86/hvm/svm/svm.c +10
-0+int ret; +unsigned int inst_len; +struct vmcb_struct *tmp_vmcb; + +if ( (inst_len =__get_instruction_length (v,INSTR_VMLOAD )) ==0 ) +return ; + +/* tmp_vmcb can't be a local variable on the stack because +* the machine stops with a sudden freeze. +*/
A VMCB structure is large, and Xen stacks are small. :) xmalloc()ing it on every call seems a bit fragile though - and silently returning if the xmalloc() fails is wrong. Is this something that could be allocated once at VCPU creation time?
Quoted 4 lines
xen/arch/x86/hvm/svm/svm.c +4
-0+ tmp_vmcb = xmalloc(struct vmcb_struct); +if (tmp_vmcb ==NULL ) +return ; +
[...]
xen/arch/x86/hvm/svm/svm.c asmlinkage void svm_vmexit_handler(struc · line 2904
Quoted 11 lines
xen/arch/x86/hvm/svm/svm.c +6
-0@@ -1623,11 +2895,25 @@ asmlinkage void svm_vmexit_handler(struccase VMEXIT_MONITOR :case VMEXIT_MWAIT : +hvm_inject_exception (TRAP_invalid_op,HVM_DELIVER_NO_ERROR_CODE ,0 ); +break ; +case VMEXIT_VMRUN : +svm_vmexit_do_vmrun (regs, v, + regs->eax); +break ;
svm_vmexit_do_vmrun() and the other routines called below don't seem to check that nested HVM is enabled; that seems like it would be bad if a non-nesting guest tries to execute VMRUN &c. Or have you changed the intercept bits somehow so that never happens?
Cheers,
Tim.
Quoted 15 lines
xen/arch/x86/hvm/svm/svm.c +8
-0case VMEXIT_VMLOAD : +svm_vmexit_do_vmload (vmcb, regs, v, regs->eax); +break ;case VMEXIT_VMSAVE : +svm_vmexit_do_vmsave (vmcb, regs, v, regs->eax); +break ;case VMEXIT_STGI : +svm_vmexit_do_stgi (regs, v); +break ;case VMEXIT_CLGI : +svm_vmexit_do_clgi (regs, v); +break ;case VMEXIT_SKINIT :hvm_inject_exception (TRAP_invalid_op,HVM_DELIVER_NO_ERROR_CODE ,0 );break ;
Signature · 3 lines
Tim Deegan <[email protected]> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
Quoted 17 lines · On Monday 09 August 2010 14:57:31 Tim Deegan wrote:
Quoted 17 lines
xen/arch/x86/hvm/svm/svm.c +4
-2@@ -692,8 +732,10 @@ static void svm_ctxt_switch_to(struct vcstatic void svm_do_resume (struct vcpu *v) {bool_t debug_state = v->domain->debugger_attached; - -if (unlikely (v->arch.hvm_vcpu.debug_state_latch != debug_state) ) +bool_t guestmode =nestedhvm_vcpu_in_guestmode (v); + +if ( !guestmode && +unlikely (v->arch.hvm_vcpu.debug_state_latch != debug_state) ) {uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3); v->arch.hvm_vcpu.debug_state_latch = debug_state;@@ -712,11 +754,14 @@ static void svm_do_resume(struct vcpu *vhvm_asid_flush_vcpu (v); }
Quoted 16 lines
xen/arch/x86/hvm/svm/svm.c static void svm_do_resume(struct vcpu *v · line 764
Quoted 13 lines
xen/arch/x86/hvm/svm/svm.c +8
-5-/* Reflect the vlapic's TPR in the hardware vtpr */ - v->arch.hvm_svm.vmcb->vintr.fields.tpr = - (vlapic_get_reg (vcpu_vlapic (v),APIC_TASKPRI ) &0xFF ) >>4 ; - -hvm_do_resume (v); +if ( !guestmode ) + { +/* Reflect the vlapic's TPR in the hardware vtpr */ + v->arch.hvm_svm.vmcb->vintr.fields.tpr = + (vlapic_get_reg (vcpu_vlapic (v),APIC_TASKPRI ) &0xFF ) >>4 ; + +hvm_do_resume (v); + }Can you explain why we shouldn't sync the vTPR and the vlapic state when the guest is in nested mode?
When the vcpu is in guest mode then v->arch.hvm_svm.vmcb->vintr.fields.tpr represents the tpr of the l2 guest. The l2 guest is not allowed to touch the l1 guest's vTPR.
Quoted 11 lines
[...]
Quoted 9 lines
xen/arch/x86/hvm/svm/svm.c +9
-0+ }else { +/* host shadow paging + guest shadow paging. */ + host_vmcb->np_enable =0 ; + host_vmcb->h_cr3 =0x0 ; + +#if 0 + host_vmcb->cr3 = v->shadow_shadow_table; + +/* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us.
Quoted 8 lines
xen/arch/x86/hvm/svm/svm.c static void svm_init_erratum_383(struct · line 1612
Quoted 6 lines
*/ + rc =
hvm_set_cr3(ns_vmcb->cr3);xen/arch/x86/hvm/svm/svm.c +5
-0+if (rc !=X86EMUL_OKAY ) +gdprintk (XENLOG_ERR ,"hvm_set_cr3 failed, rc: %u\n" , rc); +#endif + } +Please remove this.
Done in local tree. We need to revisit this when Intel comes with their shadow-on-shadow implementation.
Quoted 14 lines
[...]
Quoted 12 lines
xen/arch/x86/hvm/svm/svm.c +12
-0+static void +svm_vmexit_do_vmload (struct vmcb_struct *vmcb, +struct cpu_user_regs *regs, +struct vcpu *v,uint64_t vmcbaddr) +{ +int ret; +unsigned int inst_len; +struct vmcb_struct *tmp_vmcb; + +if ( (inst_len =__get_instruction_length (v,INSTR_VMLOAD )) ==0 ) +return ; +
Quoted 8 lines
xen/arch/x86/hvm/svm/svm.c static void svm_vmexit_do_rdtsc(struct c · line 2337
Quoted 3 lines
xen/arch/x86/hvm/svm/svm.c +3
-0+/* tmp_vmcb can't be a local variable on the stack because +* the machine stops with a sudden freeze. +*/ A VMCB structure is large, and Xen stacks are small. :) xmalloc()ing it on every call seems a bit fragile though - and silently returning if the
xmalloc()fails is wrong. Is this something that could be allocated once at VCPU creation time?
Yes. Done in local tree.
Quoted 9 lines
Quoted 4 lines
xen/arch/x86/hvm/svm/svm.c +4
-0+ tmp_vmcb = xmalloc(struct vmcb_struct); +if (tmp_vmcb ==NULL ) +return ; +[...]
Quoted 2 lines
(quoted diff) +0
-0@@ -1623,11 +2895,25 @@ asmlinkage void svm_vmexit_handler(struc
Quoted 14 lines
xen/arch/x86/hvm/svm/svm.c asmlinkage void svm_vmexit_handler(struc · line 2904
Quoted 9 lines
xen/arch/x86/hvm/svm/svm.c +5
-0case VMEXIT_MONITOR :case VMEXIT_MWAIT : +hvm_inject_exception (TRAP_invalid_op,HVM_DELIVER_NO_ERROR_CODE ,0 ); + break; +case VMEXIT_VMRUN : +svm_vmexit_do_vmrun (regs, v, + regs->eax); +break ;
svm_vmexit_do_vmrun()and the other routines called below don't seem to check that nested HVM is enabled; that seems like it would be bad if a non-nesting guest tries to execute VMRUN &c. Or have you changed the intercept bits somehow so that never happens?
No, you're right. When nestedhvm is disabled then the instructions should result in a #UD. Fixed in local tree.
Christoph
Quoted 19 lines
Cheers,
Tim.
Quoted 15 lines
xen/arch/x86/hvm/svm/svm.c +8
-0case VMEXIT_VMLOAD : +svm_vmexit_do_vmload (vmcb, regs, v, regs->eax); +break ;case VMEXIT_VMSAVE : +svm_vmexit_do_vmsave (vmcb, regs, v, regs->eax); +break ;case VMEXIT_STGI : +svm_vmexit_do_stgi (regs, v); +break ;case VMEXIT_CLGI : +svm_vmexit_do_clgi (regs, v); +break ;case VMEXIT_SKINIT : hvm_inject_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE,0); break;
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Quoted 6 lines · Christoph Egger
Quoted 2 lines
Can you explain why we shouldn't sync the vTPR and the vlapic state when the guest is in nested mode?
When the vcpu is in guest mode then
v->arch.hvm_svm.vmcb->vintr.fields.tprrepresents the tpr of the l2 guest. The l2 guest is not allowed to touch the l1 guest's vTPR.
Hmmm. I'm not sure I understand exactly what the hardware vTPR should contain when the L2 is running; I'll have to think back about exactly what the sync of vtpr <-> vlapic means and whether the L2 guest can cause surprising things to happen by setting the vTPR.
Tim.
Signature · 3 lines
Tim Deegan <[email protected]> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
Quoted 4 lines · On Tuesday 17 August 2010 18:57:55 Tim Deegan wrote:
Quoted 3 lines · Christoph Egger
Quoted 2 lines
Can you explain why we shouldn't sync the vTPR and the vlapic state when the guest is in nested mode?
Quoted 8 lines
Quoted 3 lines
When the vcpu is in guest mode then
v->arch.hvm_svm.vmcb->vintr.fields.tprrepresents the tpr of the l2 guest. The l2 guest is not allowed to touch the l1 guest's vTPR.Hmmm. I'm not sure I understand exactly what the hardware vTPR should contain when the L2 is running; I'll have to think back about exactly what the sync of vtpr <-> vlapic means and whether the L2 guest can cause surprising things to happen by setting the vTPR.
The l2 guest can confuse the l1 guest. E.g. Booting Windows 7 as l1 guest and the XP mode as l2 guest won't work when done incorrectly. The l1 guest handles the vtpr and vlapic for the l2 guest and noone else.
Christoph
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Quoted 9 lines · Christoph Egger
Quoted 8 lines · On Tuesday 17 August 2010 18:57:55 Tim Deegan wrote:
Quoted 6 lines · Christoph Egger
Quoted 2 lines
Can you explain why we shouldn't sync the vTPR and the vlapic state when the guest is in nested mode?
When the vcpu is in guest mode then
v->arch.hvm_svm.vmcb->vintr.fields.tprrepresents the tpr of the l2 guest. The l2 guest is not allowed to touch the l1 guest's vTPR.
Quoted 8 lines
Quoted 4 lines
Hmmm. I'm not sure I understand exactly what the hardware vTPR should contain when the L2 is running; I'll have to think back about exactly what the sync of vtpr <-> vlapic means and whether the L2 guest can cause surprising things to happen by setting the vTPR.
The l2 guest can confuse the l1 guest. E.g. Booting Windows 7 as l1 guest and the XP mode as l2 guest won't work when done incorrectly.
What cases will cause it to break? I had just convinced myself that your patch was correct but now you've got me worried again. :)
Cheers,
Tim.
Signature · 3 lines
Tim Deegan <[email protected]> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
Quoted 15 lines · On Wednesday 18 August 2010 12:45:11 Tim Deegan wrote:
Quoted 14 lines · Christoph Egger
Quoted 12 lines · On Tuesday 17 August 2010 18:57:55 Tim Deegan wrote:
Quoted 6 lines · Christoph Egger
Quoted 2 lines
Can you explain why we shouldn't sync the vTPR and the vlapic state when the guest is in nested mode?
When the vcpu is in guest mode then
v->arch.hvm_svm.vmcb->vintr.fields.tprrepresents the tpr of the l2 guest. The l2 guest is not allowed to touch the l1 guest's vTPR.Hmmm. I'm not sure I understand exactly what the hardware vTPR should contain when the L2 is running; I'll have to think back about exactly what the sync of vtpr <-> vlapic means and whether the L2 guest can cause surprising things to happen by setting the vTPR.
Quoted 6 lines
Quoted 3 lines
The l2 guest can confuse the l1 guest. E.g. Booting Windows 7 as l1 guest and the XP mode as l2 guest won't work when done incorrectly.
What cases will cause it to break? I had just convinced myself that your patch was correct but now you've got me worried again. :)
It is correct. Don't worry. :)
I just tried to explain what exactly goes wrong when l2 guest fiddles with l1 guest's vTPR.
Christoph
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632