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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Alejandro,

On 24.09.25 14: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. A naive grep shows not many
> sites to check.
>
>    $git grep cpu_has_svm | grep -v cpu_has_svm_ | wc -l
>    6
arch/x86/flushtlb.c:    bool asid = is_hvm_domain(d) && (cpu_has_svm || shadow);
arch/x86/hvm/hvm.c:    if ( nestedhvm_enabled(v->domain) && cpu_has_svm &&
arch/x86/spec_ctrl.c:        if ( !cpu_has_svm )

not checked yet.

>    $git grep cpu_has_vmx | grep -v cpu_has_vmx_ | wc -l
>    11
Here:
1) arch/x86/hvm/dom0_build.c:    if ( cpu_has_vmx && paging_mode_hap(d) && 
!vmx_unrestricted_guest(v) )
   arch/x86/hvm/hvm.c:        if ( !paging_mode_hap(d) || !cpu_has_vmx )

^ is safe to opt-opt. I've sent patch [1]
It gives biggest coverage benefit actually.

2) arch/x86/hvm/viridian/viridian.c:    *(u8  *)(p + 7) = (cpu_has_vmx ? 0xc1 : 
0xd9);
There is strict dependency on HW. In general, viridian should never be enabled 
on INTEL, for example,
if !INTEL_VMX. So, could be opt out

3) arch/x86/mm/mem_sharing.c:        if ( unlikely(!is_hvm_domain(d) || 
!cpu_has_vmx) )
MEM_SHARING has strict dependency on INTEL_VMX, so it is "doesn't matter" case.
MEM_SHARING should never be enabled if !INTEL_VMX.

4) arch/x86/hvm/nestedhvm.c:    unsigned nr = cpu_has_vmx ? 2 : 3;
There shadow_io_bitmap pages allocated, page number to be used returned by
nestedhvm_vcpu_iomap_get(). Opt-out should be safe for !INTEL_VMX as nr==3 and
nestedhvm_vcpu_iomap_get() will never overflow shadow_io_bitmap[].

5) arch/x86/mm/mem_access.c:    return is_hvm_domain(d) && cpu_has_vmx && 
hap_enabled(d);
It is p2m_mem_access_sanity_check() called from mem_access_memop().
Seems like whole XENMEM_access_op (MEM_ACCESS) depends on INTEL_VMX on x86.
But there is dependency on VM_EVENT is defined already.

How to proceed???

6) arch/x86/hvm/monitor.c:    if ( cpu_has_vmx )
This is hvm_monitor_descriptor_access():
    if ( cpu_has_vmx )
    {
        req.u.desc_access.arch.vmx.instr_info = exit_info;
        req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
    }

Should be safe to opt-out.

7) arch/x86/cpu-policy.c:    if ( cpu_has_vmx )
   arch/x86/cpu-policy.c:    if ( !cpu_has_vmx )
It is calculate_hvm_max_policy()

Place (a)
    if ( cpu_has_vmx )
    {

[below features have to be cleared if !INTEL_VMX, so can't just out-out 
cpu_has_vmx here.
 Possible option: remove "if ( cpu_has_vmx )"]

        if ( !cpu_has_vmx_rdtscp )
            __clear_bit(X86_FEATURE_RDTSCP, fs);

        if ( !cpu_has_vmx_invpcid )
            __clear_bit(X86_FEATURE_INVPCID, fs);

        if ( !cpu_has_vmx_mpx )
            __clear_bit(X86_FEATURE_MPX, fs);

        if ( !cpu_has_vmx_xsaves )
            __clear_bit(X86_FEATURE_XSAVES, fs);
    }

Place (b)
    if ( !cpu_has_vmx )
        __clear_bit(X86_FEATURE_PKS, fs);

Should be safe to out-out


> 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?
[1] 
https://patchwork.kernel.org/project/xen-devel/patch/20250924101417.229108-1-grygorii_strashko@xxxxxxxx/

--
Best regards,
-grygorii