| Message ID | 20170904081452.12960-8-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/04/2017 09:14 AM, Sergey Dyasli wrote: > If an IPI flushes vCPU's np2m object just before nested vmentry, there > will be a stale shadow EPTP value in VMCS02. Allow vmentry to be > restarted in such cases and add nvmx_eptp_update() to perform an update. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > xen/arch/x86/hvm/vmx/entry.S | 6 ++++++ > xen/arch/x86/hvm/vmx/vmx.c | 8 +++++++- > xen/arch/x86/hvm/vmx/vvmx.c | 14 ++++++++++++++ > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S > index 53eedc6363..9fb8f89220 100644 > --- a/xen/arch/x86/hvm/vmx/entry.S > +++ b/xen/arch/x86/hvm/vmx/entry.S > @@ -79,6 +79,8 @@ UNLIKELY_END(realmode) > > mov %rsp,%rdi > call vmx_vmenter_helper > + cmp $0,%eax > + jne .Lvmx_vmentry_restart > mov VCPU_hvm_guest_cr2(%rbx),%rax > > pop %r15 > @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry) > GET_CURRENT(bx) > jmp .Lvmx_do_vmentry > > +.Lvmx_vmentry_restart: > + sti > + jmp .Lvmx_do_vmentry > + > .Lvmx_goto_emulator: > sti > mov %rsp,%rdi > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index f6da119c9f..06509590b7 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -4223,13 +4223,17 @@ static void lbr_fixup(void) > bdw_erratum_bdf14_fixup(); > } > > -void vmx_vmenter_helper(const struct cpu_user_regs *regs) > +int vmx_vmenter_helper(const struct cpu_user_regs *regs) > { > struct vcpu *curr = current; > u32 new_asid, old_asid; > struct hvm_vcpu_asid *p_asid; > bool_t need_flush; > > + /* Shadow EPTP can't be updated here because irqs are disabled */ > + if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m ) > + return 1; > + > if ( curr->domain->arch.hvm_domain.pi_ops.do_resume ) > curr->domain->arch.hvm_domain.pi_ops.do_resume(curr); > > @@ -4290,6 +4294,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs) > __vmwrite(GUEST_RIP, regs->rip); > __vmwrite(GUEST_RSP, regs->rsp); > __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS); > + > + return 0; > } > > /* > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index ea2da14489..26ce349c76 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1405,12 +1405,26 @@ static void virtual_vmexit(struct cpu_user_regs *regs) > vmsucceed(regs); > } > > +static void nvmx_eptp_update(void) > +{ > + if ( !nestedhvm_vcpu_in_guestmode(current) || > + vcpu_nestedhvm(current).nv_vmexit_pending || > + !vcpu_nestedhvm(current).stale_np2m || > + !nestedhvm_paging_mode_hap(current) ) > + return; > + > + __vmwrite(EPT_POINTER, get_shadow_eptp(current)); > + vcpu_nestedhvm(current).stale_np2m = false; Hmm, so interrupts are enabled here. What happens if a flush IPI occurs between these two lines of code? Won't we do the vmenter with a stale np2m? It seems like we should clear stale_np2m first. If an IPI occurs then, we'll end up re-executing the vmenter unnecessarily, but it's better to do that than to not re-execute it when we need to. -George
On Fri, 2017-09-29 at 11:53 +0100, George Dunlap wrote: > On 09/04/2017 09:14 AM, Sergey Dyasli wrote: > > If an IPI flushes vCPU's np2m object just before nested vmentry, there > > will be a stale shadow EPTP value in VMCS02. Allow vmentry to be > > restarted in such cases and add nvmx_eptp_update() to perform an update. > > > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > > --- > > xen/arch/x86/hvm/vmx/entry.S | 6 ++++++ > > xen/arch/x86/hvm/vmx/vmx.c | 8 +++++++- > > xen/arch/x86/hvm/vmx/vvmx.c | 14 ++++++++++++++ > > 3 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S > > index 53eedc6363..9fb8f89220 100644 > > --- a/xen/arch/x86/hvm/vmx/entry.S > > +++ b/xen/arch/x86/hvm/vmx/entry.S > > @@ -79,6 +79,8 @@ UNLIKELY_END(realmode) > > > > mov %rsp,%rdi > > call vmx_vmenter_helper > > + cmp $0,%eax > > + jne .Lvmx_vmentry_restart > > mov VCPU_hvm_guest_cr2(%rbx),%rax > > > > pop %r15 > > @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry) > > GET_CURRENT(bx) > > jmp .Lvmx_do_vmentry > > > > +.Lvmx_vmentry_restart: > > + sti > > + jmp .Lvmx_do_vmentry > > + > > .Lvmx_goto_emulator: > > sti > > mov %rsp,%rdi > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > > index f6da119c9f..06509590b7 100644 > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -4223,13 +4223,17 @@ static void lbr_fixup(void) > > bdw_erratum_bdf14_fixup(); > > } > > > > -void vmx_vmenter_helper(const struct cpu_user_regs *regs) > > +int vmx_vmenter_helper(const struct cpu_user_regs *regs) > > { > > struct vcpu *curr = current; > > u32 new_asid, old_asid; > > struct hvm_vcpu_asid *p_asid; > > bool_t need_flush; > > > > + /* Shadow EPTP can't be updated here because irqs are disabled */ > > + if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m ) > > + return 1; > > + > > if ( curr->domain->arch.hvm_domain.pi_ops.do_resume ) > > curr->domain->arch.hvm_domain.pi_ops.do_resume(curr); > > > > @@ -4290,6 +4294,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs) > > __vmwrite(GUEST_RIP, regs->rip); > > __vmwrite(GUEST_RSP, regs->rsp); > > __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS); > > + > > + return 0; > > } > > > > /* > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > > index ea2da14489..26ce349c76 100644 > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > @@ -1405,12 +1405,26 @@ static void virtual_vmexit(struct cpu_user_regs *regs) > > vmsucceed(regs); > > } > > > > +static void nvmx_eptp_update(void) > > +{ > > + if ( !nestedhvm_vcpu_in_guestmode(current) || > > + vcpu_nestedhvm(current).nv_vmexit_pending || > > + !vcpu_nestedhvm(current).stale_np2m || > > + !nestedhvm_paging_mode_hap(current) ) > > + return; > > + > > + __vmwrite(EPT_POINTER, get_shadow_eptp(current)); > > + vcpu_nestedhvm(current).stale_np2m = false; > > Hmm, so interrupts are enabled here. What happens if a flush IPI occurs > between these two lines of code? Won't we do the vmenter with a stale np2m? > > It seems like we should clear stale_np2m first. If an IPI occurs then, > we'll end up re-executing the vmenter unnecessarily, but it's better to > do that than to not re-execute it when we need to. Good catch! Clearing of stale_np2m must indeed happen before updating a shadow EPTP. -- Thanks, Sergey
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S index 53eedc6363..9fb8f89220 100644 --- a/xen/arch/x86/hvm/vmx/entry.S +++ b/xen/arch/x86/hvm/vmx/entry.S @@ -79,6 +79,8 @@ UNLIKELY_END(realmode) mov %rsp,%rdi call vmx_vmenter_helper + cmp $0,%eax + jne .Lvmx_vmentry_restart mov VCPU_hvm_guest_cr2(%rbx),%rax pop %r15 @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry) GET_CURRENT(bx) jmp .Lvmx_do_vmentry +.Lvmx_vmentry_restart: + sti + jmp .Lvmx_do_vmentry + .Lvmx_goto_emulator: sti mov %rsp,%rdi diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index f6da119c9f..06509590b7 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -4223,13 +4223,17 @@ static void lbr_fixup(void) bdw_erratum_bdf14_fixup(); } -void vmx_vmenter_helper(const struct cpu_user_regs *regs) +int vmx_vmenter_helper(const struct cpu_user_regs *regs) { struct vcpu *curr = current; u32 new_asid, old_asid; struct hvm_vcpu_asid *p_asid; bool_t need_flush; + /* Shadow EPTP can't be updated here because irqs are disabled */ + if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m ) + return 1; + if ( curr->domain->arch.hvm_domain.pi_ops.do_resume ) curr->domain->arch.hvm_domain.pi_ops.do_resume(curr); @@ -4290,6 +4294,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs) __vmwrite(GUEST_RIP, regs->rip); __vmwrite(GUEST_RSP, regs->rsp); __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS); + + return 0; } /* diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index ea2da14489..26ce349c76 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1405,12 +1405,26 @@ static void virtual_vmexit(struct cpu_user_regs *regs) vmsucceed(regs); } +static void nvmx_eptp_update(void) +{ + if ( !nestedhvm_vcpu_in_guestmode(current) || + vcpu_nestedhvm(current).nv_vmexit_pending || + !vcpu_nestedhvm(current).stale_np2m || + !nestedhvm_paging_mode_hap(current) ) + return; + + __vmwrite(EPT_POINTER, get_shadow_eptp(current)); + vcpu_nestedhvm(current).stale_np2m = false; +} + void nvmx_switch_guest(void) { struct vcpu *v = current; struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); struct cpu_user_regs *regs = guest_cpu_user_regs(); + nvmx_eptp_update(); + /* * A pending IO emulation may still be not finished. In this case, no * virtual vmswitch is allowed. Or else, the following IO emulation will
If an IPI flushes vCPU's np2m object just before nested vmentry, there will be a stale shadow EPTP value in VMCS02. Allow vmentry to be restarted in such cases and add nvmx_eptp_update() to perform an update. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/arch/x86/hvm/vmx/entry.S | 6 ++++++ xen/arch/x86/hvm/vmx/vmx.c | 8 +++++++- xen/arch/x86/hvm/vmx/vvmx.c | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-)