Re: [PATCH v2 26/49] KVM: x86: Add a macro to init CPUID features that KVM emulates in software

[Sean Christopherson]

On Tue, Sep 10, 2024, Maxim Levitsky wrote:
> On Mon, 2024-08-05 at 12:59 -0700, Sean Christopherson wrote:
> > > And now we have:
> > > 
> > > kvm_cpu_cap_init_begin(CPUID_12_EAX);
> > >  feature_scattered(SGX1);
> > >  feature_scattered(SGX2);
> > >  feature_scattered(SGX_EDECCSSA);
> > > kvm_cpu_cap_init_end();
> > 
> > I don't love the syntax (mainly the need for a begin()+end()), but I'm a-ok
> > getting rid of the @mask param/input.
> > 
> > What about making kvm_cpu_cap_init() a variadic macro, with the relevant features
> > "unpacked" in the context of the macro?  That would avoid the need for a trailing
> > macro, and would provide a clear indication of when/where the set of features is
> > "initialized".
> > 
> > The biggest downside I see is that the last entry can't have a trailing comma,
> > i.e. adding a new feature would require updating the previous feature too.
> > 
> > #define kvm_cpu_cap_init(leaf, init_features...)			\
> > do {									\
> > 	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);	\
> > 	const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf;	\
> > 	u32 kvm_cpu_cap_virtualized= 0;					\
> > 	u32 kvm_cpu_cap_emulated = 0;					\
> > 	u32 kvm_cpu_cap_synthesized = 0;				\
> > 									\
> > 	init_features;							\
> > 									\
> > 	kvm_cpu_caps[leaf] = kvm_cpu_cap_virtualized;			\
> > 	kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) |			\
> > 			       kvm_cpu_cap_synthesized);		\
> > 	kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated;			\
> > } while (0)
> > 
> > 	kvm_cpu_cap_init(CPUID_1_ECX,
> > 		VIRTUALIZED_F(XMM3),
> > 		VIRTUALIZED_F(PCLMULQDQ),
> > 		VIRTUALIZED_F(SSSE3),
> > 		VIRTUALIZED_F(FMA),
> > 		VIRTUALIZED_F(CX16),
> > 		VIRTUALIZED_F(PDCM),
> > 		VIRTUALIZED_F(PCID),
> > 		VIRTUALIZED_F(XMM4_1),
> > 		VIRTUALIZED_F(XMM4_2),
> > 		EMULATED_F(X2APIC),
> > 		VIRTUALIZED_F(MOVBE),
> > 		VIRTUALIZED_F(POPCNT),
> > 		EMULATED_F(TSC_DEADLINE_TIMER),
> > 		VIRTUALIZED_F(AES),
> > 		VIRTUALIZED_F(XSAVE),
> > 		// DYNAMIC_F(OSXSAVE),
> > 		VIRTUALIZED_F(AVX),
> > 		VIRTUALIZED_F(F16C),
> > 		VIRTUALIZED_F(RDRAND),
> > 		EMULATED_F(HYPERVISOR)
> > 	);
> 
> Hi,
> 
> This is no doubt better than using '|'.
> 
> I still strongly prefer my version, because I don't really like the fact that
> _F macros have side effects, and yet passed as parameters to the
> kvm_cpu_cap_init function/macro.
> 
> Basically an unwritten rule, which I consider very important and because of which
> I raised my concerns over this patch series is that if a function has side effects,
> it should not be used as a parameter to another function, instead, it should be 
> called explicitly on its own.

Splitting hairs to some degree, but the above suggestion is distinctly different
than passing the _result_ of a function call as a parameter to another function.
The actual "call" happens within the body of kvm_cpu_cap_init().  

This is effectively the same as passing a function pointer to a helper, and that
function pointer implementation having side effects, which is quite common in the
kernel and KVM, e.g. msr_access_t, rmap_handler_t, tdp_handler_t, gfn_handler_t,
on_lock_fn_t, etc.

I 100% agree that it's unusual and subtle to essentially have a variable number
of function pointers, but I don't see it as being an inherently bad pattern,
especially since it is practically impossible to misuse _because_ the macro
unpacks the "calls" at compile time.

IMO, the part that is most gross is the macros operating on local variables, but
that behavior exists in all ideas we've discussed, probably because I'm pretty
sure it's unavoidable unless we do something even worse (way, waaaaay worse).

E.g. we could add 32 versions of kvm_cpu_cap_init() that invoke pairs of parameters
and pass in the variables

  fn1(f1, virtualized, emulated, synthesized)
  fn2(f2, virtualized, emulated, synthesized)
  fn3(f3, virtualized, emulated, synthesized)
  ...
  fnN(fN, virtualized, emulated, synthesized)

and

  kvm_cpu_cap_init19(CPUID_1_ECX,
	F, XMM3,
	F, PCLMULQDQ,
	F, SSE3,
	...
	EMULATED_F, HYPERVISOR
  );

But that's beyond horrific :-)

> If you strongly prefer the variadic macro over my begin/end API, I can live with
> that though, it is still better than '|'ing a mask with functions that have side
> effects.