Re: [XEN PATCH v4 10/14] x86/vmx: guard access to cpu_has_vmx_* in common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.07.2024 08:05, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5197,7 +5197,7 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>      {
>          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
>          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> -            if ( !cpu_has_monitor_trap_flag )
> +            if ( !IS_ENABLED(CONFIG_VMX) || !cpu_has_monitor_trap_flag )
>                  return -EOPNOTSUPP;
>              break;

Why at the use site here and ...

> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -196,7 +196,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t 
> leaf,
>          res->a = CPUID4A_RELAX_TIMER_INT;
>          if ( viridian_feature_mask(d) & HVMPV_hcall_remote_tlb_flush )
>              res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH;
> -        if ( !cpu_has_vmx_apic_reg_virt )
> +        if ( !IS_ENABLED(CONFIG_VMX) || !cpu_has_vmx_apic_reg_virt )
>              res->a |= CPUID4A_MSR_BASED_APIC;
>          if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
>              res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;
> @@ -236,7 +236,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t 
> leaf,
>  
>      case 6:
>          /* Detected and in use hardware features. */
> -        if ( cpu_has_vmx_virtualize_apic_accesses )
> +        if ( IS_ENABLED(CONFIG_VMX) && cpu_has_vmx_virtualize_apic_accesses )
>              res->a |= CPUID6A_APIC_OVERLAY;
>          if ( cpu_has_vmx_msr_bitmap || (read_efer() & EFER_SVME) )
>              res->a |= CPUID6A_MSR_BITMAPS;

... here (and in yet a few more places), but ...

> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> @@ -306,7 +306,8 @@ extern u64 vmx_ept_vpid_cap;
>  #define cpu_has_vmx_vnmi \
>      (vmx_pin_based_exec_control & PIN_BASED_VIRTUAL_NMIS)
>  #define cpu_has_vmx_msr_bitmap \
> -    (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP)
> +    (IS_ENABLED(CONFIG_VMX) && \
> +     vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP)

... for others right in the definitions, as was suggested before? Yet then
not consistently for all of them? Looks like if you did this consistently
here, you'd have no need at all to fiddle with any .c file.

Jan