| Message ID | 1455518254-507-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2/15/2016 8:37 AM, Corneliu ZUZU wrote: > diff --git a/xen/common/monitor.c b/xen/common/monitor.c > new file mode 100644 > index 0000000..b708cab > --- /dev/null > +++ b/xen/common/monitor.c > + int rc; > + bool_t requested_status = 0; > + > + 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: > + requested_status = 1; > + /* fallthrough */ > + case XEN_DOMCTL_MONITOR_OP_DISABLE: > + /* Check if event type is available. */ > + if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) ) > + return -EOPNOTSUPP; > + /* Arch-side handles enable/disable ops. */ > + return arch_monitor_domctl_event(d, mop); > Only noticed now, requested_status now became unused in this function. Will remove in v4. Corneliu.
>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote: > default: > - return -EOPNOTSUPP; > + /* > + * Should not be reached unless arch_monitor_get_capabilities() is not > + * properly implemented. In that case, since reaching this point does > + * not really break anything, don't crash the hypervisor, issue a > + * warning instead of BUG(). > + */ > + printk(XENLOG_WARNING > + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" > + "properly.\n"); > > - }; > + return -EOPNOTSUPP; > + } I disagree with the issuing of a message here. At the very least this should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be the way to go? What's worse though is that I can't see the checking which would make true the "should not be reached" statement above (not that you must not rely on the caller of the hypercall to be well behaved). > --- /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 __MONITOR_H__ > +#define __MONITOR_H__ __XEN_MONITOR_H__ please. > +#include <xen/sched.h> > +#include <public/domctl.h> > + > +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); Neither of the two includes seem necessary for this prototype, all you need are forward declarations of the two involved structures. Jan
On 2/15/2016 1:41 PM, Jan Beulich wrote: >>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote: >> default: >> - return -EOPNOTSUPP; >> + /* >> + * Should not be reached unless arch_monitor_get_capabilities() is not >> + * properly implemented. In that case, since reaching this point does >> + * not really break anything, don't crash the hypervisor, issue a >> + * warning instead of BUG(). >> + */ >> + printk(XENLOG_WARNING >> + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" >> + "properly.\n"); >> >> - }; >> + return -EOPNOTSUPP; >> + } > I disagree with the issuing of a message here. At the very least this > should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be > the way to go? IDK, but I agree that it doesn't look so elegant like that. Won't ASSERT_UNREACHABLE crash the hypervisor? > What's worse though is that I can't see the checking > which would make true the "should not be reached" statement above > (not that you must not rely on the caller of the hypercall to be well > behaved). The reasoning is as follows. From this part in monitor_domctl: switch ( mop->op ) { case XEN_DOMCTL_MONITOR_OP_ENABLE: case XEN_DOMCTL_MONITOR_OP_DISABLE: /* Check if event type is available. */ if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) ) return -EOPNOTSUPP; /* Arch-side handles enable/disable ops. */ return arch_monitor_domctl_event(d, mop); we can see that arch_monitor_domctl_event gets to be called only when arch_monitor_get_capabilities reports an 'available' mop->event. But if then in arch_monitor_domctl_event the default case is reached, it would mean that arch_monitor_get_capabilities reported a mop->event as being available/supported when is *not actually handled* anywhere. > >> --- /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 __MONITOR_H__ >> +#define __MONITOR_H__ > __XEN_MONITOR_H__ please. > >> +#include <xen/sched.h> >> +#include <public/domctl.h> >> + >> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); > Neither of the two includes seem necessary for this prototype, all > you need are forward declarations of the two involved structures. > > Jan Noted. Corneliu.
On 2/15/2016 1:41 PM, Jan Beulich wrote: >> +++ 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 __MONITOR_H__ >> +#define __MONITOR_H__ > __XEN_MONITOR_H__ please. Hadn't noticed this comment, also noted. Thanks, Corneliu.
On Mon, 15 Feb 2016, Jan Beulich wrote: > >>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote: > > default: > > - return -EOPNOTSUPP; > > + /* > > + * Should not be reached unless arch_monitor_get_capabilities() is not > > + * properly implemented. In that case, since reaching this point does > > + * not really break anything, don't crash the hypervisor, issue a > > + * warning instead of BUG(). > > + */ > > + printk(XENLOG_WARNING > > + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" > > + "properly.\n"); > > > > - }; > > + return -EOPNOTSUPP; > > + } > > I disagree with the issuing of a message here. At the very least this > should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be > the way to go? What's worse though is that I can't see the checking > which would make true the "should not be reached" statement above > (not that you must not rely on the caller of the hypercall to be well > behaved). ASSERT_UNREACHABLE() is appropriate here
On 2/15/2016 2:25 PM, Stefano Stabellini wrote: > On Mon, 15 Feb 2016, Jan Beulich wrote: >>>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote: >>> default: >>> - return -EOPNOTSUPP; >>> + /* >>> + * Should not be reached unless arch_monitor_get_capabilities() is not >>> + * properly implemented. In that case, since reaching this point does >>> + * not really break anything, don't crash the hypervisor, issue a >>> + * warning instead of BUG(). >>> + */ >>> + printk(XENLOG_WARNING >>> + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" >>> + "properly.\n"); >>> >>> - }; >>> + return -EOPNOTSUPP; >>> + } >> I disagree with the issuing of a message here. At the very least this >> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be >> the way to go? What's worse though is that I can't see the checking >> which would make true the "should not be reached" statement above >> (not that you must not rely on the caller of the hypercall to be well >> behaved). > ASSERT_UNREACHABLE() is appropriate here > Noted. Corneliu.
>>> On 15.02.16 at 13:14, <czuzu@bitdefender.com> wrote: > On 2/15/2016 1:41 PM, Jan Beulich wrote: >>>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote: >>> default: >>> - return -EOPNOTSUPP; >>> + /* >>> + * Should not be reached unless arch_monitor_get_capabilities() is not >>> + * properly implemented. In that case, since reaching this point does >>> + * not really break anything, don't crash the hypervisor, issue a >>> + * warning instead of BUG(). >>> + */ >>> + printk(XENLOG_WARNING >>> + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" >>> + "properly.\n"); >>> >>> - }; >>> + return -EOPNOTSUPP; >>> + } >> I disagree with the issuing of a message here. At the very least this >> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be >> the way to go? > > IDK, but I agree that it doesn't look so elegant like that. > Won't ASSERT_UNREACHABLE crash the hypervisor? In a debug build yes. In a release build no. >> What's worse though is that I can't see the checking >> which would make true the "should not be reached" statement above >> (not that you must not rely on the caller of the hypercall to be well >> behaved). > > The reasoning is as follows. > From this part in monitor_domctl: > > switch ( mop->op ) > { > case XEN_DOMCTL_MONITOR_OP_ENABLE: > case XEN_DOMCTL_MONITOR_OP_DISABLE: > /* Check if event type is available. */ > if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) ) > return -EOPNOTSUPP; > /* Arch-side handles enable/disable ops. */ > return arch_monitor_domctl_event(d, mop); > > we can see that arch_monitor_domctl_event gets to be called only when > arch_monitor_get_capabilities reports > an 'available' mop->event. > But if then in arch_monitor_domctl_event the default case is reached, it > would mean > that arch_monitor_get_capabilities reported a mop->event as being > available/supported > when is *not actually handled* anywhere. Ah, I see now that I've mis-read the default: code further below, which actually calls arch_monitor_domctl_op(), not ..._event(). However, there's an "undefined behavior" issue with the code above then when mop->event >= 31 - I think you want to left shift 1U instead of plain 1, and you need to range check mop->event first. Jan
On 2/15/2016 2:44 PM, Jan Beulich wrote: > >> switch ( mop->op ) >> { >> case XEN_DOMCTL_MONITOR_OP_ENABLE: >> case XEN_DOMCTL_MONITOR_OP_DISABLE: >> /* Check if event type is available. */ >> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) ) >> return -EOPNOTSUPP; >> /* Arch-side handles enable/disable ops. */ >> return arch_monitor_domctl_event(d, mop); > Ah, I see now that I've mis-read the default: code further below, > which actually calls arch_monitor_domctl_op(), not ..._event(). > However, there's an "undefined behavior" issue with the code > above then when mop->event >= 31 - I think you want to left > shift 1U instead of plain 1, and you need to range check > mop->event first. > > Jan > Never looked @ that part before, used it the way it was. I suppose that's because "according to the C specification, the result of a bit shift operation on a signed argument gives implementation-defined results, so in/theory/|1U << i|is more portable than|1 << i|" (taken from a stackoverflow post). After changing 1 to 1U though, I don't understand why we should also range-check mop->event. I'm imagining when (mop->event > 31): * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) would be = 0 * in which case we would return -EOPNOTSUPP , no? Corneliu.
>>> On 15.02.16 at 14:29, <czuzu@bitdefender.com> wrote: > On 2/15/2016 2:44 PM, Jan Beulich wrote: >> >>> switch ( mop->op ) >>> { >>> case XEN_DOMCTL_MONITOR_OP_ENABLE: >>> case XEN_DOMCTL_MONITOR_OP_DISABLE: >>> /* Check if event type is available. */ >>> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) ) >>> return -EOPNOTSUPP; >>> /* Arch-side handles enable/disable ops. */ >>> return arch_monitor_domctl_event(d, mop); >> Ah, I see now that I've mis-read the default: code further below, >> which actually calls arch_monitor_domctl_op(), not ..._event(). >> However, there's an "undefined behavior" issue with the code >> above then when mop->event >= 31 - I think you want to left >> shift 1U instead of plain 1, and you need to range check >> mop->event first. >> > Never looked @ that part before, used it the way it was. > I suppose that's because "according to the C specification, the result > of a bit shift > operation on a signed argument gives implementation-defined results, so > in/theory/|1U << i|is > more portable than|1 << i|" (taken from a stackoverflow post). Yes. > After changing 1 to 1U though, I don't understand why we should also > range-check mop->event. > I'm imagining when (mop->event > 31): > * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) No, it's plain undefined. > * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) > would be = 0 > * in which case we would return -EOPNOTSUPP > , no? And even that would be true only today, and would break once bit 31 gets a meaning. Whenever possible we should avoid introducing such latent issues. Jan
On Mon, Feb 15, 2016 at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 15.02.16 at 14:29, <czuzu@bitdefender.com> wrote: > > On 2/15/2016 2:44 PM, Jan Beulich wrote: > >> > >>> switch ( mop->op ) > >>> { > >>> case XEN_DOMCTL_MONITOR_OP_ENABLE: > >>> case XEN_DOMCTL_MONITOR_OP_DISABLE: > >>> /* Check if event type is available. */ > >>> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << > mop->event))) ) > >>> return -EOPNOTSUPP; > >>> /* Arch-side handles enable/disable ops. */ > >>> return arch_monitor_domctl_event(d, mop); > >> Ah, I see now that I've mis-read the default: code further below, > >> which actually calls arch_monitor_domctl_op(), not ..._event(). > >> However, there's an "undefined behavior" issue with the code > >> above then when mop->event >= 31 - I think you want to left > >> shift 1U instead of plain 1, and you need to range check > >> mop->event first. > >> > > Never looked @ that part before, used it the way it was. > > I suppose that's because "according to the C specification, the result > > of a bit shift > > operation on a signed argument gives implementation-defined results, so > > in/theory/|1U << i|is > > more portable than|1 << i|" (taken from a stackoverflow post). > > Yes. > > > After changing 1 to 1U though, I don't understand why we should also > > range-check mop->event. > > I'm imagining when (mop->event > 31): > > * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) > > No, it's plain undefined. > > > * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) > > would be = 0 > > * in which case we would return -EOPNOTSUPP > > , no? > > And even that would be true only today, and would break once > bit 31 gets a meaning. Whenever possible we should avoid > introducing such latent issues. > Ah yes, good catch, +1 for doing this extra check. Thanks, Tamas
On 2/15/2016 4:08 PM, Jan Beulich wrote: > >> After changing 1 to 1U though, I don't understand why we should also >> range-check mop->event. >> I'm imagining when (mop->event > 31): >> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) > No, it's plain undefined. Weirdo C, didn't know that! I've just read http://www.danielvik.com/2010/05/c-language-quirks.html . That's crazy, I can't believe such 'quirks' exist and that I never knew of them. So then, would this do: /* sanity check - avoid '<<' operator undefined behavior */ if ( unlikely(mop->event > 31) ) return -EINVAL; if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) return -EOPNOTSUPP; ? Thanks, Corneliu.
>>> On 15.02.16 at 17:28, <czuzu@bitdefender.com> wrote: > On 2/15/2016 4:08 PM, Jan Beulich wrote: >> >>> After changing 1 to 1U though, I don't understand why we should also >>> range-check mop->event. >>> I'm imagining when (mop->event > 31): >>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) >> No, it's plain undefined. > > Weirdo C, didn't know that! > I've just read http://www.danielvik.com/2010/05/c-language-quirks.html . > That's crazy, I can't believe such 'quirks' exist and that I never knew > of them. > So then, would this do: > > /* sanity check - avoid '<<' operator undefined behavior */ > if ( unlikely(mop->event > 31) ) > return -EINVAL; > if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) > return -EOPNOTSUPP; I'd say -EOPNOTSUPP in both cases, but if the maintainers like -EINVAL better I wouldn't insist on my preference. Jan
On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 15.02.16 at 17:28, <czuzu@bitdefender.com> wrote: > > On 2/15/2016 4:08 PM, Jan Beulich wrote: > >> > >>> After changing 1 to 1U though, I don't understand why we should also > >>> range-check mop->event. > >>> I'm imagining when (mop->event > 31): > >>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) > >> No, it's plain undefined. > > > > Weirdo C, didn't know that! > > I've just read http://www.danielvik.com/2010/05/c-language-quirks.html . > > That's crazy, I can't believe such 'quirks' exist and that I never knew > > of them. > > So then, would this do: > > > > /* sanity check - avoid '<<' operator undefined behavior */ > > if ( unlikely(mop->event > 31) ) > > return -EINVAL; > > if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) > > return -EOPNOTSUPP; > > I'd say -EOPNOTSUPP in both cases, but if the maintainers like > -EINVAL better I wouldn't insist on my preference. > The best approach of course would be if we had __MAX values defined for such enums to check against but that doesn't seem to be part of Xen's coding practice. So in this case I would say leave it as -EINVAL as it's more descriptive of the problem and may even signal to the caller some inherent bug on their side, not just that the requested option is not supported. Thanks, Tamas
On 2/15/2016 6:51 PM, Tamas K Lengyel wrote: > > > On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com > <mailto:JBeulich@suse.com>> wrote: > > >>> On 15.02.16 at 17:28, <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>> wrote: > > On 2/15/2016 4:08 PM, Jan Beulich wrote: > >> > >>> After changing 1 to 1U though, I don't understand why we > should also > >>> range-check mop->event. > >>> I'm imagining when (mop->event > 31): > >>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) > >> No, it's plain undefined. > > > > Weirdo C, didn't know that! > > I've just read > http://www.danielvik.com/2010/05/c-language-quirks.html . > > That's crazy, I can't believe such 'quirks' exist and that I > never knew > > of them. > > So then, would this do: > > > > /* sanity check - avoid '<<' operator undefined behavior */ > > if ( unlikely(mop->event > 31) ) > > return -EINVAL; > > if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << > mop->event))) ) > > return -EOPNOTSUPP; > > I'd say -EOPNOTSUPP in both cases, but if the maintainers like > -EINVAL better I wouldn't insist on my preference. > > > The best approach of course would be if we had __MAX values defined > for such enums to check against but that doesn't seem to be part of > Xen's coding practice. So in this case I would say leave it as -EINVAL > as it's more descriptive of the problem and may even signal to the > caller some inherent bug on their side, not just that the requested > option is not supported. > > Thanks, > Tamas > Good points, noted. Corneliu.
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..069c3c7 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,80 @@ 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: - return -EOPNOTSUPP; + /* + * Should not be reached unless arch_monitor_get_capabilities() is not + * properly implemented. In that case, since reaching this point does + * not really break anything, don't crash the hypervisor, issue a + * warning instead of BUG(). + */ + printk(XENLOG_WARNING + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" + "properly.\n"); - }; + 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..b708cab --- /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; + bool_t requested_status = 0; + + 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: + requested_status = 1; + /* fallthrough */ + case XEN_DOMCTL_MONITOR_OP_DISABLE: + /* Check if event type is available. */ + if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << 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..ef7547e 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,35 @@ #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. In that case, since reaching this point does + * not really break anything, don't crash the hypervisor, issue a + * warning instead of BUG(). + */ + printk(XENLOG_WARNING + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" + "properly.\n"); + + 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..ae49a39 --- /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 __MONITOR_H__ +#define __MONITOR_H__ + +#include <xen/sched.h> +#include <public/domctl.h> + +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); + +#endif /* __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> --- MAINTAINERS | 1 + xen/arch/x86/monitor.c | 158 ++++++++++++------------------------------ xen/common/Makefile | 1 + xen/common/domctl.c | 2 +- xen/common/monitor.c | 69 ++++++++++++++++++ xen/include/asm-arm/monitor.h | 34 +++++++-- xen/include/asm-x86/monitor.h | 53 ++++++++++++-- xen/include/xen/monitor.h | 30 ++++++++ 8 files changed, 226 insertions(+), 122 deletions(-) create mode 100644 xen/common/monitor.c create mode 100644 xen/include/xen/monitor.h