Re: [RFC 08/10] x86: wire cpu_has_svm/vmx_* to false when svm/vmx not enabled

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/14/23 18:36, Jan Beulich wrote:
> On 13.02.2023 15:57, Xenia Ragiadakou wrote:
> > To be able to use cpu_has_svm/vmx_* macros in common code without enclosing
> Both in the title and here the spelling you use made me first think that
> you talk about "cpu_has_svm". May I suggest you follow what we typically
> do and use "cpu_has_{svm,vmx}_*" instead in such a case? However, the
> title may want changing anyway:
Ok, I will use the conventional way from now on.

> > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h
> > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
> > @@ -80,6 +80,7 @@ extern u32 svm_feature_flags;
> >   #define SVM_FEATURE_SSS           19 /* NPT Supervisor Shadow Stacks */
> >   #define SVM_FEATURE_SPEC_CTRL     20 /* MSR_SPEC_CTRL virtualisation */
> >   
> > +#ifdef CONFIG_AMD_SVM
> >   #define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f)))
> Why don't you simply provide a 2nd form of this one macro, leaving all
> others alone?
You are right. That way there will be less changes.

> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > @@ -294,6 +294,7 @@ extern u64 vmx_ept_vpid_cap;
> >   
> >   #define VMX_TSC_MULTIPLIER_MAX                  0xffffffffffffffffULL
> >   
> > +#ifdef CONFIG_INTEL_VMX
> >   #define cpu_has_wbinvd_exiting \
> >       (vmx_secondary_exec_control & SECONDARY_EXEC_WBINVD_EXITING)
> >   #define cpu_has_vmx_virtualize_apic_accesses \
> > @@ -352,6 +353,36 @@ extern u64 vmx_ept_vpid_cap;
> >       (vmx_secondary_exec_control & SECONDARY_EXEC_BUS_LOCK_DETECTION)
> >   #define cpu_has_vmx_notify_vm_exiting \
> >       (vmx_secondary_exec_control & SECONDARY_EXEC_NOTIFY_VM_EXITING)
> > +#else
> > +#define cpu_has_wbinvd_exiting false
> > +#define cpu_has_vmx_virtualize_apic_accesses false
> > +#define cpu_has_vmx_tpr_shadow false
> > +#define cpu_has_vmx_vnmi false
> > +#define cpu_has_vmx_msr_bitmap false
> > +#define cpu_has_vmx_secondary_exec_control false
> > +#define cpu_has_vmx_ept false
> > +#define cpu_has_vmx_dt_exiting false
> > +#define cpu_has_vmx_vpid false
> > +#define cpu_has_monitor_trap_flag false
> > +#define cpu_has_vmx_pat false
> > +#define cpu_has_vmx_efer false
> > +#define cpu_has_vmx_unrestricted_guest false
> > +#define vmx_unrestricted_guest(v) false
> > +#define cpu_has_vmx_ple false
> > +#define cpu_has_vmx_apic_reg_virt false
> > +#define cpu_has_vmx_virtual_intr_delivery false
> > +#define cpu_has_vmx_virtualize_x2apic_mode false
> > +#define cpu_has_vmx_posted_intr_processing false
> > +#define cpu_has_vmx_vmcs_shadowing false
> > +#define cpu_has_vmx_vmfunc false
> > +#define cpu_has_vmx_virt_exceptions false
> > +#define cpu_has_vmx_pml false
> > +#define cpu_has_vmx_mpx false
> > +#define cpu_has_vmx_xsaves false
> > +#define cpu_has_vmx_tsc_scaling false
> > +#define cpu_has_vmx_bus_lock_detection false
> > +#define cpu_has_vmx_notify_vm_exiting false
> > +#endif /* CONFIG_INTEL_VMX */
> For VMX you first of all want to separate out vmx_unrestricted_guest(v).
> Maybe we can even get away without a 2nd form. Then I think it would be
> much neater a change if, like we have for SVM, a couple (three I think)
> helper macros were introduced, which then is all that needs providing a
> 2nd form for. (Both steps may want doing in separate, prereq patches.)
Ok will do.

> > @@ -374,8 +405,12 @@ extern u64 vmx_ept_vpid_cap;
> >   #define VMX_BASIC_DEFAULT1_ZERO               (1ULL << 55)
> >   
> >   extern u64 vmx_basic_msr;
> > +#ifdef CONFIG_INTEL_VMX
> >   #define cpu_has_vmx_ins_outs_instr_info \
> >       (!!(vmx_basic_msr & VMX_BASIC_INS_OUT_INFO))
> > +#else
> > +#define cpu_has_vmx_ins_outs_instr_info false
> > +#endif /* CONFIG_INTEL_VMX */
> I don't think you need an alternative here - it's used solely in VMX
> code. If one wanted to this could even be moved to vmcs.c.
Ok. I ll take a closer look at this one.

> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > @@ -289,6 +289,7 @@ typedef union cr_access_qual {
> >   
> >   extern uint8_t posted_intr_vector;
> >   
> > +#ifdef CONFIG_INTEL_VMX
> >   #define cpu_has_vmx_ept_exec_only_supported        \
> >       (vmx_ept_vpid_cap & VMX_EPT_EXEC_ONLY_SUPPORTED)
> >   
> > @@ -301,6 +302,17 @@ extern uint8_t posted_intr_vector;
> >   #define cpu_has_vmx_ept_ad    (vmx_ept_vpid_cap & VMX_EPT_AD_BIT)
> >   #define cpu_has_vmx_ept_invept_single_context   \
> >       (vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
> > +#else
> > +#define cpu_has_vmx_ept_exec_only_supported false
> > +
> > +#define cpu_has_vmx_ept_wl4_supported false
> > +#define cpu_has_vmx_ept_mt_uc false
> > +#define cpu_has_vmx_ept_mt_wb false
> > +#define cpu_has_vmx_ept_2mb false
> > +#define cpu_has_vmx_ept_1gb false
> > +#define cpu_has_vmx_ept_ad false
> > +#define cpu_has_vmx_ept_invept_single_context false
> > +#endif /* CONFIG_INTEL_VMX */
> Same suggestion for introducing a helper macro here, which would then
> also be usable ...
>
> > @@ -310,12 +322,18 @@ extern uint8_t posted_intr_vector;
> >   #define INVEPT_SINGLE_CONTEXT   1
> >   #define INVEPT_ALL_CONTEXT      2
> >   
> > +#ifdef CONFIG_INTEL_VMX
> >   #define cpu_has_vmx_vpid_invvpid_individual_addr                    \
> >       (vmx_ept_vpid_cap & VMX_VPID_INVVPID_INDIVIDUAL_ADDR)
> >   #define cpu_has_vmx_vpid_invvpid_single_context                     \
> >       (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT)
> >   #define cpu_has_vmx_vpid_invvpid_single_context_retaining_global    \
> >       (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL)
> > +#else
> > +#define cpu_has_vmx_vpid_invvpid_individual_addr false
> > +#define cpu_has_vmx_vpid_invvpid_single_context false
> > +#define cpu_has_vmx_vpid_invvpid_single_context_retaining_global false
> > +#endif /* CONFIG_INTEL_VMX */
> ... here.
Thanks.

> Jan
--
Xenia