Re: [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms

[Jason Wang]

On 2018年07月02日 13:14, Jan Kiszka wrote:
> On 2018-07-02 05:40, Jason Wang wrote:
> > On 2018年06月30日 14:13, Jan Kiszka wrote:
> > > On 2018-04-05 19:41, Jan Kiszka wrote:
> > > > From: Jan Kiszka <address@hidden>
> > > >
> > > > Only signal MSI/MSI-X events on rising edges. So far we re-triggered the
> > > > interrupt sources even if the guest did no consumed the pending one,
> > > > easily causing interrupt storms.
> > > >
> > > > Issue was observable with Linux 4.16 e1000e driver when MSI-X was used.
> > > > Vector 2 was causing interrupt storms after the driver activated the
> > > > device.
> > > >
> > > > Signed-off-by: Jan Kiszka <address@hidden>
> > > > ---
> > > >
> > > > Changes in v2:
> > > >    - also update msi_causes_pending after EIAC changes (required because
> > > >      there is no e1000e_update_interrupt_state after that
> > > >
> > > >    hw/net/e1000e_core.c | 11 +++++++++++
> > > >    hw/net/e1000e_core.h |  2 ++
> > > >    2 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > > > index c93c4661ed..d6ddd59986 100644
> > > > --- a/hw/net/e1000e_core.c
> > > > +++ b/hw/net/e1000e_core.c
> > > > @@ -2027,6 +2027,7 @@ e1000e_msix_notify_one(E1000ECore *core,
> > > > uint32_t cause, uint32_t int_cfg)
> > > >        }
> > > >          core->mac[ICR] &= ~effective_eiac;
> > > > +    core->msi_causes_pending &= ~effective_eiac;
> > > >          if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
> > > >            core->mac[IMS] &= ~effective_eiac;
> > > > @@ -2123,6 +2124,13 @@ e1000e_send_msi(E1000ECore *core, bool msix)
> > > >    {
> > > >        uint32_t causes = core->mac[ICR] & core->mac[IMS] &
> > > > ~E1000_ICR_ASSERTED;
> > > >    +    core->msi_causes_pending &= causes;
> > > > +    causes ^= core->msi_causes_pending;
> > > > +    if (causes == 0) {
> > > > +        return;
> > > > +    }
> > > > +    core->msi_causes_pending |= causes;
> > > > +
> > > >        if (msix) {
> > > >            e1000e_msix_notify(core, causes);
> > > >        } else {
> > > > @@ -2160,6 +2168,9 @@ e1000e_update_interrupt_state(E1000ECore *core)
> > > >        core->mac[ICS] = core->mac[ICR];
> > > >          interrupts_pending = (core->mac[IMS] & core->mac[ICR]) ? true
> > > > : false;
> > > > +    if (!interrupts_pending) {
> > > > +        core->msi_causes_pending = 0;
> > > > +    }
> > > >          trace_e1000e_irq_pending_interrupts(core->mac[ICR] &
> > > > core->mac[IMS],
> > > >                                            core->mac[ICR],
> > > > core->mac[IMS]);
> > > > diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h
> > > > index 7d8ff41890..63a15510cc 100644
> > > > --- a/hw/net/e1000e_core.h
> > > > +++ b/hw/net/e1000e_core.h
> > > > @@ -109,6 +109,8 @@ struct E1000Core {
> > > >        NICState *owner_nic;
> > > >        PCIDevice *owner;
> > > >        void (*owner_start_recv)(PCIDevice *d);
> > > > +
> > > > +    uint32_t msi_causes_pending;
> > > >    };
> > > >      void
> > > Ping again, this one is still open.
> > >
> > > Jan
> > Sorry for the late.
> >
> > Just one thing to confirm, I think the reason that we don't need to
> > migrate msi_cause_pending is that it can only lead at most one more
> > signal of MSI on destination?
> Right, a cleared msi_causes_pending on the destination may lead to one
> spurious interrupt injects per vector. That should be tolerable.
>
> Jan

Applied.

Thanks