* [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms
@ 2018-04-05 17:41 Jan Kiszka
2018-06-30 6:13 ` Jan Kiszka
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kiszka @ 2018-04-05 17:41 UTC (permalink / raw)
To: Dmitry Fleytman, qemu-devel
From: Jan Kiszka <jan.kiszka@siemens.com>
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 <jan.kiszka@siemens.com>
---
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
--
2.13.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms
2018-04-05 17:41 [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms Jan Kiszka
@ 2018-06-30 6:13 ` Jan Kiszka
2018-07-02 3:40 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kiszka @ 2018-06-30 6:13 UTC (permalink / raw)
To: Dmitry Fleytman, qemu-devel, Jason Wang
On 2018-04-05 19:41, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms
2018-06-30 6:13 ` Jan Kiszka
@ 2018-07-02 3:40 ` Jason Wang
2018-07-02 5:14 ` Jan Kiszka
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2018-07-02 3:40 UTC (permalink / raw)
To: Jan Kiszka, Dmitry Fleytman, qemu-devel
On 2018年06月30日 14:13, Jan Kiszka wrote:
> On 2018-04-05 19:41, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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?
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms
2018-07-02 3:40 ` Jason Wang
@ 2018-07-02 5:14 ` Jan Kiszka
2018-07-03 4:01 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kiszka @ 2018-07-02 5:14 UTC (permalink / raw)
To: Jason Wang, Dmitry Fleytman, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3350 bytes --]
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 <jan.kiszka@siemens.com>
>>>
>>> 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 <jan.kiszka@siemens.com>
>>> ---
>>>
>>> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms
2018-07-02 5:14 ` Jan Kiszka
@ 2018-07-03 4:01 ` Jason Wang
0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2018-07-03 4:01 UTC (permalink / raw)
To: Jan Kiszka, Dmitry Fleytman, qemu-devel
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 <jan.kiszka@siemens.com>
>>>>
>>>> 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 <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-03 4:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-05 17:41 [Qemu-devel] [PATCH v2] e1000e: Prevent MSI/MSI-X storms Jan Kiszka
2018-06-30 6:13 ` Jan Kiszka
2018-07-02 3:40 ` Jason Wang
2018-07-02 5:14 ` Jan Kiszka
2018-07-03 4:01 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.