[Qemu-devel] [PATCH for 2.8?] x86: ioapic: ignore level irq during processing

[Qemu-devel] [PATCH for 2.8?] x86: ioapic: ignore level irq during processing

[Peter Xu]

For level triggered interrupts, we will get Remote IRR bit cleared after
guest kernel finished processing specific request. Before that, we
should ignore the same interrupt from triggering again.

Signed-off-by: Peter Xu <peterx@redhat.com>
---

I discovered this during debugging some IR issues. Only did very
minimum test with e1000, but IIUC this should be the correct behavior
for level triggered interrupts, and before that we might be sending
some extra interrupts to guest (while we should not).

 hw/intc/ioapic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 2d3282a..350f761 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -129,9 +129,15 @@ static void ioapic_service(IOAPICCommonState *s)
                     }
                     continue;
                 }
-#else
-                (void)coalesce;
 #endif
+
+                if (coalesce) {
+                    /* We are level triggered interrupts, and the
+                     * guest should be still working on previous one,
+                     * so skip it. */
+                    continue;
+                }
+
                 /* No matter whether IR is enabled, we translate
                  * the IOAPIC message into a MSI one, and its
                  * address space will decide whether we need a
-- 
2.7.4

Re: [Qemu-devel] [PATCH for 2.8?] x86: ioapic: ignore level irq during processing

[Paolo Bonzini]

On 31/07/2016 16:18, Peter Xu wrote:
> For level triggered interrupts, we will get Remote IRR bit cleared after
> guest kernel finished processing specific request. Before that, we
> should ignore the same interrupt from triggering again.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> I discovered this during debugging some IR issues. Only did very
> minimum test with e1000, but IIUC this should be the correct behavior
> for level triggered interrupts, and before that we might be sending
> some extra interrupts to guest (while we should not).
> 
>  hw/intc/ioapic.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 2d3282a..350f761 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -129,9 +129,15 @@ static void ioapic_service(IOAPICCommonState *s)
>                      }
>                      continue;
>                  }
> -#else
> -                (void)coalesce;
>  #endif
> +
> +                if (coalesce) {
> +                    /* We are level triggered interrupts, and the
> +                     * guest should be still working on previous one,
> +                     * so skip it. */
> +                    continue;
> +                }
> +
>                  /* No matter whether IR is enabled, we translate
>                   * the IOAPIC message into a MSI one, and its
>                   * address space will decide whether we need a
> 

The patch is okay for 2.7, as it matches what is done in the KVM
split-irqchip case.

Paolo

Re: [Qemu-devel] [PATCH for 2.8?] x86: ioapic: ignore level irq during processing

[Peter Xu]

On Mon, Aug 01, 2016 at 12:58:42PM +0200, Paolo Bonzini wrote:
> 
> 
> On 31/07/2016 16:18, Peter Xu wrote:
> > For level triggered interrupts, we will get Remote IRR bit cleared after
> > guest kernel finished processing specific request. Before that, we
> > should ignore the same interrupt from triggering again.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 
> > I discovered this during debugging some IR issues. Only did very
> > minimum test with e1000, but IIUC this should be the correct behavior
> > for level triggered interrupts, and before that we might be sending
> > some extra interrupts to guest (while we should not).
> > 
> >  hw/intc/ioapic.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> > index 2d3282a..350f761 100644
> > --- a/hw/intc/ioapic.c
> > +++ b/hw/intc/ioapic.c
> > @@ -129,9 +129,15 @@ static void ioapic_service(IOAPICCommonState *s)
> >                      }
> >                      continue;
> >                  }
> > -#else
> > -                (void)coalesce;
> >  #endif
> > +
> > +                if (coalesce) {
> > +                    /* We are level triggered interrupts, and the
> > +                     * guest should be still working on previous one,
> > +                     * so skip it. */
> > +                    continue;
> > +                }
> > +
> >                  /* No matter whether IR is enabled, we translate
> >                   * the IOAPIC message into a MSI one, and its
> >                   * address space will decide whether we need a
> > 
> 
> The patch is okay for 2.7, as it matches what is done in the KVM
> split-irqchip case.

Cool. It'll be nice to have it in 2.7 as well. Thanks,

-- peterx