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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2024 01:36, Stefano Stabellini wrote:
> On Thu, 23 May 2024, Jan Beulich wrote:
>> 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:
>>>
>>> https://lore.kernel.org/xen-devel/8fbd604e-5e5d-410c-880f-2ad257bbe08a@xxxxxxxx/
>>
>> 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.
> 
> What does it mean to "verified it's safe to have this way"? "Safe" in
> what way?

"Safe" as in "not breaking existing logic", anywhere.

Jan