lkml.org 
[lkml]   [2017]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 1/2] x86/unwind: add ORC unwinder
On Tue, Aug 8, 2017 at 12:13 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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@redhat.com> 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

(The obvious syntax error and the naming should probably be fixed.
Also, this needs to live in an #ifdef because it needs to build on
kernels with pv support. It should also properly register itself as a
pv patch site.)

Semi-serious question: can we maybe delete lguest and 32-bit Xen PV
support some time soon? As far as I know, 32-bit Xen PV *hosts* are
all EOL and have no security support, 32-bit Xen PV guest dom0 may not
work (I've never tried, but it would certainly be nutty on a 64-bit
hypervisor), and lguest is, um, not seriously maintained any more. [1]

[1] A while back I complained that I couldn't get lguest to boot.
Someone replied with multiple workarounds for known bugs that make it
not boot. I have a hard time believing that anyone uses it for
anything other than trying to test that it still works.

\
 
 \ /
  Last update: 2017-08-08 22:10    [W:0.150 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site