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.
