diff mbox

[v3,2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

Message ID 1455518254-507-1-git-send-email-czuzu@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Corneliu ZUZU Feb. 15, 2016, 6:37 a.m. UTC
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

Comments

Corneliu ZUZU Feb. 15, 2016, 8:46 a.m. UTC | #1
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.
Jan Beulich Feb. 15, 2016, 11:41 a.m. UTC | #2
>>> 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
Corneliu ZUZU Feb. 15, 2016, 12:14 p.m. UTC | #3
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.
Corneliu ZUZU Feb. 15, 2016, 12:21 p.m. UTC | #4
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.
Stefano Stabellini Feb. 15, 2016, 12:25 p.m. UTC | #5
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
Corneliu ZUZU Feb. 15, 2016, 12:42 p.m. UTC | #6
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.
Jan Beulich Feb. 15, 2016, 12:44 p.m. UTC | #7
>>> 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
Corneliu ZUZU Feb. 15, 2016, 1:29 p.m. UTC | #8
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.
Jan Beulich Feb. 15, 2016, 2:08 p.m. UTC | #9
>>> 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
Tamas K Lengyel Feb. 15, 2016, 4:15 p.m. UTC | #10
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
Corneliu ZUZU Feb. 15, 2016, 4:28 p.m. UTC | #11
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.
Jan Beulich Feb. 15, 2016, 4:44 p.m. UTC | #12
>>> 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
Tamas K Lengyel Feb. 15, 2016, 4:51 p.m. UTC | #13
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
Corneliu ZUZU Feb. 15, 2016, 4:59 p.m. UTC | #14
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 mbox

Patch

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__ */