Re: [XEN][PATCH 1/2] x86: hvm: vmx: fix runtime vmx presence check for !CONFIG_INTEL_VMX case

[Andrew Cooper <andrew@xxxxxxx>]

On 16/09/2025 9:57 am, Grygorii Strashko wrote:
> Hi Jan,
>
> On 16.09.25 17:34, Jan Beulich wrote:
>> On 16.09.2025 12:32, Grygorii Strashko wrote:
>>> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>>>
>>> Since commit b99227347230 ("x86: Fix AMD_SVM and INTEL_VMX
>>> dependency") the
>>> HVM Intel VT-x support can be gracefully disabled, but it still
>>> keeps VMX
>>> code partially built-in, because HVM code uses mix of:
>>>
>>>   - "cpu_has_vmx" macro, which doesn't account for CONFIG_INTEL_VMX cfg
>>>   - "using_vmx()" function, which accounts for CONFIG_INTEL_VMX cfg
>>>
>>> for runtime VMX availability checking. As result compiler DCE can't
>>> remove
>>> all, unreachable VMX code.
>>>
>>> Fix it by sticking to "cpu_has_vmx" macro usage only which is
>>> updated to
>>> account CONFIG_INTEL_VMX cfg.
>>>
>>> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>>> ---
>>> Hi
>>>
>>> It could be good to have it in 4.21, so vmx/svm disabling
>>> option will be in complete state within 4.21 version.
>>
>> Imo this isn't release critical and has come too late. It's of course
>> Oleksii's call in the end.
>>
>>> --- a/xen/arch/x86/include/asm/cpufeature.h
>>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>>> @@ -136,7 +136,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_INTEL_VMX) && \
>>> +                                 boot_cpu_has(X86_FEATURE_VMX))
>>
>> I'm pretty sure using_vmx() was introduced precisely to avoid the use of
>> IS_ENABLED() here. What is completely missing from the description is a
>> discussion of the effect of this change on pre-existing uses of the
>> macro. ISTR there being at least one instance which would break with
>> that change. And no, I'm not looking forward to digging that out again,
>> when I already did at the time the using_vmx() was suggested and then
>> implemented. (I can't exclude it was the SVM counterpart; we want to
>> keep both in sync in any event, imo.)
>
> Thank you for your comments and sorry for not digging into the history of
> the related patches.
>
> All, please ignore these patches as existing places. where
> cpu_has_vmx/smv
> are still used, need to be revised one by one.
>

Off the top of my head, fixups to MSR_FEATURE_CONTROL, and AMD SKINIT
need cpu_has_vmx/svm not guarded by Kconfig like this.

~Andrew