Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder

[Andy Lutomirski]

On Wed, Aug 9, 2017 at 1:49 AM, Juergen Gross <jgross@xxxxxxxx> wrote:
> On 08/08/17 22:09, Andy Lutomirski wrote:
>> On Tue, Aug 8, 2017 at 12:13 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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 <jpoimboe@xxxxxxxxxx> 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.