* [PATCH V2 0/2] Fix vm_event resume path race condition @ 2017-05-03 9:10 Razvan Cojocaru 2017-05-03 9:10 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h,c} Razvan Cojocaru 2017-05-03 9:10 ` [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() Razvan Cojocaru 0 siblings, 2 replies; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 9:10 UTC (permalink / raw) To: xen-devel; +Cc: andrew.cooper3, tamas, jbeulich This small series first creates hvm/vm_event.{h,c}, in order to bring under vm_event maintainership the code that has previously lived in hvm_do_resume(), and then fixes a __context_switch()-related race condition when attempting to set registers via vm_event reply. Previously this has been a single patch, "x86/vm_event: fix race between vmx_vmexit_handler() and vm_event_resume()". [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h,c} [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h,c} 2017-05-03 9:10 [PATCH V2 0/2] Fix vm_event resume path race condition Razvan Cojocaru @ 2017-05-03 9:10 ` Razvan Cojocaru 2017-05-03 9:51 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} Jan Beulich 2017-05-03 9:10 ` [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() Razvan Cojocaru 1 sibling, 1 reply; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 9:10 UTC (permalink / raw) To: xen-devel; +Cc: andrew.cooper3, tamas, Razvan Cojocaru, jbeulich Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h, where HVM-specific vm_event-related code will live. This cleans up hvm_do_resume() and ensures that the vm_event maintainers are responsible for changes to that code. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- MAINTAINERS | 2 + xen/arch/x86/hvm/Makefile | 1 + xen/arch/x86/hvm/hvm.c | 64 +---------------------- xen/arch/x86/hvm/vm_event.c | 101 +++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/hvm/vm_event.h | 34 +++++++++++++ 5 files changed, 140 insertions(+), 62 deletions(-) create mode 100644 xen/arch/x86/hvm/vm_event.c create mode 100644 xen/include/asm-x86/hvm/vm_event.h diff --git a/MAINTAINERS b/MAINTAINERS index c345a50..10863bc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -404,6 +404,7 @@ S: Supported F: tools/tests/xen-access F: xen/arch/*/monitor.c F: xen/arch/*/vm_event.c +F: xen/arch/*/hvm/vm_event.c F: xen/arch/arm/mem_access.c F: xen/arch/x86/mm/mem_access.c F: xen/arch/x86/hvm/monitor.c @@ -413,6 +414,7 @@ F: xen/common/vm_event.c F: xen/include/*/mem_access.h F: xen/include/*/monitor.h F: xen/include/*/vm_event.h +F: xen/include/*/hvm/vm_event.h F: xen/include/asm-x86/hvm/monitor.h VTPM diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile index 0a3d0f4..02f0add 100644 --- a/xen/arch/x86/hvm/Makefile +++ b/xen/arch/x86/hvm/Makefile @@ -24,6 +24,7 @@ obj-y += stdvga.o obj-y += vioapic.o obj-y += viridian.o obj-y += vlapic.o +obj-y += vm_event.o obj-y += vmsi.o obj-y += vpic.o obj-y += vpt.o diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index a441955..f010394 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -34,7 +34,6 @@ #include <xen/wait.h> #include <xen/mem_access.h> #include <xen/rangeset.h> -#include <xen/vm_event.h> #include <xen/monitor.h> #include <xen/warning.h> #include <asm/shadow.h> @@ -61,6 +60,7 @@ #include <asm/hvm/nestedhvm.h> #include <asm/hvm/monitor.h> #include <asm/hvm/ioreq.h> +#include <asm/hvm/vm_event.h> #include <asm/altp2m.h> #include <asm/mtrr.h> #include <asm/apic.h> @@ -483,67 +483,7 @@ void hvm_do_resume(struct vcpu *v) if ( !handle_hvm_io_completion(v) ) return; - if ( unlikely(v->arch.vm_event) ) - { - struct monitor_write_data *w = &v->arch.vm_event->write_data; - - if ( unlikely(v->arch.vm_event->emulate_flags) ) - { - enum emul_kind kind = EMUL_KIND_NORMAL; - - /* - * Please observ the order here to match the flag descriptions - * provided in public/vm_event.h - */ - if ( v->arch.vm_event->emulate_flags & - VM_EVENT_FLAG_SET_EMUL_READ_DATA ) - kind = EMUL_KIND_SET_CONTEXT_DATA; - else if ( v->arch.vm_event->emulate_flags & - VM_EVENT_FLAG_EMULATE_NOWRITE ) - kind = EMUL_KIND_NOWRITE; - else if ( v->arch.vm_event->emulate_flags & - VM_EVENT_FLAG_SET_EMUL_INSN_DATA ) - kind = EMUL_KIND_SET_CONTEXT_INSN; - - hvm_emulate_one_vm_event(kind, TRAP_invalid_op, - X86_EVENT_NO_EC); - - v->arch.vm_event->emulate_flags = 0; - } - - if ( w->do_write.msr ) - { - if ( hvm_msr_write_intercept(w->msr, w->value, 0) == - X86EMUL_EXCEPTION ) - hvm_inject_hw_exception(TRAP_gp_fault, 0); - - w->do_write.msr = 0; - } - - if ( w->do_write.cr0 ) - { - if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION ) - hvm_inject_hw_exception(TRAP_gp_fault, 0); - - w->do_write.cr0 = 0; - } - - if ( w->do_write.cr4 ) - { - if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION ) - hvm_inject_hw_exception(TRAP_gp_fault, 0); - - w->do_write.cr4 = 0; - } - - if ( w->do_write.cr3 ) - { - if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION ) - hvm_inject_hw_exception(TRAP_gp_fault, 0); - - w->do_write.cr3 = 0; - } - } + hvm_vm_event_do_resume(v); /* Inject pending hw/sw event */ if ( v->arch.hvm_vcpu.inject_event.vector >= 0 ) diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c new file mode 100644 index 0000000..b35ee12 --- /dev/null +++ b/xen/arch/x86/hvm/vm_event.c @@ -0,0 +1,101 @@ +/* + * arch/x86/hvm/vm_event.c + * + * HVM vm_event handling routines + * + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com) + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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, see <http://www.gnu.org/licenses/>. + */ + +#include <xen/sched.h> +#include <xen/vm_event.h> +#include <asm/hvm/support.h> +#include <asm/vm_event.h> + +void hvm_vm_event_do_resume(struct vcpu *v) +{ + struct monitor_write_data *w; + + if ( likely(!v->arch.vm_event) ) + return; + + w = &v->arch.vm_event->write_data; + + if ( unlikely(v->arch.vm_event->emulate_flags) ) + { + enum emul_kind kind = EMUL_KIND_NORMAL; + + /* + * Please observe the order here to match the flag descriptions + * provided in public/vm_event.h + */ + if ( v->arch.vm_event->emulate_flags & + VM_EVENT_FLAG_SET_EMUL_READ_DATA ) + kind = EMUL_KIND_SET_CONTEXT_DATA; + else if ( v->arch.vm_event->emulate_flags & + VM_EVENT_FLAG_EMULATE_NOWRITE ) + kind = EMUL_KIND_NOWRITE; + else if ( v->arch.vm_event->emulate_flags & + VM_EVENT_FLAG_SET_EMUL_INSN_DATA ) + kind = EMUL_KIND_SET_CONTEXT_INSN; + + hvm_emulate_one_vm_event(kind, TRAP_invalid_op, + X86_EVENT_NO_EC); + + v->arch.vm_event->emulate_flags = 0; + } + + if ( w->do_write.cr0 ) + { + if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + + w->do_write.cr0 = 0; + } + + if ( w->do_write.cr4 ) + { + if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + + w->do_write.cr4 = 0; + } + + if ( w->do_write.cr3 ) + { + if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + + w->do_write.cr3 = 0; + } + + if ( w->do_write.msr ) + { + if ( hvm_msr_write_intercept(w->msr, w->value, 0) == + X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + + w->do_write.msr = 0; + } +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-x86/hvm/vm_event.h b/xen/include/asm-x86/hvm/vm_event.h new file mode 100644 index 0000000..515fa85 --- /dev/null +++ b/xen/include/asm-x86/hvm/vm_event.h @@ -0,0 +1,34 @@ +/* + * include/asm-x86/hvm/vm_event.h + * + * Hardware virtual machine vm_event abstractions. + * + * 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, see <http://www.gnu.org/licenses/>. + */ + +#ifndef __ASM_X86_HVM_VM_EVENT_H__ +#define __ASM_X86_HVM_VM_EVENT_H__ + +void hvm_vm_event_do_resume(struct vcpu *v); + +#endif /* __ASM_X86_HVM_MONITOR_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 9:10 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h,c} Razvan Cojocaru @ 2017-05-03 9:51 ` Jan Beulich 2017-05-03 10:37 ` Razvan Cojocaru 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2017-05-03 9:51 UTC (permalink / raw) To: Razvan Cojocaru; +Cc: andrew.cooper3, tamas, xen-devel >>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com> wrote: > @@ -483,67 +483,7 @@ void hvm_do_resume(struct vcpu *v) > if ( !handle_hvm_io_completion(v) ) > return; > > - if ( unlikely(v->arch.vm_event) ) > - { > - struct monitor_write_data *w = &v->arch.vm_event->write_data; > - > - if ( unlikely(v->arch.vm_event->emulate_flags) ) > - { > - enum emul_kind kind = EMUL_KIND_NORMAL; > - > - /* > - * Please observ the order here to match the flag descriptions > - * provided in public/vm_event.h > - */ > - if ( v->arch.vm_event->emulate_flags & > - VM_EVENT_FLAG_SET_EMUL_READ_DATA ) > - kind = EMUL_KIND_SET_CONTEXT_DATA; > - else if ( v->arch.vm_event->emulate_flags & > - VM_EVENT_FLAG_EMULATE_NOWRITE ) > - kind = EMUL_KIND_NOWRITE; > - else if ( v->arch.vm_event->emulate_flags & > - VM_EVENT_FLAG_SET_EMUL_INSN_DATA ) > - kind = EMUL_KIND_SET_CONTEXT_INSN; > - > - hvm_emulate_one_vm_event(kind, TRAP_invalid_op, > - X86_EVENT_NO_EC); > - > - v->arch.vm_event->emulate_flags = 0; > - } > - > - if ( w->do_write.msr ) > - { > - if ( hvm_msr_write_intercept(w->msr, w->value, 0) == > - X86EMUL_EXCEPTION ) > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > - > - w->do_write.msr = 0; > - } > - > - if ( w->do_write.cr0 ) > - { > - if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION ) > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > - > - w->do_write.cr0 = 0; > - } > - > - if ( w->do_write.cr4 ) > - { > - if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION ) > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > - > - w->do_write.cr4 = 0; > - } > - > - if ( w->do_write.cr3 ) > - { > - if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION ) > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > - > - w->do_write.cr3 = 0; > - } > - } > + hvm_vm_event_do_resume(v); As indicated before, I think we want to keep if ( unlikely(v->arch.vm_event) ) here of in an inline wrapper, to avoid the actual function call in the common case. > --- /dev/null > +++ b/xen/arch/x86/hvm/vm_event.c > @@ -0,0 +1,101 @@ > +/* > + * arch/x86/hvm/vm_event.c > + * > + * HVM vm_event handling routines > + * > + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com) I'm notoriously bad when it comes to copyrights, but you just moving code makes me wonder whether this is appropriate. > +void hvm_vm_event_do_resume(struct vcpu *v) > +{ > + struct monitor_write_data *w; > + > + if ( likely(!v->arch.vm_event) ) > + return; > + > + w = &v->arch.vm_event->write_data; > + > + if ( unlikely(v->arch.vm_event->emulate_flags) ) > + { > + enum emul_kind kind = EMUL_KIND_NORMAL; > + > + /* > + * Please observe the order here to match the flag descriptions > + * provided in public/vm_event.h > + */ > + if ( v->arch.vm_event->emulate_flags & > + VM_EVENT_FLAG_SET_EMUL_READ_DATA ) > + kind = EMUL_KIND_SET_CONTEXT_DATA; > + else if ( v->arch.vm_event->emulate_flags & > + VM_EVENT_FLAG_EMULATE_NOWRITE ) > + kind = EMUL_KIND_NOWRITE; > + else if ( v->arch.vm_event->emulate_flags & > + VM_EVENT_FLAG_SET_EMUL_INSN_DATA ) > + kind = EMUL_KIND_SET_CONTEXT_INSN; > + > + hvm_emulate_one_vm_event(kind, TRAP_invalid_op, > + X86_EVENT_NO_EC); > + > + v->arch.vm_event->emulate_flags = 0; > + } > + > + if ( w->do_write.cr0 ) > + { > + if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION ) > + hvm_inject_hw_exception(TRAP_gp_fault, 0); > + > + w->do_write.cr0 = 0; > + } > + > + if ( w->do_write.cr4 ) > + { > + if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION ) > + hvm_inject_hw_exception(TRAP_gp_fault, 0); > + > + w->do_write.cr4 = 0; > + } > + > + if ( w->do_write.cr3 ) > + { > + if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION ) > + hvm_inject_hw_exception(TRAP_gp_fault, 0); > + > + w->do_write.cr3 = 0; > + } > + > + if ( w->do_write.msr ) > + { > + if ( hvm_msr_write_intercept(w->msr, w->value, 0) == > + X86EMUL_EXCEPTION ) > + hvm_inject_hw_exception(TRAP_gp_fault, 0); > + > + w->do_write.msr = 0; > + } I wonder whether all of these outer if()-s wouldn't better have unlikely() too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 9:51 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} Jan Beulich @ 2017-05-03 10:37 ` Razvan Cojocaru 2017-05-03 10:48 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 10:37 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, tamas, xen-devel On 05/03/17 12:51, Jan Beulich wrote: >>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com> wrote: >> @@ -483,67 +483,7 @@ void hvm_do_resume(struct vcpu *v) >> if ( !handle_hvm_io_completion(v) ) >> return; >> >> - if ( unlikely(v->arch.vm_event) ) >> - { >> - struct monitor_write_data *w = &v->arch.vm_event->write_data; >> - >> - if ( unlikely(v->arch.vm_event->emulate_flags) ) >> - { >> - enum emul_kind kind = EMUL_KIND_NORMAL; >> - >> - /* >> - * Please observ the order here to match the flag descriptions >> - * provided in public/vm_event.h >> - */ >> - if ( v->arch.vm_event->emulate_flags & >> - VM_EVENT_FLAG_SET_EMUL_READ_DATA ) >> - kind = EMUL_KIND_SET_CONTEXT_DATA; >> - else if ( v->arch.vm_event->emulate_flags & >> - VM_EVENT_FLAG_EMULATE_NOWRITE ) >> - kind = EMUL_KIND_NOWRITE; >> - else if ( v->arch.vm_event->emulate_flags & >> - VM_EVENT_FLAG_SET_EMUL_INSN_DATA ) >> - kind = EMUL_KIND_SET_CONTEXT_INSN; >> - >> - hvm_emulate_one_vm_event(kind, TRAP_invalid_op, >> - X86_EVENT_NO_EC); >> - >> - v->arch.vm_event->emulate_flags = 0; >> - } >> - >> - if ( w->do_write.msr ) >> - { >> - if ( hvm_msr_write_intercept(w->msr, w->value, 0) == >> - X86EMUL_EXCEPTION ) >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> - >> - w->do_write.msr = 0; >> - } >> - >> - if ( w->do_write.cr0 ) >> - { >> - if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION ) >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> - >> - w->do_write.cr0 = 0; >> - } >> - >> - if ( w->do_write.cr4 ) >> - { >> - if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION ) >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> - >> - w->do_write.cr4 = 0; >> - } >> - >> - if ( w->do_write.cr3 ) >> - { >> - if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION ) >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> - >> - w->do_write.cr3 = 0; >> - } >> - } >> + hvm_vm_event_do_resume(v); > > As indicated before, I think we want to keep > > if ( unlikely(v->arch.vm_event) ) > > here of in an inline wrapper, to avoid the actual function call in the > common case. Will do. >> --- /dev/null >> +++ b/xen/arch/x86/hvm/vm_event.c >> @@ -0,0 +1,101 @@ >> +/* >> + * arch/x86/hvm/vm_event.c >> + * >> + * HVM vm_event handling routines >> + * >> + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com) > > I'm notoriously bad when it comes to copyrights, but you just > moving code makes me wonder whether this is appropriate. To be honest I quite agree with you, and in the beginning I just meant to have no Copyright line in there at all - but I remembered a discussion a while back where a patch was I believe rejected because it lacked one. So I've just copied Tamas' file (vm_event.c) and only changed the copyright line because I didn't really know what else to put there. I'm quite happy to remove it altogether. Will that do? >> +void hvm_vm_event_do_resume(struct vcpu *v) >> +{ >> + struct monitor_write_data *w; >> + >> + if ( likely(!v->arch.vm_event) ) >> + return; >> + >> + w = &v->arch.vm_event->write_data; >> + >> + if ( unlikely(v->arch.vm_event->emulate_flags) ) >> + { >> + enum emul_kind kind = EMUL_KIND_NORMAL; >> + >> + /* >> + * Please observe the order here to match the flag descriptions >> + * provided in public/vm_event.h >> + */ >> + if ( v->arch.vm_event->emulate_flags & >> + VM_EVENT_FLAG_SET_EMUL_READ_DATA ) >> + kind = EMUL_KIND_SET_CONTEXT_DATA; >> + else if ( v->arch.vm_event->emulate_flags & >> + VM_EVENT_FLAG_EMULATE_NOWRITE ) >> + kind = EMUL_KIND_NOWRITE; >> + else if ( v->arch.vm_event->emulate_flags & >> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA ) >> + kind = EMUL_KIND_SET_CONTEXT_INSN; >> + >> + hvm_emulate_one_vm_event(kind, TRAP_invalid_op, >> + X86_EVENT_NO_EC); >> + >> + v->arch.vm_event->emulate_flags = 0; >> + } >> + >> + if ( w->do_write.cr0 ) >> + { >> + if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION ) >> + hvm_inject_hw_exception(TRAP_gp_fault, 0); >> + >> + w->do_write.cr0 = 0; >> + } >> + >> + if ( w->do_write.cr4 ) >> + { >> + if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION ) >> + hvm_inject_hw_exception(TRAP_gp_fault, 0); >> + >> + w->do_write.cr4 = 0; >> + } >> + >> + if ( w->do_write.cr3 ) >> + { >> + if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION ) >> + hvm_inject_hw_exception(TRAP_gp_fault, 0); >> + >> + w->do_write.cr3 = 0; >> + } >> + >> + if ( w->do_write.msr ) >> + { >> + if ( hvm_msr_write_intercept(w->msr, w->value, 0) == >> + X86EMUL_EXCEPTION ) >> + hvm_inject_hw_exception(TRAP_gp_fault, 0); >> + >> + w->do_write.msr = 0; >> + } > > I wonder whether all of these outer if()-s wouldn't better have > unlikely() too. It can't hurt, unless anyone objects I'll wrap them in unlikely()s. Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 10:37 ` Razvan Cojocaru @ 2017-05-03 10:48 ` Jan Beulich 2017-05-03 20:05 ` Tamas K Lengyel 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2017-05-03 10:48 UTC (permalink / raw) To: Razvan Cojocaru; +Cc: andrew.cooper3, tamas, xen-devel >>> On 03.05.17 at 12:37, <rcojocaru@bitdefender.com> wrote: > On 05/03/17 12:51, Jan Beulich wrote: >>>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com> wrote: >>> --- /dev/null >>> +++ b/xen/arch/x86/hvm/vm_event.c >>> @@ -0,0 +1,101 @@ >>> +/* >>> + * arch/x86/hvm/vm_event.c >>> + * >>> + * HVM vm_event handling routines >>> + * >>> + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com) >> >> I'm notoriously bad when it comes to copyrights, but you just >> moving code makes me wonder whether this is appropriate. > > To be honest I quite agree with you, and in the beginning I just meant > to have no Copyright line in there at all - but I remembered a > discussion a while back where a patch was I believe rejected because it > lacked one. So I've just copied Tamas' file (vm_event.c) and only > changed the copyright line because I didn't really know what else to put > there. > > I'm quite happy to remove it altogether. Will that do? Afaic - sure. But as said, I'm quite bad at such things ... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 10:48 ` Jan Beulich @ 2017-05-03 20:05 ` Tamas K Lengyel 2017-05-03 20:16 ` Razvan Cojocaru 0 siblings, 1 reply; 13+ messages in thread From: Tamas K Lengyel @ 2017-05-03 20:05 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Razvan Cojocaru, Xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1316 bytes --] On Wed, May 3, 2017 at 6:48 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 03.05.17 at 12:37, <rcojocaru@bitdefender.com> wrote: > > On 05/03/17 12:51, Jan Beulich wrote: > >>>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com> wrote: > >>> --- /dev/null > >>> +++ b/xen/arch/x86/hvm/vm_event.c > >>> @@ -0,0 +1,101 @@ > >>> +/* > >>> + * arch/x86/hvm/vm_event.c > >>> + * > >>> + * HVM vm_event handling routines > >>> + * > >>> + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com) > >> > >> I'm notoriously bad when it comes to copyrights, but you just > >> moving code makes me wonder whether this is appropriate. > > > > To be honest I quite agree with you, and in the beginning I just meant > > to have no Copyright line in there at all - but I remembered a > > discussion a while back where a patch was I believe rejected because it > > lacked one. So I've just copied Tamas' file (vm_event.c) and only > > changed the copyright line because I didn't really know what else to put > > there. > > > > I'm quite happy to remove it altogether. Will that do? > > Afaic - sure. But as said, I'm quite bad at such things ... > Since this is just code-movement from hvm.c to a separate file I would say it should retain the copyright lines from hvm.c. Other then that it looks good to me. Tamas [-- Attachment #1.2: Type: text/html, Size: 2182 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 20:05 ` Tamas K Lengyel @ 2017-05-03 20:16 ` Razvan Cojocaru 2017-05-03 20:32 ` Tamas K Lengyel 0 siblings, 1 reply; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 20:16 UTC (permalink / raw) To: Tamas K Lengyel, Jan Beulich; +Cc: Andrew Cooper, Xen-devel On 05/03/2017 11:05 PM, Tamas K Lengyel wrote: > > > On Wed, May 3, 2017 at 6:48 AM, Jan Beulich <JBeulich@suse.com > <mailto:JBeulich@suse.com>> wrote: > > >>> On 03.05.17 at 12:37, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > > On 05/03/17 12:51, Jan Beulich wrote: > >>>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > >>> --- /dev/null > >>> +++ b/xen/arch/x86/hvm/vm_event.c > >>> @@ -0,0 +1,101 @@ > >>> +/* > >>> + * arch/x86/hvm/vm_event.c > >>> + * > >>> + * HVM vm_event handling routines > >>> + * > >>> + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>) > >> > >> I'm notoriously bad when it comes to copyrights, but you just > >> moving code makes me wonder whether this is appropriate. > > > > To be honest I quite agree with you, and in the beginning I just meant > > to have no Copyright line in there at all - but I remembered a > > discussion a while back where a patch was I believe rejected because it > > lacked one. So I've just copied Tamas' file (vm_event.c) and only > > changed the copyright line because I didn't really know what else to put > > there. > > > > I'm quite happy to remove it altogether. Will that do? > > Afaic - sure. But as said, I'm quite bad at such things ... > > > Since this is just code-movement from hvm.c to a separate file I would > say it should retain the copyright lines from hvm.c. Other then that it > looks good to me. Actually the funny part about that is that while this is indeed only moved code, I have written all of that code in the first place, so I've moved my own code. :) But I have no problem with either removing the copyright line altogether or using the lines in hvm.c as suggested. Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 20:16 ` Razvan Cojocaru @ 2017-05-03 20:32 ` Tamas K Lengyel 2017-05-03 20:38 ` Razvan Cojocaru 0 siblings, 1 reply; 13+ messages in thread From: Tamas K Lengyel @ 2017-05-03 20:32 UTC (permalink / raw) To: Razvan Cojocaru; +Cc: Andrew Cooper, Jan Beulich, Xen-devel On Wed, May 3, 2017 at 4:16 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 05/03/2017 11:05 PM, Tamas K Lengyel wrote: >> >> >> On Wed, May 3, 2017 at 6:48 AM, Jan Beulich <JBeulich@suse.com >> <mailto:JBeulich@suse.com>> wrote: >> >> >>> On 03.05.17 at 12:37, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: >> > On 05/03/17 12:51, Jan Beulich wrote: >> >>>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: >> >>> --- /dev/null >> >>> +++ b/xen/arch/x86/hvm/vm_event.c >> >>> @@ -0,0 +1,101 @@ >> >>> +/* >> >>> + * arch/x86/hvm/vm_event.c >> >>> + * >> >>> + * HVM vm_event handling routines >> >>> + * >> >>> + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>) >> >> >> >> I'm notoriously bad when it comes to copyrights, but you just >> >> moving code makes me wonder whether this is appropriate. >> > >> > To be honest I quite agree with you, and in the beginning I just meant >> > to have no Copyright line in there at all - but I remembered a >> > discussion a while back where a patch was I believe rejected because it >> > lacked one. So I've just copied Tamas' file (vm_event.c) and only >> > changed the copyright line because I didn't really know what else to put >> > there. >> > >> > I'm quite happy to remove it altogether. Will that do? >> >> Afaic - sure. But as said, I'm quite bad at such things ... >> >> >> Since this is just code-movement from hvm.c to a separate file I would >> say it should retain the copyright lines from hvm.c. Other then that it >> looks good to me. > > Actually the funny part about that is that while this is indeed only > moved code, I have written all of that code in the first place, so I've > moved my own code. :) Well, I've also added like two lines to it with VM_EVENT_FLAG_SET_EMUL_INSN_DATA ;) > > But I have no problem with either removing the copyright line altogether > or using the lines in hvm.c as suggested. Yeap, I'm fine with either route but the safe bet is to just use the one from hvm.c (and add yourself to it if you feel like). Tamas _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} 2017-05-03 20:32 ` Tamas K Lengyel @ 2017-05-03 20:38 ` Razvan Cojocaru 0 siblings, 0 replies; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 20:38 UTC (permalink / raw) To: Tamas K Lengyel; +Cc: Andrew Cooper, Jan Beulich, Xen-devel On 05/03/2017 11:32 PM, Tamas K Lengyel wrote: > On Wed, May 3, 2017 at 4:16 PM, Razvan Cojocaru > <rcojocaru@bitdefender.com> wrote: >> On 05/03/2017 11:05 PM, Tamas K Lengyel wrote: >>> >>> >>> On Wed, May 3, 2017 at 6:48 AM, Jan Beulich <JBeulich@suse.com >>> <mailto:JBeulich@suse.com>> wrote: >>> >>> >>> On 03.05.17 at 12:37, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: >>> > On 05/03/17 12:51, Jan Beulich wrote: >>> >>>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: >>> >>> --- /dev/null >>> >>> +++ b/xen/arch/x86/hvm/vm_event.c >>> >>> @@ -0,0 +1,101 @@ >>> >>> +/* >>> >>> + * arch/x86/hvm/vm_event.c >>> >>> + * >>> >>> + * HVM vm_event handling routines >>> >>> + * >>> >>> + * Copyright (c) 2017 Razvan Cojocaru (rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>) >>> >> >>> >> I'm notoriously bad when it comes to copyrights, but you just >>> >> moving code makes me wonder whether this is appropriate. >>> > >>> > To be honest I quite agree with you, and in the beginning I just meant >>> > to have no Copyright line in there at all - but I remembered a >>> > discussion a while back where a patch was I believe rejected because it >>> > lacked one. So I've just copied Tamas' file (vm_event.c) and only >>> > changed the copyright line because I didn't really know what else to put >>> > there. >>> > >>> > I'm quite happy to remove it altogether. Will that do? >>> >>> Afaic - sure. But as said, I'm quite bad at such things ... >>> >>> >>> Since this is just code-movement from hvm.c to a separate file I would >>> say it should retain the copyright lines from hvm.c. Other then that it >>> looks good to me. >> >> Actually the funny part about that is that while this is indeed only >> moved code, I have written all of that code in the first place, so I've >> moved my own code. :) > > Well, I've also added like two lines to it with > VM_EVENT_FLAG_SET_EMUL_INS_DATA ;) Right, my bad. :) >> But I have no problem with either removing the copyright line altogether >> or using the lines in hvm.c as suggested. > > Yeap, I'm fine with either route but the safe bet is to just use the > one from hvm.c (and add yourself to it if you feel like). Alright, I'll just copy it from hvm.c. Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() 2017-05-03 9:10 [PATCH V2 0/2] Fix vm_event resume path race condition Razvan Cojocaru 2017-05-03 9:10 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h,c} Razvan Cojocaru @ 2017-05-03 9:10 ` Razvan Cojocaru 2017-05-03 10:01 ` Jan Beulich 2017-05-03 20:10 ` Tamas K Lengyel 1 sibling, 2 replies; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 9:10 UTC (permalink / raw) To: xen-devel; +Cc: andrew.cooper3, tamas, Razvan Cojocaru, jbeulich The introspection agent can reply to a vm_event faster than vmx_vmexit_handler() can complete in some cases, where it is then not safe for vm_event_set_registers() to modify v->arch.user_regs. In the test scenario, we were stepping over an INT3 breakpoint by setting RIP += 1. The quick reply tended to complete before the VCPU triggering the introspection event had properly paused and been descheduled. If the reply occurs before __context_switch() happens, __context_switch() clobbers the reply by overwriting v->arch.user_regs from the stack. If the reply occurs after __context_switch(), we don't pass through __context_switch() when transitioning to idle. This patch ensures that vm_event_resume() code only sets per-VCPU data to be used for the actual setting of registers later in hvm_do_resume() (similar to the model used to control setting of CRs and MSRs). The patch additionally removes the sync_vcpu_execstate(v) call from vm_event_resume(), which is no longer necessary, which removes the associated broadcast TLB flush (read: performance improvement). Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/vm_event.c | 35 +++++++++++++++++++++++++++++++++++ xen/arch/x86/vm_event.c | 22 ++-------------------- xen/common/vm_event.c | 17 ++++++++++------- xen/include/asm-x86/vm_event.h | 2 ++ 4 files changed, 49 insertions(+), 27 deletions(-) diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c index b35ee12..bfb95a2 100644 --- a/xen/arch/x86/hvm/vm_event.c +++ b/xen/arch/x86/hvm/vm_event.c @@ -23,6 +23,39 @@ #include <asm/hvm/support.h> #include <asm/vm_event.h> +static void hvm_vm_event_set_registers(const struct vcpu *v) +{ + ASSERT(v == current); + + if ( unlikely(v->arch.vm_event->set_gprs) ) + { + struct cpu_user_regs *regs = guest_cpu_user_regs(); + + regs->rax = v->arch.vm_event->gprs.rax; + regs->rbx = v->arch.vm_event->gprs.rbx; + regs->rcx = v->arch.vm_event->gprs.rcx; + regs->rdx = v->arch.vm_event->gprs.rdx; + regs->rsp = v->arch.vm_event->gprs.rsp; + regs->rbp = v->arch.vm_event->gprs.rbp; + regs->rsi = v->arch.vm_event->gprs.rsi; + regs->rdi = v->arch.vm_event->gprs.rdi; + + regs->r8 = v->arch.vm_event->gprs.r8; + regs->r9 = v->arch.vm_event->gprs.r9; + regs->r10 = v->arch.vm_event->gprs.r10; + regs->r11 = v->arch.vm_event->gprs.r11; + regs->r12 = v->arch.vm_event->gprs.r12; + regs->r13 = v->arch.vm_event->gprs.r13; + regs->r14 = v->arch.vm_event->gprs.r14; + regs->r15 = v->arch.vm_event->gprs.r15; + + regs->rflags = v->arch.vm_event->gprs.rflags; + regs->rip = v->arch.vm_event->gprs.rip; + + v->arch.vm_event->set_gprs = false; + } +} + void hvm_vm_event_do_resume(struct vcpu *v) { struct monitor_write_data *w; @@ -30,6 +63,8 @@ void hvm_vm_event_do_resume(struct vcpu *v) if ( likely(!v->arch.vm_event) ) return; + hvm_vm_event_set_registers(v); + w = &v->arch.vm_event->write_data; if ( unlikely(v->arch.vm_event->emulate_flags) ) diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 502ccc2..a6ea42c 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -113,26 +113,8 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp) { ASSERT(atomic_read(&v->vm_event_pause_count)); - v->arch.user_regs.rax = rsp->data.regs.x86.rax; - v->arch.user_regs.rbx = rsp->data.regs.x86.rbx; - v->arch.user_regs.rcx = rsp->data.regs.x86.rcx; - v->arch.user_regs.rdx = rsp->data.regs.x86.rdx; - v->arch.user_regs.rsp = rsp->data.regs.x86.rsp; - v->arch.user_regs.rbp = rsp->data.regs.x86.rbp; - v->arch.user_regs.rsi = rsp->data.regs.x86.rsi; - v->arch.user_regs.rdi = rsp->data.regs.x86.rdi; - - v->arch.user_regs.r8 = rsp->data.regs.x86.r8; - v->arch.user_regs.r9 = rsp->data.regs.x86.r9; - v->arch.user_regs.r10 = rsp->data.regs.x86.r10; - v->arch.user_regs.r11 = rsp->data.regs.x86.r11; - v->arch.user_regs.r12 = rsp->data.regs.x86.r12; - v->arch.user_regs.r13 = rsp->data.regs.x86.r13; - v->arch.user_regs.r14 = rsp->data.regs.x86.r14; - v->arch.user_regs.r15 = rsp->data.regs.x86.r15; - - v->arch.user_regs.rflags = rsp->data.regs.x86.rflags; - v->arch.user_regs.rip = rsp->data.regs.x86.rip; + v->arch.vm_event->gprs = rsp->data.regs.x86; + v->arch.vm_event->set_gprs = true; } void vm_event_monitor_next_interrupt(struct vcpu *v) diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 0fe9a53..9291db6 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -357,6 +357,16 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved) { vm_event_response_t rsp; + /* + * vm_event_resume() runs in either XEN_DOMCTL_VM_EVENT_OP_*, or + * EVTCHN_send context from the introspection consumer. Both contexts + * are guaranteed not to be the subject of vm_event responses. + * While we could ASSERT(v != current) for each VCPU in d in the loop + * below, this covers the case where we would need to iterate over all + * of them more succintly. + */ + ASSERT(d != current->domain); + /* Pull all responses off the ring. */ while ( vm_event_get_response(d, ved, &rsp) ) { @@ -375,13 +385,6 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved) v = d->vcpu[rsp.vcpu_id]; /* - * Make sure the vCPU state has been synchronized for the custom - * handlers. - */ - if ( atomic_read(&v->vm_event_pause_count) ) - sync_vcpu_execstate(v); - - /* * In some cases the response type needs extra handling, so here * we call the appropriate handlers. */ diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h index ca73f99..39e73c8 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -32,6 +32,8 @@ struct arch_vm_event { struct vm_event_emul_insn_data insn; } emul; struct monitor_write_data write_data; + struct vm_event_regs_x86 gprs; + bool set_gprs; }; int vm_event_init_domain(struct domain *d); -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() 2017-05-03 9:10 ` [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() Razvan Cojocaru @ 2017-05-03 10:01 ` Jan Beulich 2017-05-03 10:40 ` Razvan Cojocaru 2017-05-03 20:10 ` Tamas K Lengyel 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2017-05-03 10:01 UTC (permalink / raw) To: Razvan Cojocaru; +Cc: andrew.cooper3, tamas, xen-devel >>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com> wrote: > The introspection agent can reply to a vm_event faster than > vmx_vmexit_handler() can complete in some cases, where it is then > not safe for vm_event_set_registers() to modify v->arch.user_regs. > In the test scenario, we were stepping over an INT3 breakpoint by > setting RIP += 1. The quick reply tended to complete before the VCPU > triggering the introspection event had properly paused and been > descheduled. If the reply occurs before __context_switch() happens, > __context_switch() clobbers the reply by overwriting > v->arch.user_regs from the stack. If the reply occurs after > __context_switch(), we don't pass through __context_switch() when > transitioning to idle. This last sentence still looks to be wrong (and even self-contradictory). The reply can't occur after __context_switch() if we don't make it there in the first place. How about "If we don't pass through __context_switch() (due to switching to the idle vCPU), reply data wouldn't be picked up when switching back straight to the original vCPU"? > This patch ensures that vm_event_resume() code only sets per-VCPU > data to be used for the actual setting of registers later in > hvm_do_resume() (similar to the model used to control setting of CRs > and MSRs). > > The patch additionally removes the sync_vcpu_execstate(v) call from > vm_event_resume(), which is no longer necessary, which removes the > associated broadcast TLB flush (read: performance improvement). > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Code changes themselves Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() 2017-05-03 10:01 ` Jan Beulich @ 2017-05-03 10:40 ` Razvan Cojocaru 0 siblings, 0 replies; 13+ messages in thread From: Razvan Cojocaru @ 2017-05-03 10:40 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, tamas, xen-devel On 05/03/17 13:01, Jan Beulich wrote: >>>> On 03.05.17 at 11:10, <rcojocaru@bitdefender.com> wrote: >> The introspection agent can reply to a vm_event faster than >> vmx_vmexit_handler() can complete in some cases, where it is then >> not safe for vm_event_set_registers() to modify v->arch.user_regs. >> In the test scenario, we were stepping over an INT3 breakpoint by >> setting RIP += 1. The quick reply tended to complete before the VCPU >> triggering the introspection event had properly paused and been >> descheduled. If the reply occurs before __context_switch() happens, >> __context_switch() clobbers the reply by overwriting >> v->arch.user_regs from the stack. If the reply occurs after >> __context_switch(), we don't pass through __context_switch() when >> transitioning to idle. > > This last sentence still looks to be wrong (and even self-contradictory). > The reply can't occur after __context_switch() if we don't make it there > in the first place. How about "If we don't pass through > __context_switch() (due to switching to the idle vCPU), reply data > wouldn't be picked up when switching back straight to the original > vCPU"? Quite right, it's very convoluted. I'll update the comment. Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() 2017-05-03 9:10 ` [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() Razvan Cojocaru 2017-05-03 10:01 ` Jan Beulich @ 2017-05-03 20:10 ` Tamas K Lengyel 1 sibling, 0 replies; 13+ messages in thread From: Tamas K Lengyel @ 2017-05-03 20:10 UTC (permalink / raw) To: Razvan Cojocaru; +Cc: Andrew Cooper, Jan Beulich, Xen-devel On Wed, May 3, 2017 at 5:10 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > > The introspection agent can reply to a vm_event faster than > vmx_vmexit_handler() can complete in some cases, where it is then > not safe for vm_event_set_registers() to modify v->arch.user_regs. > In the test scenario, we were stepping over an INT3 breakpoint by > setting RIP += 1. The quick reply tended to complete before the VCPU > triggering the introspection event had properly paused and been > descheduled. If the reply occurs before __context_switch() happens, > __context_switch() clobbers the reply by overwriting > v->arch.user_regs from the stack. If the reply occurs after > __context_switch(), we don't pass through __context_switch() when > transitioning to idle. > > This patch ensures that vm_event_resume() code only sets per-VCPU > data to be used for the actual setting of registers later in > hvm_do_resume() (similar to the model used to control setting of CRs > and MSRs). > > The patch additionally removes the sync_vcpu_execstate(v) call from > vm_event_resume(), which is no longer necessary, which removes the > associated broadcast TLB flush (read: performance improvement). > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-05-03 20:38 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-03 9:10 [PATCH V2 0/2] Fix vm_event resume path race condition Razvan Cojocaru 2017-05-03 9:10 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h,c} Razvan Cojocaru 2017-05-03 9:51 ` [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c} Jan Beulich 2017-05-03 10:37 ` Razvan Cojocaru 2017-05-03 10:48 ` Jan Beulich 2017-05-03 20:05 ` Tamas K Lengyel 2017-05-03 20:16 ` Razvan Cojocaru 2017-05-03 20:32 ` Tamas K Lengyel 2017-05-03 20:38 ` Razvan Cojocaru 2017-05-03 9:10 ` [PATCH V2 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume() Razvan Cojocaru 2017-05-03 10:01 ` Jan Beulich 2017-05-03 10:40 ` Razvan Cojocaru 2017-05-03 20:10 ` Tamas K Lengyel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.