linux-kernel - Re: [PATCH 4/4] KVM: x86: Load guest/host PKRU outside of the fastpath run loop

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 for Android: free password hash cracker in your pocket

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

Message-ID: <436FCED7-9413-44AB-B78E-AB96D1F5C480@nutanix.com>
Date: Mon, 3 Nov 2025 15:32:24 +0000
From: Jon Kohler <jon@...anix.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>,
        "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] KVM: x86: Load guest/host PKRU outside of the
 fastpath run loop



> On Oct 31, 2025, at 4:52 PM, Sean Christopherson <seanjc@...gle.com> wrote:
> 
> Hmm, I would say I'm flat out opposed to generic hooks of that nature.  For
> anything that _needs_ to be modified with IRQs disabled, the ordering will matter
> greatly.  E.g. we already have kvm_x86_ops.sync_pir_to_irr(), and that _must_ run
> before kvm_vcpu_exit_request() if it triggers a late request.
> 
> And I also want to push for as much stuff as possible to be handled in common x86,
> i.e. I want to actively encourage landing things like PKU and CET support in
> common x86 instead of implementing support in one vendor and then having to churn
> a pile of code to later move it to

Fair, agreed having things common-ized helps everyone

> All that said, I'm not totally opposed to shaving cycles.  Now that @run_flags
> is a thing, it's actually trivially easy to optimize the CR3/CR4 checks (famous
> last words):

A cycle saved is a cycle earned, perhaps? :)

> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 48598d017d6f..5cc1f0168b8a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1709,6 +1709,7 @@ enum kvm_x86_run_flags {
>        KVM_RUN_FORCE_IMMEDIATE_EXIT    = BIT(0),
>        KVM_RUN_LOAD_GUEST_DR6          = BIT(1),
>        KVM_RUN_LOAD_DEBUGCTL           = BIT(2),
> +       KVM_RUN_IS_FIRST_ITERATION      = BIT(3),
> };

I like this approach, as it makes the code easier to grok what we want and when
> 
> struct kvm_x86_ops {
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 55d637cea84a..3deb20b8d0c5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7439,22 +7439,28 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
>                vmx_reload_guest_debugctl(vcpu);
> 
>        /*
> -        * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> -        * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> -        * it switches back to the current->mm, which can occur in KVM context
> -        * when switching to a temporary mm to patch kernel code, e.g. if KVM
> -        * toggles a static key while handling a VM-Exit.
> +        * Refresh vmcs.HOST_CR3 if necessary.  This must be done after IRQs
> +        * are disabled, i.e. not when preparing to switch to the guest, as the
> +        * the kernel may load a new ASID (PCID) any time it switches back to
> +        * the current->mm, which can occur in KVM context when switching to a
> +        * temporary mm to patch kernel code, e.g. if KVM toggles a static key
> +        * while handling a VM-Exit.
> +        *
> +        * Refresh host CR3 and CR4 only on the first iteration of the inner
> +        * loop, as modifying CR3 or CR4 from NMI context is not allowed.
>         */
> -       cr3 = __get_current_cr3_fast();
> -       if (unlikely(cr3 != vmx->loaded_vmcs->host_state.cr3)) {
> -               vmcs_writel(HOST_CR3, cr3);
> -               vmx->loaded_vmcs->host_state.cr3 = cr3;
> -       }
> +       if (run_flags & KVM_RUN_IS_FIRST_ITERATION) {
> +               cr3 = __get_current_cr3_fast();
> +               if (unlikely(cr3 != vmx->loaded_vmcs->host_state.cr3)) {
> +                       vmcs_writel(HOST_CR3, cr3);
> +                       vmx->loaded_vmcs->host_state.cr3 = cr3;
> +               }
> 
> -       cr4 = cr4_read_shadow();
> -       if (unlikely(cr4 != vmx->loaded_vmcs->host_state.cr4)) {
> -               vmcs_writel(HOST_CR4, cr4);
> -               vmx->loaded_vmcs->host_state.cr4 = cr4;
> +               cr4 = cr4_read_shadow();
> +               if (unlikely(cr4 != vmx->loaded_vmcs->host_state.cr4)) {
> +                       vmcs_writel(HOST_CR4, cr4);
> +                       vmx->loaded_vmcs->host_state.cr4 = cr4;
> +               }
>        }
> 
>        /* When single-stepping over STI and MOV SS, we must clear the
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6924006f0796..bff08f58c29a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11286,7 +11286,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>                goto cancel_injection;
>        }
> 
> -       run_flags = 0;
> +       run_flags = KVM_RUN_IS_FIRST_ITERATION;
>        if (req_immediate_exit) {
>                run_flags |= KVM_RUN_FORCE_IMMEDIATE_EXIT;
>                kvm_make_request(KVM_REQ_EVENT, vcpu);
>