linux-kernel - Re: [RFC PATCH 06/18] KVM: VMX: Wire up Intel MBEC enable/disable logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <49556BAF-9244-4FE5-9BA9-846F2959ABD1@nutanix.com>
Date: Tue, 13 May 2025 02:16:16 +0000
From: Jon Kohler <jon@...anix.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "tglx@...utronix.de"
	<tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
	<bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 06/18] KVM: VMX: Wire up Intel MBEC enable/disable
 logic



> On May 12, 2025, at 2:23 PM, Sean Christopherson <seanjc@...gle.com> 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@...anix.com>
>> 
>> ---
>> 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