The kernel no longer allows us to pass NULL for a hard interrupt
handler without IRQF_ONESHOT. Should have been using this flag
anyway.
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43328
Signed-off-by: Alex Williamson <[email protected]>
---
virt/kvm/assigned-dev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 01f572c..e804d14 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -347,7 +347,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
dev->host_irq = dev->dev->irq;
if (request_threaded_irq(dev->host_irq, NULL,
- kvm_assigned_dev_thread_msi, 0,
+ kvm_assigned_dev_thread_msi, IRQF_ONESHOT,
dev->irq_name, dev)) {
pci_disable_msi(dev->dev);
return -EIO;
@@ -375,7 +375,7 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
for (i = 0; i < dev->entries_nr; i++) {
r = request_threaded_irq(dev->host_msix_entries[i].vector,
NULL, kvm_assigned_dev_thread_msix,
- 0, dev->irq_name, dev);
+ IRQF_ONESHOT, dev->irq_name, dev);
if (r)
goto err;
}
On 2012-06-01 18:16, Alex Williamson wrote:
> The kernel no longer allows us to pass NULL for a hard interrupt
> handler without IRQF_ONESHOT. Should have been using this flag
> anyway.
This make the IRQ handling tail a bit slower (due to
irq_finalize_oneshot). MSIs are edge-triggered, so there was no need for
masking in theory. Hmm, can't we trust the information that an IRQ
grabbed here is really a MSI type?
Jan
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43328
>
> Signed-off-by: Alex Williamson <[email protected]>
> ---
>
> virt/kvm/assigned-dev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 01f572c..e804d14 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -347,7 +347,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
>
> dev->host_irq = dev->dev->irq;
> if (request_threaded_irq(dev->host_irq, NULL,
> - kvm_assigned_dev_thread_msi, 0,
> + kvm_assigned_dev_thread_msi, IRQF_ONESHOT,
> dev->irq_name, dev)) {
> pci_disable_msi(dev->dev);
> return -EIO;
> @@ -375,7 +375,7 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
> for (i = 0; i < dev->entries_nr; i++) {
> r = request_threaded_irq(dev->host_msix_entries[i].vector,
> NULL, kvm_assigned_dev_thread_msix,
> - 0, dev->irq_name, dev);
> + IRQF_ONESHOT, dev->irq_name, dev);
> if (r)
> goto err;
> }
>
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On Fri, 2012-06-01 at 18:39 +0200, Jan Kiszka wrote:
> On 2012-06-01 18:16, Alex Williamson wrote:
> > The kernel no longer allows us to pass NULL for a hard interrupt
> > handler without IRQF_ONESHOT. Should have been using this flag
> > anyway.
>
> This make the IRQ handling tail a bit slower (due to
> irq_finalize_oneshot). MSIs are edge-triggered, so there was no need for
> masking in theory.
Aren't these asynchronous since we can theoretically do
irq_finalize_oneshot while the guest is servicing the device?
> Hmm, can't we trust the information that an IRQ
> grabbed here is really a MSI type?
Apparently not, comment added with this check (1c6c6952):
* The interrupt was requested with handler = NULL, so
* we use the default primary handler for it. But it
* does not have the oneshot flag set. In combination
* with level interrupts this is deadly, because the
* default primary handler just wakes the thread, then
* the irq lines is reenabled, but the device still
* has the level irq asserted. Rinse and repeat....
*
* While this works for edge type interrupts, we play
* it safe and reject unconditionally because we can't
* say for sure which type this interrupt really
* has. The type flags are unreliable as the
* underlying chip implementation can override them.
Thanks,
Alex
> >
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43328
> >
> > Signed-off-by: Alex Williamson <[email protected]>
> > ---
> >
> > virt/kvm/assigned-dev.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index 01f572c..e804d14 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > @@ -347,7 +347,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
> >
> > dev->host_irq = dev->dev->irq;
> > if (request_threaded_irq(dev->host_irq, NULL,
> > - kvm_assigned_dev_thread_msi, 0,
> > + kvm_assigned_dev_thread_msi, IRQF_ONESHOT,
> > dev->irq_name, dev)) {
> > pci_disable_msi(dev->dev);
> > return -EIO;
> > @@ -375,7 +375,7 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
> > for (i = 0; i < dev->entries_nr; i++) {
> > r = request_threaded_irq(dev->host_msix_entries[i].vector,
> > NULL, kvm_assigned_dev_thread_msix,
> > - 0, dev->irq_name, dev);
> > + IRQF_ONESHOT, dev->irq_name, dev);
> > if (r)
> > goto err;
> > }
> >
>
On 2012-06-01 19:03, Alex Williamson wrote:
> On Fri, 2012-06-01 at 18:39 +0200, Jan Kiszka wrote:
>> On 2012-06-01 18:16, Alex Williamson wrote:
>>> The kernel no longer allows us to pass NULL for a hard interrupt
>>> handler without IRQF_ONESHOT. Should have been using this flag
>>> anyway.
>>
>> This make the IRQ handling tail a bit slower (due to
>> irq_finalize_oneshot). MSIs are edge-triggered, so there was no need for
>> masking in theory.
>
> Aren't these asynchronous since we can theoretically do
> irq_finalize_oneshot while the guest is servicing the device?
If it runs on a different CPU. But usually it's more efficient to have
handler and user on the same CPU. And this work has to be processed
somewhere.
>
>> Hmm, can't we trust the information that an IRQ
>> grabbed here is really a MSI type?
>
>
> Apparently not, comment added with this check (1c6c6952):
>
> * The interrupt was requested with handler = NULL, so
> * we use the default primary handler for it. But it
> * does not have the oneshot flag set. In combination
> * with level interrupts this is deadly, because the
> * default primary handler just wakes the thread, then
> * the irq lines is reenabled, but the device still
> * has the level irq asserted. Rinse and repeat....
> *
> * While this works for edge type interrupts, we play
> * it safe and reject unconditionally because we can't
> * say for sure which type this interrupt really
> * has. The type flags are unreliable as the
> * underlying chip implementation can override them.
I was talking about KVM here: Can't the KVM device assignment code
ensure that only MSIs are registered as such so that the above concerns
no longer apply?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On Fri, 2012-06-01 at 19:14 +0200, Jan Kiszka wrote:
> On 2012-06-01 19:03, Alex Williamson wrote:
> > On Fri, 2012-06-01 at 18:39 +0200, Jan Kiszka wrote:
> >> On 2012-06-01 18:16, Alex Williamson wrote:
> >>> The kernel no longer allows us to pass NULL for a hard interrupt
> >>> handler without IRQF_ONESHOT. Should have been using this flag
> >>> anyway.
> >>
> >> This make the IRQ handling tail a bit slower (due to
> >> irq_finalize_oneshot). MSIs are edge-triggered, so there was no need for
> >> masking in theory.
> >
> > Aren't these asynchronous since we can theoretically do
> > irq_finalize_oneshot while the guest is servicing the device?
>
> If it runs on a different CPU. But usually it's more efficient to have
> handler and user on the same CPU. And this work has to be processed
> somewhere.
Yes we need more CPUs and it's better to not do work if we don't have
to. I don't want to go off on too much of a tangent, but I'm not
entirely convinced that host to guest handler locality is all that
important. There's just not enough touched by the host handler to make
much difference. Guest handler and guest user locality is obviously
important, just as it is on bare metal.
> >> Hmm, can't we trust the information that an IRQ
> >> grabbed here is really a MSI type?
> >
> >
> > Apparently not, comment added with this check (1c6c6952):
> >
> > * The interrupt was requested with handler = NULL, so
> > * we use the default primary handler for it. But it
> > * does not have the oneshot flag set. In combination
> > * with level interrupts this is deadly, because the
> > * default primary handler just wakes the thread, then
> > * the irq lines is reenabled, but the device still
> > * has the level irq asserted. Rinse and repeat....
> > *
> > * While this works for edge type interrupts, we play
> > * it safe and reject unconditionally because we can't
> > * say for sure which type this interrupt really
> > * has. The type flags are unreliable as the
> > * underlying chip implementation can override them.
>
> I was talking about KVM here: Can't the KVM device assignment code
> ensure that only MSIs are registered as such so that the above concerns
> no longer apply?
Is that making assumptions about how the chipset handles an MSI? Are
you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
Alex
On 2012-06-01 19:59, Alex Williamson wrote:
>>>> Hmm, can't we trust the information that an IRQ
>>>> grabbed here is really a MSI type?
>>>
>>>
>>> Apparently not, comment added with this check (1c6c6952):
>>>
>>> * The interrupt was requested with handler = NULL, so
>>> * we use the default primary handler for it. But it
>>> * does not have the oneshot flag set. In combination
>>> * with level interrupts this is deadly, because the
>>> * default primary handler just wakes the thread, then
>>> * the irq lines is reenabled, but the device still
>>> * has the level irq asserted. Rinse and repeat....
>>> *
>>> * While this works for edge type interrupts, we play
>>> * it safe and reject unconditionally because we can't
>>> * say for sure which type this interrupt really
>>> * has. The type flags are unreliable as the
>>> * underlying chip implementation can override them.
>>
>> I was talking about KVM here: Can't the KVM device assignment code
>> ensure that only MSIs are registered as such so that the above concerns
>> no longer apply?
>
> Is that making assumptions about how the chipset handles an MSI? Are
I suppose the nature of MSIs removes any need for assumptions about the
handling.
> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
I'm just wondering if that restriction for threaded IRQs is really
necessary for all use cases we have. Threaded MSIs do not appear to me
like have to be handled that conservatively, but maybe I'm missing some
detail.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On 06/01/2012 09:26 PM, Jan Kiszka wrote:
>
>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
>
> I'm just wondering if that restriction for threaded IRQs is really
> necessary for all use cases we have. Threaded MSIs do not appear to me
> like have to be handled that conservatively, but maybe I'm missing some
> detail.
>
btw, I'm hoping we can unthread assigned MSIs. If the delivery is
unicast, we can precalculate everything and all the handler has to do is
set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
from interrupt context with just RCU locking.
--
error compiling committee.c: too many arguments to function
On Sun, 3 Jun 2012, Avi Kivity wrote:
> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
> >
> >> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
> >
> > I'm just wondering if that restriction for threaded IRQs is really
> > necessary for all use cases we have. Threaded MSIs do not appear to me
> > like have to be handled that conservatively, but maybe I'm missing some
> > detail.
> >
>
> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
> unicast, we can precalculate everything and all the handler has to do is
> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
> from interrupt context with just RCU locking.
There is really no need to run MSI/MSI-X interrupts threaded for
KVM. I'm running the patch below for quite some time and it works like
a charm.
Thanks,
tglx
----
Index: linux-2.6/virt/kvm/assigned-dev.c
===================================================================
--- linux-2.6.orig/virt/kvm/assigned-dev.c
+++ linux-2.6/virt/kvm/assigned-dev.c
@@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
}
#ifdef __KVM_HAVE_MSI
-static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
+static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
{
struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
@@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
#endif
#ifdef __KVM_HAVE_MSIX
-static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
+static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
{
struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
int index = find_index_from_host_irq(assigned_dev, irq);
@@ -346,9 +346,8 @@ static int assigned_device_enable_host_m
}
dev->host_irq = dev->dev->irq;
- if (request_threaded_irq(dev->host_irq, NULL,
- kvm_assigned_dev_thread_msi, 0,
- dev->irq_name, dev)) {
+ if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
+ dev->irq_name, dev)) {
pci_disable_msi(dev->dev);
return -EIO;
}
@@ -373,9 +372,9 @@ static int assigned_device_enable_host_m
return r;
for (i = 0; i < dev->entries_nr; i++) {
- r = request_threaded_irq(dev->host_msix_entries[i].vector,
- NULL, kvm_assigned_dev_thread_msix,
- 0, dev->irq_name, dev);
+ r = request_irq(dev->host_msix_entries[i].vector,
+ kvm_assigned_dev_msix_handler, 0,
+ dev->irq_name, dev);
if (r)
goto err;
}
On 2012-06-04 13:21, Thomas Gleixner wrote:
> On Sun, 3 Jun 2012, Avi Kivity wrote:
>
>> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
>>>
>>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
>>>
>>> I'm just wondering if that restriction for threaded IRQs is really
>>> necessary for all use cases we have. Threaded MSIs do not appear to me
>>> like have to be handled that conservatively, but maybe I'm missing some
>>> detail.
>>>
>>
>> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
>> unicast, we can precalculate everything and all the handler has to do is
>> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
>> from interrupt context with just RCU locking.
>
> There is really no need to run MSI/MSI-X interrupts threaded for
> KVM. I'm running the patch below for quite some time and it works like
> a charm.
>
> Thanks,
>
> tglx
> ----
> Index: linux-2.6/virt/kvm/assigned-dev.c
> ===================================================================
> --- linux-2.6.orig/virt/kvm/assigned-dev.c
> +++ linux-2.6/virt/kvm/assigned-dev.c
> @@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
> }
>
> #ifdef __KVM_HAVE_MSI
> -static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> +static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
> {
> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>
> @@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
> #endif
>
> #ifdef __KVM_HAVE_MSIX
> -static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
> +static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
> {
> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> int index = find_index_from_host_irq(assigned_dev, irq);
> @@ -346,9 +346,8 @@ static int assigned_device_enable_host_m
> }
>
> dev->host_irq = dev->dev->irq;
> - if (request_threaded_irq(dev->host_irq, NULL,
> - kvm_assigned_dev_thread_msi, 0,
> - dev->irq_name, dev)) {
> + if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
> + dev->irq_name, dev)) {
> pci_disable_msi(dev->dev);
> return -EIO;
> }
> @@ -373,9 +372,9 @@ static int assigned_device_enable_host_m
> return r;
>
> for (i = 0; i < dev->entries_nr; i++) {
> - r = request_threaded_irq(dev->host_msix_entries[i].vector,
> - NULL, kvm_assigned_dev_thread_msix,
> - 0, dev->irq_name, dev);
> + r = request_irq(dev->host_msix_entries[i].vector,
> + kvm_assigned_dev_msix_handler, 0,
> + dev->irq_name, dev);
> if (r)
> goto err;
> }
This may work in practice but has two conceptual problems:
- we do not want to run a potential broadcast to all VCPUs to run in
a host IRQ handler
- crazy user space could have configured the route to end up in the
PIC or IOAPIC, and both are not hard-IRQ safe (this should probably
be caught on setup)
So this shortcut requires some checks before being applied to a specific
MSI/MSI-X vector.
Taking KVM aside, my general question remains if threaded MSI handlers
of all devices really need to apply IRQF_ONESHOT though they should have
no use for it.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On Mon, 4 Jun 2012, Jan Kiszka wrote:
> On 2012-06-04 13:21, Thomas Gleixner wrote:
> So this shortcut requires some checks before being applied to a specific
> MSI/MSI-X vector.
>
>
> Taking KVM aside, my general question remains if threaded MSI handlers
> of all devices really need to apply IRQF_ONESHOT though they should have
> no use for it.
In theory no, but we had more than one incident, where threaded irqs
w/o a primary handler and w/o IRQF_ONEHSOT lead to full system
starvation. Linus requested this sanity check and I think it's sane
and required.
In fact it's a non issue for MSI. MSI uses handle_edge_irq which does
not mask the interrupt. IRQF_ONESHOT is a noop for that flow handler.
Thanks,
tglx
On 2012-06-04 15:07, Thomas Gleixner wrote:
> On Mon, 4 Jun 2012, Jan Kiszka wrote:
>> On 2012-06-04 13:21, Thomas Gleixner wrote:
>> So this shortcut requires some checks before being applied to a specific
>> MSI/MSI-X vector.
>>
>>
>> Taking KVM aside, my general question remains if threaded MSI handlers
>> of all devices really need to apply IRQF_ONESHOT though they should have
>> no use for it.
>
> In theory no, but we had more than one incident, where threaded irqs
> w/o a primary handler and w/o IRQF_ONEHSOT lead to full system
> starvation. Linus requested this sanity check and I think it's sane
> and required.
OK.
>
> In fact it's a non issue for MSI. MSI uses handle_edge_irq which does
> not mask the interrupt. IRQF_ONESHOT is a noop for that flow handler.
Isn't irq_finalize_oneshot processes for all flows?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On Mon, 4 Jun 2012, Jan Kiszka wrote:
> On 2012-06-04 15:07, Thomas Gleixner wrote:
> > On Mon, 4 Jun 2012, Jan Kiszka wrote:
> >> On 2012-06-04 13:21, Thomas Gleixner wrote:
> >> So this shortcut requires some checks before being applied to a specific
> >> MSI/MSI-X vector.
> >>
> >>
> >> Taking KVM aside, my general question remains if threaded MSI handlers
> >> of all devices really need to apply IRQF_ONESHOT though they should have
> >> no use for it.
> >
> > In theory no, but we had more than one incident, where threaded irqs
> > w/o a primary handler and w/o IRQF_ONEHSOT lead to full system
> > starvation. Linus requested this sanity check and I think it's sane
> > and required.
>
> OK.
>
> >
> > In fact it's a non issue for MSI. MSI uses handle_edge_irq which does
> > not mask the interrupt. IRQF_ONESHOT is a noop for that flow handler.
>
> Isn't irq_finalize_oneshot processes for all flows?
Right, forgot about that. The only way we can avoid that, is that we
get a hint from the underlying irq chip/ handler setup with an extra
flag to tell the core, that it's safe to avoid the ONESHOT/finalize
magic.
Thanks,
tglx
On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote:
> On 2012-06-04 13:21, Thomas Gleixner wrote:
> > On Sun, 3 Jun 2012, Avi Kivity wrote:
> >
> >> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
> >>>
> >>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
> >>>
> >>> I'm just wondering if that restriction for threaded IRQs is really
> >>> necessary for all use cases we have. Threaded MSIs do not appear to me
> >>> like have to be handled that conservatively, but maybe I'm missing some
> >>> detail.
> >>>
> >>
> >> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
> >> unicast, we can precalculate everything and all the handler has to do is
> >> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
> >> from interrupt context with just RCU locking.
> >
> > There is really no need to run MSI/MSI-X interrupts threaded for
> > KVM. I'm running the patch below for quite some time and it works like
> > a charm.
> >
> > Thanks,
> >
> > tglx
> > ----
> > Index: linux-2.6/virt/kvm/assigned-dev.c
> > ===================================================================
> > --- linux-2.6.orig/virt/kvm/assigned-dev.c
> > +++ linux-2.6/virt/kvm/assigned-dev.c
> > @@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
> > }
> >
> > #ifdef __KVM_HAVE_MSI
> > -static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > +static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
> > {
> > struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> >
> > @@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
> > #endif
> >
> > #ifdef __KVM_HAVE_MSIX
> > -static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
> > +static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
> > {
> > struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > int index = find_index_from_host_irq(assigned_dev, irq);
> > @@ -346,9 +346,8 @@ static int assigned_device_enable_host_m
> > }
> >
> > dev->host_irq = dev->dev->irq;
> > - if (request_threaded_irq(dev->host_irq, NULL,
> > - kvm_assigned_dev_thread_msi, 0,
> > - dev->irq_name, dev)) {
> > + if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
> > + dev->irq_name, dev)) {
> > pci_disable_msi(dev->dev);
> > return -EIO;
> > }
> > @@ -373,9 +372,9 @@ static int assigned_device_enable_host_m
> > return r;
> >
> > for (i = 0; i < dev->entries_nr; i++) {
> > - r = request_threaded_irq(dev->host_msix_entries[i].vector,
> > - NULL, kvm_assigned_dev_thread_msix,
> > - 0, dev->irq_name, dev);
> > + r = request_irq(dev->host_msix_entries[i].vector,
> > + kvm_assigned_dev_msix_handler, 0,
> > + dev->irq_name, dev);
> > if (r)
> > goto err;
> > }
>
> This may work in practice but has two conceptual problems:
> - we do not want to run a potential broadcast to all VCPUs to run in
> a host IRQ handler
> - crazy user space could have configured the route to end up in the
> PIC or IOAPIC, and both are not hard-IRQ safe (this should probably
> be caught on setup)
>
> So this shortcut requires some checks before being applied to a specific
> MSI/MSI-X vector.
I did this in the past:
https://lkml.org/lkml/2012/1/18/287
Have no hw to test this atm but if there are any takers
wanting to play with it I can update and post.
--
mst
On 2012-06-08 09:47, Michael S. Tsirkin wrote:
> On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote:
>> On 2012-06-04 13:21, Thomas Gleixner wrote:
>>> On Sun, 3 Jun 2012, Avi Kivity wrote:
>>>
>>>> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
>>>>>
>>>>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
>>>>>
>>>>> I'm just wondering if that restriction for threaded IRQs is really
>>>>> necessary for all use cases we have. Threaded MSIs do not appear to me
>>>>> like have to be handled that conservatively, but maybe I'm missing some
>>>>> detail.
>>>>>
>>>>
>>>> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
>>>> unicast, we can precalculate everything and all the handler has to do is
>>>> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
>>>> from interrupt context with just RCU locking.
>>>
>>> There is really no need to run MSI/MSI-X interrupts threaded for
>>> KVM. I'm running the patch below for quite some time and it works like
>>> a charm.
>>>
>>> Thanks,
>>>
>>> tglx
>>> ----
>>> Index: linux-2.6/virt/kvm/assigned-dev.c
>>> ===================================================================
>>> --- linux-2.6.orig/virt/kvm/assigned-dev.c
>>> +++ linux-2.6/virt/kvm/assigned-dev.c
>>> @@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
>>> }
>>>
>>> #ifdef __KVM_HAVE_MSI
>>> -static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
>>> +static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
>>> {
>>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>>>
>>> @@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
>>> #endif
>>>
>>> #ifdef __KVM_HAVE_MSIX
>>> -static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
>>> +static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
>>> {
>>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>>> int index = find_index_from_host_irq(assigned_dev, irq);
>>> @@ -346,9 +346,8 @@ static int assigned_device_enable_host_m
>>> }
>>>
>>> dev->host_irq = dev->dev->irq;
>>> - if (request_threaded_irq(dev->host_irq, NULL,
>>> - kvm_assigned_dev_thread_msi, 0,
>>> - dev->irq_name, dev)) {
>>> + if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
>>> + dev->irq_name, dev)) {
>>> pci_disable_msi(dev->dev);
>>> return -EIO;
>>> }
>>> @@ -373,9 +372,9 @@ static int assigned_device_enable_host_m
>>> return r;
>>>
>>> for (i = 0; i < dev->entries_nr; i++) {
>>> - r = request_threaded_irq(dev->host_msix_entries[i].vector,
>>> - NULL, kvm_assigned_dev_thread_msix,
>>> - 0, dev->irq_name, dev);
>>> + r = request_irq(dev->host_msix_entries[i].vector,
>>> + kvm_assigned_dev_msix_handler, 0,
>>> + dev->irq_name, dev);
>>> if (r)
>>> goto err;
>>> }
>>
>> This may work in practice but has two conceptual problems:
>> - we do not want to run a potential broadcast to all VCPUs to run in
>> a host IRQ handler
>> - crazy user space could have configured the route to end up in the
>> PIC or IOAPIC, and both are not hard-IRQ safe (this should probably
>> be caught on setup)
>>
>> So this shortcut requires some checks before being applied to a specific
>> MSI/MSI-X vector.
>
> I did this in the past:
> https://lkml.org/lkml/2012/1/18/287
>
> Have no hw to test this atm but if there are any takers
> wanting to play with it I can update and post.
Just add check that allow only unicasts, and this should be fine.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On Fri, Jun 08, 2012 at 09:55:01AM +0200, Jan Kiszka wrote:
> On 2012-06-08 09:47, Michael S. Tsirkin wrote:
> > On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote:
> >> On 2012-06-04 13:21, Thomas Gleixner wrote:
> >>> On Sun, 3 Jun 2012, Avi Kivity wrote:
> >>>
> >>>> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
> >>>>>
> >>>>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
> >>>>>
> >>>>> I'm just wondering if that restriction for threaded IRQs is really
> >>>>> necessary for all use cases we have. Threaded MSIs do not appear to me
> >>>>> like have to be handled that conservatively, but maybe I'm missing some
> >>>>> detail.
> >>>>>
> >>>>
> >>>> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
> >>>> unicast, we can precalculate everything and all the handler has to do is
> >>>> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
> >>>> from interrupt context with just RCU locking.
> >>>
> >>> There is really no need to run MSI/MSI-X interrupts threaded for
> >>> KVM. I'm running the patch below for quite some time and it works like
> >>> a charm.
> >>>
> >>> Thanks,
> >>>
> >>> tglx
> >>> ----
> >>> Index: linux-2.6/virt/kvm/assigned-dev.c
> >>> ===================================================================
> >>> --- linux-2.6.orig/virt/kvm/assigned-dev.c
> >>> +++ linux-2.6/virt/kvm/assigned-dev.c
> >>> @@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
> >>> }
> >>>
> >>> #ifdef __KVM_HAVE_MSI
> >>> -static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> >>> +static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
> >>> {
> >>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> >>>
> >>> @@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
> >>> #endif
> >>>
> >>> #ifdef __KVM_HAVE_MSIX
> >>> -static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
> >>> +static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
> >>> {
> >>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> >>> int index = find_index_from_host_irq(assigned_dev, irq);
> >>> @@ -346,9 +346,8 @@ static int assigned_device_enable_host_m
> >>> }
> >>>
> >>> dev->host_irq = dev->dev->irq;
> >>> - if (request_threaded_irq(dev->host_irq, NULL,
> >>> - kvm_assigned_dev_thread_msi, 0,
> >>> - dev->irq_name, dev)) {
> >>> + if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
> >>> + dev->irq_name, dev)) {
> >>> pci_disable_msi(dev->dev);
> >>> return -EIO;
> >>> }
> >>> @@ -373,9 +372,9 @@ static int assigned_device_enable_host_m
> >>> return r;
> >>>
> >>> for (i = 0; i < dev->entries_nr; i++) {
> >>> - r = request_threaded_irq(dev->host_msix_entries[i].vector,
> >>> - NULL, kvm_assigned_dev_thread_msix,
> >>> - 0, dev->irq_name, dev);
> >>> + r = request_irq(dev->host_msix_entries[i].vector,
> >>> + kvm_assigned_dev_msix_handler, 0,
> >>> + dev->irq_name, dev);
> >>> if (r)
> >>> goto err;
> >>> }
> >>
> >> This may work in practice but has two conceptual problems:
> >> - we do not want to run a potential broadcast to all VCPUs to run in
> >> a host IRQ handler
> >> - crazy user space could have configured the route to end up in the
> >> PIC or IOAPIC, and both are not hard-IRQ safe (this should probably
> >> be caught on setup)
> >>
> >> So this shortcut requires some checks before being applied to a specific
> >> MSI/MSI-X vector.
> >
> > I did this in the past:
> > https://lkml.org/lkml/2012/1/18/287
> >
> > Have no hw to test this atm but if there are any takers
> > wanting to play with it I can update and post.
>
> Just add check that allow only unicasts, and this should be fine.
>
> Jan
If I code it up you can test it?
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-06-08 10:00, Michael S. Tsirkin wrote:
> On Fri, Jun 08, 2012 at 09:55:01AM +0200, Jan Kiszka wrote:
>> On 2012-06-08 09:47, Michael S. Tsirkin wrote:
>>> On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote:
>>>> On 2012-06-04 13:21, Thomas Gleixner wrote:
>>>>> On Sun, 3 Jun 2012, Avi Kivity wrote:
>>>>>
>>>>>> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
>>>>>>>
>>>>>>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
>>>>>>>
>>>>>>> I'm just wondering if that restriction for threaded IRQs is really
>>>>>>> necessary for all use cases we have. Threaded MSIs do not appear to me
>>>>>>> like have to be handled that conservatively, but maybe I'm missing some
>>>>>>> detail.
>>>>>>>
>>>>>>
>>>>>> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
>>>>>> unicast, we can precalculate everything and all the handler has to do is
>>>>>> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
>>>>>> from interrupt context with just RCU locking.
>>>>>
>>>>> There is really no need to run MSI/MSI-X interrupts threaded for
>>>>> KVM. I'm running the patch below for quite some time and it works like
>>>>> a charm.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> tglx
>>>>> ----
>>>>> Index: linux-2.6/virt/kvm/assigned-dev.c
>>>>> ===================================================================
>>>>> --- linux-2.6.orig/virt/kvm/assigned-dev.c
>>>>> +++ linux-2.6/virt/kvm/assigned-dev.c
>>>>> @@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
>>>>> }
>>>>>
>>>>> #ifdef __KVM_HAVE_MSI
>>>>> -static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
>>>>> +static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
>>>>> {
>>>>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>>>>>
>>>>> @@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
>>>>> #endif
>>>>>
>>>>> #ifdef __KVM_HAVE_MSIX
>>>>> -static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
>>>>> +static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
>>>>> {
>>>>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>>>>> int index = find_index_from_host_irq(assigned_dev, irq);
>>>>> @@ -346,9 +346,8 @@ static int assigned_device_enable_host_m
>>>>> }
>>>>>
>>>>> dev->host_irq = dev->dev->irq;
>>>>> - if (request_threaded_irq(dev->host_irq, NULL,
>>>>> - kvm_assigned_dev_thread_msi, 0,
>>>>> - dev->irq_name, dev)) {
>>>>> + if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
>>>>> + dev->irq_name, dev)) {
>>>>> pci_disable_msi(dev->dev);
>>>>> return -EIO;
>>>>> }
>>>>> @@ -373,9 +372,9 @@ static int assigned_device_enable_host_m
>>>>> return r;
>>>>>
>>>>> for (i = 0; i < dev->entries_nr; i++) {
>>>>> - r = request_threaded_irq(dev->host_msix_entries[i].vector,
>>>>> - NULL, kvm_assigned_dev_thread_msix,
>>>>> - 0, dev->irq_name, dev);
>>>>> + r = request_irq(dev->host_msix_entries[i].vector,
>>>>> + kvm_assigned_dev_msix_handler, 0,
>>>>> + dev->irq_name, dev);
>>>>> if (r)
>>>>> goto err;
>>>>> }
>>>>
>>>> This may work in practice but has two conceptual problems:
>>>> - we do not want to run a potential broadcast to all VCPUs to run in
>>>> a host IRQ handler
>>>> - crazy user space could have configured the route to end up in the
>>>> PIC or IOAPIC, and both are not hard-IRQ safe (this should probably
>>>> be caught on setup)
>>>>
>>>> So this shortcut requires some checks before being applied to a specific
>>>> MSI/MSI-X vector.
>>>
>>> I did this in the past:
>>> https://lkml.org/lkml/2012/1/18/287
>>>
>>> Have no hw to test this atm but if there are any takers
>>> wanting to play with it I can update and post.
>>
>> Just add check that allow only unicasts, and this should be fine.
>>
>> Jan
>
> If I code it up you can test it?
Yep, no problem.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote:
> On 2012-06-04 13:21, Thomas Gleixner wrote:
> > On Sun, 3 Jun 2012, Avi Kivity wrote:
> >
> >> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
> >>>
> >>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
> >>>
> >>> I'm just wondering if that restriction for threaded IRQs is really
> >>> necessary for all use cases we have. Threaded MSIs do not appear to me
> >>> like have to be handled that conservatively, but maybe I'm missing some
> >>> detail.
> >>>
> >>
> >> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
> >> unicast, we can precalculate everything and all the handler has to do is
> >> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
> >> from interrupt context with just RCU locking.
> >
> > There is really no need to run MSI/MSI-X interrupts threaded for
> > KVM. I'm running the patch below for quite some time and it works like
> > a charm.
> >
> > Thanks,
> >
> > tglx
> > ----
....
>
> This may work in practice but has two conceptual problems:
> - we do not want to run a potential broadcast to all VCPUs to run in
> a host IRQ handler
I'm not sure why this one is a problem: injecting an interrupt
once you know the vcpu seems really cheap.
It's true that scanning vcpus might take a bit more time
when there are lots of them but it's a single
linear scan that we do anyway.
And we also inject msi from irqfd callback with interrupts
disabled which seems equivalent.
Pls correct me if I'm wrong.
> - crazy user space could have configured the route to end up in the
> PIC or IOAPIC, and both are not hard-IRQ safe (this should probably
> be caught on setup)
Yes this needs to be fixed up.
> So this shortcut requires some checks before being applied to a specific
> MSI/MSI-X vector.
>
>
> Taking KVM aside, my general question remains if threaded MSI handlers
> of all devices really need to apply IRQF_ONESHOT though they should have
> no use for it.
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 2012-06-08 16:39, Michael S. Tsirkin wrote:
> On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote:
>> On 2012-06-04 13:21, Thomas Gleixner wrote:
>>> On Sun, 3 Jun 2012, Avi Kivity wrote:
>>>
>>>> On 06/01/2012 09:26 PM, Jan Kiszka wrote:
>>>>>
>>>>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks,
>>>>>
>>>>> I'm just wondering if that restriction for threaded IRQs is really
>>>>> necessary for all use cases we have. Threaded MSIs do not appear to me
>>>>> like have to be handled that conservatively, but maybe I'm missing some
>>>>> detail.
>>>>>
>>>>
>>>> btw, I'm hoping we can unthread assigned MSIs. If the delivery is
>>>> unicast, we can precalculate everything and all the handler has to do is
>>>> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done
>>>> from interrupt context with just RCU locking.
>>>
>>> There is really no need to run MSI/MSI-X interrupts threaded for
>>> KVM. I'm running the patch below for quite some time and it works like
>>> a charm.
>>>
>>> Thanks,
>>>
>>> tglx
>>> ----
>
>
> ....
>
>>
>> This may work in practice but has two conceptual problems:
>> - we do not want to run a potential broadcast to all VCPUs to run in
>> a host IRQ handler
>
> I'm not sure why this one is a problem: injecting an interrupt
> once you know the vcpu seems really cheap.
> It's true that scanning vcpus might take a bit more time
> when there are lots of them but it's a single
> linear scan that we do anyway.
>
> And we also inject msi from irqfd callback with interrupts
> disabled which seems equivalent.
Interesting, need to check.
>
> Pls correct me if I'm wrong.
Well, IIRC, the "don't loop over all vcpus with IRQs or preemption
disabled" was one argument against direct legacy interrupt injection as
well. That's what I kept in mind from those discussions. Maybe Avi can
comment on the current position.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
On 06/08/2012 05:50 PM, Jan Kiszka wrote:
>
>>
>> Pls correct me if I'm wrong.
>
> Well, IIRC, the "don't loop over all vcpus with IRQs or preemption
> disabled" was one argument against direct legacy interrupt injection as
> well. That's what I kept in mind from those discussions. Maybe Avi can
> comment on the current position.
It's still my position.
IMO we need something like struct gfn_to_hva_cache for interrupts. If
it's in the cache, we fast-path it from the interrupt handler. If not,
fall back to a workqueue and let it refill the cache.
--
error compiling committee.c: too many arguments to function
On Mon, Jun 11, 2012 at 01:01:41PM +0300, Avi Kivity wrote:
> On 06/08/2012 05:50 PM, Jan Kiszka wrote:
> >
> >>
> >> Pls correct me if I'm wrong.
> >
> > Well, IIRC, the "don't loop over all vcpus with IRQs or preemption
> > disabled" was one argument against direct legacy interrupt injection as
> > well. That's what I kept in mind from those discussions. Maybe Avi can
> > comment on the current position.
>
> It's still my position.
>
> IMO we need something like struct gfn_to_hva_cache for interrupts. If
> it's in the cache, we fast-path it from the interrupt handler. If not,
> fall back to a workqueue and let it refill the cache.
And you class the irqfd behaviour of injecting multicast
with interrupts disabled a bug then?
> --
> error compiling committee.c: too many arguments to function
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:[email protected]]
> Sent: Monday, June 11, 2012 6:22 PM
> To: Avi Kivity
> Cc: Jan Kiszka; Thomas Gleixner; Alex Williamson; [email protected];
> [email protected]; [email protected]; Ren, Yongjie
> Subject: Re: [PATCH] KVM: Use IRQF_ONESHOT for assigned device MSI
> interrupts
>
> On Mon, Jun 11, 2012 at 01:01:41PM +0300, Avi Kivity wrote:
> > On 06/08/2012 05:50 PM, Jan Kiszka wrote:
> > >
> > >>
> > >> Pls correct me if I'm wrong.
> > >
> > > Well, IIRC, the "don't loop over all vcpus with IRQs or preemption
> > > disabled" was one argument against direct legacy interrupt injection as
> > > well. That's what I kept in mind from those discussions. Maybe Avi can
> > > comment on the current position.
> >
> > It's still my position.
> >
> > IMO we need something like struct gfn_to_hva_cache for interrupts. If
> > it's in the cache, we fast-path it from the interrupt handler. If not,
> > fall back to a workqueue and let it refill the cache.
>
> And you class the irqfd behaviour of injecting multicast
> with interrupts disabled a bug then?
>
Hi Avi & Michael,
Any more news on this issue ?
> > --
> > error compiling committee.c: too many arguments to function
On 06/11/2012 01:21 PM, Michael S. Tsirkin wrote:
> On Mon, Jun 11, 2012 at 01:01:41PM +0300, Avi Kivity wrote:
>> On 06/08/2012 05:50 PM, Jan Kiszka wrote:
>> >
>> >>
>> >> Pls correct me if I'm wrong.
>> >
>> > Well, IIRC, the "don't loop over all vcpus with IRQs or preemption
>> > disabled" was one argument against direct legacy interrupt injection as
>> > well. That's what I kept in mind from those discussions. Maybe Avi can
>> > comment on the current position.
>>
>> It's still my position.
>>
>> IMO we need something like struct gfn_to_hva_cache for interrupts. If
>> it's in the cache, we fast-path it from the interrupt handler. If not,
>> fall back to a workqueue and let it refill the cache.
>
> And you class the irqfd behaviour of injecting multicast
> with interrupts disabled a bug then?
Yes (a minor one).
--
error compiling committee.c: too many arguments to function