Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1706828 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-08 21:00 +0200
    Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-08 21:10 +0200
      Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-08 21:20 +0200
        Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Andy Lutomirski <luto@kernel.org> - 2017-08-08 22:10 +0200
          Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-09 00:10 +0200
          Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-09 10:50 +0200
            Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Peter Zijlstra <peterz@infradead.org> - 2017-08-09 11:20 +0200
              Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-09 11:30 +0200
                Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Peter Zijlstra <peterz@infradead.org> - 2017-08-09 11:40 +0200
                  Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-09 12:00 +0200
                    Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-09 22:20 +0200
                      Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-10 09:10 +0200
                        Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-10 16:10 +0200
                          Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-10 16:30 +0200
                            Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-10 16:40 +0200
                              Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-10 17:10 +0200
                                Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-10 17:50 +0200
                          Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-10 16:50 +0200
            Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Andy Lutomirski <luto@kernel.org> - 2017-08-09 18:20 +0200
              Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder Juergen Gross <jgross@suse.com> - 2017-08-09 20:00 +0200

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

On Fri, Jul 28, 2017 at 10:54:37PM -0500, Josh Poimboeuf wrote:
> On Fri, Jul 28, 2017 at 07:59:12PM +0000, Levin, Alexander (Sasha Levin) wrote:
> > On Fri, Jul 28, 2017 at 01:57:20PM -0500, Josh Poimboeuf wrote:
> > >Thanks, that's much better.  I'm relieved the unwinder didn't screw that
> > >up, at least.
> > >
> > >This looks like a tricky one.  Is it easily recreatable?
> > 
> > Yeah, I just hit it again with slightly different initial calls:
> 
> Sasha sent me some data privately.  As I suspected, the cause is some
> bad ORC data.  Objtool incorrectly assumes that once the frame pointer
> is set up, it no longer gets touched.

So as it turns out, my pre-vacation self was completely wrong.  The
problem is actually some runtime instruction patching which affects the
accuracy of the ORC data.

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

The push offsets the stack pointer by 8 bytes, confusing the ORC
unwinder when it tries to unwind from the IRQ.

The race should be somewhat rare, though Sasha has no problems hitting
it with syzkaller.

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.

I haven't looked much at either option to see how feasible they would
be.

Any thoughts/opinions?

-- 
Josh

[toc] | [next] | [standalone]

#1706832

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?

                    Linus

[toc] | [prev] | [next] | [standalone]

#1706840

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.

And it can spit out a warning if we get two different ORC states for the
same address after doing the "union".  Then I guess we'd have to
rearrange things or sprinkle some nops to work around it.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1706852

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.

[toc] | [prev] | [next] | [standalone]

#1706903

On Tue, Aug 08, 2017 at 01:09:08PM -0700, Andy Lutomirski wrote:
> >> 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.

Ah, right.  Objtool doesn't know about paravirt patching, unfortunately.

> 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.)

Yeah, that would be really nice, assuming it's possible.  Otherwise I'll
need to teach objtool about the paravirt patches.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1707159

On 08/08/17 22:09, Andy Lutomirski wrote:
> 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

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.

> (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]

Hmm, I suggested drop of lguest support about 3 months ago and got no
response. OTOH there was no objection either. :-)

Regarding 32 bit Xen PV guests: even 32 bit dom0 is supposed to work.
In case you want to drop support in Linux you might ask that question
on xen-devel@lists.xenproject.org (with PVH support for guests nearly
complete your chances might be >0 to succeed).


Juergen

[toc] | [prev] | [next] | [standalone]

#1707177

On Wed, Aug 09, 2017 at 10:49:43AM +0200, Juergen Gross wrote:
> > 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.

Where does that live? I know of the inline patching for native, but
didn't know the guests did any of that too.

[toc] | [prev] | [next] | [standalone]

#1707182

On 09/08/17 11:16, Peter Zijlstra wrote:
> On Wed, Aug 09, 2017 at 10:49:43AM +0200, Juergen Gross wrote:
>>> 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.
> 
> Where does that live? I know of the inline patching for native, but
> didn't know the guests did any of that too.

See arch/x86/xen/enlighten_pv.c xen_patch().


Juergen

[toc] | [prev] | [next] | [standalone]

#1707193

On Wed, Aug 09, 2017 at 11:24:07AM +0200, Juergen Gross wrote:
> On 09/08/17 11:16, Peter Zijlstra wrote:
> > On Wed, Aug 09, 2017 at 10:49:43AM +0200, Juergen Gross wrote:
> >>> 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.
> > 
> > Where does that live? I know of the inline patching for native, but
> > didn't know the guests did any of that too.
> 
> See arch/x86/xen/enlighten_pv.c xen_patch().

'obvious' name that :-) I see that the actual code that's patched in
lives in xen-asm.S which unlike the native case doesn't appear to have
its own section. So that might make things even more difficult.

[toc] | [prev] | [next] | [standalone]

#1707207

On 09/08/17 11:35, Peter Zijlstra wrote:
> On Wed, Aug 09, 2017 at 11:24:07AM +0200, Juergen Gross wrote:
>> On 09/08/17 11:16, Peter Zijlstra wrote:
>>> On Wed, Aug 09, 2017 at 10:49:43AM +0200, Juergen Gross wrote:
>>>>> 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.
>>>
>>> Where does that live? I know of the inline patching for native, but
>>> didn't know the guests did any of that too.
>>
>> See arch/x86/xen/enlighten_pv.c xen_patch().
> 
> 'obvious' name that :-) I see that the actual code that's patched in
> lives in xen-asm.S which unlike the native case doesn't appear to have
> its own section. So that might make things even more difficult.

I don't see why this couldn't be changed.


Juergen

[toc] | [prev] | [next] | [standalone]

#1707837

On Wed, Aug 09, 2017 at 11:55:35AM +0200, Juergen Gross wrote:
> On 09/08/17 11:35, Peter Zijlstra wrote:
> > On Wed, Aug 09, 2017 at 11:24:07AM +0200, Juergen Gross wrote:
> >> On 09/08/17 11:16, Peter Zijlstra wrote:
> >>> On Wed, Aug 09, 2017 at 10:49:43AM +0200, Juergen Gross wrote:
> >>>>> 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.
> >>>
> >>> Where does that live? I know of the inline patching for native, but
> >>> didn't know the guests did any of that too.
> >>
> >> See arch/x86/xen/enlighten_pv.c xen_patch().
> > 
> > 'obvious' name that :-) I see that the actual code that's patched in
> > lives in xen-asm.S which unlike the native case doesn't appear to have
> > its own section. So that might make things even more difficult.
> 
> I don't see why this couldn't be changed.

I'm wondering why xen_patch() even exists.  The main difference between
xen_patch() and native_patch() seems to be that xen_patch() does some
relocs when doing an inline patch after calling paravirt_patch_insns().

But I can't see how that code path would ever run, because the
replacement functions are all larger than the size of the call
instruction to be replaced (7 bytes).  So they would never fit, and
instead the paravirt_patch_default() case would always run.  Or am I
missing something?

If we could get rid of the hypervisor-specific patching functions
(pv_init_ops) -- including possibly removing the lguest and vsmp code,
if nobody cares about them anymore -- that might make it easier to
consolidate all the patching things into a single place.

For example, maybe the paravirt patching could just use alternatives
under the hood somehow (insert hand waving).

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1708261

On 09/08/17 22:15, Josh Poimboeuf wrote:
> On Wed, Aug 09, 2017 at 11:55:35AM +0200, Juergen Gross wrote:
>> On 09/08/17 11:35, Peter Zijlstra wrote:
>>> On Wed, Aug 09, 2017 at 11:24:07AM +0200, Juergen Gross wrote:
>>>> On 09/08/17 11:16, Peter Zijlstra wrote:
>>>>> On Wed, Aug 09, 2017 at 10:49:43AM +0200, Juergen Gross wrote:
>>>>>>> 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.
>>>>>
>>>>> Where does that live? I know of the inline patching for native, but
>>>>> didn't know the guests did any of that too.
>>>>
>>>> See arch/x86/xen/enlighten_pv.c xen_patch().
>>>
>>> 'obvious' name that :-) I see that the actual code that's patched in
>>> lives in xen-asm.S which unlike the native case doesn't appear to have
>>> its own section. So that might make things even more difficult.
>>
>> I don't see why this couldn't be changed.
> 
> I'm wondering why xen_patch() even exists.  The main difference between
> xen_patch() and native_patch() seems to be that xen_patch() does some
> relocs when doing an inline patch after calling paravirt_patch_insns().
> 
> But I can't see how that code path would ever run, because the
> replacement functions are all larger than the size of the call
> instruction to be replaced (7 bytes).  So they would never fit, and
> instead the paravirt_patch_default() case would always run.  Or am I
> missing something?

Hmm, interesting. Just checked it and it seems you are right.

> If we could get rid of the hypervisor-specific patching functions
> (pv_init_ops) -- including possibly removing the lguest and vsmp code,
> if nobody cares about them anymore -- that might make it easier to
> consolidate all the patching things into a single place.

I'll send some patches to:

- remove xen_patch()
- remove lguest
- remove vsmp

In case nobody objects to apply those patches we can possibly simplify
some more code.

I'd love that. :-)


Juergen

[toc] | [prev] | [next] | [standalone]

#1708662

On Thu, Aug 10, 2017 at 09:05:19AM +0200, Juergen Gross wrote:
> > I'm wondering why xen_patch() even exists.  The main difference between
> > xen_patch() and native_patch() seems to be that xen_patch() does some
> > relocs when doing an inline patch after calling paravirt_patch_insns().
> > 
> > But I can't see how that code path would ever run, because the
> > replacement functions are all larger than the size of the call
> > instruction to be replaced (7 bytes).  So they would never fit, and
> > instead the paravirt_patch_default() case would always run.  Or am I
> > missing something?
> 
> Hmm, interesting. Just checked it and it seems you are right.
> 
> > If we could get rid of the hypervisor-specific patching functions
> > (pv_init_ops) -- including possibly removing the lguest and vsmp code,
> > if nobody cares about them anymore -- that might make it easier to
> > consolidate all the patching things into a single place.
> 
> I'll send some patches to:
> 
> - remove xen_patch()
> - remove lguest
> - remove vsmp
> 
> In case nobody objects to apply those patches we can possibly simplify
> some more code.
> 
> I'd love that. :-)

Well, I might have spoken too soon about getting rid of vsmp.  The
scalemp.com domain still exists.  The code hasn't changed much in three
years, but maybe it's simple enough that it hasn't needed to change.

Also, looking at the lguest mailing list, there seem to have been at
least a few people trying lguest out in the past year or so.

Even if we couldn't get rid of vsmp or lguest, I wonder if the PVOP_CALL
stuff could be reworked to something like the following:

static inline notrace unsigned long arch_local_save_flags(void)
{
	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
} 

Which would eventually translate to something like:

asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
			   : ... pvop clobber stuff ... );

where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
CPU_FEATURE_NATIVE would always be set.

It might need some more macro magic, but if it worked I think it would
be a lot clearer than the current voodoo.

Thoughts?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1708679

On 10/08/17 16:09, Josh Poimboeuf wrote:
> On Thu, Aug 10, 2017 at 09:05:19AM +0200, Juergen Gross wrote:
>>> I'm wondering why xen_patch() even exists.  The main difference between
>>> xen_patch() and native_patch() seems to be that xen_patch() does some
>>> relocs when doing an inline patch after calling paravirt_patch_insns().
>>>
>>> But I can't see how that code path would ever run, because the
>>> replacement functions are all larger than the size of the call
>>> instruction to be replaced (7 bytes).  So they would never fit, and
>>> instead the paravirt_patch_default() case would always run.  Or am I
>>> missing something?
>>
>> Hmm, interesting. Just checked it and it seems you are right.
>>
>>> If we could get rid of the hypervisor-specific patching functions
>>> (pv_init_ops) -- including possibly removing the lguest and vsmp code,
>>> if nobody cares about them anymore -- that might make it easier to
>>> consolidate all the patching things into a single place.
>>
>> I'll send some patches to:
>>
>> - remove xen_patch()
>> - remove lguest
>> - remove vsmp
>>
>> In case nobody objects to apply those patches we can possibly simplify
>> some more code.
>>
>> I'd love that. :-)
> 
> Well, I might have spoken too soon about getting rid of vsmp.  The
> scalemp.com domain still exists.  The code hasn't changed much in three
> years, but maybe it's simple enough that it hasn't needed to change.

Lets see. I have made the experience that asking whether some code can
be removed almost never get answers. Sending a patch which actually
removes the stuff results much more often in objections. :-)

> Also, looking at the lguest mailing list, there seem to have been at
> least a few people trying lguest out in the past year or so.

Well, yes. The question is here whether there is a _need_ for lguest
or was it just out of curiosity?

In the end it is 32 bit only and you can easily test boot code via
KVM, Xen or qemu.

> Even if we couldn't get rid of vsmp or lguest, I wonder if the PVOP_CALL
> stuff could be reworked to something like the following:
> 
> static inline notrace unsigned long arch_local_save_flags(void)
> {
> 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
> 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> 			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> 			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> 			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
> } 
> 
> Which would eventually translate to something like:
> 
> asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
> 			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> 			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> 			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> 			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
> 			   : ... pvop clobber stuff ... );
> 
> where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
> CPU_FEATURE_NATIVE would always be set.
> 
> It might need some more macro magic, but if it worked I think it would
> be a lot clearer than the current voodoo.
> 
> Thoughts?

Hmm, this would modify the current approach of pvops completely: instead
of letting each user of pvops (xen, lguest, vsmp, ...) set the functions
it is needing, you'd have to modify the core definition of each pvops
function for each user. Or would you want to let Xen, lguest etc. opt in
for pvops and generate above code at build time from some templates?


Juergen

[toc] | [prev] | [next] | [standalone]

#1708682

On Thu, Aug 10, 2017 at 04:24:58PM +0200, Juergen Gross wrote:
> >> I'll send some patches to:
> >>
> >> - remove xen_patch()
> >> - remove lguest
> >> - remove vsmp
> >>
> >> In case nobody objects to apply those patches we can possibly simplify
> >> some more code.
> >>
> >> I'd love that. :-)
> > 
> > Well, I might have spoken too soon about getting rid of vsmp.  The
> > scalemp.com domain still exists.  The code hasn't changed much in three
> > years, but maybe it's simple enough that it hasn't needed to change.
> 
> Lets see. I have made the experience that asking whether some code can
> be removed almost never get answers. Sending a patch which actually
> removes the stuff results much more often in objections. :-)
> 
> > Also, looking at the lguest mailing list, there seem to have been at
> > least a few people trying lguest out in the past year or so.
> 
> Well, yes. The question is here whether there is a _need_ for lguest
> or was it just out of curiosity?
> 
> In the end it is 32 bit only and you can easily test boot code via
> KVM, Xen or qemu.

Good points.  I'm all for removing code, so you have no objections from
me :-)

> > Even if we couldn't get rid of vsmp or lguest, I wonder if the PVOP_CALL
> > stuff could be reworked to something like the following:
> > 
> > static inline notrace unsigned long arch_local_save_flags(void)
> > {
> > 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
> > 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> > 			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> > 			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> > 			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
> > } 
> > 
> > Which would eventually translate to something like:
> > 
> > asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
> > 			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> > 			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> > 			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> > 			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
> > 			   : ... pvop clobber stuff ... );
> > 
> > where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
> > CPU_FEATURE_NATIVE would always be set.
> > 
> > It might need some more macro magic, but if it worked I think it would
> > be a lot clearer than the current voodoo.
> > 
> > Thoughts?
> 
> Hmm, this would modify the current approach of pvops completely: instead
> of letting each user of pvops (xen, lguest, vsmp, ...) set the functions
> it is needing, you'd have to modify the core definition of each pvops
> function for each user.

Right.  The callers (arch_local_save_flags, etc) would have to know
about the different hypervisors' functions.  But this knowledge could be
hidden in inline functions and/or macros, so I don't see it being too
much of a problem.

The upsides are that the behavior is much clearer (IMO), and we could
get rid of the .parainstructions stuff altogether.

> Or would you want to let Xen, lguest etc. opt in
> for pvops and generate above code at build time from some templates?

I'm not sure what you mean, can you clarify?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1708709

On 10/08/17 16:39, Josh Poimboeuf wrote:
> On Thu, Aug 10, 2017 at 04:24:58PM +0200, Juergen Gross wrote:
>>>> I'll send some patches to:
>>>>
>>>> - remove xen_patch()
>>>> - remove lguest
>>>> - remove vsmp
>>>>
>>>> In case nobody objects to apply those patches we can possibly simplify
>>>> some more code.
>>>>
>>>> I'd love that. :-)
>>>
>>> Well, I might have spoken too soon about getting rid of vsmp.  The
>>> scalemp.com domain still exists.  The code hasn't changed much in three
>>> years, but maybe it's simple enough that it hasn't needed to change.
>>
>> Lets see. I have made the experience that asking whether some code can
>> be removed almost never get answers. Sending a patch which actually
>> removes the stuff results much more often in objections. :-)
>>
>>> Also, looking at the lguest mailing list, there seem to have been at
>>> least a few people trying lguest out in the past year or so.
>>
>> Well, yes. The question is here whether there is a _need_ for lguest
>> or was it just out of curiosity?
>>
>> In the end it is 32 bit only and you can easily test boot code via
>> KVM, Xen or qemu.
> 
> Good points.  I'm all for removing code, so you have no objections from
> me :-)
> 
>>> Even if we couldn't get rid of vsmp or lguest, I wonder if the PVOP_CALL
>>> stuff could be reworked to something like the following:
>>>
>>> static inline notrace unsigned long arch_local_save_flags(void)
>>> {
>>> 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
>>> 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
>>> 			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
>>> 			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
>>> 			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
>>> } 
>>>
>>> Which would eventually translate to something like:
>>>
>>> asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
>>> 			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
>>> 			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
>>> 			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
>>> 			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
>>> 			   : ... pvop clobber stuff ... );
>>>
>>> where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
>>> CPU_FEATURE_NATIVE would always be set.
>>>
>>> It might need some more macro magic, but if it worked I think it would
>>> be a lot clearer than the current voodoo.
>>>
>>> Thoughts?
>>
>> Hmm, this would modify the current approach of pvops completely: instead
>> of letting each user of pvops (xen, lguest, vsmp, ...) set the functions
>> it is needing, you'd have to modify the core definition of each pvops
>> function for each user.
> 
> Right.  The callers (arch_local_save_flags, etc) would have to know
> about the different hypervisors' functions.  But this knowledge could be
> hidden in inline functions and/or macros, so I don't see it being too
> much of a problem.
> 
> The upsides are that the behavior is much clearer (IMO), and we could
> get rid of the .parainstructions stuff altogether.
> 
>> Or would you want to let Xen, lguest etc. opt in
>> for pvops and generate above code at build time from some templates?
> 
> I'm not sure what you mean, can you clarify?

It shouldn't be too much work to let each pvops user have a file in a
common paravirt directory containing the needed information to create:

static inline notrace unsigned long arch_local_save_flags(void)
{
    return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
	"pushfq; popq %rax", CPU_FEATURE_NATIVE,
	"call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
	"call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
	"call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
}

and all other needed functions at build time. It could look e.g. like
(for xen: xen.pv):

@@feature CPU_FEATURE_XEN
PV_IRQ_OPS_SAVE_FL "call __raw_callee_save_xen_save_fl"

and the pre-processor could be used to assemble all configured users
(pvops.pv):

#ifdef CONFIG_XEN_PV
#include "xen.pv"
#endif
#ifdef CONFIG_LGUEST
#include "lguest.pv"
#endif

The resulting file would the be mangled by e.g. a python or awk script
to a header containing macro definitions like:

#define PV_IRQ_OPS_SAVE_FL \
   "pushfq; popq %rax", CPU_FEATURE_NATIVE, \
	"call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN, \
	"call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP, \
	"call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST

which can then be used in paravirt.h:

static inline notrace unsigned long arch_local_save_flags(void)
{
    return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
                        PV_IRQ_OPS_SAVE_FL);
}


Juergen

[toc] | [prev] | [next] | [standalone]

#1708768

On Thu, Aug 10, 2017 at 04:59:36PM +0200, Juergen Gross wrote:
> On 10/08/17 16:39, Josh Poimboeuf wrote:
> > On Thu, Aug 10, 2017 at 04:24:58PM +0200, Juergen Gross wrote:
> >>>> I'll send some patches to:
> >>>>
> >>>> - remove xen_patch()
> >>>> - remove lguest
> >>>> - remove vsmp
> >>>>
> >>>> In case nobody objects to apply those patches we can possibly simplify
> >>>> some more code.
> >>>>
> >>>> I'd love that. :-)
> >>>
> >>> Well, I might have spoken too soon about getting rid of vsmp.  The
> >>> scalemp.com domain still exists.  The code hasn't changed much in three
> >>> years, but maybe it's simple enough that it hasn't needed to change.
> >>
> >> Lets see. I have made the experience that asking whether some code can
> >> be removed almost never get answers. Sending a patch which actually
> >> removes the stuff results much more often in objections. :-)
> >>
> >>> Also, looking at the lguest mailing list, there seem to have been at
> >>> least a few people trying lguest out in the past year or so.
> >>
> >> Well, yes. The question is here whether there is a _need_ for lguest
> >> or was it just out of curiosity?
> >>
> >> In the end it is 32 bit only and you can easily test boot code via
> >> KVM, Xen or qemu.
> > 
> > Good points.  I'm all for removing code, so you have no objections from
> > me :-)
> > 
> >>> Even if we couldn't get rid of vsmp or lguest, I wonder if the PVOP_CALL
> >>> stuff could be reworked to something like the following:
> >>>
> >>> static inline notrace unsigned long arch_local_save_flags(void)
> >>> {
> >>> 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
> >>> 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> >>> 			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> >>> 			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> >>> 			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
> >>> } 
> >>>
> >>> Which would eventually translate to something like:
> >>>
> >>> asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
> >>> 			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> >>> 			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> >>> 			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> >>> 			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
> >>> 			   : ... pvop clobber stuff ... );
> >>>
> >>> where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
> >>> CPU_FEATURE_NATIVE would always be set.
> >>>
> >>> It might need some more macro magic, but if it worked I think it would
> >>> be a lot clearer than the current voodoo.
> >>>
> >>> Thoughts?
> >>
> >> Hmm, this would modify the current approach of pvops completely: instead
> >> of letting each user of pvops (xen, lguest, vsmp, ...) set the functions
> >> it is needing, you'd have to modify the core definition of each pvops
> >> function for each user.
> > 
> > Right.  The callers (arch_local_save_flags, etc) would have to know
> > about the different hypervisors' functions.  But this knowledge could be
> > hidden in inline functions and/or macros, so I don't see it being too
> > much of a problem.
> > 
> > The upsides are that the behavior is much clearer (IMO), and we could
> > get rid of the .parainstructions stuff altogether.
> > 
> >> Or would you want to let Xen, lguest etc. opt in
> >> for pvops and generate above code at build time from some templates?
> > 
> > I'm not sure what you mean, can you clarify?
> 
> It shouldn't be too much work to let each pvops user have a file in a
> common paravirt directory containing the needed information to create:
> 
> static inline notrace unsigned long arch_local_save_flags(void)
> {
>     return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
> 	"pushfq; popq %rax", CPU_FEATURE_NATIVE,
> 	"call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> 	"call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> 	"call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
> }
> 
> and all other needed functions at build time. It could look e.g. like
> (for xen: xen.pv):
> 
> @@feature CPU_FEATURE_XEN
> PV_IRQ_OPS_SAVE_FL "call __raw_callee_save_xen_save_fl"
> 
> and the pre-processor could be used to assemble all configured users
> (pvops.pv):
> 
> #ifdef CONFIG_XEN_PV
> #include "xen.pv"
> #endif
> #ifdef CONFIG_LGUEST
> #include "lguest.pv"
> #endif
> 
> The resulting file would the be mangled by e.g. a python or awk script
> to a header containing macro definitions like:
> 
> #define PV_IRQ_OPS_SAVE_FL \
>    "pushfq; popq %rax", CPU_FEATURE_NATIVE, \
> 	"call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN, \
> 	"call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP, \
> 	"call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
> 
> which can then be used in paravirt.h:
> 
> static inline notrace unsigned long arch_local_save_flags(void)
> {
>     return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
>                         PV_IRQ_OPS_SAVE_FL);
> }

That could work, though I'd prefer the code-based approach because I get
the feeling it would be less obtuse.  I can play around with it, though
it may be a few weeks.  Feel free to delete code in the meantime :-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1708686

On Thu, Aug 10, 2017 at 09:09:03AM -0500, Josh Poimboeuf wrote:
> static inline notrace unsigned long arch_local_save_flags(void)
> {
> 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
> 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> 			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> 			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> 			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
> } 

Just a few clarifications on this idea:

It would probably be better to have the PVOP macros do the function name
translation, so maybe it would be something like this instead:

 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
 			    xen_save_fl, CPU_FEATURE_XEN,
 			    vsmp_save_fl, CPU_FEATURE_VSMP,
 			    lguest_save_fl, CPU_FEATURE_LGUEST);

One issue is that it would fail to link if CONFIG_XEN or
CONFIG_LGUEST_GUEST isn't set.  However I've seen some crazy macro magic
which lets you detect the number of variable arguments.  So if we got
that to work, it could be:

 	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
 			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
#ifdef CONFIG_XEN
 			    xen_save_fl, CPU_FEATURE_XEN,
#endif
 			    vsmp_save_fl, CPU_FEATURE_VSMP,
#ifdef CONFIG_LGUEST_GUEST
 			    lguest_save_fl, CPU_FEATURE_LGUEST,
#endif
			    );

And then maybe the PVOP_CALL macros could detect the number of arguments
and call the corresponding version of ALTERNATIVE_X.  Macro fun :-)

> Which would eventually translate to something like:
> 
> asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
> 			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
> 			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
> 			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
> 			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
> 			   : ... pvop clobber stuff ... );
> 
> where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
> CPU_FEATURE_NATIVE would always be set.
> 
> It might need some more macro magic, but if it worked I think it would
> be a lot clearer than the current voodoo.
> 
> Thoughts?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1707452

On Wed, Aug 9, 2017 at 1:49 AM, Juergen Gross <jgross@suse.com> wrote:
> On 08/08/17 22:09, Andy Lutomirski wrote:
>> 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
>
> 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.

[toc] | [prev] | [next] | [standalone]

#1707638

On 09/08/17 18:11, Andy Lutomirski wrote:
> On Wed, Aug 9, 2017 at 1:49 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 08/08/17 22:09, Andy Lutomirski wrote:
>>> 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
>>
>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web