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

[Maxim Levitsky]

On Mon, 2024-08-05 at 12:59 -0700, Sean Christopherson wrote:
> On Mon, Aug 05, 2024, mlevitsk@xxxxxxxxxx wrote:
> > У пт, 2024-07-26 у 17:06 -0700, Sean Christopherson пише:
> > > > > > And kvm_cpu_cap_init_begin, can set some cap_in_progress variable.
> > > > 
> > > > Ya, but then compile-time asserts become run-time asserts.
> > 
> > Not really, it all can be done with macros, in exactly the same way IMHO,
> > we do have BUILD_BUG_ON after all.
> > 
> > I am not against using macros, I am only against collecting a bitmask
> > while applying various side effects, and then passing the bitmask to
> > the kvm_cpu_cap_init.
> 
> Gah, I wasn't grokking that, obviously.  Sorry for not catching on earlier.
> 
> > > > To provide equivalent functionality, we also would need to pass in extra
> > > > state to begin/end() (as mentioned earlier).
> > 
> > Besides the number of leaf currently initialized, I don't see which other
> > extra state we need.
> > 
> > In fact I can prove that this is possible:
> > 
> > Roughly like this:
> > 
> > #define kvm_cpu_cap_init_begin(leaf)							\
> > do {											\
> >  const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf; 				\
> >  u32 kvm_cpu_cap_emulated = 0; 								\
> >  u32 kvm_cpu_cap_synthesized = 0; 							\
> > 	u32 kvm_cpu_cap_regular = 0;
> 
> Maybe "virtualized" instead of "regular"?
> 
> > #define feature_scattered(name) 							\
> >  BUILD_BUG_ON(X86_FEATURE_##name >= MAX_CPU_FEATURES); 					\
> >  KVM_VALIDATE_CPU_CAP_USAGE(name); 							\
> > 											\
> > 	if (boot_cpu_has(X86_FEATURE_##name) 						\
> > 		kvm_cpu_cap_regular |= feature_bit(name);
> > 
> > 
> > #define kvm_cpu_cap_init_end() 								\
> > 	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);			\
> > 											\
> > 	if (kvm_cpu_cap_init_in_progress < NCAPINTS) 					\
> >  		kvm_cpu_caps[kvm_cpu_cap_init_in_progress] &= kvm_cpu_cap_regular; 	\
> >  	else 										\
> >  		kvm_cpu_caps[kvm_cpu_cap_init_in_progress] = kvm_cpu_cap_regular; 	\
> >  											\
> >  	kvm_cpu_caps[kvm_cpu_cap_init_in_progress] &= (raw_cpuid_get(cpuid) | 		\
> >  	kvm_cpu_cap_synthesized); 							\
> >  	kvm_cpu_caps[kvm_cpu_cap_init_in_progress] |= kvm_cpu_cap_emulated; 		\
> > } while(0);
> > 
> > 
> > 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.

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.

Best regards,
	Maxim Levitsky


> 
> 
> Alternatively, we could force a trailing comma by omitting the semicolon after
> init_features, but that looks weird for the the macro itself, and arguably a bit
> weird for the users too.
>