Christoph Egger
2010-Aug-05 15:04 UTC
[Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Aug-09 12:57 UTC
Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
> @@ -692,8 +732,10 @@ static void svm_ctxt_switch_to(struct vc > static 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 *v > hvm_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; > - > - 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? [...]> + } 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. */ > + 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. [...]> +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; > + > + /* 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?> + tmp_vmcb = xmalloc(struct vmcb_struct); > + if (tmp_vmcb == NULL) > + return; > +[...]> @@ -1623,11 +2895,25 @@ asmlinkage void svm_vmexit_handler(struc > > case 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.> case 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;-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Aug-17 15:49 UTC
Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
On Monday 09 August 2010 14:57:31 Tim Deegan wrote:> > @@ -692,8 +732,10 @@ static void svm_ctxt_switch_to(struct vc > > static 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 *v > > hvm_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; > > - > > - 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.> [...] > > > + } 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. > > */ + 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.Done in local tree. We need to revisit this when Intel comes with their shadow-on-shadow implementation.> [...] > > > +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; > > + > > + /* 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.> > + tmp_vmcb = xmalloc(struct vmcb_struct); > > + if (tmp_vmcb == NULL) > > + return; > > + > > [...] > > > @@ -1623,11 +2895,25 @@ asmlinkage void svm_vmexit_handler(struc > > > > case 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> Cheers, > > Tim. > > > case 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;-- ---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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Aug-17 16:57 UTC
Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
At 16:49 +0100 on 17 Aug (1282063795), Christoph Egger wrote:> > 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.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. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Aug-18 07:48 UTC
Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
On Tuesday 17 August 2010 18:57:55 Tim Deegan wrote:> At 16:49 +0100 on 17 Aug (1282063795), Christoph Egger wrote: > > > 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. > > 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 -- ---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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Aug-18 10:45 UTC
Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
At 08:48 +0100 on 18 Aug (1282121312), Christoph Egger wrote:> On Tuesday 17 August 2010 18:57:55 Tim Deegan wrote: > > At 16:49 +0100 on 17 Aug (1282063795), Christoph Egger wrote: > > > > 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. > > > > 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. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Aug-18 10:54 UTC
Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
On Wednesday 18 August 2010 12:45:11 Tim Deegan wrote:> At 08:48 +0100 on 18 Aug (1282121312), Christoph Egger wrote: > > On Tuesday 17 August 2010 18:57:55 Tim Deegan wrote: > > > At 16:49 +0100 on 17 Aug (1282063795), Christoph Egger wrote: > > > > > 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. > > > > > > 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. :)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 -- ---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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel