From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side. Date: Mon, 15 Feb 2016 14:14:45 +0200 Message-ID: <56C1C135.9070707@bitdefender.com> References: <1455518118-414-1-git-send-email-czuzu@bitdefender.com> <1455518254-507-1-git-send-email-czuzu@bitdefender.com> <56C1C79002000078000D20B8@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56C1C79002000078000D20B8@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tamas K Lengyel , Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , xen-devel@lists.xen.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 2/15/2016 1:41 PM, Jan Beulich wrote: >>>> On 15.02.16 at 07:37, 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 . >> + */ >> + >> +#ifndef __MONITOR_H__ >> +#define __MONITOR_H__ > __XEN_MONITOR_H__ please. > >> +#include >> +#include >> + >> +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.