From patchwork Thu Jun 23 14:59:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Marek_Marczykowski-G=C3=B3recki?= X-Patchwork-Id: 9195417 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1C0B46075F for ; Thu, 23 Jun 2016 15:01:59 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0C8DA2845C for ; Thu, 23 Jun 2016 15:01:59 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F3EE82845E; Thu, 23 Jun 2016 15:01:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 0EF362845C for ; Thu, 23 Jun 2016 15:01:57 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bG66j-0006Vu-PT; Thu, 23 Jun 2016 14:59:53 +0000 Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bG66i-0006Ve-9U for xen-devel@lists.xen.org; Thu, 23 Jun 2016 14:59:52 +0000 Received: from [85.158.137.68] by server-8.bemta-3.messagelabs.com id 1D/13-03780-769FB675; Thu, 23 Jun 2016 14:59:51 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrFIsWRWlGSWpSXmKPExsXilM8irZv2Mzv cYP0KY4slHxezODB6HN39mymAMYo1My8pvyKBNePbl0PsBR8sKtYce8rcwLhPp4uRi0NIYBWj xObb39m6GDmBnGyJRRf6GEFsFoEFzBLTVsSDFLEIzGOW+DLhCStIQkLAWeLfpg9MEN3rGCVuT XwM1M0BVKUq0dEeDlLDJhAscX3JL7B6EQFdiWcLnoGVMAsUS7z/LQXSKiwwmVHi/voDYIt5Bb Qlvr/ZyAhxxClmia/9hhBxQYmTM5+wQPSWSlw5JQJhSkss/8cBUsEpYCfRcPIlO4gtKqAs0TD jAfMERqFZSJpnITTPQmgGqWAWUJf4M+8SM4awtsSyha+ZIWxbiXXr3rMsYGRfxahRnFpUllqk a2ihl1SUmZ5RkpuYmaNraGCsl5taXJyYnpqTmFSsl5yfu4kRGCf1DAyMOxh/n/Y8xCjJwaQky su4OTtciC8pP6UyI7E4I76oNCe1+BCjDAeHkgSv3g+gnGBRanpqRVpmDjBiYdISHDxKIrymIG ne4oLE3OLMdIjUKUZFKXHeHpCEAEgiozQPrg2WJC4xykoJ8zIyMDAI8RSkFuVmlqDKv2IU52B UEuZtBpnCk5lXAjf9FdBiJqDFd/vBFpckIqSkGhh1a1pnrFi84Nu6qRfyhKasuWPZ9yT1q39x rcnmqsIzdXtk91w4Pcde/eDHS0wlwmz+r93/dB/78tbcn2FSR+zJfPvTfb790U/vz5rPuGzLs XU2383t9mn+4wk4FmRw+3pN4aGsWeEpng+zyyfdqTsz9f3x/6cNLodPPX5Zz8E84qCcBV9Gnr mzEktxRqKhFnNRcSIAS1lXsQ0DAAA= X-Env-Sender: marmarek@invisiblethingslab.com X-Msg-Ref: server-5.tower-31.messagelabs.com!1466693989!43703882!1 X-Originating-IP: [66.111.4.27] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTExLjQuMjcgPT4gODQ2Mw==\n X-StarScan-Received: X-StarScan-Version: 8.46; banners=-,-,- X-VirusChecked: Checked Received: (qmail 46412 invoked from network); 23 Jun 2016 14:59:50 -0000 Received: from out3-smtp.messagingengine.com (HELO out3-smtp.messagingengine.com) (66.111.4.27) by server-5.tower-31.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 23 Jun 2016 14:59:50 -0000 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 9D80B2084E; Thu, 23 Jun 2016 10:59:49 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute4.internal (MEProxy); Thu, 23 Jun 2016 10:59:49 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= invisiblethingslab.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-sasl-enc :x-sasl-enc; s=mesmtp; bh=l5COytCQVyYkSzIMN8giFS8Q+jM=; b=qa3h0q dL/8e7JXtvinie++8xBzkUK4twuvvwQLodAPc5dHft8vJBC80ffOMONAEoZOcKjk wRJZEKy05d+WBang7Y4Vv721yCowkJ1ud5N5GkzFQsR9D5ZkXvg8VqYsZIUaf3WP 5sak5+1pwszEJpxu5YSU0wNFMdsebE/R6iMWw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-sasl-enc :x-sasl-enc; s=smtpout; bh=l5COytCQVyYkSzIMN8giFS8Q+jM=; b=uEmRO ZXqqLvTVBahNbbOirwTXUBraaw4inohetgT8srf4xurjj+qCkxWuFlwlIjx0qcd7 2AWuyW1HfYvfa/Px3RiV63xf2IgWOBLObZQM4M6FxxYieKM6snrT/VI6/4Nfn4ds eSpGOJpXknxgb0tl22KcjPe9kqzwY5v75nWHFo= X-Sasl-enc: 76vcpAhRzyN5IGcFTNOzVwtt4ACPS2pAYAAOefvfH5if 1466693989 Received: from mail-itl (89-70-93-48.dynamic.chello.pl [89.70.93.48]) by mail.messagingengine.com (Postfix) with ESMTPA id 5DA03CCD34; Thu, 23 Jun 2016 10:59:48 -0400 (EDT) Date: Thu, 23 Jun 2016 16:59:43 +0200 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= To: Andrew Cooper Message-ID: <20160623145943.GF410@mail-itl> References: <576AC98102000078000F7BDE@prv-mh.provo.novell.com> <576BBABD02000078000F7F14@prv-mh.provo.novell.com> <20160623085706.GG1593@mail-itl> <576BC42F02000078000F7F94@prv-mh.provo.novell.com> <20160623091824.GH1593@mail-itl> <20160623092353.GI1593@mail-itl> <576BCC2602000078000F7FC9@prv-mh.provo.novell.com> <20160623132551.GE410@mail-itl> <4b6d4a3a-fa46-bee9-ec38-3c7b2fe34a7b@citrix.com> MIME-Version: 1.0 In-Reply-To: <4b6d4a3a-fa46-bee9-ec38-3c7b2fe34a7b@citrix.com> User-Agent: Mutt/1.6.1 (2016-04-27) Cc: Daniel De Graaf , Jan Beulich , xen-devel Subject: Re: [Xen-devel] PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote: > On 23/06/16 14:25, Marek Marczykowski-Górecki wrote: > > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote: > >>>>> On 23.06.16 at 11:23, wrote: > >>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: > >>>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: > >>>>>>>> On 23.06.16 at 10:57, wrote: > >>>>>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >>>>>>> I wonder what good the duplication of the returned domain ID does: I'm > >>>>>>> tempted to remove the one in the command-specific structure. Does > >>>>>>> anyone have insight into why it was done that way? > >>>>>> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first > >>>>>> existing domain with ID >= passed one? Reading various comments in code > >>>>>> it looks to be used to domain enumeration. This patch changes this > >>>>>> behaviour. > >>>>> No, it doesn't. It adjusts the behavior only for the DM case (which > >>>>> isn't supposed to get information on other than the target domain, > >>>>> i.e. in this one specific case the very domain ID needs to be passed > >>>>> in). > >>>> int xc_domain_getinfo(xc_interface *xch, > >>>> uint32_t first_domid, > >>>> unsigned int max_doms, > >>>> xc_dominfo_t *info) > >>>> { > >>>> unsigned int nr_doms; > >>>> uint32_t next_domid = first_domid; > >>>> DECLARE_DOMCTL; > >>>> int rc = 0; > >>>> > >>>> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); > >>>> > >>>> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) > >>>> { > >>>> domctl.cmd = XEN_DOMCTL_getdomaininfo; > >>>> domctl.domain = (domid_t)next_domid; > >>>> if ( (rc = do_domctl(xch, &domctl)) < 0 ) > >>>> break; > >>>> info->domid = (uint16_t)domctl.domain; > >>>> (...) > >>>> next_domid = (uint16_t)domctl.domain + 1; > >>>> > >>>> > >>>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next > >>> valid > >>>> domain. > >>> Hmm, looks like I've misread you patch. Reading again... > >>> > >>> But now I see rcu_read_lock(&domlist_read_lock) is gets called only when > >>> looping over domains, but rcu_read_unlock is called in any case. Is it > >>> correct? > >> How that? There is this third hunk: > > Ok, after drawing a flowchart of the control in this function after your > > change, on a piece of paper, this case looks fine. But depending on how > > the domain was found (explicit loop or rcu_lock_domain_by_id), different > > locks are held, which makes it harder to follow what is going on. > > > > Crazy idea: how about making the code easy/easier to read instead of > > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is > > convolved enough. How about this version (2 patches): > > > > xen: move domain lookup for getdomaininfo to the same > > > > XEN_DOMCTL_getdomaininfo have different semantics than most of others > > domctls - it returns information about first valid domain with ID >= > > argument. But that's no excuse for having the lookup done in a different > > place, which made handling different corner cases unnecessary complex. > > Move the lookup to the first switch clause. And adjust locking to be the > > same as for other cases. > > > > Signed-off-by: Marek Marczykowski-Górecki > > FWIW, I prefer this solution to the issue. > > Reviewed-by: Andrew Cooper , with a few style > nits. Fixed patch according to your comments: xen: move domain lookup for getdomaininfo to the same XEN_DOMCTL_getdomaininfo have different semantics than most of others domctls - it returns information about first valid domain with ID >= argument. But that's no excuse for having the lookup code in a different place, which made handling different corner cases unnecessary complex. Move the lookup to the first switch clause. And adjust locking to be the same as for other cases. Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Andrew Cooper --- xen/common/domctl.c | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/xen/common/domctl.c b/xen/common/domctl.c index e43904e..41de3e8 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; + + case XEN_DOMCTL_getdomaininfo: + d = rcu_lock_domain_by_id(op->domain); + + if ( d == NULL ) + { + /* Search for the next available domain. */ + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + if ( d->domain_id >= op->domain ) + { + rcu_lock_domain(d); + break; + } + + rcu_read_unlock(&domlist_read_lock); + if ( d == NULL ) + return -ESRCH; + } + break; + default: d = rcu_lock_domain_by_id(op->domain); if ( d == NULL ) @@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) break; case XEN_DOMCTL_getdomaininfo: - { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); - - for_each_domain ( d ) - if ( d->domain_id >= dom ) - break; - - ret = -ESRCH; - if ( d == NULL ) - goto getdomaininfo_out; - ret = xsm_getdomaininfo(XSM_HOOK, d); if ( ret ) - goto getdomaininfo_out; + break; getdomaininfo(d, &op->u.getdomaininfo); - op->domain = op->u.getdomaininfo.domain; copyback = 1; - - getdomaininfo_out: - rcu_read_unlock(&domlist_read_lock); - d = NULL; break; - } case XEN_DOMCTL_getvcpucontext: {