Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <0000000000007d3b2d05ac1c303e@google.com> <20200805132629.GA87338@elver.google.com>
 <20200805134232.GR2674@hirez.programming.kicks-ass.net> <20200805135940.GA156343@elver.google.com>
 <20200805141237.GS2674@hirez.programming.kicks-ass.net> <20200805141709.GD35926@hirez.programming.kicks-ass.net>
In-Reply-To: <20200805141709.GD35926@hirez.programming.kicks-ass.net>
From:   Marco Elver <elver@google.com>
Date:   Wed, 5 Aug 2020 16:36:12 +0200
Message-ID: <CANpmjNN6FWZ+MsAn3Pj+WEez97diHzqF8hjONtHG15C2gSpSgw@mail.gmail.com>
Subject: Re: [PATCH] x86/paravirt: Add missing noinstr to arch_local*() helpers
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        fenghua.yu@intel.com, "H. Peter Anvin" <hpa@zytor.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Luck, Tony" <tony.luck@intel.com>,
        "the arch/x86 maintainers" <x86@kernel.org>, yu-cheng.yu@intel.com,
        jgross@suse.com, sdeep@vmware.com,
        virtualization@lists.linux-foundation.org,
        kasan-dev <kasan-dev@googlegroups.com>,
        syzbot <syzbot+8db9e1ecde74e590a657@syzkaller.appspotmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 5 Aug 2020 at 16:17, <peterz@infradead.org> wrote:
>
> On Wed, Aug 05, 2020 at 04:12:37PM +0200, peterz@infradead.org wrote:
> > On Wed, Aug 05, 2020 at 03:59:40PM +0200, Marco Elver wrote:
> > > On Wed, Aug 05, 2020 at 03:42PM +0200, peterz@infradead.org wrote:
> >
> > > > Shouldn't we __always_inline those? They're going to be really small.
> > >
> > > I can send a v2, and you can choose. For reference, though:
> > >
> > >     ffffffff86271ee0 <arch_local_save_flags>:
> > >     ffffffff86271ee0:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> > >     ffffffff86271ee5:       48 83 3d 43 87 e4 01    cmpq   $0x0,0x1e48743(%rip)        # ffffffff880ba630 <pv_ops+0x120>
> > >     ffffffff86271eec:       00
> > >     ffffffff86271eed:       74 0d                   je     ffffffff86271efc <arch_local_save_flags+0x1c>
> > >     ffffffff86271eef:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> > >     ffffffff86271ef4:       ff 14 25 30 a6 0b 88    callq  *0xffffffff880ba630
> > >     ffffffff86271efb:       c3                      retq
> > >     ffffffff86271efc:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> > >     ffffffff86271f01:       0f 0b                   ud2
> >
> > >     ffffffff86271a90 <arch_local_irq_restore>:
> > >     ffffffff86271a90:       53                      push   %rbx
> > >     ffffffff86271a91:       48 89 fb                mov    %rdi,%rbx
> > >     ffffffff86271a94:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> > >     ffffffff86271a99:       48 83 3d 97 8b e4 01    cmpq   $0x0,0x1e48b97(%rip)        # ffffffff880ba638 <pv_ops+0x128>
> > >     ffffffff86271aa0:       00
> > >     ffffffff86271aa1:       74 11                   je     ffffffff86271ab4 <arch_local_irq_restore+0x24>
> > >     ffffffff86271aa3:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> > >     ffffffff86271aa8:       48 89 df                mov    %rbx,%rdi
> > >     ffffffff86271aab:       ff 14 25 38 a6 0b 88    callq  *0xffffffff880ba638
> > >     ffffffff86271ab2:       5b                      pop    %rbx
> > >     ffffffff86271ab3:       c3                      retq
> > >     ffffffff86271ab4:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> > >     ffffffff86271ab9:       0f 0b                   ud2
> >
> >
> > Blergh, that's abysmall. In part I suspect because you have
> > CONFIG_PARAVIRT_DEBUG, let me try and untangle that PV macro maze.
>
> Yeah, look here:
>
> 0000 0000000000462149 <arch_local_save_flags>:
> 0000   462149:  ff 14 25 00 00 00 00    callq  *0x0
> 0003                    46214c: R_X86_64_32S    pv_ops+0x120
> 0007   462150:  c3                      retq
>
>
> That's exactly what I was expecting.

Ah, for some reason the __always_inline version does *not* work with
KCSAN -- I'm getting various warnings, including the same lockdep
warning. I think there is some weirdness when this stuff gets inlined
into instrumented functions. At least with KCSAN, when any accesses
here are instrumented, and then KCSAN disable/enables interrupts,
things break. So, these functions should never be instrumented,
noinstr or not. Marking them 'inline noinstr' seems like the safest
option. Without CONFIG_PARAVIRT_DEBUG, any compiler should hopefully
inline them?

Thanks,
-- Marco