| Message ID | 20240124102904.334595-1-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] igb: fix link state on resume | expand |
On 2024/01/24 19:29, Laurent Vivier wrote: > On resume igb_vm_state_change() always calls igb_autoneg_resume() > that sets link_down to false, and thus activates the link even > if we have disabled it. > > The problem can be reproduced starting qemu in paused state (-S) and > then set the link to down. When we resume the machine the link appears > to be up. > > Reproducer: > > # qemu-system-x86_64 ... -device igb,netdev=netdev0,id=net0 -S > > {"execute": "qmp_capabilities" } > {"execute": "set_link", "arguments": {"name": "net0", "up": false}} > {"execute": "cont" } > > To fix the problem, merge the content of igb_vm_state_change() > into igb_core_post_load() as e1000 does. > > Buglink: https://issues.redhat.com/browse/RHEL-21867 > Fixes: 3a977deebe6b ("Intrdocue igb device emulation") > Cc: akihiko.odaki@daynix.com > Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > > Notes: > v2: Add Fixes: and a comment about igb_intrmgr_resume() purpose. Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
On Wed, Jan 24, 2024 at 6:30 PM Laurent Vivier <lvivier@redhat.com> wrote: > > On resume igb_vm_state_change() always calls igb_autoneg_resume() > that sets link_down to false, and thus activates the link even > if we have disabled it. > > The problem can be reproduced starting qemu in paused state (-S) and > then set the link to down. When we resume the machine the link appears > to be up. > > Reproducer: > > # qemu-system-x86_64 ... -device igb,netdev=netdev0,id=net0 -S > > {"execute": "qmp_capabilities" } > {"execute": "set_link", "arguments": {"name": "net0", "up": false}} > {"execute": "cont" } > > To fix the problem, merge the content of igb_vm_state_change() > into igb_core_post_load() as e1000 does. > > Buglink: https://issues.redhat.com/browse/RHEL-21867 > Fixes: 3a977deebe6b ("Intrdocue igb device emulation") > Cc: akihiko.odaki@daynix.com > Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > > Notes: > v2: Add Fixes: and a comment about igb_intrmgr_resume() purpose. > Queued. Thanks
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index 2a7a11aa9ed5..bcd5f6cd9cdd 100644 --- a/hw/net/igb_core.c +++ b/hw/net/igb_core.c @@ -160,14 +160,6 @@ igb_intmgr_timer_resume(IGBIntrDelayTimer *timer) } } -static void -igb_intmgr_timer_pause(IGBIntrDelayTimer *timer) -{ - if (timer->running) { - timer_del(timer->timer); - } -} - static void igb_intrmgr_on_msix_throttling_timer(void *opaque) { @@ -212,16 +204,6 @@ igb_intrmgr_resume(IGBCore *core) } } -static void -igb_intrmgr_pause(IGBCore *core) -{ - int i; - - for (i = 0; i < IGB_INTR_NUM; i++) { - igb_intmgr_timer_pause(&core->eitr[i]); - } -} - static void igb_intrmgr_reset(IGBCore *core) { @@ -4290,12 +4272,6 @@ igb_core_read(IGBCore *core, hwaddr addr, unsigned size) return 0; } -static inline void -igb_autoneg_pause(IGBCore *core) -{ - timer_del(core->autoneg_timer); -} - static void igb_autoneg_resume(IGBCore *core) { @@ -4307,22 +4283,6 @@ igb_autoneg_resume(IGBCore *core) } } -static void -igb_vm_state_change(void *opaque, bool running, RunState state) -{ - IGBCore *core = opaque; - - if (running) { - trace_e1000e_vm_state_running(); - igb_intrmgr_resume(core); - igb_autoneg_resume(core); - } else { - trace_e1000e_vm_state_stopped(); - igb_autoneg_pause(core); - igb_intrmgr_pause(core); - } -} - void igb_core_pci_realize(IGBCore *core, const uint16_t *eeprom_templ, @@ -4335,8 +4295,6 @@ igb_core_pci_realize(IGBCore *core, igb_autoneg_timer, core); igb_intrmgr_pci_realize(core); - core->vmstate = qemu_add_vm_change_state_handler(igb_vm_state_change, core); - for (i = 0; i < IGB_NUM_QUEUES; i++) { net_tx_pkt_init(&core->tx[i].tx_pkt, E1000E_MAX_TX_FRAGS); } @@ -4360,8 +4318,6 @@ igb_core_pci_uninit(IGBCore *core) igb_intrmgr_pci_unint(core); - qemu_del_vm_change_state_handler(core->vmstate); - for (i = 0; i < IGB_NUM_QUEUES; i++) { net_tx_pkt_uninit(core->tx[i].tx_pkt); } @@ -4586,5 +4542,12 @@ igb_core_post_load(IGBCore *core) */ nc->link_down = (core->mac[STATUS] & E1000_STATUS_LU) == 0; + /* + * we need to restart intrmgr timers, as an older version of + * QEMU can have stopped them before migration + */ + igb_intrmgr_resume(core); + igb_autoneg_resume(core); + return 0; } diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index bf8c46f26b51..d70b54e318f1 100644 --- a/hw/net/igb_core.h +++ b/hw/net/igb_core.h @@ -90,8 +90,6 @@ struct IGBCore { IGBIntrDelayTimer eitr[IGB_INTR_NUM]; - VMChangeStateEntry *vmstate; - uint32_t eitr_guest_value[IGB_INTR_NUM]; uint8_t permanent_mac[ETH_ALEN];
On resume igb_vm_state_change() always calls igb_autoneg_resume() that sets link_down to false, and thus activates the link even if we have disabled it. The problem can be reproduced starting qemu in paused state (-S) and then set the link to down. When we resume the machine the link appears to be up. Reproducer: # qemu-system-x86_64 ... -device igb,netdev=netdev0,id=net0 -S {"execute": "qmp_capabilities" } {"execute": "set_link", "arguments": {"name": "net0", "up": false}} {"execute": "cont" } To fix the problem, merge the content of igb_vm_state_change() into igb_core_post_load() as e1000 does. Buglink: https://issues.redhat.com/browse/RHEL-21867 Fixes: 3a977deebe6b ("Intrdocue igb device emulation") Cc: akihiko.odaki@daynix.com Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com> Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- Notes: v2: Add Fixes: and a comment about igb_intrmgr_resume() purpose. hw/net/igb_core.c | 51 +++++++---------------------------------------- hw/net/igb_core.h | 2 -- 2 files changed, 7 insertions(+), 46 deletions(-)