From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752687AbdHIRw4 (ORCPT ); Wed, 9 Aug 2017 13:52:56 -0400 Received: from mx2.suse.de ([195.135.220.15]:56332 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752047AbdHIRwz (ORCPT ); Wed, 9 Aug 2017 13:52:55 -0400 Subject: Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder To: Andy Lutomirski Cc: Josh Poimboeuf , Linus Torvalds , "Levin, Alexander (Sasha Levin)" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "live-patching@vger.kernel.org" , Jiri Slaby , Ingo Molnar , "H. Peter Anvin" , Peter Zijlstra , Mike Galbraith References: <0a6cbfb40f8da99b7a45a1a8302dc6aef16ec812.1500938583.git.jpoimboe@redhat.com> <20170728164844.tee7ujqluv2fgarf@sasha-lappy> <20170728175234.pmou7z6dosbi7krh@treble> <20170728182954.imivl45vrc7toarp@sasha-lappy> <20170728185720.fwczcezbkrlxmwqb@treble> <20170728195912.panyn4p6e6ovueey@sasha-lappy> <20170729035437.fmnxmeedrq55bpwm@treble> <20170808185809.qaug762cj7nw3bcd@treble> <20170808191344.kw5nlicmq4g6fpii@treble> <79d6e04a-0a1d-592d-5e35-e4d6e7c7526b@suse.com> From: Juergen Gross Message-ID: <1c189bd3-e604-3ba1-6827-21510e4f3cfe@suse.com> Date: Wed, 9 Aug 2017 19:52:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/08/17 18:11, Andy Lutomirski wrote: > On Wed, Aug 9, 2017 at 1:49 AM, Juergen Gross wrote: >> On 08/08/17 22:09, Andy Lutomirski wrote: >>> On Tue, Aug 8, 2017 at 12:13 PM, Josh Poimboeuf wrote: >>>> On Tue, Aug 08, 2017 at 12:03:51PM -0700, Linus Torvalds wrote: >>>>> On Tue, Aug 8, 2017 at 11:58 AM, Josh Poimboeuf wrote: >>>>>> >>>>>> Take for example the lock_is_held_type() function. In vmlinux, it has >>>>>> the following instruction: >>>>>> >>>>>> callq *0xffffffff85a94880 (pv_irq_ops.save_fl) >>>>>> >>>>>> At runtime, that instruction is patched and replaced with a fast inline >>>>>> version of arch_local_save_flags() which eliminates the call: >>>>>> >>>>>> pushfq >>>>>> pop %rax >>>>>> >>>>>> The problem is when an interrupt hits after the push: >>>>>> >>>>>> pushfq >>>>>> --- irq --- >>>>>> pop %rax >>>>> >>>>> That should actually be something easily fixable, for an odd reason: >>>>> the instruction boundaries are different. >>>>> >>>>>> I'm not sure what the solution should be. It will probably need to be >>>>>> one of the following: >>>>>> >>>>>> a) either don't allow runtime "alternative" patches to mess with the >>>>>> stack pointer (objtool could enforce this); or >>>>>> >>>>>> b) come up with some way to register such patches with the ORC >>>>>> unwinder at runtime. >>>>> >>>>> c) just add ORC data for the alternative statically and _unconditionally_. >>>>> >>>>> No runtime registration. Just an unconditional entry for the >>>>> particular IP that comes after the "pushfq". It cannot match the >>>>> "callq" instruction, since it would be in the middle of that >>>>> instruction. >>>>> >>>>> Basically, just do a "union" of the ORC data for all the alternatives. >>>>> >>>>> Now, objtool should still verify that the instruction pointers for >>>>> alternatives are unique - or that they share the same ORC unwinder >>>>> information if they are not. >>>>> >>>>> But in cases like this, when the instruction boundaires are different, >>>>> things should "just work", with no need for any special cases. >>>>> >>>>> Hmm? >>>> >>>> Yeah, that might work. Objtool already knows about alternatives, so it >>>> might not be too hard. I'll try it. >>> >>> But this one's not an actual alternative, right? It's a pv op. >>> >>> I would advocate that we make it an alternative after all. I frickin' >>> hate the PV irq ops. It would like roughly like this: >>> >>> ALTERNATIVE "pushfq; popq %rax", "callq *pv_irq_ops.save_fl", >>> X86_FEATURE_GODDAMN_PV_IRQ_OPS >> >> You are aware that at least some of the Xen irq pvops functionality is >> patched inline? Your modification would slow down pv guests quite a >> bit, I guess. > > Yes, but what I had in mind was having both the alternative *and* the > paravirt patch entry. We'd obviously have to make sure to run > alternatives before paravirt patching, but that's possibly already the > case. So instead of having the "callq *pv_irq_ops.save_fl" as initial code you would end up with the "pushfq; popq %rax" until the alternatives are applied. I don't think this will work. In the end you are not allowed to use any irq ops in a Xen guest until that happens. And I think it happens rather late compared to the usage of any irq ops. And in case you are swapping oldinstr and newinstr in above ALTERNATIVE usage you will end up with exactly the same as with today's pvops, just the patching mechanism for the bare metal case would be different. And you would need more table entries (pvops and alternatives) for the same functionality. Or do I miss something here? Juergen