Re: [RFC PATCH 03/12] kvm/vmx: Introduce the new tertiary processor-based VM-execution controls

[Robert Hoo <robert.hu@linux.intel.com>]

On Mon, 2021-01-25 at 10:41 +0100, Vitaly Kuznetsov wrote:
> Robert Hoo <robert.hu@linux.intel.com> writes:
> 
[...]
> >  
> >  /*
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 47b8357..12a926e 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2376,6 +2376,7 @@ static __init int setup_vmcs_config(struct
> > vmcs_config *vmcs_conf,
> >  	u32 _pin_based_exec_control = 0;
> >  	u32 _cpu_based_exec_control = 0;
> >  	u32 _cpu_based_2nd_exec_control = 0;
> > +	u64 _cpu_based_3rd_exec_control = 0;
> >  	u32 _vmexit_control = 0;
> >  	u32 _vmentry_control = 0;
> >  
> > @@ -2397,7 +2398,8 @@ static __init int setup_vmcs_config(struct
> > vmcs_config *vmcs_conf,
> >  
> >  	opt = CPU_BASED_TPR_SHADOW |
> >  	      CPU_BASED_USE_MSR_BITMAPS |
> > -	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> > +	      CPU_BASED_ACTIVATE_SECONDARY_CONTROLS |
> > +	      CPU_BASED_ACTIVATE_TERTIARY_CONTROLS;
> >  	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PROCBASED_CTLS,
> >  				&_cpu_based_exec_control) < 0)
> >  		return -EIO;
> > @@ -2557,6 +2559,7 @@ static __init int setup_vmcs_config(struct
> > vmcs_config *vmcs_conf,
> >  	vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control;
> >  	vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control;
> >  	vmcs_conf->cpu_based_2nd_exec_ctrl =
> > _cpu_based_2nd_exec_control;
> > +	vmcs_conf->cpu_based_3rd_exec_ctrl =
> > _cpu_based_3rd_exec_control;
> >  	vmcs_conf->vmexit_ctrl         = _vmexit_control;
> >  	vmcs_conf->vmentry_ctrl        = _vmentry_control;
> >  
> > @@ -4200,6 +4203,12 @@ u32 vmx_exec_control(struct vcpu_vmx *vmx)
> >  #define vmx_adjust_sec_exec_exiting(vmx, exec_control, lname,
> > uname) \
> >  	vmx_adjust_sec_exec_control(vmx, exec_control, lname, uname,
> > uname##_EXITING, true)
> >  
> > +static u64 vmx_tertiary_exec_control(struct vcpu_vmx *vmx)
> > +{
> > +	/* Though currently, no special adjustment. There might be in
> > the future*/
> > +	return vmcs_config.cpu_based_3rd_exec_ctrl;
> > +}
> > +
> >  static void vmx_compute_secondary_exec_control(struct vcpu_vmx
> > *vmx)
> >  {
> >  	struct kvm_vcpu *vcpu = &vmx->vcpu;
> > @@ -4310,6 +4319,9 @@ static void init_vmcs(struct vcpu_vmx *vmx)
> >  		secondary_exec_controls_set(vmx, vmx-
> > >secondary_exec_control);
> >  	}
> >  
> > +	if (cpu_has_tertiary_exec_ctrls())
> > +		tertiary_exec_controls_set(vmx,
> > vmx_tertiary_exec_control(vmx));
> > +
> >  	if (kvm_vcpu_apicv_active(&vmx->vcpu)) {
> >  		vmcs_write64(EOI_EXIT_BITMAP0, 0);
> >  		vmcs_write64(EOI_EXIT_BITMAP1, 0);
> > @@ -5778,6 +5790,7 @@ void dump_vmcs(void)
> >  {
> >  	u32 vmentry_ctl, vmexit_ctl;
> >  	u32 cpu_based_exec_ctrl, pin_based_exec_ctrl,
> > secondary_exec_control;
> > +	u64 tertiary_exec_control = 0;
> >  	unsigned long cr4;
> >  	u64 efer;
> >  
> > @@ -5796,6 +5809,9 @@ void dump_vmcs(void)
> >  	if (cpu_has_secondary_exec_ctrls())
> >  		secondary_exec_control =
> > vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> >  
> > +	if (cpu_has_tertiary_exec_ctrls())
> > +		tertiary_exec_control =
> > vmcs_read64(TERTIARY_VM_EXEC_CONTROL);
> 
> We'll have to do something about Enlightened VMCS I believe. In
> theory,
> when eVMCS is in use, 'CPU_BASED_ACTIVATE_TERTIARY_CONTROLS' should
> not
> be exposed, e.g. when KVM hosts a EVMCS enabled guest the control
> should
> be filtered out. Something like (completely untested):
> 
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 41f24661af04..c44ff05f3235 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -299,6 +299,7 @@ const unsigned int nr_evmcs_1_fields =
> ARRAY_SIZE(vmcs_field_to_evmcs_1);
>  
>  __init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
>  {
> +       vmcs_conf->cpu_based_exec_ctrl &=
> ~EVMCS1_UNSUPPORTED_EXEC_CTRL;
>         vmcs_conf->pin_based_exec_ctrl &=
> ~EVMCS1_UNSUPPORTED_PINCTRL;
>         vmcs_conf->cpu_based_2nd_exec_ctrl &=
> ~EVMCS1_UNSUPPORTED_2NDEXEC;
>  
> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
> index bd41d9462355..bf2c5e7a4a8f 100644
> --- a/arch/x86/kvm/vmx/evmcs.h
> +++ b/arch/x86/kvm/vmx/evmcs.h
> @@ -50,6 +50,7 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
>   */
>  #define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \
>                                     PIN_BASED_VMX_PREEMPTION_TIMER)
> +#define EVMCS1_UNSUPPORTED_EXEC_CTRL
> (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
>  #define
> EVMCS1_UNSUPPORTED_2NDEXEC                                     \
>         (SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY
> |                         \
>          SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES
> |                      \
> 
> should do the job I think.

Agree, until L0 HyperV supports the new tertiary control, L1 KVM shall
not expose it to its guests. Thanks Vitaly pointing out, and with
detailed explanation.

BTW, when would enlightened VMCS support VMCS Tertiary Control and Key
Locker feature?
> 
> > +
> >  	pr_err("*** Guest State ***\n");
> >  	pr_err("CR0: actual=0x%016lx, shadow=0x%016lx,
> > gh_mask=%016lx\n",
> >  	       vmcs_readl(GUEST_CR0), vmcs_readl(CR0_READ_SHADOW),
> > @@ -5878,8 +5894,9 @@ void dump_vmcs(void)
> >  		       vmcs_read64(HOST_IA32_PERF_GLOBAL_CTRL));
> >  
> >  	pr_err("*** Control State ***\n");
> > -	pr_err("PinBased=%08x CPUBased=%08x SecondaryExec=%08x\n",
> > -	       pin_based_exec_ctrl, cpu_based_exec_ctrl,
> > secondary_exec_control);
> > +	pr_err("PinBased=0x%08x CPUBased=0x%08x SecondaryExec=0x%08x
> > TertiaryExec=0x%016llx\n",
> > +	       pin_based_exec_ctrl, cpu_based_exec_ctrl,
> > secondary_exec_control,
> > +	       tertiary_exec_control);
> >  	pr_err("EntryControls=%08x ExitControls=%08x\n", vmentry_ctl,
> > vmexit_ctl);
> >  	pr_err("ExceptionBitmap=%08x PFECmask=%08x PFECmatch=%08x\n",
> >  	       vmcs_read32(EXCEPTION_BITMAP),
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index f6f66e5..94f1c27 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -373,6 +373,14 @@ static inline u8 vmx_get_rvi(void)
> >  BUILD_CONTROLS_SHADOW(exec, CPU_BASED_VM_EXEC_CONTROL)
> >  BUILD_CONTROLS_SHADOW(secondary_exec, SECONDARY_VM_EXEC_CONTROL)
> >  
> > +static inline void tertiary_exec_controls_set(struct vcpu_vmx
> > *vmx, u64 val)
> > +{
> > +	if (vmx->loaded_vmcs->controls_shadow.tertiary_exec != val) {
> > +		vmcs_write64(TERTIARY_VM_EXEC_CONTROL, val);
> > +		vmx->loaded_vmcs->controls_shadow.tertiary_exec = val;
> > +	}
> > +}
> > +
> >  static inline void vmx_register_cache_reset(struct kvm_vcpu *vcpu)
> >  {
> >  	vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 <<
> > VCPU_REGS_RSP)
> 
>