Re: [PATCH v4.5 3/8] VMX: tertiary execution control infrastructure

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Feb 05, 2024 at 02:37:44PM +0100, Jan Beulich wrote:
> This is a prereq to enabling e.g. the MSRLIST feature.
> 
> Note that the PROCBASED_CTLS3 MSR is different from other VMX feature
> reporting MSRs, in that all 64 bits report allowed 1-settings.
> 
> vVMX code is left alone, though, for the time being.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v4.5: Bump zero-padding width in vmcs_dump_vcpu(). Add
>       TERTIARY_EXEC_VIRT_SPEC_CTRL. Constify
>       vmx_update_tertiary_exec_control()'s parameter. Re-base.
> v2: New.
> 
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -164,6 +164,7 @@ static int cf_check parse_ept_param_runt
>  u32 vmx_pin_based_exec_control __read_mostly;
>  u32 vmx_cpu_based_exec_control __read_mostly;
>  u32 vmx_secondary_exec_control __read_mostly;
> +uint64_t vmx_tertiary_exec_control __read_mostly;
>  u32 vmx_vmexit_control __read_mostly;
>  u32 vmx_vmentry_control __read_mostly;
>  u64 vmx_ept_vpid_cap __read_mostly;
> @@ -228,10 +229,32 @@ static u32 adjust_vmx_controls(
>      return ctl;
>  }
>  
> -static bool cap_check(const char *name, u32 expected, u32 saw)
> +static uint64_t adjust_vmx_controls2(
> +    const char *name, uint64_t ctl_min, uint64_t ctl_opt, unsigned int msr,
> +    bool *mismatch)
> +{
> +    uint64_t vmx_msr, ctl = ctl_min | ctl_opt;
> +
> +    rdmsrl(msr, vmx_msr);
> +
> +    ctl &= vmx_msr; /* bit == 0 ==> must be zero */
> +
> +    /* Ensure minimum (required) set of control bits are supported. */
> +    if ( ctl_min & ~ctl )
> +    {
> +        *mismatch = true;
> +        printk("VMX: CPU%u has insufficient %s (%#lx; requires %#lx)\n",
> +               smp_processor_id(), name, ctl, ctl_min);
> +    }
> +
> +    return ctl;
> +}
> +
> +static bool cap_check(
> +    const char *name, unsigned long expected, unsigned long saw)
>  {
>      if ( saw != expected )
> -        printk("VMX %s: saw %#x expected %#x\n", name, saw, expected);
> +        printk("VMX %s: saw %#lx expected %#lx\n", name, saw, expected);
>      return saw != expected;
>  }
>  
> @@ -241,6 +264,7 @@ static int vmx_init_vmcs_config(bool bsp
>      u32 _vmx_pin_based_exec_control;
>      u32 _vmx_cpu_based_exec_control;
>      u32 _vmx_secondary_exec_control = 0;
> +    uint64_t _vmx_tertiary_exec_control = 0;
>      u64 _vmx_ept_vpid_cap = 0;
>      u64 _vmx_misc_cap = 0;
>      u32 _vmx_vmexit_control;
> @@ -274,7 +298,8 @@ static int vmx_init_vmcs_config(bool bsp
>      opt = (CPU_BASED_ACTIVATE_MSR_BITMAP |
>             CPU_BASED_TPR_SHADOW |
>             CPU_BASED_MONITOR_TRAP_FLAG |
> -           CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
> +           CPU_BASED_ACTIVATE_SECONDARY_CONTROLS |
> +           CPU_BASED_ACTIVATE_TERTIARY_CONTROLS);
>      _vmx_cpu_based_exec_control = adjust_vmx_controls(
>          "CPU-Based Exec Control", min, opt,
>          MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
> @@ -338,6 +363,15 @@ static int vmx_init_vmcs_config(bool bsp
>              MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>      }
>  
> +    if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS )
> +    {
> +        uint64_t opt = 0;
> +
> +        _vmx_tertiary_exec_control = adjust_vmx_controls2(
> +            "Tertiary Exec Control", 0, opt,
> +            MSR_IA32_VMX_PROCBASED_CTLS3, &mismatch);
> +    }
> +
>      /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */
>      if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
>                                          SECONDARY_EXEC_ENABLE_VPID) )
> @@ -468,6 +502,7 @@ static int vmx_init_vmcs_config(bool bsp
>          vmx_pin_based_exec_control = _vmx_pin_based_exec_control;
>          vmx_cpu_based_exec_control = _vmx_cpu_based_exec_control;
>          vmx_secondary_exec_control = _vmx_secondary_exec_control;
> +        vmx_tertiary_exec_control  = _vmx_tertiary_exec_control;
>          vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
>          vmx_vmexit_control         = _vmx_vmexit_control;
>          vmx_vmentry_control        = _vmx_vmentry_control;
> @@ -503,6 +538,9 @@ static int vmx_init_vmcs_config(bool bsp
>              "Secondary Exec Control",
>              vmx_secondary_exec_control, _vmx_secondary_exec_control);
>          mismatch |= cap_check(
> +            "Tertiary Exec Control",
> +            vmx_tertiary_exec_control, _vmx_tertiary_exec_control);
> +        mismatch |= cap_check(
>              "VMExit Control",
>              vmx_vmexit_control, _vmx_vmexit_control);
>          mismatch |= cap_check(
> @@ -1080,6 +1118,7 @@ static int construct_vmcs(struct vcpu *v
>          v->arch.hvm.vmx.exec_control |= CPU_BASED_RDTSC_EXITING;
>  
>      v->arch.hvm.vmx.secondary_exec_control = vmx_secondary_exec_control;
> +    v->arch.hvm.vmx.tertiary_exec_control  = vmx_tertiary_exec_control;
>  
>      /*
>       * Disable features which we don't want active by default:
> @@ -1134,6 +1173,10 @@ static int construct_vmcs(struct vcpu *v
>          __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>                    v->arch.hvm.vmx.secondary_exec_control);
>  
> +    if ( cpu_has_vmx_tertiary_exec_control )
> +        __vmwrite(TERTIARY_VM_EXEC_CONTROL,
> +                  v->arch.hvm.vmx.tertiary_exec_control);
> +
>      /* MSR access bitmap. */
>      if ( cpu_has_vmx_msr_bitmap )
>      {
> @@ -2068,10 +2111,12 @@ void vmcs_dump_vcpu(struct vcpu *v)
>                 vmr(HOST_PERF_GLOBAL_CTRL));
>  
>      printk("*** Control State ***\n");
> -    printk("PinBased=%08x CPUBased=%08x SecondaryExec=%08x\n",
> +    printk("PinBased=%08x CPUBased=%08x\n",
>             vmr32(PIN_BASED_VM_EXEC_CONTROL),
> -           vmr32(CPU_BASED_VM_EXEC_CONTROL),
> -           vmr32(SECONDARY_VM_EXEC_CONTROL));
> +           vmr32(CPU_BASED_VM_EXEC_CONTROL));
> +    printk("SecondaryExec=%08x TertiaryExec=%016lx\n",

I thought you wanted to split the print into two 32bit halves here?

Thanks, Roger.