1. Patchew
  2. linux
  3. [PATCH v5 0/3] KVM: x86: Optimize "stale" EOI bitmap exiting
  4. View patch

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

Sean Christopherson posted 3 patches 11 months, 1 week ago
[PATCH v5 1/3] KVM: x86: Isolate edge vs. level check in userspace I/O APIC route scanning
Posted by Sean Christopherson 11 months, 1 week ago 
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@google.com>
---
 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;
+
+			if (kvm_apic_match_dest(vcpu, NULL, APIC_DEST_NOSHORT,
+						irq.dest_id, irq.dest_mode) ||
+			     kvm_apic_pending_eoi(vcpu, irq.vector))
 				__set_bit(irq.vector, ioapic_handled_vectors);
 		}
 	}
-- 
2.48.1.711.g2feabab25a-goog
Re: [PATCH v5 1/3] KVM: x86: Isolate edge vs. level check in userspace I/O APIC route scanning
Posted by Huang, Kai 11 months, 1 week ago 
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@google.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

> ---
>  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.

> +
> +			if (kvm_apic_match_dest(vcpu, NULL, APIC_DEST_NOSHORT,
> +						irq.dest_id, irq.dest_mode) ||
> +			     kvm_apic_pending_eoi(vcpu, irq.vector))
>  				__set_bit(irq.vector, ioapic_handled_vectors);
>  		}
>  	}
Re: [PATCH v5 1/3] KVM: x86: Isolate edge vs. level check in userspace I/O APIC route scanning
Posted by Sean Christopherson 10 months ago 
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@google.com>
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> 
> > ---
> >  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;

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.
Re: [PATCH v5 1/3] KVM: x86: Isolate edge vs. level check in userspace I/O APIC route scanning
Posted by Kai Huang 10 months ago 
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@google.com>
>>
>> Reviewed-by: Kai Huang <kai.huang@intel.com>
>>
>>> ---
>>>   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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.