Re: [PATCH v5 1/3] KVM: x86: Isolate edge vs. level check in userspace I/O APIC route scanning

[Kai Huang]

On 11/04/25 10:46, Sean Christopherson wrote:
> On Tue, Mar 04, 2025, Kai Huang wrote:
> > On Mon, 2025-03-03 at 17:33 -0800, Sean Christopherson wrote:
> > > Extract and isolate the trigger mode check in kvm_scan_ioapic_routes() in
> > > anticipation of moving destination matching logic to a common helper (for
> > > userspace vs. in-kernel I/O APIC emulation).
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> >
> > Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>
> >
> > > ---
> > >   arch/x86/kvm/irq_comm.c | 10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> > > index 8136695f7b96..866f84392797 100644
> > > --- a/arch/x86/kvm/irq_comm.c
> > > +++ b/arch/x86/kvm/irq_comm.c
> > > @@ -424,10 +424,12 @@ void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,
> > >   
> > >   			kvm_set_msi_irq(vcpu->kvm, entry, &irq);
> > >   
> > > -			if (irq.trig_mode &&
> > > -			    (kvm_apic_match_dest(vcpu, NULL, APIC_DEST_NOSHORT,
> > > -						 irq.dest_id, irq.dest_mode) ||
> > > -			     kvm_apic_pending_eoi(vcpu, irq.vector)))
> > > +			if (!irq.trig_mode)
> > > +				continue;
> >
> > Perhaps take this chance to make it explicit?
> >
> > 			if (irq.trig_mode != IOAPIC_LEVEL_TRIG)
> > 				continue;
> >
> > kvm_ioapic_scan_entry() also checks against IOAPIC_LEVEL_TRIG explicitly.
>
> Hmm, I'm leaning "no".  kvm_set_msi_irq() isn't I/O APIC specific (and obviously
> neither is "struct kvm_lapic_irq").  The fact that it sets irq.trig_mode to '0'
> or '1', and that the '1' value in particular happens to match IOAPIC_LEVEL_TRIG
> is somewhat of a coincidence.
>
> kvm_ioapic_scan_entry() on the other operates on a "union kvm_ioapic_redirect_entry"
> object, in which case trig_mode is guaranteed to be '0' or '1', i.e. is exactly
> IOAPIC_EDGE_TRIG or IOAPIC_LEVEL_TRIG.
>
> 	u8 trig_mode:1;

This makes sense.  Thanks for clarifying.

> So as much as I advocate for consistency, I think in this case it makes sense to
> be consistent with __apic_accept_irq(), which only cares about zero vs. non-zero.

Yeah LGTM.