linux-kernel - Re: [PATCH V2 1/5] KVM: SVM: Move STGI and CLGI intercept handling

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: <ugrjf3qqpeqafg6tnavw6p4l5seapl6mfx6ypypka25shvu6by@pq4qpwn24dyi>
Date: Thu, 15 Jan 2026 17:05:10 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Kevin Cheng <chengkev@...gle.com>, pbonzini@...hat.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/5] KVM: SVM: Move STGI and CLGI intercept handling

On Thu, Jan 15, 2026 at 09:00:07AM -0800, Sean Christopherson wrote:
> On Thu, Jan 15, 2026, Yosry Ahmed wrote:
> > On Wed, Jan 14, 2026 at 05:39:13PM -0800, Sean Christopherson wrote:
> > > On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > > As for how to fix this, a few ideas:
> > > 
> > >  1. Set KVM_REQ_EVENT to force KVM to re-evulate all events.  kvm_check_and_inject_events()
> > >     will see the pending NMI and/or SMI, that the NMI/SMI is not allowed, and
> > >     re-call enable_{nmi,smi}_window().
> > > 
> > >  2. Manually check for pending+blocked NMI/SMIs.
> > > 
> > >  3. Combine parts of #1 and #2.  Set KVM_REQ_EVENT, but only if there's a pending
> > >     NMI or SMI.
> > > 
> > >  4. Add flags to vcpu_svm to explicitly track if a vCPU has an NMI/SMI window,
> > >     similar to what we're planning on doing for IRQs[*], and use that to more
> > >     confidently do the right thing when recomputing intercepts.
> > > 
> > > I don't love any of those ideas.  Ah, at least not until I poke around KVM.  In
> > > svm_set_gif() there's already this:
> > > 
> > > 		if (svm->vcpu.arch.smi_pending ||
> > > 		    svm->vcpu.arch.nmi_pending ||
> > > 		    kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> > > 		    kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> > > 			kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > > 
> > > So I think it makes sense to bundle that into a helper, e.g. (no idea what to
> > > call it)
> > > 
> > > static bool svm_think_of_a_good_name(struct kvm_vcpu *vcpu)
> > > {
> > > 	if (svm->vcpu.arch.smi_pending ||
> > > 	    svm->vcpu.arch.nmi_pending ||
> > > 	    kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> > > 	    kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> > > 		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > > }
> > 
> > Maybe svm_check_gif_events() or svm_check_gif_interrupts()?
> > 
> > Or maybe it's clearer if we just put the checks in a helper like
> > svm_waiting_for_gif() or svm_pending_gif_interrupt().
> 
> This was my first idea as well, though I would name it svm_has_pending_gif_event()
> to better align with kvm_vcpu_has_events().

svm_has_pending_gif_event() sounds good.

> 
> I suggested a single helper because I don't love that how to react to the pending
> event is duplicated.  But I definitely don't object to open coding the request if
> the consensus is that it's more readable overall.

A single helper is nice, but I can't think of a name that would read
well. My first instinct is svm_check_pending_gif_event(), but we are not
really checking the event as much as requesting for it to be checked.

We can do svm_request_gif_event(), perhaps? Not sure if that's better or
worse than svm_has_pending_gif_event().

> 
> > Then in svm_recalc_instruction_intercepts() we do:
> > 
> > 	/*
> > 	 * If there is a pending interrupt controlled by GIF, set
> > 	 * KVM_REQ_EVENT to re-evaluate if the intercept needs to be set
> > 	 * again to track when GIF is re-enabled (e.g. for NMI
> > 	 * injection).
> > 	 */
> > 	svm_clr_intercept(svm, INTERCEPT_STGI);	
> > 	if (svm_pending_gif_interrupt())
> > 		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > 
> > and in svm_set_gif() it reads well semantically:
> > 	
> > 	enable_gif(svm);
> > 	if (svm_pending_gif_interrupt())
> > 		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);