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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2025 13:23, Alejandro Vallejo wrote:
> 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.

Such a proof could be a statement clarifying that all use sites were audited. If
then a reviewer found an issue with that, it would get interesting (as 
[possibly]
in: it being questionable whether an audit was actually done; what I mean to say
here is that it's not a matter of merely stating that an audit was [supposedly]
done).

Jan

> 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