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

[Sergiy Kibrik <sergiy_kibrik@xxxxxxxx>]

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