>>> On 15.02.16 at 13:14, <cz...@bitdefender.com> wrote: > On 2/15/2016 1:41 PM, Jan Beulich wrote: >>>>> On 15.02.16 at 07:37, <cz...@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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel