| Message ID | 20240123112501.305681-1-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] igb: fix link state on resume | expand |
On 2024/01/23 20:25, 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 Please also add Fixes: tag as described in: docs/devel/submitting-a-patch.rst > Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/net/igb_core.c | 47 +++-------------------------------------------- > hw/net/igb_core.h | 2 -- > 2 files changed, 3 insertions(+), 46 deletions(-) > > diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c > index 2a7a11aa9ed5..31ab959ab8ff 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,8 @@ igb_core_post_load(IGBCore *core) > */ > nc->link_down = (core->mac[STATUS] & E1000_STATUS_LU) == 0; > > + igb_intrmgr_resume(core); Please add a comment to note that an old version of QEMU pauses the intrmgr timers. Otherwise this looks unnecessary. Regards, Akihiko Odaki
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index 2a7a11aa9ed5..31ab959ab8ff 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,8 @@ igb_core_post_load(IGBCore *core) */ nc->link_down = (core->mac[STATUS] & E1000_STATUS_LU) == 0; + 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 Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com> Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- hw/net/igb_core.c | 47 +++-------------------------------------------- hw/net/igb_core.h | 2 -- 2 files changed, 3 insertions(+), 46 deletions(-)