From patchwork Thu Jun 23 15:22:47 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: 9195459 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 068D36075A for ; Thu, 23 Jun 2016 15:24:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E98942841A for ; Thu, 23 Jun 2016 15:24:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DB4152845F; Thu, 23 Jun 2016 15:24:57 +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 30AF328458 for ; Thu, 23 Jun 2016 15:24: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 1bG6T5-0002n4-8F; Thu, 23 Jun 2016 15:22:59 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bG6T3-0002mW-G8 for xen-devel@lists.xen.org; Thu, 23 Jun 2016 15:22:57 +0000 Received: from [85.158.139.211] by server-14.bemta-5.messagelabs.com id 15/40-29442-FCEFB675; Thu, 23 Jun 2016 15:22:55 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupileJIrShJLcpLzFFi42JxymeR1j3/Lzv c4O12ZoslHxezODB6HN39mymAMYo1My8pvyKBNePXu7yCtUYV63snszcwrtfsYuTiEBJYxSgx c00rSxcjJ5CTLfFk4j42kASLwAJmiS1XZzNDOPOYJd5PvccEUiUh4Czxe85aVoj2dYwSr7q/M YMkWARUJS7s+M4OYrMJBEtcX/KLFcQWEdCW2PS2AyzOLOAh0Xb2CdhUYYHJjBL31x9gA0nwCu hI/HjXzg4x9SyzxKSlz1ggEoISJ2c+YYHoLpV433MUqJsDyJaWWP6PAyTMKeAo0bS7EWyOqIC yRMOMB8wTGIVmIemehaR7FkI3RFhd4s+8S5jC2hLLFr5mhrBtJdate8+ygJF9FaN6cWpRWWqR rrleUlFmekZJbmJmjq6hgalebmpxcWJ6ak5iUrFecn7uJkZgvDAAwQ7GY5OdDzFKcjApifIyb s4OF+JLyk+pzEgszogvKs1JLT7EKMPBoSTBe+MvUE6wKDU9tSItMwcYuTBpCQ4eJRFeSWD0Cv EWFyTmFmemQ6ROMepybFlwYy2TEEtefl6qlDhvP8gMAZCijNI8uBGwJHKJUVZKmJcR6CghnoL UotzMElT5V4ziHIxKwrxvQabwZOaVwG16BXQEE9ARd/vBjihJREhJNTC2sbKk2j9lX8/3VWv2 lc1Syjt3Zm+rv/6xeofwxe6TLfu48wW6nTc+3nxAZEokg9xC5+3zWn04XWdblLqaLK/brK/cl LPp7gkP4zvVb8q6l105/nCtpvx2D76bHp47PE8XX7LO4nq3dunMcOacFlOduGJRcZsFswNUuU u+mZhlJHJFKUndXqXEUpyRaKjFXFScCACj4otzHQMAAA== X-Env-Sender: marmarek@invisiblethingslab.com X-Msg-Ref: server-5.tower-206.messagelabs.com!1466695374!46463060!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 39216 invoked from network); 23 Jun 2016 15:22:54 -0000 Received: from out3-smtp.messagingengine.com (HELO out3-smtp.messagingengine.com) (66.111.4.27) by server-5.tower-206.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 23 Jun 2016 15:22:54 -0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id 0C7502031B; Thu, 23 Jun 2016 11:22:54 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute2.internal (MEProxy); Thu, 23 Jun 2016 11:22:54 -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=RckIwKsLyd1X/oekU5ZnuPULVXM=; b=blfbp0 48zoHV6j0Bn863/I08fklq4lM0suOvv5j3N9NvQtN6VU5VKnRzw1UYQJpU2ZkR43 L4d7SOnG1cyeNqyD1H9utp7vvLE2VlcZDW4n0OYOLFLXQQUndZWZpSMiYFJqm4gn x60FWgPV/BKRuFdJs2dQZZLataQeoeW/F0rKY= 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=RckIwKsLyd1X/oekU5ZnuPULVXM=; b=RQxD2 MhECXiVnpvgUYY2I9mAGIIVOstam3wytX/F/+EBb/uqthIFdXYWcKfWyk9jQrMdR 9j1kjPhH9NmN9+CmAX0TFFCmoeP7Ay1vwJMsWY4EFnNMAroiy4YQSeo1qiAlQ+gJ sNfk9IgQiVEEUcJI+NVGNCZzvZeyxgDtVhU39U= X-Sasl-enc: 5tw9aJBvUyJvVZnG8Ywt3QHTm9dvc22Byxg/QRMrG+Ca 1466695373 Received: from mail-itl (89-70-93-48.dynamic.chello.pl [89.70.93.48]) by mail.messagingengine.com (Postfix) with ESMTPA id 712C0F29FE; Thu, 23 Jun 2016 11:22:52 -0400 (EDT) Date: Thu, 23 Jun 2016 17:22:47 +0200 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= To: Daniel De Graaf Message-ID: <20160623152247.GK1593@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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) Cc: 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 11:00:42AM -0400, Daniel De Graaf wrote: > On 06/23/2016 09:25 AM, Marek Marczykowski-Górecki wrote: > [...] > > 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: 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); > > } > > I would prefer testing for the xenstore flag instead of testing for the > target field. It ends up being the same thing in reality, since nobody > sane would make the xenstore also a device model (and not also dom0). > > case XEN_DOMCTL_getdomaininfo: > if ( src->is_xenstore ) > return 0; > return xsm_default_action(XSM_DM_PRIV, current->domain, d); > > This makes it clear that xenstore is the special case, and removes the > need for the one-off XSM_XS_PRIV constant. This was my initial idea, but I don't really understand the comment about link-time verification if the behaviour is the same for xsm not compiled vs disabled. But if skipping xsm_default_action here doesn't break this magic, I'm for it. Updated patch (with removal of XSM_XS_PRIV): xen: allow XEN_DOMCTL_getdomaininfo for device model domains 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. Also, since this was the only usage of XSM_XS_PRIV, which now gets handled inline, drop it. The problem was exposed by c428c9f "tools/libxl: handle the iomem parameter with the memory_mapping hcall". Signed-off-by: Marek Marczykowski-Górecki Acked-by: Daniel De Graaf --- xen/include/xsm/dummy.h | 8 +++----- xen/include/xsm/xsm.h | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 406cd18..2768861 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -71,10 +71,6 @@ static always_inline int xsm_default_action( if ( src->is_privileged ) return 0; return -EPERM; - case XSM_XS_PRIV: - if ( src->is_xenstore || src->is_privileged ) - return 0; - return -EPERM; default: LINKER_BUG_ON(1); return -EPERM; @@ -128,7 +124,9 @@ 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->is_xenstore ) + return 0; + return xsm_default_action(XSM_DM_PRIV, current->domain, d); default: return xsm_default_action(XSM_PRIV, current->domain, d); } diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 0d525ec..09672e7 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -38,7 +38,6 @@ enum xsm_default { XSM_DM_PRIV, /* Device model can perform on its target domain */ XSM_TARGET, /* Can perform on self or your target domain */ XSM_PRIV, /* Privileged - normally restricted to dom0 */ - XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ XSM_OTHER /* Something more complex */ }; typedef enum xsm_default xsm_default_t;