RE: [RFC PATCH v1] KVM: x86: Introduce macros to simplify KVM_X86_OPS static calls

[Wang, Wei W]

On Thursday, April 18, 2024 10:20 PM, Wang, Wei W wrote:
> 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(). 

I meant the KVM_X86_OP in the current implementation as you shared.
If we don't need KVM_X86_OP_OPTIONAL(), the WARN_ON() from KVM_X86_OP
will need to be removed to allow that all the hooks could be optional.

> 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?