Re: [PATCH 2/3] KVM: VMX: Use proper type-safe functions for vCPU => LBRs helpers

From: Sean Christopherson <seanjc@google.com>
To: Like Xu <like.xu.linux@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] KVM: VMX: Use proper type-safe functions for vCPU => LBRs helpers
Date: Fri, 29 Jul 2022 16:08:24 +0000	[thread overview]
Message-ID: <YuQF+BBWA1SkXjT/@google.com> (raw)
In-Reply-To: <5f93760c-cb93-2c58-11d7-f9ddef7f640c@gmail.com>

On Fri, Jul 29, 2022, Like Xu wrote:
> On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > Turn vcpu_to_lbr_desc() and vcpu_to_lbr_records() into functions in order
> > to provide type safety, to document exactly what they return, and to
> 
> Considering the prevalence of similar practices, perhaps we (at least me)
> need doc more benefits of "type safety" or the risks of not doing so.

There already is documentation, see "#ifdef and preprocessor use in general" in
Documentation/process/4.Coding.rst:

  C preprocessor macros present a number of hazards, including possible
  multiple evaluation of expressions with side effects and no type safety.
  If you are tempted to define a macro, consider creating an inline function
  instead.  The code which results will be the same, but inline functions are
  easier to read, do not evaluate their arguments multiple times, and allow
  the compiler to perform type checking on the arguments and return value.

To elaborate on type safety, or lack thereof, let's pretend that struct kvm_vcpu's
"mutex" was actually named "lock", i.e. that both struct kvm_vcpu and struct kvm had:

	struct mutex lock;

And for whatever reason, we wanted to add helpers to lock/unlock.  If the helpers
were implemented via macros, e.g.

  	#define kvm_lock_vm(kvm) mutex_lock(&(kvm)->lock)

then this ends up being 100% legal code from the compilers perspective

	kvm_lock_vm(vcpu);

i.e. it will compile cleanly and only cause problems at run time because the
preprocesor expands that to:

	mutex_lock(&vcpu->lock);

A function on the other hand provides type safety because even though both kvm.lock
and kvm_vcpu.lock exist, the compiler will yell due to attempting to pass a
"struct kvm_vcpu *" into a function that takes a "struct kvm *kvm".

	static inline void kvm_lock_vm(struct kvm *kvm)
	{
		mutex_lock(&kvm->lock);
	}

There are instances where a macro is used because it's deemed to be the lesser
evil.  E.g. KVM x86 does

  #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)

to allow inlining without creating a circular dependency.  include/linux/kvm_host.h
uses kvm_arch_vcpu_memslots_id() and so it needs to be fully defined by
arch/x86/include/asm/kvm_host.h.  But because linux/kvm_host.h includes asm/kvm_host.h,
asm/kvm_host.h can't deference @vcpu due to "struct kvm_vcpu" not yet being defined.

The macro trick works for this case because the preprocessor doesn't compile or check
anything, it just expands the macro in its user.  I.e. the derefence of @vcpu occurs
in kvm_vcpu_memslots() from the compilers perspective, and at that point "struct kvm_vcpu"
is fully defined and everyone is happy.

There's still some amount of risk in misuing kvm_arch_vcpu_memslots_id(), but in
this case we've decided that the benefits of inlining outweigh the risk of misuse.

> > allow consuming the helpers in vmx.h.  Move the definitions as necessary
> > (the macros "reference" to_vmx() before its definition).
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/vmx/vmx.h | 26 +++++++++++++++++---------
> >   1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 286c88e285ea..690421b7d26c 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -6,6 +6,7 @@
> >   #include <asm/kvm.h>
> >   #include <asm/intel_pt.h>
> > +#include <asm/perf_event.h>
> >   #include "capabilities.h"
> >   #include "kvm_cache_regs.h"
> > @@ -91,15 +92,6 @@ union vmx_exit_reason {
> >   	u32 full;
> >   };
> > -#define vcpu_to_lbr_desc(vcpu) (&to_vmx(vcpu)->lbr_desc)
> > -#define vcpu_to_lbr_records(vcpu) (&to_vmx(vcpu)->lbr_desc.records)
> 
> More targets can be found in the arch/x86/kvm/pmu.h:
> 
> #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
> #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> #define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)

We should definitely convert those, assuming it's not too painful and not
completely impossible due to circular dependencies.