From patchwork Thu Jun 23 08:32:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jan Beulich X-Patchwork-Id: 9194683 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 90D506077D for ; Thu, 23 Jun 2016 08:34:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7F0DC20223 for ; Thu, 23 Jun 2016 08:34:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 71A1C2843A; Thu, 23 Jun 2016 08:34:55 +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.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED 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 C486F20223 for ; Thu, 23 Jun 2016 08:34:54 +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 1bG03u-0004Dr-KS; Thu, 23 Jun 2016 08:32:34 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bG03t-0004Dl-VC for xen-devel@lists.xen.org; Thu, 23 Jun 2016 08:32:34 +0000 Received: from [85.158.139.211] by server-5.bemta-5.messagelabs.com id 51/18-29837-1AE9B675; Thu, 23 Jun 2016 08:32:33 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrMIsWRWlGSWpSXmKPExsXS6fjDS3fBvOx wg+cXdCyWfFzM4sDocXT3b6YAxijWzLyk/IoE1owdW5YwF2wwrmj9+Ju9gfGZehcjJ4eQQJ7E 1RMd7CA2r4CdxNV3U1lAbAkBQ4l981exdTFycLAIqEp8+q0KEmYTUJdoe7adFcQWEQiUmN49n wmkhFlAU2L+96guRi4OYYEpjBJd7d9YQRwhgdeMEhfbF7CBNHAKOEocPrqaEaSBV0BQ4u8OYY hedYn184RAKpgF5CWat85mhghLSyz/xzGBkW8WQv0shPpZSOpnIdQvYGRZxahRnFpUllqka2i ul1SUmZ5RkpuYmaNraGCql5taXJyYnpqTmFSsl5yfu4kRGHgMQLCD8eJpz0OMkhxMSqK8Vg7Z 4UJ8SfkplRmJxRnxRaU5qcWHGGU4OJQkeMvnAuUEi1LTUyvSMnOAMQCTluDgURLhXQ6S5i0uS MwtzkyHSJ1iVJQS5zUHSQiAJDJK8+DaYHF3iVFWSpiXEegQIZ6C1KLczBJU+VeM4hyMSsK8DS BTeDLzSuCmvwJazAS0+G4/2OKSRISUVAOjU+11xmXvl9ebTHI3a4rJW2EfYLS3XMBD+dHjtGC pvDQtOcmA9w+a/soLSa6xE6+dcJzl3lHff0ZOEhuda7+y2ah9CU8uv3aY/dHVrexRB7+W/l9Z tuovo++jH1+T9Rd4Pz7yKejEne1f1p08enZOb7Z5TabN3WutnMU5XKsf+SRNMf0S4WOuxFKck WioxVxUnAgAvXyJzLYCAAA= X-Env-Sender: JBeulich@suse.com X-Msg-Ref: server-10.tower-206.messagelabs.com!1466670750!29119756!1 X-Originating-IP: [137.65.248.74] X-SpamReason: No, hits=0.0 required=7.0 tests= X-StarScan-Received: X-StarScan-Version: 8.46; banners=-,-,- X-VirusChecked: Checked Received: (qmail 11295 invoked from network); 23 Jun 2016 08:32:32 -0000 Received: from prv-mh.provo.novell.com (HELO prv-mh.provo.novell.com) (137.65.248.74) by server-10.tower-206.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 23 Jun 2016 08:32:32 -0000 Received: from INET-PRV-MTA by prv-mh.provo.novell.com with Novell_GroupWise; Thu, 23 Jun 2016 02:32:30 -0600 Message-Id: <576BBABD02000078000F7F14@prv-mh.provo.novell.com> X-Mailer: Novell GroupWise Internet Agent 14.2.0 Date: Thu, 23 Jun 2016 02:32:29 -0600 From: "Jan Beulich" To: , "Daniel De Graaf" References: <20160622130335.GA410@mail-itl> <576AB3B102000078000F7B53@prv-mh.provo.novell.com> <20160622141314.GD1593@mail-itl> <576AC98102000078000F7BDE@prv-mh.provo.novell.com> In-Reply-To: Mime-Version: 1.0 Content-Disposition: inline Cc: 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 22.06.16 at 20:24, wrote: > On 06/22/2016 11:23 AM, Jan Beulich wrote: >>>>> On 22.06.16 at 16:13, wrote: >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >>>>>>> On 22.06.16 at 15:03, wrote: >>>>> I've finally found what was causing long standing issue of not working >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without >>>>> the other one in dom0). It looks to be this patch: >>>>> >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 >>>>> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with >>>>> EPERM in stubdomain. >>>>> >>>>> What would be the best solution for this? Allowing xc_domain_getinfo >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly >>>>> implementing the logic from that commit solely in libxl? >>>> >>>> Once we fixed the quirky behavior of the current implementation >>>> (allowing information to be returned for other than the requested >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. >>> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? >> >> Which fix? I talked of one to be made. >> >>>> But let's ask Daniel explicitly. And in that context I then also wonder >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to >>>> the respective sysctl. >>> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) >>> - xsm_domctl (which enforce actual policy) >>> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be >>> useful to be able to just check if given domain is alive, without >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find >>> this useful also for any other inter-domain communication (for example >>> libxenvchan connection). >>> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when >>> device-model domain is asking about its target domain, or calling domain >>> is xenstore domain/privileged domain. Right? >> >> Yes, that's what I think too. >> >>> How to combine those >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only >>> usage of XSM_XS_PRIV)? > > Changing the definition of XSM_XS_PRIV seems like the best solution, since > this is the only use. I don't think it matters if the constant is renamed > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a > caller, it could be removed and the actual logic placed in the switch > statement - that way it's clear this is a special case for getdomaininfo > instead of attempting to make this generic. > > Either method works, and I agree allowing DM to invoke this domctl is both > useful and not going to introduce problems. The getdomaininfo permission > will also need to be added to the device_model macro in xen.if. What exactly this last sentence means I need to add I'm not sure about. Apart from that, how about the change below? Jan domctl: relax getdomaininfo permissions Qemu needs access to this for the domain it controls, both due to it being used by xc_domain_memory_mapping() (which qemu calls) and the explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). This at once avoids a for_each_domain() loop when the ID of an existing domain gets passed in. Reported-by: Marek Marczykowski-Górecki --- 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? I further wonder why we have XSM_OTHER: The respective conversion into other XSM_* values in xsm/dummy.h could as well move into the callers, making intentions more obvious when looking at the actual code. --- unstable.orig/xen/common/domctl.c +++ unstable/xen/common/domctl.c @@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe 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; default: d = rcu_lock_domain_by_id(op->domain); - if ( d == NULL ) + if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) return -ESRCH; } @@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_getdomaininfo: { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); + domid_t dom = DOMID_INVALID; - for_each_domain ( d ) - if ( d->domain_id >= dom ) + if ( !d ) + { + ret = -EINVAL; + if ( op->domain >= DOMID_FIRST_RESERVED ) break; + rcu_read_lock(&domlist_read_lock); + + dom = op->domain; + for_each_domain ( d ) + if ( d->domain_id >= dom ) + break; + } + ret = -ESRCH; if ( d == NULL ) goto getdomaininfo_out; @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe copyback = 1; getdomaininfo_out: + if ( dom == DOMID_INVALID ) + break; + rcu_read_unlock(&domlist_read_lock); d = NULL; break; --- unstable.orig/xen/include/xsm/dummy.h +++ unstable/xen/include/xsm/dummy.h @@ -61,7 +61,12 @@ static always_inline int xsm_default_act return 0; case XSM_TARGET: if ( src == target ) + { return 0; + case XSM_XS_PRIV: + if ( src->is_xenstore ) + return 0; + } /* fall through */ case XSM_DM_PRIV: if ( target && src->target == target ) @@ -71,10 +76,6 @@ static always_inline int xsm_default_act 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;