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
