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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2024 12:27, Sergiy Kibrik wrote:
> 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.

Note how to comment (necessarily) mixes SVM and VT-x. The !cpu_have_svm
really looks to mean cpu_has_vmx there (i.e. absence of SVM on a HVM-only
path implying VT-x). This would be broken if cpu_has_svm had
IS_ENABLED() added to it. You'll want to consult the commit introducing
the construct as well as perhaps Andrew directly as to whether switching
to cpu_has_vmx here would be appropriate. I'm pretty sure there was a
reason why this was written using the negative (!SVM) form.

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

Fundamentally it does, with said one exception. Yet even in the absence
of that exception the question would then remain whether we really want
to close the road to one of the two possible uses of these constructs.

Jan