| Message ID | 72d4d282dd20b79ebdbaf1f70865ea38b075c5c0.1589561218.git.tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vm_event: fix race-condition when disabling monitor events | expand |
On 15.05.2020 18:53, Tamas K Lengyel wrote: > Extend the monitor_op domctl to include option that enables > controlling what values certain registers are permitted to hold > by a monitor subscriber. This needs a bit more explanation, especially for those of us who aren't that introspection savvy. For example, from the text here I didn't expect a simple bool control, but something where actual (register) values get passed back and forth. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2263,9 +2263,10 @@ int hvm_set_cr0(unsigned long value, bool may_defer) > { > ASSERT(v->arch.vm_event); > > - if ( hvm_monitor_crX(CR0, value, old_value) ) > + if ( hvm_monitor_crX(CR0, value, old_value) && > + v->domain->arch.monitor.control_register_values ) > { > - /* The actual write will occur in hvm_do_resume(), if permitted. */ > + /* The actual write will occur in hvm_do_resume, if permitted. */ Please can you leave alone this and the similar comments below. And for consistency _add_ parentheses to the one new instance you add? > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d, > struct xen_domctl_monitor_op *mop) > { > struct arch_domain *ad = &d->arch; > - bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); > + bool requested_status; > + > + if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op ) > + { > + ad->monitor.control_register_values = true; And there's no way to clear this flag again? Jan
On Wed, May 20, 2020 at 7:36 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 15.05.2020 18:53, Tamas K Lengyel wrote: > > Extend the monitor_op domctl to include option that enables > > controlling what values certain registers are permitted to hold > > by a monitor subscriber. > > This needs a bit more explanation, especially for those of us > who aren't that introspection savvy. For example, from the text > here I didn't expect a simple bool control, but something where > actual (register) values get passed back and forth. > > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -2263,9 +2263,10 @@ int hvm_set_cr0(unsigned long value, bool may_defer) > > { > > ASSERT(v->arch.vm_event); > > > > - if ( hvm_monitor_crX(CR0, value, old_value) ) > > + if ( hvm_monitor_crX(CR0, value, old_value) && > > + v->domain->arch.monitor.control_register_values ) > > { > > - /* The actual write will occur in hvm_do_resume(), if permitted. */ > > + /* The actual write will occur in hvm_do_resume, if permitted. */ > > Please can you leave alone this and the similar comments below. > And for consistency _add_ parentheses to the one new instance > you add? I changed to because now it doesn't fit into the 80-line limit below, and then changed it everywhere _for_ consistency. > > > --- a/xen/arch/x86/monitor.c > > +++ b/xen/arch/x86/monitor.c > > @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d, > > struct xen_domctl_monitor_op *mop) > > { > > struct arch_domain *ad = &d->arch; > > - bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); > > + bool requested_status; > > + > > + if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op ) > > + { > > + ad->monitor.control_register_values = true; > > And there's no way to clear this flag again? There is. Disable the monitor vm_event interface and reinitialize. Tamas
On 20.05.2020 15:42, Tamas K Lengyel wrote: > On Wed, May 20, 2020 at 7:36 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 15.05.2020 18:53, Tamas K Lengyel wrote: >>> Extend the monitor_op domctl to include option that enables >>> controlling what values certain registers are permitted to hold >>> by a monitor subscriber. >> >> This needs a bit more explanation, especially for those of us >> who aren't that introspection savvy. For example, from the text >> here I didn't expect a simple bool control, but something where >> actual (register) values get passed back and forth. >> >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -2263,9 +2263,10 @@ int hvm_set_cr0(unsigned long value, bool may_defer) >>> { >>> ASSERT(v->arch.vm_event); >>> >>> - if ( hvm_monitor_crX(CR0, value, old_value) ) >>> + if ( hvm_monitor_crX(CR0, value, old_value) && >>> + v->domain->arch.monitor.control_register_values ) >>> { >>> - /* The actual write will occur in hvm_do_resume(), if permitted. */ >>> + /* The actual write will occur in hvm_do_resume, if permitted. */ >> >> Please can you leave alone this and the similar comments below. >> And for consistency _add_ parentheses to the one new instance >> you add? > > I changed to because now it doesn't fit into the 80-line limit below, > and then changed it everywhere _for_ consistency. The 80-char limit is easy to deal with - wrap the line. >>> --- a/xen/arch/x86/monitor.c >>> +++ b/xen/arch/x86/monitor.c >>> @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d, >>> struct xen_domctl_monitor_op *mop) >>> { >>> struct arch_domain *ad = &d->arch; >>> - bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); >>> + bool requested_status; >>> + >>> + if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op ) >>> + { >>> + ad->monitor.control_register_values = true; >> >> And there's no way to clear this flag again? > > There is. Disable the monitor vm_event interface and reinitialize. Quite heavy handed, isn't it? Jan
On Wed, May 20, 2020 at 7:48 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 20.05.2020 15:42, Tamas K Lengyel wrote: > > On Wed, May 20, 2020 at 7:36 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 15.05.2020 18:53, Tamas K Lengyel wrote: > >>> Extend the monitor_op domctl to include option that enables > >>> controlling what values certain registers are permitted to hold > >>> by a monitor subscriber. > >> > >> This needs a bit more explanation, especially for those of us > >> who aren't that introspection savvy. For example, from the text > >> here I didn't expect a simple bool control, but something where > >> actual (register) values get passed back and forth. > >> > >>> --- a/xen/arch/x86/hvm/hvm.c > >>> +++ b/xen/arch/x86/hvm/hvm.c > >>> @@ -2263,9 +2263,10 @@ int hvm_set_cr0(unsigned long value, bool may_defer) > >>> { > >>> ASSERT(v->arch.vm_event); > >>> > >>> - if ( hvm_monitor_crX(CR0, value, old_value) ) > >>> + if ( hvm_monitor_crX(CR0, value, old_value) && > >>> + v->domain->arch.monitor.control_register_values ) > >>> { > >>> - /* The actual write will occur in hvm_do_resume(), if permitted. */ > >>> + /* The actual write will occur in hvm_do_resume, if permitted. */ > >> > >> Please can you leave alone this and the similar comments below. > >> And for consistency _add_ parentheses to the one new instance > >> you add? > > > > I changed to because now it doesn't fit into the 80-line limit below, > > and then changed it everywhere _for_ consistency. > > The 80-char limit is easy to deal with - wrap the line. > > >>> --- a/xen/arch/x86/monitor.c > >>> +++ b/xen/arch/x86/monitor.c > >>> @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d, > >>> struct xen_domctl_monitor_op *mop) > >>> { > >>> struct arch_domain *ad = &d->arch; > >>> - bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); > >>> + bool requested_status; > >>> + > >>> + if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op ) > >>> + { > >>> + ad->monitor.control_register_values = true; > >> > >> And there's no way to clear this flag again? > > > > There is. Disable the monitor vm_event interface and reinitialize. > > Quite heavy handed, isn't it? Not really. It's perfectly suitable for what its used for. You either need this feature for the duration of your monitoring or you don't. There is no in-between. Tamas
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 814b7020d8..063f8ddc18 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2263,9 +2263,10 @@ int hvm_set_cr0(unsigned long value, bool may_defer) { ASSERT(v->arch.vm_event); - if ( hvm_monitor_crX(CR0, value, old_value) ) + if ( hvm_monitor_crX(CR0, value, old_value) && + v->domain->arch.monitor.control_register_values ) { - /* The actual write will occur in hvm_do_resume(), if permitted. */ + /* The actual write will occur in hvm_do_resume, if permitted. */ v->arch.vm_event->write_data.do_write.cr0 = 1; v->arch.vm_event->write_data.cr0 = value; @@ -2362,9 +2363,10 @@ int hvm_set_cr3(unsigned long value, bool may_defer) { ASSERT(v->arch.vm_event); - if ( hvm_monitor_crX(CR3, value, old) ) + if ( hvm_monitor_crX(CR3, value, old) && + v->domain->arch.monitor.control_register_values ) { - /* The actual write will occur in hvm_do_resume(), if permitted. */ + /* The actual write will occur in hvm_do_resume, if permitted. */ v->arch.vm_event->write_data.do_write.cr3 = 1; v->arch.vm_event->write_data.cr3 = value; @@ -2443,9 +2445,10 @@ int hvm_set_cr4(unsigned long value, bool may_defer) { ASSERT(v->arch.vm_event); - if ( hvm_monitor_crX(CR4, value, old_cr) ) + if ( hvm_monitor_crX(CR4, value, old_cr) && + v->domain->arch.monitor.control_register_values ) { - /* The actual write will occur in hvm_do_resume(), if permitted. */ + /* The actual write will occur in hvm_do_resume, if permitted. */ v->arch.vm_event->write_data.do_write.cr4 = 1; v->arch.vm_event->write_data.cr4 = value; @@ -3587,13 +3590,17 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, ASSERT(v->arch.vm_event); - /* The actual write will occur in hvm_do_resume() (if permitted). */ - v->arch.vm_event->write_data.do_write.msr = 1; - v->arch.vm_event->write_data.msr = msr; - v->arch.vm_event->write_data.value = msr_content; - hvm_monitor_msr(msr, msr_content, msr_old_content); - return X86EMUL_OKAY; + + if ( v->domain->arch.monitor.control_register_values ) + { + /* The actual write will occur in hvm_do_resume, if permitted. */ + v->arch.vm_event->write_data.do_write.msr = 1; + v->arch.vm_event->write_data.msr = msr; + v->arch.vm_event->write_data.value = msr_content; + + return X86EMUL_OKAY; + } } if ( (ret = guest_wrmsr(v, msr, msr_content)) != X86EMUL_UNHANDLEABLE ) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index bbcb7536c7..1517a97f50 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d, struct xen_domctl_monitor_op *mop) { struct arch_domain *ad = &d->arch; - bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); + bool requested_status; + + if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op ) + { + ad->monitor.control_register_values = true; + return 0; + } + + requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); switch ( mop->event ) { diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 5b6d909266..d890ab7a22 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -416,6 +416,7 @@ struct arch_domain * This is used to filter out pagefaults. */ unsigned int inguest_pagefault_disabled : 1; + unsigned int control_register_values : 1; struct monitor_msr_bitmap *msr_bitmap; uint64_t write_ctrlreg_mask[4]; } monitor; diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 1ad34c35eb..cbcd25f12c 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1025,6 +1025,7 @@ struct xen_domctl_psr_cmt_op { #define XEN_DOMCTL_MONITOR_OP_DISABLE 1 #define XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES 2 #define XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP 3 +#define XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS 4 #define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG 0 #define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR 1
Extend the monitor_op domctl to include option that enables controlling what values certain registers are permitted to hold by a monitor subscriber. Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> --- xen/arch/x86/hvm/hvm.c | 31 +++++++++++++++++++------------ xen/arch/x86/monitor.c | 10 +++++++++- xen/include/asm-x86/domain.h | 1 + xen/include/public/domctl.h | 1 + 4 files changed, 30 insertions(+), 13 deletions(-)