Re: [RFC PATCH 07/12] kvm/vmx/nested: Support new IA32_VMX_PROCBASED_CTLS3 vmx feature control MSR

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

On Mon, 2021-04-05 at 15:44 +0000, Sean Christopherson wrote:
> On Mon, Jan 25, 2021, Robert Hoo wrote:
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 89af692..9eb1c0b 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -1285,6 +1285,13 @@ static int vmx_restore_vmx_basic(struct
> > vcpu_vmx *vmx, u64 data)
> >  		lowp = &vmx->nested.msrs.secondary_ctls_low;
> >  		highp = &vmx->nested.msrs.secondary_ctls_high;
> >  		break;
> > +	/*
> > +	 * MSR_IA32_VMX_PROCBASED_CTLS3 is 64bit, all allow-1.
> > +	 * No need to check. Just return.
> 
> Uh, yes need to check.  Unsupported bits need to be '0'.

Right! Thanks Sean. Going to refactor this function.
> 
> > +	 */
> > +	case MSR_IA32_VMX_PROCBASED_CTLS3:
> > +		vmx->nested.msrs.tertiary_ctls = data;
> > +		return 0;
> >  	default:
> >  		BUG();
> >  	}
> > @@ -1421,6 +1428,7 @@ int vmx_set_vmx_msr(struct kvm_vcpu *vcpu,
> > u32 msr_index, u64 data)
> >  	case MSR_IA32_VMX_TRUE_EXIT_CTLS:
> >  	case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> >  	case MSR_IA32_VMX_PROCBASED_CTLS2:
> > +	case MSR_IA32_VMX_PROCBASED_CTLS3:
> >  		return vmx_restore_control_msr(vmx, msr_index, data);
> >  	case MSR_IA32_VMX_MISC:
> >  		return vmx_restore_vmx_misc(vmx, data);
> > @@ -1516,6 +1524,9 @@ int vmx_get_vmx_msr(struct nested_vmx_msrs
> > *msrs, u32 msr_index, u64 *pdata)
> >  			msrs->secondary_ctls_low,
> >  			msrs->secondary_ctls_high);
> >  		break;
> > +	case MSR_IA32_VMX_PROCBASED_CTLS3:
> > +		*pdata = msrs->tertiary_ctls;
> > +		break;
> >  	case MSR_IA32_VMX_EPT_VPID_CAP:
> >  		*pdata = msrs->ept_caps |
> >  			((u64)msrs->vpid_caps << 32);
> > @@ -6375,7 +6386,8 @@ void nested_vmx_setup_ctls_msrs(struct
> > nested_vmx_msrs *msrs, u32 ept_caps)
> >  		CPU_BASED_USE_IO_BITMAPS | CPU_BASED_MONITOR_TRAP_FLAG
> > |
> >  		CPU_BASED_MONITOR_EXITING | CPU_BASED_RDPMC_EXITING |
> >  		CPU_BASED_RDTSC_EXITING | CPU_BASED_PAUSE_EXITING |
> > -		CPU_BASED_TPR_SHADOW |
> > CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> > +		CPU_BASED_TPR_SHADOW |
> > CPU_BASED_ACTIVATE_SECONDARY_CONTROLS |
> > +		CPU_BASED_ACTIVATE_TERTIARY_CONTROLS;
> >  	/*
> >  	 * We can allow some features even when not supported by the
> >  	 * hardware. For example, L1 can specify an MSR bitmap - and we
> > @@ -6413,6 +6425,10 @@ void nested_vmx_setup_ctls_msrs(struct
> > nested_vmx_msrs *msrs, u32 ept_caps)
> >  		SECONDARY_EXEC_RDSEED_EXITING |
> >  		SECONDARY_EXEC_XSAVES;
> >  
> > +	if (msrs->procbased_ctls_high &
> > CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
> > +		rdmsrl(MSR_IA32_VMX_PROCBASED_CTLS3,
> > +		      msrs->tertiary_ctls);
> 
> No need to split that into two lines.

OK
> 
> > +	msrs->tertiary_ctls &= ~TERTIARY_EXEC_LOADIWKEY_EXITING;
> 
> That's wrong, it should simply be "msrs->tertiary_ctls &= 0" until
> LOADIWKEY is
> supported.

OK, after pondering, yes, you're right.
Since tertiary_ctls is allow-1 semantics, it shall imitate
secondary_ctls_high.
Look at above code
	msrs->secondary_ctls_high &=
		SECONDARY_EXEC_DESC |
		SECONDARY_EXEC_ENABLE_RDTSCP |
		SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
		SECONDARY_EXEC_WBINVD_EXITING |
		SECONDARY_EXEC_APIC_REGISTER_VIRT |
		SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
		SECONDARY_EXEC_RDRAND_EXITING |
		SECONDARY_EXEC_ENABLE_INVPCID |
		SECONDARY_EXEC_RDSEED_EXITING |
		SECONDARY_EXEC_XSAVES;

secondary_ctls_high clears all native set-bits but leaves some by
purpose.

Is this what your mean?

> 
> >  	/*
> >  	 * We can emulate "VMCS shadowing," even if the hardware
> >  	 * doesn't support it.