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

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Tue Sep 16, 2025 at 7:14 PM CEST, Andrew Cooper wrote:
> 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.)

Apologies if this has already been discussed, but I didn't participate in prior
discussions. Targeted lookups in lore are not shedding a lot of light either.

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

What do you mean? AFAICS SKINIT is guarded by cpu_has_skinit, not cpu_has_svm.

And MSR_IA32_FEATURE_CONTROL tweaking seems self-contained in xen/hvm/vmx/ which
is compiled out when !CONFIG_INTEL_VMX.

For the hypothetical case in which we might want to know the real HW value
we can go look at the raw policy, as in "raw_cpu_policy.basic.vmx" or
"raw_cpu_policy.extd.svm". Or what's mentioned in passing here.

https://lore.kernel.org/xen-devel/a881c6a6-2c36-4e5c-8336-21cd0e14b873@xxxxxxxx/

Forcing the common case to use a helper and leaving the rare case in the
shorthand macro seems like a bad idea. This ought to follow what cpu_has_nx
already does.

Is there a specific code instance in which having IS_ENABLED() in the
cpu_has_{svm,vmx} macros would cause issues today? While there are some dubious
choices of svm vs vmx with or without negation, they all seem to resolve
to correct code, with less codegen after IS_ENABLED() ends up in all the
conditionals.

IOW: I have seen fear of incorrectness, but not proof of it. Now, obviously the
burden of proof rests on the submitter, indeed, but I'd like to know where we
stand in terms of what that proof would look like. A naive grep shows not many
sites to check.

  $git grep cpu_has_svm | grep -v cpu_has_svm_ | wc -l
  6

  $git grep cpu_has_vmx | grep -v cpu_has_vmx_ | wc -l
  11

cpu_has_X_Y would be off when cpu_has_X is off, but those shouldn't matter for
this discussion.

Am I missing something here?

Cheers,
Alejandro