lkml.org 
[lkml]   [2020]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: WARNING: can't access registers at asm_common_interrupt
On Wed, Nov 11, 2020 at 06:47:36PM +0100, Peter Zijlstra wrote:
> This is PARAVIRT_XXL only, which is a Xen special. My preference, as
> always, is to kill it... Sadly the Xen people have a different opinion.

That would be soooo nice... then we could get rid of paravirt patching
altogether and replace it with static calls.

> > Objtool doesn't know about the pushf/pop paravirt patch, so ORC gets
> > confused by the changed stack layout.
> >
> > I'm thinking we either need to teach objtool how to deal with
> > save_fl/restore_fl patches, or we need to just get rid of those nasty
> > patches somehow. Peter, any thoughts?
>
> Don't use Xen? ;-)
>
> So with PARAVIRT_XXL the compiler will emit something like:
>
> "CALL *pvops.save_fl"
>
> Which we then overwrite at runtime with "pushf; pop %[re]ax" and a few
> NOPs.
>
> Now, objtool understands alternatives, and ensures they have the same
> stack layout, it has no chance in hell of understanding this, simply
> because paravirt_patch.c is magic.
>
> I don't have any immediate clever ideas, but let me ponder it a wee bit.
>
> ....
>
> Something really disguisting we could do is recognise the indirect call
> offset and emit an extra ORC entry for RIP+1. So the cases are:
>
> CALL *pv_ops.save_fl -- 7 bytes IIRC
> CALL $imm; -- 5 bytes
> PUSHF; POP %[RE]AX -- 2 bytes
>
> so the RIP+1 (the POP insn) will only ever exist in this case. The
> indirect and direct call cases would never land on that IP.

I had a similar idea, and a bit of deja vu - we may have talked about
this before. At least I know we talked about doing something similar
for alternatives which muck with the stack.

> > It looks like 044d0d6de9f5 ("lockdep: Only trace IRQ edges") is making
> > the problem more likely, by adding the irqs_disabled() check for every
> > local_irq_disable().
> >
> > Also - Peter, Nicholas - is that irqs_disabled() check really necessary
> > in local_irq_disable()? Presumably irqs would typically be be enabled
> > before calling it?
>
> Yeah, so it's all a giant can of worms that; also see:
>
> https://lkml.kernel.org/r/20200821084738.508092956@infradead.org
>
> The basic idea is to only trace edges, ie. when the hardware state
> actually changes. Sadly this means doing a pushf/pop before the cli.
> Ideally CLI would store the old IF in CF or something like that, but
> alas.

Right, that makes sense for save/restore, but is the disabled check
really needed for local_irq_disable()? Wouldn't that always be an edge?

And anyway I don't see a similar check for local_irq_enable().

--
Josh

\
 
 \ /
  Last update: 2020-11-11 19:13    [W:0.102 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site