Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 2/15/2016 1:41 PM, Jan Beulich wrote:
> > > > On 15.02.16 at 07:37, <czuzu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx)
> > + * 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel