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