* [PATCH 1/2] igb: fix link state on resume
@ 2024-01-23 11:25 Laurent Vivier
2024-01-23 11:25 ` [PATCH 2/2] e1000e: " Laurent Vivier
2024-01-24 3:34 ` [PATCH 1/2] igb: " Akihiko Odaki
0 siblings, 2 replies; 3+ messages in thread
From: Laurent Vivier @ 2024-01-23 11:25 UTC (permalink / raw)
To: qemu-devel
Cc: Akihiko Odaki, Sriram Yagnaraman, Jason Wang, Dmitry Fleytman,
Laurent Vivier
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(-)
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];
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] e1000e: fix link state on resume
2024-01-23 11:25 [PATCH 1/2] igb: fix link state on resume Laurent Vivier
@ 2024-01-23 11:25 ` Laurent Vivier
2024-01-24 3:34 ` [PATCH 1/2] igb: " Akihiko Odaki
1 sibling, 0 replies; 3+ messages in thread
From: Laurent Vivier @ 2024-01-23 11:25 UTC (permalink / raw)
To: qemu-devel
Cc: Akihiko Odaki, Sriram Yagnaraman, Jason Wang, Dmitry Fleytman,
Laurent Vivier
On resume e1000e_vm_state_change() always calls e1000e_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 e1000e,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 e1000e_vm_state_change()
into e1000e_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/e1000e_core.c | 50 +++-----------------------------------------
hw/net/e1000e_core.h | 2 --
2 files changed, 3 insertions(+), 49 deletions(-)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index e324c02dd589..95311fd60530 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -123,14 +123,6 @@ e1000e_intmgr_timer_resume(E1000IntrDelayTimer *timer)
}
}
-static void
-e1000e_intmgr_timer_pause(E1000IntrDelayTimer *timer)
-{
- if (timer->running) {
- timer_del(timer->timer);
- }
-}
-
static inline void
e1000e_intrmgr_stop_timer(E1000IntrDelayTimer *timer)
{
@@ -398,24 +390,6 @@ e1000e_intrmgr_resume(E1000ECore *core)
}
}
-static void
-e1000e_intrmgr_pause(E1000ECore *core)
-{
- int i;
-
- e1000e_intmgr_timer_pause(&core->radv);
- e1000e_intmgr_timer_pause(&core->rdtr);
- e1000e_intmgr_timer_pause(&core->raid);
- e1000e_intmgr_timer_pause(&core->tidv);
- e1000e_intmgr_timer_pause(&core->tadv);
-
- e1000e_intmgr_timer_pause(&core->itr);
-
- for (i = 0; i < E1000E_MSIX_VEC_NUM; i++) {
- e1000e_intmgr_timer_pause(&core->eitr[i]);
- }
-}
-
static void
e1000e_intrmgr_reset(E1000ECore *core)
{
@@ -3351,22 +3325,6 @@ e1000e_autoneg_resume(E1000ECore *core)
}
}
-static void
-e1000e_vm_state_change(void *opaque, bool running, RunState state)
-{
- E1000ECore *core = opaque;
-
- if (running) {
- trace_e1000e_vm_state_running();
- e1000e_intrmgr_resume(core);
- e1000e_autoneg_resume(core);
- } else {
- trace_e1000e_vm_state_stopped();
- e1000e_autoneg_pause(core);
- e1000e_intrmgr_pause(core);
- }
-}
-
void
e1000e_core_pci_realize(E1000ECore *core,
const uint16_t *eeprom_templ,
@@ -3379,9 +3337,6 @@ e1000e_core_pci_realize(E1000ECore *core,
e1000e_autoneg_timer, core);
e1000e_intrmgr_pci_realize(core);
- core->vmstate =
- qemu_add_vm_change_state_handler(e1000e_vm_state_change, core);
-
for (i = 0; i < E1000E_NUM_QUEUES; i++) {
net_tx_pkt_init(&core->tx[i].tx_pkt, E1000E_MAX_TX_FRAGS);
}
@@ -3405,8 +3360,6 @@ e1000e_core_pci_uninit(E1000ECore *core)
e1000e_intrmgr_pci_unint(core);
- qemu_del_vm_change_state_handler(core->vmstate);
-
for (i = 0; i < E1000E_NUM_QUEUES; i++) {
net_tx_pkt_uninit(core->tx[i].tx_pkt);
}
@@ -3576,5 +3529,8 @@ e1000e_core_post_load(E1000ECore *core)
*/
nc->link_down = (core->mac[STATUS] & E1000_STATUS_LU) == 0;
+ e1000e_intrmgr_resume(core);
+ e1000e_autoneg_resume(core);
+
return 0;
}
diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h
index 66b025cc43f1..01510ca78b47 100644
--- a/hw/net/e1000e_core.h
+++ b/hw/net/e1000e_core.h
@@ -98,8 +98,6 @@ struct E1000Core {
E1000IntrDelayTimer eitr[E1000E_MSIX_VEC_NUM];
- VMChangeStateEntry *vmstate;
-
uint32_t itr_guest_value;
uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM];
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] igb: fix link state on resume
2024-01-23 11:25 [PATCH 1/2] igb: fix link state on resume Laurent Vivier
2024-01-23 11:25 ` [PATCH 2/2] e1000e: " Laurent Vivier
@ 2024-01-24 3:34 ` Akihiko Odaki
1 sibling, 0 replies; 3+ messages in thread
From: Akihiko Odaki @ 2024-01-24 3:34 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: Sriram Yagnaraman, Jason Wang, Dmitry Fleytman
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-24 3:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-23 11:25 [PATCH 1/2] igb: fix link state on resume Laurent Vivier
2024-01-23 11:25 ` [PATCH 2/2] e1000e: " Laurent Vivier
2024-01-24 3:34 ` [PATCH 1/2] igb: " Akihiko Odaki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).