linux-kernel - Re: [PATCH 14/17] KVM: monolithic: x86: inline more exit handlers in vmx.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Sep 2019 21:55:27 -0400
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        Peter Xu <peterx@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 14/17] KVM: monolithic: x86: inline more exit handlers in
 vmx.c

On Tue, Sep 24, 2019 at 03:25:34AM +0200, Paolo Bonzini wrote:
> On 24/09/19 03:00, Andrea Arcangeli wrote:
> > Before and after this specific commit there is a difference with gcc 8.3.
> > 
> > full patchset applied
> > 
> >  753699   87971    9616  851286   cfd56 build/arch/x86/kvm/kvm-intel.ko
> > 
> > git revert
> > 
> >  753739   87971    9616  851326   cfd7e  build/arch/x86/kvm/kvm-intel.ko
> > 
> > git reset --hard HEAD^
> > 
> >  753699   87971    9616  851286   cfd56  build/arch/x86/kvm/kvm-intel.ko
> > 
> > git revert
> > 
> >  753739   87971    9616  851326   cfd7e  build/arch/x86/kvm/kvm-intel.ko
> 
> So it's forty bytes.  I think we can leave this out.

This commit I reverted adds literally 3 inlines called by 3 functions,
in a very fast path, how many bytes of .text difference did you expect
by dropping some call/ret from a very fast path when you asked me to
test it? I mean it's just a couple of insn each.

I thought the question was if gcc was already inlining without the
hint or not or if it actually grew in size in case I got it wrong and
there were many callers and it was better off not inline, so now I
don't get what was the point of this test if with the result that
confirms it's needed, the patch should be dropped.

It's possible that this patch may not be relevant anymore with the
rename in place of the vmx/svm functions, but if this patch is to be
dropped with the optimal result, then I recommend you to go ahead and
submit a patch to drop __always_inline from the whole kernel because
if it's not good to use it here in a extreme fast path like
handle_external_interrupt and handle_halt, then I don't know what
__always_inline is good for anywhere else in the kernel.

Thanks,
Andrea