From patchwork Thu Jun 23 13:25:51 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: 9195129 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 F0D0A6077D for ; Thu, 23 Jun 2016 13:28:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DF4B528455 for ; Thu, 23 Jun 2016 13:28:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D39EB28457; Thu, 23 Jun 2016 13:28:00 +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 2D37328455 for ; Thu, 23 Jun 2016 13:28:00 +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 1bG4dt-00068U-BA; Thu, 23 Jun 2016 13:26:01 +0000 Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bG4dr-00068M-RL for xen-devel@lists.xen.org; Thu, 23 Jun 2016 13:26:00 +0000 Received: from [85.158.137.68] by server-8.bemta-3.messagelabs.com id FB/D0-03780-663EB675; Thu, 23 Jun 2016 13:25:58 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrLIsWRWlGSWpSXmKPExsXilM8irZv2ODv cYPUVHoslHxezODB6HN39mymAMYo1My8pvyKBNaN37ly2ggW2Fb+3fGdpYPyu38XIxSEksIpR YsnNZtYuRk4gJ1vi4/2LLCAJFoEFzBInjzWzQzjzmCWWLLrO1MXIwSEh4Czx9J4SRPc6RomFl xcxg3SzCKhKzJi0hAXEZhMIlri+5BfYVBEBZYneX7/B4swCgRJ9LzaADRUWmMwocX/9ATaQBK +AtsT0zevYIc44wywx5ZEuRFxQ4uTMJ1DNpRI3V/ewgBzBLCAtsfwfB0iYU8BeYnHHLLAxokC 7GmY8YJ7AKDQLSfcsJN2zELohwuoSf+ZdYsYQ1pZYtvA1M4RtK7Fu3XuWBYzsqxjVi1OLylKL dM31kooy0zNKchMzc3QNDYz1clOLixPTU3MSk4r1kvNzNzECo4UBCHYwNn53OsQoycGkJMrLu Dk7XIgvKT+lMiOxOCO+qDQntfgQowwHh5IEr9sjoJxgUWp6akVaZg4wbmHSEhw8SiK80SBp3u KCxNzizHSI1ClGRSlxXgGQhABIIqM0D64NliouMcpKCfMyAh0ixFOQWpSbWYIq/4pRnINRSZg 3AWQKT2ZeCdz0V0CLmYAW3+0HW1ySiJCSamBcxOS9KIxNTGzLF4ZJXDOUlu1cGHoo08StvE2c q2UZW+J0Q7XlXC80267bHlpmmteru/Gt0+7LDe+DPA+xPDP9krZywgbVRDOu1OwOlqOsrJ3// 922awjIKd/iUbNbw3zWqjWWhzcEOHXxTDp7vO1miLPj04XzVq2p/G1qLFmrPfX9hZ+X5RYpsR RnJBpqMRcVJwIA7XhJMhADAAA= X-Env-Sender: marmarek@invisiblethingslab.com X-Msg-Ref: server-12.tower-31.messagelabs.com!1466688357!30138032!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 39034 invoked from network); 23 Jun 2016 13:25:58 -0000 Received: from out3-smtp.messagingengine.com (HELO out3-smtp.messagingengine.com) (66.111.4.27) by server-12.tower-31.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 23 Jun 2016 13:25:58 -0000 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id D4EA62045D; Thu, 23 Jun 2016 09:25:56 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute6.internal (MEProxy); Thu, 23 Jun 2016 09:25:56 -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=1QRYXyKSpGo3TpdoLjeESg9+XEo=; b=RXy4ZJ Sec+J96DHAGxVIjZ6K03oXQdeZUtR2ZwSJcNH6LBlzG9m1WWVxbYLWAM4x5YMlxj ufz0RR4hIHY+1nhAwCkh7ty/F8KOYLrgA9FBZWI4n0pJmB40hRNVsPT53v8UV6dW i9u0vW0wOLHiHKulkxaZIABbpWpLLOLxhKCHQ= 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=1QRYXyKSpGo3TpdoLjeESg9+XEo=; b=DL6IV 7m9nIzAq0qOf9cVgwGSNmyZAl8Ue5Gqwvr8c8K9IijOmm3F11qJLSYfZXek1q4NH xO2tkZGq3Urjqf58aigOSF4YUFeAGZLUjffN+Dck9NAJirARVcd/1daDNJSjCerI zz1z/oG/kCCol83/2QmaopGTocIOft2EKT69g0= X-Sasl-enc: Y9n8Tu6KKhxUJimMDUQbqacsaX/VaYoXrtnOwHao9ANJ 1466688356 Received: from mail-itl (89-70-93-48.dynamic.chello.pl [89.70.93.48]) by mail.messagingengine.com (Postfix) with ESMTPA id 89666CCD26; Thu, 23 Jun 2016 09:25:55 -0400 (EDT) Date: Thu, 23 Jun 2016 15:25:51 +0200 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= To: Jan Beulich Message-ID: <20160623132551.GE410@mail-itl> References: <576AB3B102000078000F7B53@prv-mh.provo.novell.com> <20160622141314.GD1593@mail-itl> <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> MIME-Version: 1.0 In-Reply-To: <576BCC2602000078000F7FC9@prv-mh.provo.novell.com> User-Agent: Mutt/1.6.1 (2016-04-27) Cc: Daniel De Graaf , 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: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 Reviewed-by: Andrew Cooper , with a few style --- xen/common/domctl.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/xen/common/domctl.c b/xen/common/domctl.c index e43904e..6ae1fe0 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -442,11 +442,29 @@ 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 valid 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 +880,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: { -- 2.5.5 xen: allow XEN_DOMCTL_getdomaininfo for device model Allow device model domain to get info about its target domain. It is used during PCI passthrough setup (xc_domain_memory_mapping checks for guest being auto-translated). While it happens in stubdomain, it failed, breaking PCI passthrough in such setup. While it is possible to workaround this at toolstack side, it seems logical to allow device model to get information about its target domain. The problem was exposed by c428c9f "tools/libxl: handle the iomem parameter with the memory_mapping hcall". Signed-off-by: Marek Marczykowski-Górecki --- xen/include/xsm/dummy.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 406cd18..70a1633 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -128,7 +128,10 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd) case XEN_DOMCTL_unbind_pt_irq: return xsm_default_action(XSM_DM_PRIV, current->domain, d); case XEN_DOMCTL_getdomaininfo: - return xsm_default_action(XSM_XS_PRIV, current->domain, d); + if (current->domain->target) + return xsm_default_action(XSM_DM_PRIV, current->domain, d); + else + return xsm_default_action(XSM_XS_PRIV, current->domain, d); default: return xsm_default_action(XSM_PRIV, current->domain, d); }