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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2024 11:59, Sergiy Kibrik wrote:
> 22.07.24 14:43, Jan Beulich:
>> 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.
>>
> 
> these modifications in .c files are made mainly to track places where 
> build fails and to highlight where global variables are causing a trouble.
> cpu_has_monitor_trap_flag and fellow macros are used within VMX code 
> mostly and don't need these checks inside of them most of the time -- at 
> least so I felt.
> 
> As for those cpu_has_vmx_* macros that are modified in header -- these 
> are being checked in a bit more tricky way, so instead of making complex 
> conditionals even more complicated, I've integrated CONFIG_VMX condition 
> check inside these macros instead.
> 
> Ofc we can proceed with only .h files modifications, if this is more 
> consistent with what is planned for cpu_has_vmx_* checks in future.

Going that route would imo also be more future proof towards guarding
calls to yet to be introduced VMX-only functions.

Jan