From patchwork Wed Aug 3 16:00:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Roger_Pau_Monn=C3=A9?= X-Patchwork-Id: 9261559 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 1F7DA60467 for ; Wed, 3 Aug 2016 16:03:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0FCB7282EC for ; Wed, 3 Aug 2016 16:03:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0453728306; Wed, 3 Aug 2016 16:03:42 +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 38002282EC for ; Wed, 3 Aug 2016 16:03:41 +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 1bUybU-0005Ga-Jt; Wed, 03 Aug 2016 16:01:08 +0000 Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bUybT-0005Eb-Gy for xen-devel@lists.xenproject.org; Wed, 03 Aug 2016 16:01:07 +0000 Received: from [85.158.143.35] by server-3.bemta-6.messagelabs.com id 98/F7-05661-24512A75; Wed, 03 Aug 2016 16:01:06 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrFIsWRWlGSWpSXmKPExsXitHRDpK6T6KJ wg/1HuCy+b5nM5MDocfjDFZYAxijWzLyk/IoE1owDFzgLJtlUPHp/hqmBcY9RFyMnh4SAv8Sy ddfZQGwWARWJFe1HmEFsNgEdiYtzd4LFRQSUJXp//WbpYuTiYBY4zSixcsU3sCJhgXCJryvXg dm8AoYSSz6/BCsSEpjMItGx7xwbREJQ4uTMJywgNrOAnsSNqVOA4hxAtrTE8n8cEGF5ieats8 HmcArYS/y9sIARxBYVUJd42/kWrFVIQFGif94DsFYJAW6Jv932ExgFZiFZMAvJglkIC2YhWbC AkWUVo3pxalFZapGuuV5SUWZ6RkluYmaOrqGBmV5uanFxYnpqTmJSsV5yfu4mRmC4MgDBDsaZ l/0PMUpyMCmJ8k48uDBciC8pP6UyI7E4I76oNCe1+BCjDAeHkgQvu8iicCHBotT01Iq0zBxg5 MCkJTh4lER4o4WB0rzFBYm5xZnpEKlTjLocjxbeWMskxJKXn5cqJc47HaRIAKQoozQPbgQsii 8xykoJ8zICHSXEU5BalJtZgir/ilGcg1FJmHcXyBSezLwSuE2vgI5gAjrihMECkCNKEhFSUg2 MPqtqFd5ObNjLXiy2ucS9pkUuYMq6/AsndY5JvhBpi12cejrsq4DbiYztySdmMiaHFH5Q12Jl 2D3Hfb/Y5HOPBNP2cew78JS/YNOFBZGCKuwmJUkPAzWLmKaujam81t77kfn81pOK9z6ueKl+W sU/99nzJ6vSK+ONuQ68XneZjXVpv8g6fz8FJZbijERDLeai4kQAkyX+Sd0CAAA= X-Env-Sender: prvs=016a9660e=roger.pau@citrix.com X-Msg-Ref: server-14.tower-21.messagelabs.com!1470240064!26919143!1 X-Originating-IP: [66.165.176.89] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni44OSA9PiAyMDMwMDc=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 8.77; banners=-,-,- X-VirusChecked: Checked Received: (qmail 52524 invoked from network); 3 Aug 2016 16:01:05 -0000 Received: from smtp.citrix.com (HELO SMTP.CITRIX.COM) (66.165.176.89) by server-14.tower-21.messagelabs.com with RC4-SHA encrypted SMTP; 3 Aug 2016 16:01:05 -0000 X-IronPort-AV: E=Sophos;i="5.28,466,1464652800"; d="scan'208";a="370116326" Date: Wed, 3 Aug 2016 18:00:37 +0200 From: Roger Pau Monne To: Jan Beulich Message-ID: <20160803160037.ydi3ztjgjtbygxg6@mac> References: <1469809747-11176-1-git-send-email-roger.pau@citrix.com> <1469809747-11176-2-git-send-email-roger.pau@citrix.com> <909c0b68-f63f-0dde-bbd6-c37b4bfcc587@citrix.com> <20160802094722.2umis4u5tpua567z@mac> <20160802154937.sf5lufgpbmuxtkpj@mac> <57A0E27B0200007800101E90@prv-mh.provo.novell.com> <57A229200200007800102581@prv-mh.provo.novell.com> <8ee96cc2-f914-692e-ac5e-a5d6e387e5bb@citrix.com> <57A22BE502000078001025B0@prv-mh.provo.novell.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57A22BE502000078001025B0@prv-mh.provo.novell.com> User-Agent: Mutt/1.6.2-neo (2016-06-11) X-DLP: MIA1 Cc: George Dunlap , Andrew Cooper , TimDeegan , George Dunlap , xen-devel Subject: Re: [Xen-devel] [PATCH RFC 01/12] x86/paging: introduce paging_set_allocation 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 Wed, Aug 03, 2016 at 09:37:41AM -0600, Jan Beulich wrote: > >>> On 03.08.16 at 17:28, wrote: > > On 03/08/16 16:25, Jan Beulich wrote: > >>>>> On 03.08.16 at 17:11, wrote: > >>> On Tue, Aug 2, 2016 at 5:12 PM, Jan Beulich wrote: > >>>>>>> On 02.08.16 at 17:49, wrote: > >>>>> On Tue, Aug 02, 2016 at 11:47:22AM +0200, Roger Pau Monne wrote: > >>>>>> On Fri, Jul 29, 2016 at 05:47:24PM +0100, Andrew Cooper wrote: > >>>>>>> As this is for the construction of dom0, it would be better to take a > >>>>>>> preemptible pointer, loop in construct_dom0(), with a > >>>>>>> process_pending_softirqs() call in between. > >>>>>> > >>>>>> Now fixed. > >>>>> > >>>>> Hm, I have to stand corrected, using hypercall_preempt_check (as > >>>>> any of the *_set_allocation function use), is not safe at this point: > >>>>> > >>>>> (XEN) ----[ Xen-4.8-unstable x86_64 debug=y Tainted: C ]---- > >>>>> (XEN) CPU: 0 > >>>>> (XEN) RIP: e008:[] > >>> hap.c#local_events_need_delivery+0x27/0x40 > >>>>> (XEN) RFLAGS: 0000000000010246 CONTEXT: hypervisor > >>>>> (XEN) rax: 0000000000000000 rbx: ffff83023f5a5000 rcx: ffff82d080312900 > >>>>> (XEN) rdx: 0000000000000001 rsi: ffff83023f5a56c8 rdi: ffff8300b213d000 > >>>>> (XEN) rbp: ffff82d080307cc8 rsp: ffff82d080307cc8 r8: 0180000000000000 > >>>>> (XEN) r9: 0000000000000000 r10: 0000000000247000 r11: ffff82d08029a5b0 > >>>>> (XEN) r12: 0000000000000011 r13: 00000000000023ac r14: ffff82d080307d4c > >>>>> (XEN) r15: ffff83023f5a56c8 cr0: 000000008005003b cr4: 00000000001526e0 > >>>>> (XEN) cr3: 00000000b20fc000 cr2: 0000000000000000 > >>>>> (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: e008 > >>>>> (XEN) Xen code around > >>> (hap.c#local_events_need_delivery+0x27/0x40): > >>>>> (XEN) 0d ad fa ff 48 8b 47 08 <80> 38 00 74 09 80 78 01 00 0f 94 c0 eb 02 31 > >>> c0 > >>>>> (XEN) Xen stack trace from rsp=ffff82d080307cc8: > >>>>> (XEN) ffff82d080307d08 ffff82d08022fc47 0000000000000000 ffff83023f5a5000 > >>>>> (XEN) ffff83023f5a5648 0000000000000000 ffff82d080307d4c 0000000000002400 > >>>>> (XEN) ffff82d080307d38 ffff82d08020008c 00000000000ffffd ffff8300b1efd000 > >>>>> (XEN) ffff83023f5a5000 ffff82d080307d4c ffff82d080307d78 ffff82d0802cad30 > >>>>> (XEN) 0000000000203000 ffff83023f5a5000 ffff82d0802bf860 0000000000000000 > >>>>> (XEN) 0000000000000001 ffff83000008bef0 ffff82d080307de8 ffff82d0802c91e0 > >>>>> (XEN) ffff82d080307de8 ffff82d080143900 ffff82d080307de8 0000000000000000 > >>>>> (XEN) ffff83000008bf00 ffff82d0802eb480 ffff82d080307dc4 ffff82d08028b1cd > >>>>> (XEN) ffff83000008bf00 0000000000000000 0000000000000001 ffff83023f5a5000 > >>>>> (XEN) ffff82d080307f08 ffff82d0802bf0c9 0000000000000000 0000000000000000 > >>>>> (XEN) 0000000000000000 ffff82d080307f18 ffff83000008bee0 0000000000000001 > >>>>> (XEN) 0000000000000001 0000000000000001 0000000000000000 0000000000100000 > >>>>> (XEN) 0000000000000001 0000000000247000 ffff83000008bef4 0000000000100000 > >>>>> (XEN) ffff830100000000 0000000000247001 0000000000000014 0000000100000000 > >>>>> (XEN) ffff8300ffffffec ffff83000008bef0 ffff82d0802e0640 ffff83000008bfb0 > >>>>> (XEN) 0000000000000000 0000000000000000 0000000000000111 0000000800000000 > >>>>> (XEN) 000000010000006e 0000000000000003 00000000000002f8 0000000000000000 > >>>>> (XEN) 00000000ad5c0bd0 0000000000000000 0000000000000001 0000000000000008 > >>>>> (XEN) 0000000000000000 ffff82d080100073 0000000000000000 0000000000000000 > >>>>> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > >>>>> (XEN) Xen call trace: > >>>>> (XEN) [] hap.c#local_events_need_delivery+0x27/0x40 > >>>>> (XEN) [] hap_set_allocation+0x107/0x130 > >>>>> (XEN) [] paging_set_allocation+0x4c/0x80 > >>>>> (XEN) [] domain_build.c#hvm_setup_p2m+0x70/0x1a0 > >>>>> (XEN) [] domain_build.c#construct_dom0_hvm+0x60/0x120 > >>>>> (XEN) [] __start_xen+0x1ea9/0x23a0 > >>>>> (XEN) [] __high_start+0x53/0x60 > >>>>> (XEN) > >>>>> (XEN) Pagetable walk from 0000000000000000: > >>>> > >>>> Sadly you don't make clear what pointer it is that is NULL at that point. > >>> > >>> It sounds from what he says in the following paragraph like current is NULL. > >> > >> I don't recall us re-setting current to this late in the boot process. > >> Even during early boot we set it to a bogus non-NULL value rather > >> than NULL. > >> > >>>>> I've tried setting current to d->vcpu[0], but that just makes the call to > >>>>> hypercall_preempt_check crash in some scheduler assert. In any case, I've > >>>>> added the preempt parameter to the paging_set_allocation function, but I > >>>>> don't plan to use it in the domain builder for the time being. Does that > >>>>> sound right? > >>>> > >>>> Not really, new huge latency issues like this shouldn't be reintroduced; > >>>> we've been fighting hard to get rid of those (and we still occasionally > >>>> find some no-one had noticed before). > >>> > >>> You mean latency in processing softirqs? > >>> > >>> Maybe what we need to do is to make local_events_need_delivery() safe > >>> to call at this point by having it return 0 if current is NULL rather > >>> than crashing? > >> > >> That would have the same effect - no softirq processing, and hence > >> possible time issues on huge systems. > > > > No, local_events_delivery() only checks to see if the current vcpu has > > outstanding virtual interrupts. The other half of > > hypercall_preempt_check() checks for softirqs, which doesn't appear to > > rely on having current available. > > Good point, but > - current should nevertheless not be NULL (afaict at least), > - hypercall_preempt_check() is probably the wrong construct, > as we're no in a hypercall. > The latter of course could be addressed by, as you did suggest, > some refinement to one of the pieces it's being made up from, > but I'm not sure that would be better than perhaps making its > invocation conditional (with some better alternative in the "else" > case) in hap_set_allocation(). Not the least because any > adjustment to hypercall_preempt_check() itself would affect all > other of its users. I had added the following patch to my queue in order to fix this: --- xen/x86: allow calling hypercall_preempt_check with the idle domain This allows using hypercall_preempt_check while building Dom0. Signed-off-by: Roger Pau Monné --- But seeing your comments I now wonder whether that's appropriate. Is there anyway in Xen to know whether Xen is in hypercall context or not? Another way to fix this would be to change both {hap/sh}_set_allocation functions to only call hypercall_check_preempt if current != idle_domain, and in the idle domain case just call softirq_pending (which is the same that the above change achieves). Roger. diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h index a82062e..d55a8bd 100644 --- a/xen/include/asm-x86/event.h +++ b/xen/include/asm-x86/event.h @@ -23,6 +23,10 @@ int hvm_local_events_need_delivery(struct vcpu *v); static inline int local_events_need_delivery(void) { struct vcpu *v = current; + + if ( is_idle_vcpu(v) ) + return 0; + return (has_hvm_container_vcpu(v) ? hvm_local_events_need_delivery(v) : (vcpu_info(v, evtchn_upcall_pending) && !vcpu_info(v, evtchn_upcall_mask)));