> On May 12, 2025, at 2:23 PM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On Thu, Mar 13, 2025, Jon Kohler wrote:
>> Add logic to enable / disable Intel Mode Based Execution Control (MBEC)
>> based on specific conditions.
>>
>> MBEC depends on:
>> - User space exposing secondary execution control bit 22
>> - Extended Page Tables (EPT)
>> - The KVM module parameter `enable_pt_guest_exec_control`
>>
>> If any of these conditions are not met, MBEC will be disabled
>> accordingly.
>
> Why? I know why, but I know why despite the changeloge, not because of the
> changelog.
>
>> Store runtime enablement within `kvm_vcpu_arch.pt_guest_exec_control`.
>
> Again, why? If you actually tried to explain this, I think/hope you would realize
> why it's wrong.
>
>> Signed-off-by: Jon Kohler <jon@xxxxxxxxxxx>
>>
>> ---
>> arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
>> arch/x86/kvm/vmx/vmx.h | 7 +++++++
>> 2 files changed, 18 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 7a98f03ef146..116910159a3f 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -2694,6 +2694,7 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>> return -EIO;
>>
>> vmx_cap->ept = 0;
>> + _cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_MODE_BASED_EPT_EXEC;
>> _cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_EPT_VIOLATION_VE;
>> }
>> if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_VPID) &&
>> @@ -4641,11 +4642,15 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>> exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
>> if (!enable_ept) {
>> exec_control &= ~SECONDARY_EXEC_ENABLE_EPT;
>> + exec_control &= ~SECONDARY_EXEC_MODE_BASED_EPT_EXEC;
>> exec_control &= ~SECONDARY_EXEC_EPT_VIOLATION_VE;
>> enable_unrestricted_guest = 0;
>> }
>> if (!enable_unrestricted_guest)
>> exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;
>> + if (!enable_pt_guest_exec_control)
>> + exec_control &= ~SECONDARY_EXEC_MODE_BASED_EPT_EXEC;
>
> This is wrong and unnecessary. As mentioned early, the input that matters is
> vmcs12. This flag should *never* be set for vmcs01.
I’ll page this back in, but I’m like 75% sure it didn’t work when I did it that way.
Either way, thanks for the feedback, I’ll chase that do ground.
>
>> if (kvm_pause_in_guest(vmx->vcpu.kvm))
>> exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING;
>> if (!kvm_vcpu_apicv_active(vcpu))
>> @@ -4770,6 +4775,9 @@ static void init_vmcs(struct vcpu_vmx *vmx)
>> if (vmx->ve_info)
>> vmcs_write64(VE_INFORMATION_ADDRESS,
>> __pa(vmx->ve_info));
>> +
>> + vmx->vcpu.arch.pt_guest_exec_control =
>> + enable_pt_guest_exec_control && vmx_has_mbec(vmx);
>
> This should effectively be dead code, because vmx_has_mbec() should never be
> true at vCPU creation.
Ack, will fix
