Re: [XEN PATCH v2 07/15] x86: guard cpu_has_{svm/vmx} macros with CONFIG_{SVM/VMX}

[Sergiy Kibrik <sergiy_kibrik@xxxxxxxx>]

23.05.24 17:50, Jan Beulich:
> On 23.05.2024 15:07, Sergiy Kibrik wrote:
> > 16.05.24 14:12, Jan Beulich:
> > > On 15.05.2024 11:12, Sergiy Kibrik wrote:
> > > > --- a/xen/arch/x86/include/asm/cpufeature.h
> > > > +++ b/xen/arch/x86/include/asm/cpufeature.h
> > > > @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat)
> > > >    #define cpu_has_sse3            boot_cpu_has(X86_FEATURE_SSE3)
> > > >    #define cpu_has_pclmulqdq       boot_cpu_has(X86_FEATURE_PCLMULQDQ)
> > > >    #define cpu_has_monitor         boot_cpu_has(X86_FEATURE_MONITOR)
> > > > -#define cpu_has_vmx             boot_cpu_has(X86_FEATURE_VMX)
> > > > +#define cpu_has_vmx             ( IS_ENABLED(CONFIG_VMX) && \
> > > > +                                  boot_cpu_has(X86_FEATURE_VMX))
> > > >    #define cpu_has_eist            boot_cpu_has(X86_FEATURE_EIST)
> > > >    #define cpu_has_ssse3           boot_cpu_has(X86_FEATURE_SSSE3)
> > > >    #define cpu_has_fma             boot_cpu_has(X86_FEATURE_FMA)
> > > > @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat)
> > > >    
> > > >    /* CPUID level 0x80000001.ecx */
> > > >    #define cpu_has_cmp_legacy      boot_cpu_has(X86_FEATURE_CMP_LEGACY)
> > > > -#define cpu_has_svm             boot_cpu_has(X86_FEATURE_SVM)
> > > > +#define cpu_has_svm             ( IS_ENABLED(CONFIG_SVM) && \
> > > > +                                  boot_cpu_has(X86_FEATURE_SVM))
> > > >    #define cpu_has_sse4a           boot_cpu_has(X86_FEATURE_SSE4A)
> > > >    #define cpu_has_xop             boot_cpu_has(X86_FEATURE_XOP)
> > > >    #define cpu_has_skinit          boot_cpu_has(X86_FEATURE_SKINIT)
> > > Hmm, leaving aside the style issue (stray blanks after opening parentheses,
> > > and as a result one-off indentation on the wrapped lines) I'm not really
> > > certain we can do this. The description goes into detail why we would want
> > > this, but it doesn't cover at all why it is safe for all present (and
> > > ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks
> > > just to derive further knowledge from that, without them being directly
> > > related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(),
> > > for example. While it looks to be okay there, it may give you an idea of
> > > what I mean.
> > >
> > > Things might become better separated if instead for such checks we used
> > > host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But
> > > that's still pretty far out, I'm afraid.
> > I've followed a suggestion you made for patch in previous series:
[..]
> See the "If not, ..." that I had put there. Doing the change just mechanically
> isn't enough, you also need to make clear (in the description) that you
> verified it's safe to have this way.
>
> > yet if this approach can potentially be unsafe (I'm not completely sure
> > it's safe), should we instead fallback to the way it was done in v1
> > series? I.e. guard calls to vmx/svm-specific calls where needed, like in
> > these 3 patches:
[..]
> I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want
> to have two new helpers (say using_svm() and using_vmx()), to be used in place
> of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation
> would then kind of implicitly answer the safety question above, as at every
> use site you'd need to judge whether the replacement is correct. If it's
> correct everywhere, the construct(s) as proposed in this version could then be
> considered to be used in this very shape (instead of introducing the two new
> helpers). But of course the transition could also be done gradually then,
> touching only those uses that previously you touched in 1), 2), and 3).
now I might be seeing your concerns, if I understood correctly, 
situation is the following.
 As an example of cpu_has_vmx macro, it can be used to prove either of 
following two statements: 1) VMX features can be used or 2) CPU provides 
VMX features.
Currently they're the same for Xen, yet after this patch series they're 
not, as the situation possible when non-vmx build would be able to get 
executed on vmx-enabled machine. E.g. the case of PV guest, or (if that 
makes any sense) at least hypervisor's code is still able to run until 
HVM guest has to be created. Changes in this patch makes 
indistinguishable for a user whether VMX support is absent in code or in 
hardware -- hence we may need two separate macros for these.
Still the question remains whether a separate macro to check if CPU 
provides VMX/SVM is really needed at all at this point.
I've counted only 16 uses of cpu_has_vmx in the code, not that much to 
check every one of them, so I did that.
Most of uses are obviously checks before using vmx features, so logic 
not broken.
As for the others, the surrounding context presumes that HVM domain 
required there or had already been created. But non-vmx build can't 
create HVM VMX domain anyway, so the logic not broken either.
As for cpu_has_svm only 8 uses I've counted, all but one also don't seem 
to break logic as described above. One check of cpu_has_svm in 
init_speculation_mitigations(), where default speculation control flag 
gets set, not uses SVM features directly. Yet from the comment I can 
assume that it's also related to running HVM domain and usage of VMX 
features at later time.
With all above, at the moment there doesn't seem to be uses of 
cpu_has_{svm,vmx} macros without subsequent usage of svm/vmx features, 
so this patch should be quite safe.
Please let me know whether the above reasoning makes sense.

 -Sergiy