Interrupt throttling is broken in several ways:
- Timer expiry sends an interrupt even if there is no cause.
- Timer expiry that results in an interrupt does not re-arm
the timer so an interrupt can appear immediately after the
timer expiry interrupt.
- Interrupt auto-clear should not clear cause if an interrupt
is delayed by throttling.
Fix these by skipping the auto-clear logic if an interrupt is
delayed, and when the throttle timer expires check the cause
bits corresponding to the msix vector before sending an irq,
and send it using the functions that run the throttling state
machine and perform auto-clear.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/net/e1000e_core.c | 60 +++++++++++++++++++++++++++++++++++++++-----
hw/net/igb_core.c | 28 +++++++++++++--------
2 files changed, 72 insertions(+), 16 deletions(-)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index e32955d244b..c5be20bcbbe 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -168,16 +168,63 @@ e1000e_intrmgr_on_throttling_timer(void *opaque)
}
}
+/* Find causes from IVAR vectors and only interrupt if causes are set */
+static uint32_t find_msix_causes(E1000ECore *core, int vec)
+{
+ uint32_t causes = 0;
+ uint32_t int_cfg;
+
+ int_cfg = E1000_IVAR_RXQ0(core->mac[IVAR]);
+ if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
+ E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
+ causes |= E1000_ICR_RXQ0;
+ }
+
+ int_cfg = E1000_IVAR_RXQ1(core->mac[IVAR]);
+ if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
+ E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
+ causes |= E1000_ICR_RXQ1;
+ }
+
+ int_cfg = E1000_IVAR_TXQ0(core->mac[IVAR]);
+ if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
+ E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
+ causes |= E1000_ICR_TXQ0;
+ }
+
+ int_cfg = E1000_IVAR_TXQ1(core->mac[IVAR]);
+ if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
+ E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
+ causes |= E1000_ICR_TXQ1;
+ }
+
+ int_cfg = E1000_IVAR_OTHER(core->mac[IVAR]);
+ if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
+ E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
+ causes |= E1000_ICR_OTHER;
+ }
+
+ return causes;
+}
+
+static void
+e1000e_msix_notify(E1000ECore *core, uint32_t causes);
+
static void
e1000e_intrmgr_on_msix_throttling_timer(void *opaque)
{
E1000IntrDelayTimer *timer = opaque;
- int idx = timer - &timer->core->eitr[0];
+ E1000ECore *core = timer->core;
+ int idx = timer - &core->eitr[0];
+ uint32_t causes;
timer->running = false;
- trace_e1000e_irq_msix_notify_postponed_vec(idx);
- msix_notify(timer->core->owner, idx);
+ causes = find_msix_causes(core, idx);
+ if (core->mac[IMS] & core->mac[ICR] & causes) {
+ trace_e1000e_irq_msix_notify_postponed_vec(idx);
+ e1000e_msix_notify(core, causes);
+ }
}
static void
@@ -1982,10 +2029,11 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg)
if (E1000_IVAR_ENTRY_VALID(int_cfg)) {
uint32_t vec = E1000_IVAR_ENTRY_VEC(int_cfg);
if (vec < E1000E_MSIX_VEC_NUM) {
- if (!e1000e_eitr_should_postpone(core, vec)) {
- trace_e1000e_irq_msix_notify_vec(vec);
- msix_notify(core->owner, vec);
+ if (e1000e_eitr_should_postpone(core, vec)) {
+ return;
}
+ trace_e1000e_irq_msix_notify_vec(vec);
+ msix_notify(core->owner, vec);
} else {
trace_e1000e_wrn_msix_vec_wrong(cause, int_cfg);
}
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index cdebc917d2e..dad32be54fd 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -168,16 +168,7 @@ igb_intmgr_timer_resume(IGBIntrDelayTimer *timer)
}
static void
-igb_intrmgr_on_msix_throttling_timer(void *opaque)
-{
- IGBIntrDelayTimer *timer = opaque;
- int idx = timer - &timer->core->eitr[0];
-
- timer->running = false;
-
- trace_e1000e_irq_msix_notify_postponed_vec(idx);
- igb_msix_notify(timer->core, idx);
-}
+igb_intrmgr_on_msix_throttling_timer(void *opaque);
static void
igb_intrmgr_initialize_all_timers(IGBCore *core, bool create)
@@ -2279,6 +2270,23 @@ static void igb_send_msix(IGBCore *core, uint32_t causes)
}
}
+static void
+igb_intrmgr_on_msix_throttling_timer(void *opaque)
+{
+ IGBIntrDelayTimer *timer = opaque;
+ IGBCore *core = timer->core;
+ int vector = timer - &core->eitr[0];
+ uint32_t causes;
+
+ timer->running = false;
+
+ causes = core->mac[EICR] & core->mac[EIMS];
+ if ((causes & BIT(vector)) && !igb_eitr_should_postpone(core, vector)) {
+ trace_e1000e_irq_msix_notify_postponed_vec(vector);
+ igb_msix_notify(core, vector);
+ }
+}
+
static inline void
igb_fix_icr_asserted(IGBCore *core)
{
--
2.45.2On 2025/01/18 2:03, Nicholas Piggin wrote:
> Interrupt throttling is broken in several ways:
> - Timer expiry sends an interrupt even if there is no cause.
> - Timer expiry that results in an interrupt does not re-arm
> the timer so an interrupt can appear immediately after the
> timer expiry interrupt.
> - Interrupt auto-clear should not clear cause if an interrupt
> is delayed by throttling.
>
> Fix these by skipping the auto-clear logic if an interrupt is
> delayed, and when the throttle timer expires check the cause
> bits corresponding to the msix vector before sending an irq,
> and send it using the functions that run the throttling state
> machine and perform auto-clear.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/net/e1000e_core.c | 60 +++++++++++++++++++++++++++++++++++++++-----
> hw/net/igb_core.c | 28 +++++++++++++--------
> 2 files changed, 72 insertions(+), 16 deletions(-)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index e32955d244b..c5be20bcbbe 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -168,16 +168,63 @@ e1000e_intrmgr_on_throttling_timer(void *opaque)
> }
> }
>
> +/* Find causes from IVAR vectors and only interrupt if causes are set */
This comment is misplaced as find_msix_causes() is just to find causes
and will not cause an interrupt by its own.
The function name is descriptive enough and the comment is a bit
redundant so I suggest simply removing it.
> +static uint32_t find_msix_causes(E1000ECore *core, int vec)
> +{> + uint32_t causes = 0;
> + uint32_t int_cfg;
> +
> + int_cfg = E1000_IVAR_RXQ0(core->mac[IVAR]);
> + if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
> + E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
> + causes |= E1000_ICR_RXQ0;
> + }
> +
> + int_cfg = E1000_IVAR_RXQ1(core->mac[IVAR]);
> + if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
> + E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
> + causes |= E1000_ICR_RXQ1;
> + }
> +
> + int_cfg = E1000_IVAR_TXQ0(core->mac[IVAR]);
> + if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
> + E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
> + causes |= E1000_ICR_TXQ0;
> + }
> +
> + int_cfg = E1000_IVAR_TXQ1(core->mac[IVAR]);
> + if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
> + E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
> + causes |= E1000_ICR_TXQ1;
> + }
> +
> + int_cfg = E1000_IVAR_OTHER(core->mac[IVAR]);
> + if (E1000_IVAR_ENTRY_VALID(int_cfg) &&
> + E1000_IVAR_ENTRY_VEC(int_cfg) == vec) {
> + causes |= E1000_ICR_OTHER;
> + }
> +
> + return causes;
> +}
> +
> +static void
> +e1000e_msix_notify(E1000ECore *core, uint32_t causes);
> +
> static void
> e1000e_intrmgr_on_msix_throttling_timer(void *opaque)
> {
> E1000IntrDelayTimer *timer = opaque;
> - int idx = timer - &timer->core->eitr[0];
> + E1000ECore *core = timer->core;
> + int idx = timer - &core->eitr[0];
> + uint32_t causes;
>
> timer->running = false;
>
> - trace_e1000e_irq_msix_notify_postponed_vec(idx);
> - msix_notify(timer->core->owner, idx);
> + causes = find_msix_causes(core, idx);
> + if (core->mac[IMS] & core->mac[ICR] & causes) {
To raise only pending interrupts, I think you should do:
causes = core->mac[IMS] & core->mac[ICR] & find_msix_causes(core, idx);
if (causes) {
> + trace_e1000e_irq_msix_notify_postponed_vec(idx);
> + e1000e_msix_notify(core, causes);
> + }
> }
>
> static void
> @@ -1982,10 +2029,11 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg)
> if (E1000_IVAR_ENTRY_VALID(int_cfg)) {
> uint32_t vec = E1000_IVAR_ENTRY_VEC(int_cfg);
> if (vec < E1000E_MSIX_VEC_NUM) {
> - if (!e1000e_eitr_should_postpone(core, vec)) {
> - trace_e1000e_irq_msix_notify_vec(vec);
> - msix_notify(core->owner, vec);
> + if (e1000e_eitr_should_postpone(core, vec)) {
> + return;
> }
> + trace_e1000e_irq_msix_notify_vec(vec);
> + msix_notify(core->owner, vec);
> } else {
> trace_e1000e_wrn_msix_vec_wrong(cause, int_cfg);
> }
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index cdebc917d2e..dad32be54fd 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -168,16 +168,7 @@ igb_intmgr_timer_resume(IGBIntrDelayTimer *timer)
> }
>
> static void
> -igb_intrmgr_on_msix_throttling_timer(void *opaque)
> -{
> - IGBIntrDelayTimer *timer = opaque;
> - int idx = timer - &timer->core->eitr[0];
> -
> - timer->running = false;
> -
> - trace_e1000e_irq_msix_notify_postponed_vec(idx);
> - igb_msix_notify(timer->core, idx);
> -}
> +igb_intrmgr_on_msix_throttling_timer(void *opaque);
>
> static void
> igb_intrmgr_initialize_all_timers(IGBCore *core, bool create)
> @@ -2279,6 +2270,23 @@ static void igb_send_msix(IGBCore *core, uint32_t causes)
> }
> }
>
> +static void
> +igb_intrmgr_on_msix_throttling_timer(void *opaque)
> +{
> + IGBIntrDelayTimer *timer = opaque;
> + IGBCore *core = timer->core;
> + int vector = timer - &core->eitr[0];
> + uint32_t causes;
> +
> + timer->running = false;
> +
> + causes = core->mac[EICR] & core->mac[EIMS];
> + if ((causes & BIT(vector)) && !igb_eitr_should_postpone(core, vector)) {
Why does it check for igb_eitr_should_postpone() while
e1000e_intrmgr_on_msix_throttling_timer() doesn't?
Please remove it unless it is necessary; it implies
igb_eitr_should_postpone() can be true in this function and is
potentially misleading.
> + trace_e1000e_irq_msix_notify_postponed_vec(vector);
> + igb_msix_notify(core, vector);
> + }
> +}
> +
> static inline void
> igb_fix_icr_asserted(IGBCore *core)
> {
© 2016 - 2025 Red Hat, Inc.