KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): 628f014fb1efb8e2208db03d13198ba301a3a34c.1288771873.git.jan.kiszka@web.de
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 19 lines · Jan Kiszka
Quoted 11 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>Quoted 6 lines
I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.
Quoted 3 lines
I understand the problem, but I don't get why the kernel should bother. User space has to filter the config space access, returning precisely the value of the INTx disabled bit that the guest wrote.
Yes but if guest disables INTx it should not get interrupts :)
Quoted 11 lines
Quoted 4 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?You mean on the guest IRQ line (host-side sharing is obviously fine)? Don't know, but that wouldn't be a new issue. Need to study the sources /wrt IRQ line arbitration and concurrent use.
Jan
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Jan Kiszka
Quoted 17 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.
Quoted 5 lines
Quoted 3 lines
I understand the problem, but I don't get why the kernel should bother. User space has to filter the config space access, returning precisely the value of the INTx disabled bit that the guest wrote.
Yes but if guest disables INTx it should not get interrupts :)
Right, got this meanwhile. KVM-in-KVM with nested device assignment would break otherwise - intolerable. :)
Jan
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Michael S. Tsirkin
Quoted 22 lines · Jan Kiszka
Quoted 11 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>Quoted 6 lines
I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.I understand the problem, but I don't get why the kernel should bother. User space has to filter the config space access, returning precisely the value of the INTx disabled bit that the guest wrote.
Quoted 7 lines
Quoted 1 line
Yes but if guest disables INTx it should not get interrupts :)
Right, got this meanwhile. KVM-in-KVM with nested device assignment would break otherwise - intolerable. :)
Jan
Or something simpler like WHQL :)
Signature · 1 line
MST
Quoted 18 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.
Quoted 3 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?
I've been pondering this with VFIO too. There it seems to work, even when I enable irqfd. The VFIO kernel/qemu driver needs to filter EOIs based on whether the interrupt was actually asserted by the device, but I think we're likely relying somewhat on interrupts being reasserted to help us keep everything serviced.
Alex
Quoted 23 lines · Alex Williamson
Quoted 18 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.Quoted 3 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?
Quoted 5 lines
I've been pondering this with VFIO too. There it seems to work, even when I enable irqfd. The VFIO kernel/qemu driver needs to filter EOIs based on whether the interrupt was actually asserted by the device, but I think we're likely relying somewhat on interrupts being reasserted to help us keep everything serviced.
I don't think this filtering exists. The ack notifier that is fired on EOI matches the GSI, hitting anyone who is registered.
I think the problem is that, while user space properly or's the input of all PCI devices on a IRQ line (e.g. in piix3_set_irq), kernel-side users apparently prefer to mess directly with the irqchip. Unless I'm missing something, that is long broken.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 30 lines · Jan Kiszka
Quoted 23 lines · Alex Williamson
Quoted 18 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.Quoted 3 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?Quoted 5 lines
I've been pondering this with VFIO too. There it seems to work, even when I enable irqfd. The VFIO kernel/qemu driver needs to filter EOIs based on whether the interrupt was actually asserted by the device, but I think we're likely relying somewhat on interrupts being reasserted to help us keep everything serviced.
Quoted 7 lines
I don't think this filtering exists. The ack notifier that is fired on EOI matches the GSI, hitting anyone who is registered.
I think the problem is that, while user space properly or's the input of all PCI devices on a IRQ line (e.g. in
piix3_set_irq), kernel-side users apparently prefer to mess directly with the irqchip. Unless I'm missing something, that is long broken.
No, it's actually or'ed in the kernel as well: kvm_irq_line_state. So, any issue remaining?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 34 lines · Jan Kiszka
Quoted 33 lines · Jan Kiszka
Quoted 23 lines · Alex Williamson
Quoted 18 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.Quoted 3 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?Quoted 5 lines
I've been pondering this with VFIO too. There it seems to work, even when I enable irqfd. The VFIO kernel/qemu driver needs to filter EOIs based on whether the interrupt was actually asserted by the device, but I think we're likely relying somewhat on interrupts being reasserted to help us keep everything serviced.
I don't think this filtering exists. The ack notifier that is fired on EOI matches the GSI, hitting anyone who is registered.
Quoted 7 lines
Quoted 4 lines
I think the problem is that, while user space properly or's the input of all PCI devices on a IRQ line (e.g. in
piix3_set_irq), kernel-side users apparently prefer to mess directly with the irqchip. Unless I'm missing something, that is long broken.No, it's actually or'ed in the kernel as well:
kvm_irq_line_state. So, any issue remaining?
No issue. Assigned devices have a different source id, this is the trick I forgot about.
Current VFIO patches go through userspace for level IRQs so it's simple, to make VFIO go directly through kernel we'll need to get a source id for each VFIO device, and associate it with eventfds somehow.
Quoted 5 lines
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 30 lines · Jan Kiszka
Quoted 23 lines · Alex Williamson
Quoted 18 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.Quoted 3 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?Quoted 5 lines
I've been pondering this with VFIO too. There it seems to work, even when I enable irqfd. The VFIO kernel/qemu driver needs to filter EOIs based on whether the interrupt was actually asserted by the device, but I think we're likely relying somewhat on interrupts being reasserted to help us keep everything serviced.
Quoted 8 lines
I don't think this filtering exists. The ack notifier that is fired on EOI matches the GSI, hitting anyone who is registered.
I think the problem is that, while user space properly or's the input of all PCI devices on a IRQ line (e.g. inpiix3_set_irq), kernel-side users apparently prefer to mess directly with the irqchip. Unless I'm missing something, that is long broken.Jan
Well we do check host_irq_disabled. I think this is why it's there.
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 38 lines · Michael S. Tsirkin
Quoted 37 lines · Jan Kiszka
Quoted 23 lines · Alex Williamson
Quoted 18 lines · Michael S. Tsirkin
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest. This feature is optional, user space has to request it explicitly.
Signed-off-by Jan Kiszka <[email protected]>I just realized something.
With this patch, if guest ever looks at
interrupt disable bit, it will go crazy as that bit goes on/off by itself. I guess we could have an ioctl to set/clear the bit on device, and have qemu call that on config write into command/status register.Quoted 3 lines
There's also something I don't completely unerstand with current code:
how does interrupt sharing work? E.g. can assigned and emulated devices share an interrupt?Quoted 5 lines
I've been pondering this with VFIO too. There it seems to work, even when I enable irqfd. The VFIO kernel/qemu driver needs to filter EOIs based on whether the interrupt was actually asserted by the device, but I think we're likely relying somewhat on interrupts being reasserted to help us keep everything serviced.
I don't think this filtering exists. The ack notifier that is fired on EOI matches the GSI, hitting anyone who is registered.
I think the problem is that, while user space properly or's the input of all PCI devices on a IRQ line (e.g. inpiix3_set_irq), kernel-side users apparently prefer to mess directly with the irqchip. Unless I'm missing something, that is long broken.
Quoted 3 lines
Quoted 1 line
Jan
Well we do check
host_irq_disabled. I think this is why it's there.
Probably. As an optimization. Once we force user space to inform the kernel about INTx masking, we can restore this also for PCI 2.3.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · Jan Kiszka
Quoted 4 lines
Hmm, this does an extra config read on each interrupt (another one is in
pci_2_3_irq_unmask). These reads are pretty expensive... I do realize locking becomes ugly, though. Maybe my idea to avoid set level to 0 was silly? Thoughts?
Quoted 7 lines
Well, reading twice is the price to pay here, putting
kvm_set_irqunderspin_lock_irqagain is a no-go. From that POV, the previous version was probably the cheapest: no extra efforts in the common case, but still avoiding reassertion via the host IRQ handler whenever possible.Jan
Sigh. I guess so. Any chance of a benchmark to let us figure this out?
Signature · 1 line
MST
Quoted 11 lines · Michael S. Tsirkin
Quoted 10 lines · Jan Kiszka
Quoted 4 lines
Hmm, this does an extra config read on each interrupt (another one is in
pci_2_3_irq_unmask). These reads are pretty expensive... I do realize locking becomes ugly, though. Maybe my idea to avoid set level to 0 was silly? Thoughts?Well, reading twice is the price to pay here, putting
kvm_set_irqunderspin_lock_irqagain is a no-go. From that POV, the previous version was probably the cheapest: no extra efforts in the common case, but still avoiding reassertion via the host IRQ handler whenever possible.
Quoted 5 lines
Quoted 2 lines
Jan
Sigh. I guess so. Any chance of a benchmark to let us figure this out?
Will see what I can do. 2.6.37 claims to have improved IRQ load accounting, maybe that helps to measure something useful.
Jan
A message that is not in the archive (sent off-list or not yet received): 128511f28870098dbd57bdaa081dd30ac2af70df.1288771873.git.jan.kiszka@web.de
Re: [PATCH v3 2/5] KVM: Switch assigned device IRQ forwarding to threaded handler
Quoted 69 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
This improves the IRQ forwarding for assigned devices: By using the kernel's threaded IRQ scheme, we can get rid of the latency-prone work queue and simplify the code in the same run.
Moreover, we no longer have to hold
assigned_dev_lockwhile raising the guest IRQ, which can be a lenghty operation as we may have to iterate over all VCPUs. The lock is now only used for synchronizing masking vs. unmasking of INTx-type IRQs, thus is renames tointx_lock.Signed-off-by Jan Kiszka <[email protected]>--- include/linux/kvm_host.h | 12 +---- virt/kvm/assigned-dev.c | 106 ++++++++++++++------------------------------- 2 files changed, 35 insertions(+), 83 deletions(-)include/linux/kvm_host.h +2
-10@@ -457,16 +457,8 @@ struct kvm_irq_ack_notifier {void (*irq_acked)(struct kvm_irq_ack_notifier *kian); }; -#define KVM_ASSIGNED_MSIX_PENDING 0x1 -struct kvm_guest_msix_entry { -u32 vector; -u16 entry; -u16 flags; -}; -struct kvm_assigned_dev_kernel {struct kvm_irq_ack_notifier ack_notifier; -struct work_struct interrupt_work;struct list_head list;int assigned_dev_id;int host_segnr;@@ -477,13 +469,13 @@ struct kvm_assigned_dev_kernel {bool host_irq_disabled;struct msix_entry *host_msix_entries;int guest_irq; -struct kvm_guest_msix_entry *guest_msix_entries; +struct msix_entry *guest_msix_entries;unsigned long irq_requested_type;int irq_source_id;int flags;struct pci_dev *dev;struct kvm *kvm; -spinlock_t assigned_dev_lock; +spinlock_t intx_lock; };struct kvm_irq_mask_notifier {virt/kvm/assigned-dev.c +3
-3@@ -55,58 +55,31 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernelreturn index; } -static void kvm_assigned_dev_interrupt_work_handler (struct work_struct *work) +static irqreturn_t kvm_assigned_dev_thread (int irq,void *dev_id) { -struct kvm_assigned_dev_kernel *assigned_dev; -int i; +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +u32 vector;
virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 69
Quoted 10 lines
virt/kvm/assigned-dev.c +7
-2+int index; - assigned_dev = container_of(work, struct kvm_assigned_dev_kernel, - interrupt_work); +if (assigned_dev->irq_requested_type &KVM_DEV_IRQ_HOST_INTX ) { +spin_lock (&assigned_dev->intx_lock); +disable_irq_nosync (irq); + assigned_dev->host_irq_disabled = true; +spin_unlock (&assigned_dev->intx_lock); + }
Shouldnt this happen in the hardirq handler? Otherwise host will receive interrupts between ->eoi and this point.
Re: [PATCH v3 2/5] KVM: Switch assigned device IRQ forwarding to threaded handler
Quoted 72 lines · Marcelo Tosatti
Quoted 71 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
This improves the IRQ forwarding for assigned devices: By using the kernel's threaded IRQ scheme, we can get rid of the latency-prone work queue and simplify the code in the same run.
Moreover, we no longer have to hold
assigned_dev_lockwhile raising the guest IRQ, which can be a lenghty operation as we may have to iterate over all VCPUs. The lock is now only used for synchronizing masking vs. unmasking of INTx-type IRQs, thus is renames tointx_lock.Signed-off-by Jan Kiszka <[email protected]>--- include/linux/kvm_host.h | 12 +---- virt/kvm/assigned-dev.c | 106 ++++++++++++++------------------------------- 2 files changed, 35 insertions(+), 83 deletions(-)include/linux/kvm_host.h +2
-10@@ -457,16 +457,8 @@ struct kvm_irq_ack_notifier {void (*irq_acked)(struct kvm_irq_ack_notifier *kian); }; -#define KVM_ASSIGNED_MSIX_PENDING 0x1 -struct kvm_guest_msix_entry { -u32 vector; -u16 entry; -u16 flags; -}; -struct kvm_assigned_dev_kernel {struct kvm_irq_ack_notifier ack_notifier; -struct work_struct interrupt_work;struct list_head list;int assigned_dev_id;int host_segnr;@@ -477,13 +469,13 @@ struct kvm_assigned_dev_kernel {bool host_irq_disabled;struct msix_entry *host_msix_entries;int guest_irq; -struct kvm_guest_msix_entry *guest_msix_entries; +struct msix_entry *guest_msix_entries;unsigned long irq_requested_type;int irq_source_id;int flags;struct pci_dev *dev;struct kvm *kvm; -spinlock_t assigned_dev_lock; +spinlock_t intx_lock; };struct kvm_irq_mask_notifier {virt/kvm/assigned-dev.c +4
-3@@ -55,58 +55,31 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernelreturn index; } -static void kvm_assigned_dev_interrupt_work_handler (struct work_struct *work) +static irqreturn_t kvm_assigned_dev_thread (int irq,void *dev_id) { -struct kvm_assigned_dev_kernel *assigned_dev; -int i; +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +u32 vector; +int index;
Quoted 12 lines
virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 69
Quoted 8 lines
virt/kvm/assigned-dev.c +6
-2- assigned_dev = container_of(work, struct kvm_assigned_dev_kernel, - interrupt_work); +if (assigned_dev->irq_requested_type &KVM_DEV_IRQ_HOST_INTX ) { +spin_lock (&assigned_dev->intx_lock); +disable_irq_nosync (irq); + assigned_dev->host_irq_disabled = true; +spin_unlock (&assigned_dev->intx_lock); + }Shouldnt this happen in the hardirq handler? Otherwise host will receive interrupts between ->eoi and this point.
Yeah, there is indeed still a bug. I thought the kernel would automatically keep the line masked until the threaded handler returned, but that's only the case if we set IRQF_ONESHOT. Will fix in v4.
Thanks,
Jan