linux-kernel - RE: [RFC PATCH v1] KVM: x86: Introduce macros to simplify KVM_X86

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: <DS0PR11MB6373B4170ECA01D31830CA20DC0E2@DS0PR11MB6373.namprd11.prod.outlook.com>
Date: Thu, 18 Apr 2024 14:20:01 +0000
From: "Wang, Wei W" <wei.w.wang@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: "pbonzini@...hat.com" <pbonzini@...hat.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH v1] KVM: x86: Introduce macros to simplify KVM_X86_OPS
 static calls

On Thursday, April 18, 2024 9:59 PM, Sean Christopherson wrote:
> On Thu, Apr 18, 2024, Wei W Wang wrote:
> > On Thursday, April 18, 2024 12:27 AM, Sean Christopherson wrote:
> > > On Wed, Apr 17, 2024, Wei Wang wrote:
> > > > Introduces two new macros, KVM_X86_SC() and KVM_X86_SCC(), to
> > > > streamline the usage of KVM_X86_OPS static calls. The current
> > > > implementation of these calls is verbose and can lead to alignment
> > > > challenges due to the two pairs of parentheses. This makes the
> > > > code susceptible to exceeding the "80 columns per single line of code"
> > > > limit as defined in the coding-style document. The two macros are
> > > > added to improve code readability and maintainability, while
> > > > adhering to
> > > the coding style guidelines.
> > >
> > > Heh, I've considered something similar on multiple occasionsi.  Not
> > > because the verbosity bothers me, but because I often search for
> > > exact "word" matches when looking for function usage and the kvm_x86_
> prefix trips me up.
> >
> > Yeah, that's another compelling reason for the improvement.
> >
> > > IIRC, static_call_cond() is essentially dead code, i.e. it's the
> > > exact same as static_call().  I believe there's details buried in a
> > > proposed series to remove it[*].  And to not lead things astray, I
> > > verified that invoking a NULL kvm_x86_op with static_call() does no harm
> (well, doesn't explode at least).
> > >
> > > So if we add wrapper macros, I would be in favor in removing all
> > > static_call_cond() as a prep patch so that we can have a single macro.
> >
> > Sounds good. Maybe KVM_X86_OP_OPTIONAL could now also be removed?
> 
> No, KVM_X86_OP_OPTIONAL() is what allow KVM to WARN if a mandatory
> hook isn't defined.  Without the OPTIONAL and OPTIONAL_RET variants, KVM
> would need to assume every hook is optional, and thus couldn't WARN.

Yes, KVM_X86_OP_OPTIONAL is used to enforce the definition of mandatory hooks
with WARN_ON(). But the distinction between mandatory and optional hooks
has now become ambiguous. For example, all the hooks, whether defined or
undefined (NULL), are invoked via static_call() without issues now. In some sense,
all hooks could potentially be deemed as optional, and the undefined ones just lead
to NOOP when unconditionally invoked by the kvm/x86 core code. 
(the KVM_X86_OP_RET0 is needed)
Would you see any practical issues without that WARN_ON?