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
