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]

Message-ID: <20190924025623.GD4658@redhat.com>
Date:   Mon, 23 Sep 2019 22:56:23 -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 Mon, Sep 23, 2019 at 09:55:27PM -0400, Andrea Arcangeli wrote:
> 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.

Thinking more at this I don't think the result of the size check was
nearly enough to come to any conclusion. The only probably conclusion
one can take is that if the size didn't change it was a fail, because
there would be high probability that it wouldn't be beneficial because
it was a noop (even that wouldn't be 100% certain).

One needs to look at why it changed to take any conclusion, and the
reason it got smaller is that all dynamic tracing for ftrace was
dropped, the functions were already inlined fine in the RETPOLINE
case.

Those are tiny functions so it looks a positive thing to make them as
small as possible, there are already proper tracepoints in
kvm_exit/kvm_entry for bpf tracing all all KVM events so there's no
need of the fentry in those tiny functions with just an instruction
that are only ever compiled as static because of the pointer to
function.

This however also means it helps the CONFIG_RETPOLINE=n case and it
doesn't help at all the CONFIG_RETPOLINE=y case, so it's fully
orthogonal to the patchset and can be splitted off but I think it's
worth it.

Thanks,
Andrea