Received-SPF: pass (google.com: domain of linux-kernel+bounces-150225-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 18 Apr 2024 06:58:56 -0700
In-Reply-To: <DS0PR11MB63739BE4347EC6369ED22EBADC0E2@DS0PR11MB6373.namprd11.prod.outlook.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240417150354.275353-1-wei.w.wang@intel.com> <Zh_4QN7eFxyu9hgA@google.com>
 <DS0PR11MB63739BE4347EC6369ED22EBADC0E2@DS0PR11MB6373.namprd11.prod.outlook.com>
Message-ID: <ZiEnIFW3ZQhDwdZ-@google.com>
Subject: Re: [RFC PATCH v1] KVM: x86: Introduce macros to simplify KVM_X86_OPS
 static calls
From: Sean Christopherson <seanjc@google.com>
To: Wei W Wang <wei.w.wang@intel.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"

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.

  static inline void kvm_ops_update(struct kvm_x86_init_ops *ops)
  {
	memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));

#define __KVM_X86_OP(func) \
	static_call_update(kvm_x86_##func, kvm_x86_ops.func);
#define KVM_X86_OP(func) \
	WARN_ON(!kvm_x86_ops.func); __KVM_X86_OP(func)
#define KVM_X86_OP_OPTIONAL __KVM_X86_OP
#define KVM_X86_OP_OPTIONAL_RET0(func) \
	static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
					   (void *)__static_call_return0);
#include <asm/kvm-x86-ops.h>
#undef __KVM_X86_OP

	kvm_pmu_ops_update(ops->pmu_ops);
  }

> > kvm_ops_update() already WARNs if a mandatory hook isn't defined, so doing
> > more checks at runtime wouldn't provide any value.
> 
> > 
> > As for the name, what about KVM_X86_CALL() instead of KVM_X86_SC()?  Two
> > extra characters, but should make it much more obvious what's going on for
> > readers that aren't familiar with the infrastructure.
> 
> I thought the macro definition is quite intuitive and those encountering it for the
> first time could get familiar with it easily from the definition.
> Similarly, KVM_X86_CALL() is fine to me, despite the fact that it doesn't explicitly
> denote "static" calls.

Repeat readers/developers will get used to KVM_X86_SC(), but for someone that has
never read KVM code before, KVM_X86_SC() is unintuitive, e.g. basically requires
looking at the implementation, especially if the reader isn't familiar with the
kernel's static call framework.

In other words, there needs to be "CALL" somewhere in the name to convey the basic
gist of the code.  And IMO, the "static" part is a low level detail that isn't
necessary to understand the core functionality of the code, so omitting it to
shorten line lengths is a worthwhile tradeoff.