Move irq sharing information to irqchip level.
PATCH 1/1 Move irq sharing information to irqchip level.
This removes assumptions that max GSIs is smaller than number of pins. Sharing is tracked on pin level not GSI level.
arch/x86/include/asm/kvm_host.h +0 -1
@@ -413,7 +413,6 @@ struct kvm_arch{gpa_t ept_identity_map_addr;unsigned long irq_sources_bitmap; -unsigned long irq_states[KVM_IOAPIC_NUM_PINS ];u64 vm_init_tsc; };
arch/x86/kvm/irq.h +1 -0
@@ -71,6 +71,7 @@ struct kvm_pic {int output;/* intr from master PIC */ struct kvm_io_device dev;void (*ack_notifier)(void *opaque,int irq); +unsigned long irq_states[16 ]; };struct kvm_pic *kvm_create_pic (struct kvm *kvm);
include/linux/kvm_host.h +1 -1
@@ -121,7 +121,7 @@ struct kvm_kernel_irq_routing_entry {u32 gsi;u32 type;int (*set)(struct kvm_kernel_irq_routing_entry *e, -struct kvm *kvm,int level); +struct kvm *kvm,int irq_source_id,int level); union { struct { unsigned irqchip;
virt/kvm/ioapic.h +1 -0
@@ -41,6 +41,7 @@ struct kvm_ioapic {u32 irr;u32 pad;union kvm_ioapic_redirect_entry redirtbl[IOAPIC_NUM_PINS ]; +unsigned long irq_states[IOAPIC_NUM_PINS ];struct kvm_io_device dev;struct kvm *kvm;void (*ack_notifier)(void *opaque,int irq);
virt/kvm/irq_comm.c +34 -23
106 lines not on this page · raw message
--
Gleb.
Quoted 21 lines · Gleb Natapov
This removes assumptions that max GSIs is smaller than number of pins. Sharing is tracked on pin level not GSI level.
Signed-off-by Gleb Natapov<[email protected]>arch/x86/include/asm/kvm_host.h +0
-1@@ -413,7 +413,6 @@ struct kvm_arch{gpa_t ept_identity_map_addr;unsigned long irq_sources_bitmap; -unsigned long irq_states[KVM_IOAPIC_NUM_PINS ];u64 vm_init_tsc; };arch/x86/kvm/irq.h +0
-0
arch/x86/kvm/irq.h struct kvm_pic { · line 77
Quoted 7 lines
arch/x86/kvm/irq.h +1
-0@@ -71,6 +71,7 @@ struct kvm_pic {int output;/* intr from master PIC */ struct kvm_io_device dev;void (*ack_notifier)(void *opaque,int irq); +unsigned long irq_states[16 ]; };
I think it's cleaner to move this into the routing table to avoid duplication. Currently there is no object which is unique to a gsi, but after your array patch, it can be placed next to the hlist_head.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 17 lines · Avi Kivity
Quoted 16 lines · Gleb Natapov
This removes assumptions that max GSIs is smaller than number of pins. Sharing is tracked on pin level not GSI level.
Signed-off-by Gleb Natapov<[email protected]>arch/x86/include/asm/kvm_host.h +0
-1@@ -413,7 +413,6 @@ struct kvm_arch{gpa_t ept_identity_map_addr;unsigned long irq_sources_bitmap; -unsigned long irq_states[KVM_IOAPIC_NUM_PINS ];u64 vm_init_tsc; };
Quoted 16 lines
arch/x86/kvm/irq.h struct kvm_pic { · line 76
Quoted 11 lines
arch/x86/kvm/irq.h +1
-0@@ -71,6 +71,7 @@ struct kvm_pic {int output;/* intr from master PIC */ struct kvm_io_device dev;void (*ack_notifier)(void *opaque,int irq); +unsigned long irq_states[16 ]; };I think it's cleaner to move this into the routing table to avoid duplication. Currently there is no object which is unique to a gsi, but after your array patch, it can be placed next to the
hlist_head.
The problem is that at this level it is not known if GSI is MSI or not. Current code dials with this by arbitrary assuming that GSI is MSI if it is greater then KVM_IOAPIC_NUM_PINS, but this is not enforced when routing table is installed and userspace is free to use any GSI as MSI. Of cause this problem would not exist if MSIs were not linked to GSIs in the first place.
The code duplication is very small (one line of code) and logically it is more correct to track duplications at irqchip level. Theoretically GSI may be shared when PIC is used and not shared when IOAPIC is used.
--
Gleb.
Quoted 37 lines · Gleb Natapov
Quoted 18 lines · Avi Kivity
Quoted 16 lines · Gleb Natapov
This removes assumptions that max GSIs is smaller than number of pins. Sharing is tracked on pin level not GSI level.
Signed-off-by Gleb Natapov<[email protected]>arch/x86/include/asm/kvm_host.h +0
-1@@ -413,7 +413,6 @@ struct kvm_arch{gpa_t ept_identity_map_addr;unsigned long irq_sources_bitmap; -unsigned long irq_states[KVM_IOAPIC_NUM_PINS ];u64 vm_init_tsc; };Quoted 17 lines
arch/x86/kvm/irq.h struct kvm_pic { · line 77
Quoted 12 lines
arch/x86/kvm/irq.h +1
-0@@ -71,6 +71,7 @@ struct kvm_pic {int output;/* intr from master PIC */ struct kvm_io_device dev;void (*ack_notifier)(void *opaque,int irq); +unsigned long irq_states[16 ]; };I think it's cleaner to move this into the routing table to avoid duplication. Currently there is no object which is unique to a gsi, but after your array patch, it can be placed next to the
hlist_head.
Quoted 7 lines
The problem is that at this level it is not known if GSI is MSI or not. Current code dials with this by arbitrary assuming that GSI is MSI if it is greater then
KVM_IOAPIC_NUM_PINS, but this is not enforced when routing table is installed and userspace is free to use any GSI as MSI. Of cause this problem would not exist if MSIs were not linked to GSIs in the first place.
Does msi actually care about the states? I don't think it does.
Quoted 5 lines
The code duplication is very small (one line of code) and logically it is more correct to track duplications at irqchip level. Theoretically GSI may be shared when PIC is used and not shared when IOAPIC is used.
No, it's more correct at the GSI level, since all interrupt lines connected to one GSI are shared.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 34 lines · Avi Kivity
Quoted 33 lines · Gleb Natapov
Quoted 32 lines · Avi Kivity
Quoted 21 lines · Gleb Natapov
This removes assumptions that max GSIs is smaller than number of pins. Sharing is tracked on pin level not GSI level.
Signed-off-by Gleb Natapov<[email protected]>arch/x86/include/asm/kvm_host.h +0
-1@@ -413,7 +413,6 @@ struct kvm_arch{gpa_t ept_identity_map_addr;unsigned long irq_sources_bitmap; -unsigned long irq_states[KVM_IOAPIC_NUM_PINS ];u64 vm_init_tsc; };arch/x86/kvm/irq.h +0
-0arch/x86/kvm/irq.h struct kvm_pic { · line 76
Quoted 6 lines
arch/x86/kvm/irq.h +1
-0@@ -71,6 +71,7 @@ struct kvm_pic {int output;/* intr from master PIC */ struct kvm_io_device dev;void (*ack_notifier)(void *opaque,int irq); +unsigned long irq_states[16 ]; };I think it's cleaner to move this into the routing table to avoid duplication. Currently there is no object which is unique to a gsi, but after your array patch, it can be placed next to the
hlist_head.
Quoted 9 lines
Quoted 6 lines
The problem is that at this level it is not known if GSI is MSI or not. Current code dials with this by arbitrary assuming that GSI is MSI if it is greater then
KVM_IOAPIC_NUM_PINS, but this is not enforced when routing table is installed and userspace is free to use any GSI as MSI. Of cause this problem would not exist if MSIs were not linked to GSIs in the first place.Does msi actually care about the states? I don't think it does.
That is the point. MSI doesn't care, but we don't know if GSI is MSI or not.
Quoted 8 lines
Quoted 4 lines
The code duplication is very small (one line of code) and logically it is more correct to track duplications at irqchip level. Theoretically GSI may be shared when PIC is used and not shared when IOAPIC is used.
No, it's more correct at the GSI level, since all interrupt lines connected to one GSI are shared.
Why? All interrupts connected to the same irqchip pin (through pci irq router or other onboard logic) are shared. GSI is pure logical abstraction.
--
Gleb.
Quoted 8 lines · Gleb Natapov
Quoted 8 lines
Quoted 8 lines
The problem is that at this level it is not known if GSI is MSI or not. Current code dials with this by arbitrary assuming that GSI is MSI if it is greater then
KVM_IOAPIC_NUM_PINS, but this is not enforced when routing table is installed and userspace is free to use any GSI as MSI. Of cause this problem would not exist if MSIs were not linked to GSIs in the first place.
Quoted 6 lines
Quoted 3 lines
Does msi actually care about the states? I don't think it does.
That is the point. MSI doesn't care, but we don't know if GSI is MSI or not.
So we toggle the bits needlessly. Just like with edge-triggered interrupts.
Quoted 8 lines
Quoted 4 lines
No, it's more correct at the GSI level, since all interrupt lines connected to one GSI are shared.
Why? All interrupts connected to the same irqchip pin (through pci irq router or other onboard logic) are shared. GSI is pure logical abstraction.
All interrupts connected through one GSI are shared, so why not keep the level information shared as well?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 7 lines
Quoted 7 lines
The problem is that at this level it is not known if GSI is MSI or not. Current code dials with this by arbitrary assuming that GSI is MSI if it is greater then
KVM_IOAPIC_NUM_PINS, but this is not enforced when routing table is installed and userspace is free to use any GSI as MSI. Of cause this problem would not exist if MSIs were not linked to GSIs in the first place.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Does msi actually care about the states? I don't think it does.
That is the point. MSI doesn't care, but we don't know if GSI is MSI or not.
So we toggle the bits needlessly. Just like with edge-triggered interrupts.
If we can avoid it why not?
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
No, it's more correct at the GSI level, since all interrupt lines connected to one GSI are shared.
Why? All interrupts connected to the same irqchip pin (through pci irq router or other onboard logic) are shared. GSI is pure logical abstraction.
All interrupts connected through one GSI are shared, so why not keep the level information shared as well?
If we will keep sharing info at irq routing table level how will we recalculate sharing state when irq routing table changes?
--
Gleb.
Quoted 9 lines · Gleb Natapov
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Does msi actually care about the states? I don't think it does.
That is the point. MSI doesn't care, but we don't know if GSI is MSI or not.
So we toggle the bits needlessly. Just like with edge-triggered interrupts.
If we can avoid it why not?
It simplifies the code. You do it at the kvm_set_irq() entry point regardless of whether the interrupt is level-triggered, edge-triggered, or msi. As a bonus, you only do it once (not twice for pic/ioapic interrupts).
Quoted 7 lines
Quoted 4 lines
All interrupts connected through one GSI are shared, so why not keep the level information shared as well?
If we will keep sharing info at irq routing table level how will we recalculate sharing state when irq routing table changes?
Good question. Move from old table to new table? but that interferes with rcu.
Okay, at irqchip level is better after all.
Signature · 1 line
error compiling committee.c: too many arguments to function