linux-kernel - Re: [PATCH 03/17] KVM: monolithic: x86: handle the request_immediate

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: <20190924002442.GA2975@redhat.com>
Date:   Mon, 23 Sep 2019 20:24:42 -0400
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        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 03/17] KVM: monolithic: x86: handle the
 request_immediate_exit variation

On Mon, Sep 23, 2019 at 04:45:00PM -0700, Sean Christopherson wrote:
> With a straight rename to kvm_x86_<function>() instead of wrappers, we
> shouldn't need kvm_ops.c.  kvm_ops.h might be helpful, but it'd be just
> as easy to keep them in kvm_host.h and would likely yield a more
> insightful diff[*].

Yes, I already commented about this change Paolo asked.

> Hmm, I was thinking more along the lines of extending the kvm_host.h
> pattern down into vendor specific code, e.g. arch/x86/kvm/vmx/kvm_host.h.
> Probably with a different name though, two of those is confusing enough.
> 
> It'd still need Makefile changes, but we wouldn't litter the code with
> #ifdefs.  Future enhancments can also take advantage of the per-vendor
> header to inline other things.  Such a header would also make it possible
> to fully remove kvm_x86_ops in this series (I think).

It's common in include/linux/* to include the same .h file that just
implements the inlines depending on some #ifdef, but in this case it's
simpler to have different .h files to keep the two versions more
separated as they may be maintained by different groups, so including
different .h file sounds better than -D__VMX__ -D__VMX__ agreed.

> [*] Tying into the thought above, if we go for a straight rename and
>     eliminate the conditionally-implemented kvm_x86_ops ahead of time,
>     e.g. with inlines that return -EINVAL or something, then the
>     conversion to direct calls can be a straight replacement of
>     "kvm_x86_ops->" with "kvm_x86_" at the same time the declarations
>     are changed from members of kvm_x86_ops to externs.
> 
> Actually, typing out the above made me realize the immediate exit code
> can be:
> 
> 	if (req_immediate_exit) {
> 		kvm_make_request(KVM_REQ_EVENT, vcpu);
> 		if (kvm_x86_request_immediate_exit(vcpu))
> 			smp_send_reschedule(vcpu->cpu);
> 	}
> 
> Where kvm_x86_request_immediate_exit() returns 0 on success, e.g. the SVM
> implementation can be "return -EINVAL" or whatever is appropriate, which
> I assume the compiler can optimize out.  Or maybe a boolean return is
> better in this case?

While the final cleanup of kvm_x86_ops doesn't strictly require the
inlining Makefile tricks to be functional just yet, it would be
beneficial to remove some branch at runtime from non frequently
invoked methods and to get the final optimal implementation sorted out
during the initial cleanup of the structure, this should reduce the
number of patches to get to the most optimal possible end result. So I
think making the inline work in the Makefile as a dependency to remove
kvm_x86_ops is a fine plan.

Thanks,
Andrea