| Message ID | 1455606514-18705-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2/16/2016 9:08 AM, Corneliu ZUZU wrote: > This patch moves monitor_domctl to common-side. > Purpose: move what's common to common, prepare for implementation > of such vm-events on ARM. > > * move get_capabilities to arch-side => arch_monitor_get_capabilities. > * add arch-side monitor op handling function => arch_monitor_domctl_op. > e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op > * add arch-side monitor event handling function => arch_monitor_domctl_event. > e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable > * remove status_check > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > > --- > Changed since v3: > * monitor_domctl @ common/monitor.c: > - remove unused requested_status > - sanity check mop->event range to avoid left-shift undefined behavior Due to left-shift undefined behavior situations, shouldn't I also: * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<' * in X86 arch_monitor_domctl_event, XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case add a sanity check of mop->u.mov_to_cr.index before: unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); , which basically translates to: unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index); ? (especially since mop->u.mov_to_cr.index is set by the caller). Would have been good if I'd thought of that before sending this patch series :). Thanks, Corneliu.
>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote: > On 2/16/2016 9:08 AM, Corneliu ZUZU wrote: >> This patch moves monitor_domctl to common-side. >> Purpose: move what's common to common, prepare for implementation >> of such vm-events on ARM. >> >> * move get_capabilities to arch-side => arch_monitor_get_capabilities. >> * add arch-side monitor op handling function => arch_monitor_domctl_op. >> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op >> * add arch-side monitor event handling function => arch_monitor_domctl_event. >> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event > enable/disable >> * remove status_check >> >> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >> >> --- >> Changed since v3: >> * monitor_domctl @ common/monitor.c: >> - remove unused requested_status >> - sanity check mop->event range to avoid left-shift undefined behavior > > Due to left-shift undefined behavior situations, shouldn't I also: > > * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<' There's no undefinedness there, since the right side operands of << are all constant. Using 1U here would be okay, but is not strictly needed. > * in X86 arch_monitor_domctl_event, > XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case > add a sanity check of mop->u.mov_to_cr.index before: > unsigned int ctrlreg_bitmask = > monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); > , which basically translates to: > unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index); > > ? (especially since mop->u.mov_to_cr.index is set by the caller). Yes, there a range check would be needed, but preferably as a separate patch (as this has nothing to do with the code motion you perform here). Jan
On Tue, 16 Feb 2016, Corneliu ZUZU wrote: > This patch moves monitor_domctl to common-side. > Purpose: move what's common to common, prepare for implementation > of such vm-events on ARM. > > * move get_capabilities to arch-side => arch_monitor_get_capabilities. > * add arch-side monitor op handling function => arch_monitor_domctl_op. > e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op > * add arch-side monitor event handling function => arch_monitor_domctl_event. > e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable > * remove status_check > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> For the ARM part Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Changed since v3: > * monitor_domctl @ common/monitor.c: > - remove unused requested_status > - sanity check mop->event range to avoid left-shift undefined behavior > * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning > * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef > --- > MAINTAINERS | 1 + > xen/arch/x86/monitor.c | 153 +++++++++++------------------------------- > xen/common/Makefile | 1 + > xen/common/domctl.c | 2 +- > xen/common/monitor.c | 69 +++++++++++++++++++ > xen/include/asm-arm/monitor.h | 30 +++++++-- > xen/include/asm-x86/monitor.h | 53 +++++++++++++-- > xen/include/xen/monitor.h | 30 +++++++++ > 8 files changed, 217 insertions(+), 122 deletions(-) > create mode 100644 xen/common/monitor.c > create mode 100644 xen/include/xen/monitor.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index f07384c..5cbb1dc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -355,6 +355,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com> > S: Supported > F: xen/common/vm_event.c > F: xen/common/mem_access.c > +F: xen/common/monitor.c > F: xen/arch/x86/hvm/event.c > F: xen/arch/x86/monitor.c > F: xen/arch/*/vm_event.c > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index 1d43880..660b92c 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -1,9 +1,10 @@ > /* > * arch/x86/monitor.c > * > - * Architecture-specific monitor_op domctl handler. > + * Arch-specific monitor_op domctl handler. > * > * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public > @@ -18,87 +19,14 @@ > * License along with this program; If not, see <http://www.gnu.org/licenses/>. > */ > > -#include <xen/config.h> > -#include <xen/sched.h> > -#include <xen/mm.h> > -#include <asm/domain.h> > #include <asm/monitor.h> > -#include <public/domctl.h> > -#include <xsm/xsm.h> > +#include <public/vm_event.h> > > -/* > - * Sanity check whether option is already enabled/disabled > - */ > -static inline > -int status_check(struct xen_domctl_monitor_op *mop, bool_t status) > -{ > - bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE); > - > - if ( status == requested_status ) > - return -EEXIST; > - > - return 0; > -} > - > -static inline uint32_t get_capabilities(struct domain *d) > -{ > - uint32_t capabilities = 0; > - > - /* > - * At the moment only Intel HVM domains are supported. However, event > - * delivery could be extended to AMD and PV domains. > - */ > - if ( !is_hvm_domain(d) || !cpu_has_vmx ) > - return capabilities; > - > - capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | > - (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | > - (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | > - (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); > - > - /* Since we know this is on VMX, we can just call the hvm func */ > - if ( hvm_is_singlestep_supported() ) > - capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); > - > - return capabilities; > -} > - > -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > +int arch_monitor_domctl_event(struct domain *d, > + struct xen_domctl_monitor_op *mop) > { > - int rc; > struct arch_domain *ad = &d->arch; > - uint32_t capabilities = get_capabilities(d); > - > - if ( current->domain == d ) /* no domain_pause() */ > - return -EPERM; > - > - rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event); > - if ( rc ) > - return rc; > - > - switch ( mop->op ) > - { > - case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: > - mop->event = capabilities; > - return 0; > - > - case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: > - domain_pause(d); > - ad->mem_access_emulate_each_rep = !!mop->event; > - domain_unpause(d); > - return 0; > - } > - > - /* > - * Sanity check > - */ > - if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE && > - mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE ) > - return -EOPNOTSUPP; > - > - /* Check if event type is available. */ > - if ( !(capabilities & (1 << mop->event)) ) > - return -EOPNOTSUPP; > + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); > > switch ( mop->event ) > { > @@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > { > unsigned int ctrlreg_bitmask = > monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); > - bool_t status = > + bool_t old_status = > !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask); > - struct vcpu *v; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > > @@ -126,15 +52,18 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > else > ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; > > - if ( !status ) > + if ( !old_status ) > ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; > else > ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; > > - if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 ) > - /* Latches new CR3 mask through CR0 code */ > + if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index ) > + { > + struct vcpu *v; > + /* Latches new CR3 mask through CR0 code. */ > for_each_vcpu ( d, v ) > hvm_update_guest_cr(v, 0); > + } > > domain_unpause(d); > > @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > > case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: > { > - bool_t status = ad->monitor.mov_to_msr_enabled; > + bool_t old_status = ad->monitor.mov_to_msr_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && > - mop->u.mov_to_msr.extended_capture && > + if ( requested_status && mop->u.mov_to_msr.extended_capture && > !hvm_enable_msr_exit_interception(d) ) > return -EOPNOTSUPP; > > domain_pause(d); > > - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && > - mop->u.mov_to_msr.extended_capture ) > - ad->monitor.mov_to_msr_extended = 1; > + if ( requested_status && mop->u.mov_to_msr.extended_capture ) > + ad->monitor.mov_to_msr_extended = 1; > else > ad->monitor.mov_to_msr_extended = 0; > > - ad->monitor.mov_to_msr_enabled = !status; > + ad->monitor.mov_to_msr_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: > { > - bool_t status = ad->monitor.singlestep_enabled; > + bool_t old_status = ad->monitor.singlestep_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > - ad->monitor.singlestep_enabled = !status; > + ad->monitor.singlestep_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: > { > - bool_t status = ad->monitor.software_breakpoint_enabled; > + bool_t old_status = ad->monitor.software_breakpoint_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > - ad->monitor.software_breakpoint_enabled = !status; > + ad->monitor.software_breakpoint_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: > { > - bool_t status = ad->monitor.guest_request_enabled; > + bool_t old_status = ad->monitor.guest_request_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > ad->monitor.guest_request_sync = mop->u.guest_request.sync; > - ad->monitor.guest_request_enabled = !status; > + ad->monitor.guest_request_enabled = !old_status; > domain_unpause(d); > break; > } > > default: > + /* > + * Should not be reached unless arch_monitor_get_capabilities() is not > + * properly implemented. > + */ > + ASSERT_UNREACHABLE(); > return -EOPNOTSUPP; > - > - }; > + } > > return 0; > } > diff --git a/xen/common/Makefile b/xen/common/Makefile > index 6e82b33..0d76efe 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -20,6 +20,7 @@ obj-y += lib.o > obj-y += lzo.o > obj-$(CONFIG_HAS_MEM_ACCESS) += mem_access.o > obj-y += memory.o > +obj-y += monitor.o > obj-y += multicall.o > obj-y += notifier.o > obj-y += page_alloc.o > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 22fa5d5..b34c0a1 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -25,11 +25,11 @@ > #include <xen/paging.h> > #include <xen/hypercall.h> > #include <xen/vm_event.h> > +#include <xen/monitor.h> > #include <asm/current.h> > #include <asm/irq.h> > #include <asm/page.h> > #include <asm/p2m.h> > -#include <asm/monitor.h> > #include <public/domctl.h> > #include <xsm/xsm.h> > > diff --git a/xen/common/monitor.c b/xen/common/monitor.c > new file mode 100644 > index 0000000..2a99a04 > --- /dev/null > +++ b/xen/common/monitor.c > @@ -0,0 +1,69 @@ > +/* > + * xen/common/monitor.c > + * > + * Common monitor_op domctl handler. > + * > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > + * > + * 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/monitor.h> > +#include <xen/sched.h> > +#include <xsm/xsm.h> > +#include <public/domctl.h> > +#include <asm/monitor.h> > + > +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > +{ > + int rc; > + > + if ( unlikely(current->domain == d) ) /* no domain_pause() */ > + return -EPERM; > + > + rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event); > + if ( unlikely(rc) ) > + return rc; > + > + switch ( mop->op ) > + { > + case XEN_DOMCTL_MONITOR_OP_ENABLE: > + case XEN_DOMCTL_MONITOR_OP_DISABLE: > + /* Check if event type is available. */ > + /* sanity check: avoid left-shift undefined behavior */ > + if ( unlikely(mop->event > 31) ) > + return -EINVAL; > + if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) > + return -EOPNOTSUPP; > + /* Arch-side handles enable/disable ops. */ > + return arch_monitor_domctl_event(d, mop); > + > + case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: > + mop->event = arch_monitor_get_capabilities(d); > + return 0; > + > + default: > + /* The monitor op is probably handled on the arch-side. */ > + return arch_monitor_domctl_op(d, mop); > + } > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h > index a3a9703..82a4a66 100644 > --- a/xen/include/asm-arm/monitor.h > +++ b/xen/include/asm-arm/monitor.h > @@ -1,9 +1,10 @@ > /* > * include/asm-arm/monitor.h > * > - * Architecture-specific monitor_op domctl handler. > + * Arch-specific monitor_op domctl handler. > * > * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public > @@ -24,10 +25,31 @@ > #include <xen/sched.h> > #include <public/domctl.h> > > +static inline uint32_t arch_monitor_get_capabilities(struct domain *d) > +{ > + /* No monitor vm-events implemented on ARM. */ > + return 0; > +} > + > +static inline > +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) > +{ > + /* No arch-specific monitor ops on ARM. */ > + return -EOPNOTSUPP; > +} > + > static inline > -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op) > +int arch_monitor_domctl_event(struct domain *d, > + struct xen_domctl_monitor_op *mop) > { > - return -ENOSYS; > + /* > + * No arch-specific monitor vm-events on ARM. > + * > + * Should not be reached unless arch_monitor_get_capabilities() is not > + * properly implemented. > + */ > + ASSERT_UNREACHABLE(); > + return -EOPNOTSUPP; > } > > -#endif /* __ASM_X86_MONITOR_H__ */ > +#endif /* __ASM_ARM_MONITOR_H__ */ > diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h > index 7c8280b..c789f71 100644 > --- a/xen/include/asm-x86/monitor.h > +++ b/xen/include/asm-x86/monitor.h > @@ -1,9 +1,10 @@ > /* > * include/asm-x86/monitor.h > * > - * Architecture-specific monitor_op domctl handler. > + * Arch-specific monitor_op domctl handler. > * > * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public > @@ -21,11 +22,55 @@ > #ifndef __ASM_X86_MONITOR_H__ > #define __ASM_X86_MONITOR_H__ > > -struct domain; > -struct xen_domctl_monitor_op; > +#include <xen/sched.h> > +#include <public/domctl.h> > +#include <asm/cpufeature.h> > +#include <asm/hvm/hvm.h> > > #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) > > -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); > +static inline uint32_t arch_monitor_get_capabilities(struct domain *d) > +{ > + uint32_t capabilities = 0; > + > + /* > + * At the moment only Intel HVM domains are supported. However, event > + * delivery could be extended to AMD and PV domains. > + */ > + if ( !is_hvm_domain(d) || !cpu_has_vmx ) > + return capabilities; > + > + capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | > + (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | > + (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | > + (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); > + > + /* Since we know this is on VMX, we can just call the hvm func */ > + if ( hvm_is_singlestep_supported() ) > + capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); > + > + return capabilities; > +} > + > +static inline > +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) > +{ > + switch ( mop->op ) > + { > + case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: > + domain_pause(d); > + d->arch.mem_access_emulate_each_rep = !!mop->event; > + domain_unpause(d); > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +int arch_monitor_domctl_event(struct domain *d, > + struct xen_domctl_monitor_op *mop); > > #endif /* __ASM_X86_MONITOR_H__ */ > diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h > new file mode 100644 > index 0000000..7015e6d > --- /dev/null > +++ b/xen/include/xen/monitor.h > @@ -0,0 +1,30 @@ > +/* > + * include/xen/monitor.h > + * > + * Common monitor_op domctl handler. > + * > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > + * > + * 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/>. > + */ > + > +#ifndef __XEN_MONITOR_H__ > +#define __XEN_MONITOR_H__ > + > +struct domain; > +struct xen_domctl_monitor_op; > + > +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); > + > +#endif /* __XEN_MONITOR_H__ */ > -- > 2.5.0 >
On 2/16/2016 12:45 PM, Jan Beulich wrote: >>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote: >> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote: >>> This patch moves monitor_domctl to common-side. >>> Purpose: move what's common to common, prepare for implementation >>> of such vm-events on ARM. >>> >>> * move get_capabilities to arch-side => arch_monitor_get_capabilities. >>> * add arch-side monitor op handling function => arch_monitor_domctl_op. >>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op >>> * add arch-side monitor event handling function => arch_monitor_domctl_event. >>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event >> enable/disable >>> * remove status_check >>> >>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >>> >>> --- >>> Changed since v3: >>> * monitor_domctl @ common/monitor.c: >>> - remove unused requested_status >>> - sanity check mop->event range to avoid left-shift undefined behavior >> Due to left-shift undefined behavior situations, shouldn't I also: >> >> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<' > There's no undefinedness there, since the right side operands of > << are all constant. Using 1U here would be okay, but is not > strictly needed. I reasoned based on this ISO C99 quote: [for an E1 << E2 operation, ] "If E1 has a signed type and nonnegative value, and E1 × 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined." I inferred that this means that code such as '(1 << 31)' would render undefined behavior, since (1 x 2^31) is not representable on 'int'. The standard doesn't seem to mention different behavior if the operands are constants. This would render undefined behavior if bit 31 of capabilities would be used @ some point, i.e. if one day someone would e.g. unknowingly: #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31 Have I misinterpreted the 'representable in the result type' part? > >> * in X86 arch_monitor_domctl_event, >> XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case >> add a sanity check of mop->u.mov_to_cr.index before: >> unsigned int ctrlreg_bitmask = >> monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); >> , which basically translates to: >> unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index); >> >> ? (especially since mop->u.mov_to_cr.index is set by the caller). > Yes, there a range check would be needed, but preferably as a > separate patch (as this has nothing to do with the code motion > you perform here). > > Jan > > Great, I'll do these changes in a separate patch then. Let me know if you have any other comments on this. Thanks, Corneliu.
>>> On 16.02.16 at 12:20, <czuzu@bitdefender.com> wrote: > On 2/16/2016 12:45 PM, Jan Beulich wrote: >>>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote: >>> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote: >>>> This patch moves monitor_domctl to common-side. >>>> Purpose: move what's common to common, prepare for implementation >>>> of such vm-events on ARM. >>>> >>>> * move get_capabilities to arch-side => arch_monitor_get_capabilities. >>>> * add arch-side monitor op handling function => arch_monitor_domctl_op. >>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op >>>> * add arch-side monitor event handling function => arch_monitor_domctl_event. >>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event >>> enable/disable >>>> * remove status_check >>>> >>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >>>> >>>> --- >>>> Changed since v3: >>>> * monitor_domctl @ common/monitor.c: >>>> - remove unused requested_status >>>> - sanity check mop->event range to avoid left-shift undefined behavior >>> Due to left-shift undefined behavior situations, shouldn't I also: >>> >>> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<' >> There's no undefinedness there, since the right side operands of >> << are all constant. Using 1U here would be okay, but is not >> strictly needed. > > I reasoned based on this ISO C99 quote: > [for an E1 << E2 operation, ] > "If E1 has a signed type and nonnegative value, and E1 × 2^E2 is > representable in the result type, then that is the resulting value; > otherwise, the behavior is undefined." > > I inferred that this means that code such as '(1 << 31)' would render > undefined behavior, since (1 x 2^31) is not representable on 'int'. > The standard doesn't seem to mention different behavior if the operands > are constants. > This would render undefined behavior if bit 31 of capabilities would be > used @ some point, i.e. if one day someone would e.g. unknowingly: > #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31 > Have I misinterpreted the 'representable in the result type' part? No, that's all correct. It's just that right now no XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence there's only a very minor latent issue here (someone blindly copying the existing 1 << ... without adding the necessary U at that point; one might hope the compiler would then point this out though). Jan
On 2/16/2016 2:34 PM, Jan Beulich wrote: >>>> On 16.02.16 at 12:20, <czuzu@bitdefender.com> wrote: >> On 2/16/2016 12:45 PM, Jan Beulich wrote: >>>>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote: >>>> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote: >>>>> This patch moves monitor_domctl to common-side. >>>>> Purpose: move what's common to common, prepare for implementation >>>>> of such vm-events on ARM. >>>>> >>>>> * move get_capabilities to arch-side => arch_monitor_get_capabilities. >>>>> * add arch-side monitor op handling function => arch_monitor_domctl_op. >>>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op >>>>> * add arch-side monitor event handling function => arch_monitor_domctl_event. >>>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event >>>> enable/disable >>>>> * remove status_check >>>>> >>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >>>>> >>>>> --- >>>>> Changed since v3: >>>>> * monitor_domctl @ common/monitor.c: >>>>> - remove unused requested_status >>>>> - sanity check mop->event range to avoid left-shift undefined behavior >>>> Due to left-shift undefined behavior situations, shouldn't I also: >>>> >>>> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<' >>> There's no undefinedness there, since the right side operands of >>> << are all constant. Using 1U here would be okay, but is not >>> strictly needed. >> I reasoned based on this ISO C99 quote: >> [for an E1 << E2 operation, ] >> "If E1 has a signed type and nonnegative value, and E1 × 2^E2 is >> representable in the result type, then that is the resulting value; >> otherwise, the behavior is undefined." >> >> I inferred that this means that code such as '(1 << 31)' would render >> undefined behavior, since (1 x 2^31) is not representable on 'int'. >> The standard doesn't seem to mention different behavior if the operands >> are constants. >> This would render undefined behavior if bit 31 of capabilities would be >> used @ some point, i.e. if one day someone would e.g. unknowingly: >> #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31 >> Have I misinterpreted the 'representable in the result type' part? > No, that's all correct. It's just that right now no > XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence > there's only a very minor latent issue here (someone blindly > copying the existing 1 << ... without adding the necessary U at > that point; one might hope the compiler would then point this out > though). > > Jan > Ah, I see. Did a fast test earlier w/ GCC 5.1, unfortunately I think the compiler doesn't issue any warning in this situation, would have preferred a heads-up too (couldn't even force it to do so). (it would be nice if Xen shipped w/ a gravitational-waves detector though....) Corneliu.
On 02/16/2016 09:08 AM, Corneliu ZUZU wrote: > This patch moves monitor_domctl to common-side. > Purpose: move what's common to common, prepare for implementation > of such vm-events on ARM. > > * move get_capabilities to arch-side => arch_monitor_get_capabilities. > * add arch-side monitor op handling function => arch_monitor_domctl_op. > e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op > * add arch-side monitor event handling function => arch_monitor_domctl_event. > e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable > * remove status_check > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > > --- > Changed since v3: > * monitor_domctl @ common/monitor.c: > - remove unused requested_status > - sanity check mop->event range to avoid left-shift undefined behavior > * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning > * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef > --- > MAINTAINERS | 1 + > xen/arch/x86/monitor.c | 153 +++++++++++------------------------------- > xen/common/Makefile | 1 + > xen/common/domctl.c | 2 +- > xen/common/monitor.c | 69 +++++++++++++++++++ > xen/include/asm-arm/monitor.h | 30 +++++++-- > xen/include/asm-x86/monitor.h | 53 +++++++++++++-- > xen/include/xen/monitor.h | 30 +++++++++ > 8 files changed, 217 insertions(+), 122 deletions(-) > create mode 100644 xen/common/monitor.c > create mode 100644 xen/include/xen/monitor.h Fair enough. Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Thanks, Razvan
> > > @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct > xen_domctl_monitor_op *mop) > > case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: > { > So since we will now have two separate booleans, requested_status and old_status and then manually verify they are opposite.. > - bool_t status = ad->monitor.mov_to_msr_enabled; > + bool_t old_status = ad->monitor.mov_to_msr_enabled; > ...here we should set the field to requested_status, not !old_status. While they are technically equivalent, the code would read better to other way around. > > - ad->monitor.mov_to_msr_enabled = !status; > + ad->monitor.mov_to_msr_enabled = !old_status; domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: > { > - bool_t status = ad->monitor.singlestep_enabled; > + bool_t old_status = ad->monitor.singlestep_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > Here as well.. > - ad->monitor.singlestep_enabled = !status; > + ad->monitor.singlestep_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: > { > - bool_t status = ad->monitor.software_breakpoint_enabled; > + bool_t old_status = ad->monitor.software_breakpoint_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > ..and here.. > - ad->monitor.software_breakpoint_enabled = !status; > + ad->monitor.software_breakpoint_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: > { > - bool_t status = ad->monitor.guest_request_enabled; > + bool_t old_status = ad->monitor.guest_request_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > ad->monitor.guest_request_sync = mop->u.guest_request.sync; > ..and here. > - ad->monitor.guest_request_enabled = !status; > + ad->monitor.guest_request_enabled = !old_status; > domain_unpause(d); > break; > } > Otherwise the patch looks good. Thanks, Tamas
On 2/16/2016 6:02 PM, Tamas K Lengyel wrote: > > > @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct > xen_domctl_monitor_op *mop) > > case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: > { > > > So since we will now have two separate booleans, requested_status and > old_status and then manually verify they are opposite.. > > - bool_t status = ad->monitor.mov_to_msr_enabled; > + bool_t old_status = ad->monitor.mov_to_msr_enabled; > > > ...here we should set the field to requested_status, not !old_status. > While they are technically equivalent, the code would read better to > other way around. > > > - ad->monitor.mov_to_msr_enabled = !status; > + ad->monitor.mov_to_msr_enabled = !old_status; > > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: > { > - bool_t status = ad->monitor.singlestep_enabled; > + bool_t old_status = ad->monitor.singlestep_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > > > Here as well.. > > - ad->monitor.singlestep_enabled = !status; > + ad->monitor.singlestep_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: > { > - bool_t status = ad->monitor.software_breakpoint_enabled; > + bool_t old_status = ad->monitor.software_breakpoint_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > > > ..and here.. > > - ad->monitor.software_breakpoint_enabled = !status; > + ad->monitor.software_breakpoint_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: > { > - bool_t status = ad->monitor.guest_request_enabled; > + bool_t old_status = ad->monitor.guest_request_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > ad->monitor.guest_request_sync = mop->u.guest_request.sync; > > > ..and here. > > - ad->monitor.guest_request_enabled = !status; > + ad->monitor.guest_request_enabled = !old_status; > domain_unpause(d); > break; > } > > > Otherwise the patch looks good. > > Thanks, > Tamas > Oh, right, that would look better. Shall I send a v5 then with that change? (and if yes I guess it won't hurt if I also include the left-shift sanity checks I mentioned I should have added (?)) Thanks, Corneliu.
On Tue, Feb 16, 2016 at 10:48 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 2/16/2016 6:02 PM, Tamas K Lengyel wrote: > > >> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct >> xen_domctl_monitor_op *mop) >> >> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: >> { >> > > So since we will now have two separate booleans, requested_status and > old_status and then manually verify they are opposite.. > > >> - bool_t status = ad->monitor.mov_to_msr_enabled; >> + bool_t old_status = ad->monitor.mov_to_msr_enabled; >> > > ...here we should set the field to requested_status, not !old_status. > While they are technically equivalent, the code would read better to other > way around. > > >> >> - ad->monitor.mov_to_msr_enabled = !status; >> + ad->monitor.mov_to_msr_enabled = !old_status; > > domain_unpause(d); >> break; >> } >> >> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: >> { >> - bool_t status = ad->monitor.singlestep_enabled; >> + bool_t old_status = ad->monitor.singlestep_enabled; >> >> - rc = status_check(mop, status); >> - if ( rc ) >> - return rc; >> + if ( unlikely(old_status == requested_status) ) >> + return -EEXIST; >> >> domain_pause(d); >> > > Here as well.. > > >> - ad->monitor.singlestep_enabled = !status; >> + ad->monitor.singlestep_enabled = !old_status; >> domain_unpause(d); >> break; >> } >> >> case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: >> { >> - bool_t status = ad->monitor.software_breakpoint_enabled; >> + bool_t old_status = ad->monitor.software_breakpoint_enabled; >> >> - rc = status_check(mop, status); >> - if ( rc ) >> - return rc; >> + if ( unlikely(old_status == requested_status) ) >> + return -EEXIST; >> >> domain_pause(d); >> > > ..and here.. > > >> - ad->monitor.software_breakpoint_enabled = !status; >> + ad->monitor.software_breakpoint_enabled = !old_status; >> domain_unpause(d); >> break; >> } >> >> case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: >> { >> - bool_t status = ad->monitor.guest_request_enabled; >> + bool_t old_status = ad->monitor.guest_request_enabled; >> >> - rc = status_check(mop, status); >> - if ( rc ) >> - return rc; >> + if ( unlikely(old_status == requested_status) ) >> + return -EEXIST; >> >> domain_pause(d); >> ad->monitor.guest_request_sync = mop->u.guest_request.sync; >> > > ..and here. > > >> - ad->monitor.guest_request_enabled = !status; >> + ad->monitor.guest_request_enabled = !old_status; >> domain_unpause(d); >> break; >> } >> > > Otherwise the patch looks good. > > Thanks, > Tamas > > > Oh, right, that would look better. Shall I send a v5 then with that > change? (and if yes I guess it won't hurt if I also include the left-shift > sanity checks I mentioned I should have added (?)) > Please do send another revision with these changes. As I understood Jan prefers the sanity-checks to be added as a separate patch, so do that. Thanks, Tamas
diff --git a/MAINTAINERS b/MAINTAINERS index f07384c..5cbb1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -355,6 +355,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com> S: Supported F: xen/common/vm_event.c F: xen/common/mem_access.c +F: xen/common/monitor.c F: xen/arch/x86/hvm/event.c F: xen/arch/x86/monitor.c F: xen/arch/*/vm_event.c diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 1d43880..660b92c 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -1,9 +1,10 @@ /* * arch/x86/monitor.c * - * Architecture-specific monitor_op domctl handler. + * Arch-specific monitor_op domctl handler. * * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public @@ -18,87 +19,14 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <xen/config.h> -#include <xen/sched.h> -#include <xen/mm.h> -#include <asm/domain.h> #include <asm/monitor.h> -#include <public/domctl.h> -#include <xsm/xsm.h> +#include <public/vm_event.h> -/* - * Sanity check whether option is already enabled/disabled - */ -static inline -int status_check(struct xen_domctl_monitor_op *mop, bool_t status) -{ - bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE); - - if ( status == requested_status ) - return -EEXIST; - - return 0; -} - -static inline uint32_t get_capabilities(struct domain *d) -{ - uint32_t capabilities = 0; - - /* - * At the moment only Intel HVM domains are supported. However, event - * delivery could be extended to AMD and PV domains. - */ - if ( !is_hvm_domain(d) || !cpu_has_vmx ) - return capabilities; - - capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | - (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | - (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | - (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); - - /* Since we know this is on VMX, we can just call the hvm func */ - if ( hvm_is_singlestep_supported() ) - capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); - - return capabilities; -} - -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) +int arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop) { - int rc; struct arch_domain *ad = &d->arch; - uint32_t capabilities = get_capabilities(d); - - if ( current->domain == d ) /* no domain_pause() */ - return -EPERM; - - rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event); - if ( rc ) - return rc; - - switch ( mop->op ) - { - case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: - mop->event = capabilities; - return 0; - - case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: - domain_pause(d); - ad->mem_access_emulate_each_rep = !!mop->event; - domain_unpause(d); - return 0; - } - - /* - * Sanity check - */ - if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE && - mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE ) - return -EOPNOTSUPP; - - /* Check if event type is available. */ - if ( !(capabilities & (1 << mop->event)) ) - return -EOPNOTSUPP; + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); switch ( mop->event ) { @@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) { unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); - bool_t status = + bool_t old_status = !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask); - struct vcpu *v; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); @@ -126,15 +52,18 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) else ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; - if ( !status ) + if ( !old_status ) ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; else ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; - if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 ) - /* Latches new CR3 mask through CR0 code */ + if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index ) + { + struct vcpu *v; + /* Latches new CR3 mask through CR0 code. */ for_each_vcpu ( d, v ) hvm_update_guest_cr(v, 0); + } domain_unpause(d); @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: { - bool_t status = ad->monitor.mov_to_msr_enabled; + bool_t old_status = ad->monitor.mov_to_msr_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && - mop->u.mov_to_msr.extended_capture && + if ( requested_status && mop->u.mov_to_msr.extended_capture && !hvm_enable_msr_exit_interception(d) ) return -EOPNOTSUPP; domain_pause(d); - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && - mop->u.mov_to_msr.extended_capture ) - ad->monitor.mov_to_msr_extended = 1; + if ( requested_status && mop->u.mov_to_msr.extended_capture ) + ad->monitor.mov_to_msr_extended = 1; else ad->monitor.mov_to_msr_extended = 0; - ad->monitor.mov_to_msr_enabled = !status; + ad->monitor.mov_to_msr_enabled = !old_status; domain_unpause(d); break; } case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: { - bool_t status = ad->monitor.singlestep_enabled; + bool_t old_status = ad->monitor.singlestep_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); - ad->monitor.singlestep_enabled = !status; + ad->monitor.singlestep_enabled = !old_status; domain_unpause(d); break; } case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: { - bool_t status = ad->monitor.software_breakpoint_enabled; + bool_t old_status = ad->monitor.software_breakpoint_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); - ad->monitor.software_breakpoint_enabled = !status; + ad->monitor.software_breakpoint_enabled = !old_status; domain_unpause(d); break; } case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: { - bool_t status = ad->monitor.guest_request_enabled; + bool_t old_status = ad->monitor.guest_request_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); ad->monitor.guest_request_sync = mop->u.guest_request.sync; - ad->monitor.guest_request_enabled = !status; + ad->monitor.guest_request_enabled = !old_status; domain_unpause(d); break; } default: + /* + * Should not be reached unless arch_monitor_get_capabilities() is not + * properly implemented. + */ + ASSERT_UNREACHABLE(); return -EOPNOTSUPP; - - }; + } return 0; } diff --git a/xen/common/Makefile b/xen/common/Makefile index 6e82b33..0d76efe 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -20,6 +20,7 @@ obj-y += lib.o obj-y += lzo.o obj-$(CONFIG_HAS_MEM_ACCESS) += mem_access.o obj-y += memory.o +obj-y += monitor.o obj-y += multicall.o obj-y += notifier.o obj-y += page_alloc.o diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 22fa5d5..b34c0a1 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -25,11 +25,11 @@ #include <xen/paging.h> #include <xen/hypercall.h> #include <xen/vm_event.h> +#include <xen/monitor.h> #include <asm/current.h> #include <asm/irq.h> #include <asm/page.h> #include <asm/p2m.h> -#include <asm/monitor.h> #include <public/domctl.h> #include <xsm/xsm.h> diff --git a/xen/common/monitor.c b/xen/common/monitor.c new file mode 100644 index 0000000..2a99a04 --- /dev/null +++ b/xen/common/monitor.c @@ -0,0 +1,69 @@ +/* + * xen/common/monitor.c + * + * Common monitor_op domctl handler. + * + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. + * + * 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/monitor.h> +#include <xen/sched.h> +#include <xsm/xsm.h> +#include <public/domctl.h> +#include <asm/monitor.h> + +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) +{ + int rc; + + if ( unlikely(current->domain == d) ) /* no domain_pause() */ + return -EPERM; + + rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event); + if ( unlikely(rc) ) + return rc; + + switch ( mop->op ) + { + case XEN_DOMCTL_MONITOR_OP_ENABLE: + case XEN_DOMCTL_MONITOR_OP_DISABLE: + /* Check if event type is available. */ + /* sanity check: avoid left-shift undefined behavior */ + if ( unlikely(mop->event > 31) ) + return -EINVAL; + if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) + return -EOPNOTSUPP; + /* Arch-side handles enable/disable ops. */ + return arch_monitor_domctl_event(d, mop); + + case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: + mop->event = arch_monitor_get_capabilities(d); + return 0; + + default: + /* The monitor op is probably handled on the arch-side. */ + return arch_monitor_domctl_op(d, mop); + } +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index a3a9703..82a4a66 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -1,9 +1,10 @@ /* * include/asm-arm/monitor.h * - * Architecture-specific monitor_op domctl handler. + * Arch-specific monitor_op domctl handler. * * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public @@ -24,10 +25,31 @@ #include <xen/sched.h> #include <public/domctl.h> +static inline uint32_t arch_monitor_get_capabilities(struct domain *d) +{ + /* No monitor vm-events implemented on ARM. */ + return 0; +} + +static inline +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) +{ + /* No arch-specific monitor ops on ARM. */ + return -EOPNOTSUPP; +} + static inline -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op) +int arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop) { - return -ENOSYS; + /* + * No arch-specific monitor vm-events on ARM. + * + * Should not be reached unless arch_monitor_get_capabilities() is not + * properly implemented. + */ + ASSERT_UNREACHABLE(); + return -EOPNOTSUPP; } -#endif /* __ASM_X86_MONITOR_H__ */ +#endif /* __ASM_ARM_MONITOR_H__ */ diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 7c8280b..c789f71 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -1,9 +1,10 @@ /* * include/asm-x86/monitor.h * - * Architecture-specific monitor_op domctl handler. + * Arch-specific monitor_op domctl handler. * * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public @@ -21,11 +22,55 @@ #ifndef __ASM_X86_MONITOR_H__ #define __ASM_X86_MONITOR_H__ -struct domain; -struct xen_domctl_monitor_op; +#include <xen/sched.h> +#include <public/domctl.h> +#include <asm/cpufeature.h> +#include <asm/hvm/hvm.h> #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); +static inline uint32_t arch_monitor_get_capabilities(struct domain *d) +{ + uint32_t capabilities = 0; + + /* + * At the moment only Intel HVM domains are supported. However, event + * delivery could be extended to AMD and PV domains. + */ + if ( !is_hvm_domain(d) || !cpu_has_vmx ) + return capabilities; + + capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | + (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | + (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | + (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); + + /* Since we know this is on VMX, we can just call the hvm func */ + if ( hvm_is_singlestep_supported() ) + capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); + + return capabilities; +} + +static inline +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) +{ + switch ( mop->op ) + { + case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: + domain_pause(d); + d->arch.mem_access_emulate_each_rep = !!mop->event; + domain_unpause(d); + break; + + default: + return -EOPNOTSUPP; + } + + return 0; +} + +int arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop); #endif /* __ASM_X86_MONITOR_H__ */ diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h new file mode 100644 index 0000000..7015e6d --- /dev/null +++ b/xen/include/xen/monitor.h @@ -0,0 +1,30 @@ +/* + * include/xen/monitor.h + * + * Common monitor_op domctl handler. + * + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. + * + * 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/>. + */ + +#ifndef __XEN_MONITOR_H__ +#define __XEN_MONITOR_H__ + +struct domain; +struct xen_domctl_monitor_op; + +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); + +#endif /* __XEN_MONITOR_H__ */
This patch moves monitor_domctl to common-side. Purpose: move what's common to common, prepare for implementation of such vm-events on ARM. * move get_capabilities to arch-side => arch_monitor_get_capabilities. * add arch-side monitor op handling function => arch_monitor_domctl_op. e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op * add arch-side monitor event handling function => arch_monitor_domctl_event. e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable * remove status_check Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- Changed since v3: * monitor_domctl @ common/monitor.c: - remove unused requested_status - sanity check mop->event range to avoid left-shift undefined behavior * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef --- MAINTAINERS | 1 + xen/arch/x86/monitor.c | 153 +++++++++++------------------------------- xen/common/Makefile | 1 + xen/common/domctl.c | 2 +- xen/common/monitor.c | 69 +++++++++++++++++++ xen/include/asm-arm/monitor.h | 30 +++++++-- xen/include/asm-x86/monitor.h | 53 +++++++++++++-- xen/include/xen/monitor.h | 30 +++++++++ 8 files changed, 217 insertions(+), 122 deletions(-) create mode 100644 xen/common/monitor.c create mode 100644 xen/include/xen/monitor.h