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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:

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

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

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

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

Jan