| Message ID | 163524428177.1917083.7115508068018047923.stgit@pasha-ThinkPad-X280 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net: store timers for e1000 in vmstate | expand |
* Pavel Dovgalyuk (pavel.dovgalyuk@ispras.ru) wrote: > Setting timers randomly when vmstate is loaded breaks > execution determinism. > Therefore this patch allows saving mit and autoneg timers > for e1000. It makes execution deterministic and allows > snapshotting and reverse debugging in icount mode. > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru> I think you need to wire those needed's to machine types (via a parameter or something) so that they don't break backwards migration compatibility. Dave > --- > hw/net/e1000.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 50 insertions(+), 11 deletions(-) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index a30546c5d5..2f706f7298 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -37,6 +37,7 @@ > #include "qemu/iov.h" > #include "qemu/module.h" > #include "qemu/range.h" > +#include "sysemu/replay.h" > > #include "e1000x_common.h" > #include "trace.h" > @@ -1407,7 +1408,7 @@ static int e1000_pre_save(void *opaque) > * complete auto-negotiation immediately. This allows us to look > * at MII_SR_AUTONEG_COMPLETE to infer link status on load. > */ > - if (nc->link_down && have_autoneg(s)) { > + if (replay_mode == REPLAY_MODE_NONE && nc->link_down && have_autoneg(s)) { > s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE; > } > > @@ -1438,22 +1439,12 @@ static int e1000_post_load(void *opaque, int version_id) > s->mac_reg[TADV] = 0; > s->mit_irq_level = false; > } > - s->mit_ide = 0; > - s->mit_timer_on = true; > - timer_mod(s->mit_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1); > > /* nc.link_down can't be migrated, so infer link_down according > * to link status bit in mac_reg[STATUS]. > * Alternatively, restart link negotiation if it was in progress. */ > nc->link_down = (s->mac_reg[STATUS] & E1000_STATUS_LU) == 0; > > - if (have_autoneg(s) && > - !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE)) { > - nc->link_down = false; > - timer_mod(s->autoneg_timer, > - qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500); > - } > - > s->tx.props = s->mig_props; > if (!s->received_tx_tso) { > /* We received only one set of offload data (tx.props) > @@ -1472,6 +1463,13 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id) > return 0; > } > > +static int e1000_mit_timer_post_load(void *opaque, int version_id) > +{ > + E1000State *s = opaque; > + s->mit_timer_on = true; > + return 0; > +} > + > static bool e1000_mit_state_needed(void *opaque) > { > E1000State *s = opaque; > @@ -1493,6 +1491,21 @@ static bool e1000_tso_state_needed(void *opaque) > return chkflag(TSO); > } > > +static bool e1000_mit_timer_needed(void *opaque) > +{ > + E1000State *s = opaque; > + > + return s->mit_timer_on; > +} > + > +static bool e1000_autoneg_timer_needed(void *opaque) > +{ > + E1000State *s = opaque; > + > + return have_autoneg(s) > + && !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE); > +} > + > static const VMStateDescription vmstate_e1000_mit_state = { > .name = "e1000/mit_state", > .version_id = 1, > @@ -1541,6 +1554,30 @@ static const VMStateDescription vmstate_e1000_tx_tso_state = { > } > }; > > +static const VMStateDescription vmstate_e1000_mit_timer = { > + .name = "e1000/mit_timer", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = e1000_mit_timer_needed, > + .post_load = e1000_mit_timer_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_TIMER_PTR(mit_timer, E1000State), > + VMSTATE_UINT32(mit_ide, E1000State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_e1000_autoneg_timer = { > + .name = "e1000/autoneg_timer", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = e1000_autoneg_timer_needed, > + .fields = (VMStateField[]) { > + VMSTATE_TIMER_PTR(autoneg_timer, E1000State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_e1000 = { > .name = "e1000", > .version_id = 2, > @@ -1622,6 +1659,8 @@ static const VMStateDescription vmstate_e1000 = { > &vmstate_e1000_mit_state, > &vmstate_e1000_full_mac_state, > &vmstate_e1000_tx_tso_state, > + &vmstate_e1000_mit_timer, > + &vmstate_e1000_autoneg_timer, > NULL > } > }; >
On Tue, Oct 26, 2021 at 6:36 PM Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote: > > Setting timers randomly when vmstate is loaded breaks > execution determinism. > Therefore this patch allows saving mit and autoneg timers > for e1000. It makes execution deterministic and allows > snapshotting and reverse debugging in icount mode. > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru> > --- > hw/net/e1000.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 50 insertions(+), 11 deletions(-) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index a30546c5d5..2f706f7298 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -37,6 +37,7 @@ > #include "qemu/iov.h" > #include "qemu/module.h" > #include "qemu/range.h" > +#include "sysemu/replay.h" > > #include "e1000x_common.h" > #include "trace.h" > @@ -1407,7 +1408,7 @@ static int e1000_pre_save(void *opaque) > * complete auto-negotiation immediately. This allows us to look > * at MII_SR_AUTONEG_COMPLETE to infer link status on load. > */ > - if (nc->link_down && have_autoneg(s)) { > + if (replay_mode == REPLAY_MODE_NONE && nc->link_down && have_autoneg(s)) { > s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE; > } > > @@ -1438,22 +1439,12 @@ static int e1000_post_load(void *opaque, int version_id) > s->mac_reg[TADV] = 0; > s->mit_irq_level = false; > } > - s->mit_ide = 0; > - s->mit_timer_on = true; > - timer_mod(s->mit_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1); > > /* nc.link_down can't be migrated, so infer link_down according > * to link status bit in mac_reg[STATUS]. > * Alternatively, restart link negotiation if it was in progress. */ > nc->link_down = (s->mac_reg[STATUS] & E1000_STATUS_LU) == 0; > > - if (have_autoneg(s) && > - !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE)) { > - nc->link_down = false; > - timer_mod(s->autoneg_timer, > - qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500); > - } So we won't get those timers armed after migration unconditionally. Is this intended? Thanks > - > s->tx.props = s->mig_props; > if (!s->received_tx_tso) { > /* We received only one set of offload data (tx.props) > @@ -1472,6 +1463,13 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id) > return 0; > } > > +static int e1000_mit_timer_post_load(void *opaque, int version_id) > +{ > + E1000State *s = opaque; > + s->mit_timer_on = true; > + return 0; > +} > + > static bool e1000_mit_state_needed(void *opaque) > { > E1000State *s = opaque; > @@ -1493,6 +1491,21 @@ static bool e1000_tso_state_needed(void *opaque) > return chkflag(TSO); > } > > +static bool e1000_mit_timer_needed(void *opaque) > +{ > + E1000State *s = opaque; > + > + return s->mit_timer_on; > +} > + > +static bool e1000_autoneg_timer_needed(void *opaque) > +{ > + E1000State *s = opaque; > + > + return have_autoneg(s) > + && !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE); > +} > + > static const VMStateDescription vmstate_e1000_mit_state = { > .name = "e1000/mit_state", > .version_id = 1, > @@ -1541,6 +1554,30 @@ static const VMStateDescription vmstate_e1000_tx_tso_state = { > } > }; > > +static const VMStateDescription vmstate_e1000_mit_timer = { > + .name = "e1000/mit_timer", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = e1000_mit_timer_needed, > + .post_load = e1000_mit_timer_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_TIMER_PTR(mit_timer, E1000State), > + VMSTATE_UINT32(mit_ide, E1000State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_e1000_autoneg_timer = { > + .name = "e1000/autoneg_timer", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = e1000_autoneg_timer_needed, > + .fields = (VMStateField[]) { > + VMSTATE_TIMER_PTR(autoneg_timer, E1000State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_e1000 = { > .name = "e1000", > .version_id = 2, > @@ -1622,6 +1659,8 @@ static const VMStateDescription vmstate_e1000 = { > &vmstate_e1000_mit_state, > &vmstate_e1000_full_mac_state, > &vmstate_e1000_tx_tso_state, > + &vmstate_e1000_mit_timer, > + &vmstate_e1000_autoneg_timer, > NULL > } > }; >
On 27.10.2021 07:05, Jason Wang wrote: > On Tue, Oct 26, 2021 at 6:36 PM Pavel Dovgalyuk > <pavel.dovgalyuk@ispras.ru> wrote: >> >> Setting timers randomly when vmstate is loaded breaks >> execution determinism. >> Therefore this patch allows saving mit and autoneg timers >> for e1000. It makes execution deterministic and allows >> snapshotting and reverse debugging in icount mode. >> >> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru> >> --- >> hw/net/e1000.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 50 insertions(+), 11 deletions(-) >> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c >> index a30546c5d5..2f706f7298 100644 >> --- a/hw/net/e1000.c >> +++ b/hw/net/e1000.c >> @@ -37,6 +37,7 @@ >> #include "qemu/iov.h" >> #include "qemu/module.h" >> #include "qemu/range.h" >> +#include "sysemu/replay.h" >> >> #include "e1000x_common.h" >> #include "trace.h" >> @@ -1407,7 +1408,7 @@ static int e1000_pre_save(void *opaque) >> * complete auto-negotiation immediately. This allows us to look >> * at MII_SR_AUTONEG_COMPLETE to infer link status on load. >> */ >> - if (nc->link_down && have_autoneg(s)) { >> + if (replay_mode == REPLAY_MODE_NONE && nc->link_down && have_autoneg(s)) { >> s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE; >> } >> >> @@ -1438,22 +1439,12 @@ static int e1000_post_load(void *opaque, int version_id) >> s->mac_reg[TADV] = 0; >> s->mit_irq_level = false; >> } >> - s->mit_ide = 0; >> - s->mit_timer_on = true; >> - timer_mod(s->mit_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1); >> >> /* nc.link_down can't be migrated, so infer link_down according >> * to link status bit in mac_reg[STATUS]. >> * Alternatively, restart link negotiation if it was in progress. */ >> nc->link_down = (s->mac_reg[STATUS] & E1000_STATUS_LU) == 0; >> >> - if (have_autoneg(s) && >> - !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE)) { >> - nc->link_down = false; >> - timer_mod(s->autoneg_timer, >> - qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500); >> - } > > So we won't get those timers armed after migration unconditionally. Is > this intended? Not really. I think there could be several solutions: 1. Save some flag to distinguish between old and new state. 2. Use deterministic version for icount (or even record/replay) mode only. 3. Check machine type to change the behavior (as Dave proposed) > > Thanks > >> - >> s->tx.props = s->mig_props; >> if (!s->received_tx_tso) { >> /* We received only one set of offload data (tx.props) >> @@ -1472,6 +1463,13 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id) >> return 0; >> } >> >> +static int e1000_mit_timer_post_load(void *opaque, int version_id) >> +{ >> + E1000State *s = opaque; >> + s->mit_timer_on = true; >> + return 0; >> +} >> + >> static bool e1000_mit_state_needed(void *opaque) >> { >> E1000State *s = opaque; >> @@ -1493,6 +1491,21 @@ static bool e1000_tso_state_needed(void *opaque) >> return chkflag(TSO); >> } >> >> +static bool e1000_mit_timer_needed(void *opaque) >> +{ >> + E1000State *s = opaque; >> + >> + return s->mit_timer_on; >> +} >> + >> +static bool e1000_autoneg_timer_needed(void *opaque) >> +{ >> + E1000State *s = opaque; >> + >> + return have_autoneg(s) >> + && !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE); >> +} >> + >> static const VMStateDescription vmstate_e1000_mit_state = { >> .name = "e1000/mit_state", >> .version_id = 1, >> @@ -1541,6 +1554,30 @@ static const VMStateDescription vmstate_e1000_tx_tso_state = { >> } >> }; >> >> +static const VMStateDescription vmstate_e1000_mit_timer = { >> + .name = "e1000/mit_timer", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .needed = e1000_mit_timer_needed, >> + .post_load = e1000_mit_timer_post_load, >> + .fields = (VMStateField[]) { >> + VMSTATE_TIMER_PTR(mit_timer, E1000State), >> + VMSTATE_UINT32(mit_ide, E1000State), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static const VMStateDescription vmstate_e1000_autoneg_timer = { >> + .name = "e1000/autoneg_timer", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .needed = e1000_autoneg_timer_needed, >> + .fields = (VMStateField[]) { >> + VMSTATE_TIMER_PTR(autoneg_timer, E1000State), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static const VMStateDescription vmstate_e1000 = { >> .name = "e1000", >> .version_id = 2, >> @@ -1622,6 +1659,8 @@ static const VMStateDescription vmstate_e1000 = { >> &vmstate_e1000_mit_state, >> &vmstate_e1000_full_mac_state, >> &vmstate_e1000_tx_tso_state, >> + &vmstate_e1000_mit_timer, >> + &vmstate_e1000_autoneg_timer, >> NULL >> } >> }; >> >
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index a30546c5d5..2f706f7298 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -37,6 +37,7 @@ #include "qemu/iov.h" #include "qemu/module.h" #include "qemu/range.h" +#include "sysemu/replay.h" #include "e1000x_common.h" #include "trace.h" @@ -1407,7 +1408,7 @@ static int e1000_pre_save(void *opaque) * complete auto-negotiation immediately. This allows us to look * at MII_SR_AUTONEG_COMPLETE to infer link status on load. */ - if (nc->link_down && have_autoneg(s)) { + if (replay_mode == REPLAY_MODE_NONE && nc->link_down && have_autoneg(s)) { s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE; } @@ -1438,22 +1439,12 @@ static int e1000_post_load(void *opaque, int version_id) s->mac_reg[TADV] = 0; s->mit_irq_level = false; } - s->mit_ide = 0; - s->mit_timer_on = true; - timer_mod(s->mit_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1); /* nc.link_down can't be migrated, so infer link_down according * to link status bit in mac_reg[STATUS]. * Alternatively, restart link negotiation if it was in progress. */ nc->link_down = (s->mac_reg[STATUS] & E1000_STATUS_LU) == 0; - if (have_autoneg(s) && - !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE)) { - nc->link_down = false; - timer_mod(s->autoneg_timer, - qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500); - } - s->tx.props = s->mig_props; if (!s->received_tx_tso) { /* We received only one set of offload data (tx.props) @@ -1472,6 +1463,13 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id) return 0; } +static int e1000_mit_timer_post_load(void *opaque, int version_id) +{ + E1000State *s = opaque; + s->mit_timer_on = true; + return 0; +} + static bool e1000_mit_state_needed(void *opaque) { E1000State *s = opaque; @@ -1493,6 +1491,21 @@ static bool e1000_tso_state_needed(void *opaque) return chkflag(TSO); } +static bool e1000_mit_timer_needed(void *opaque) +{ + E1000State *s = opaque; + + return s->mit_timer_on; +} + +static bool e1000_autoneg_timer_needed(void *opaque) +{ + E1000State *s = opaque; + + return have_autoneg(s) + && !(s->phy_reg[PHY_STATUS] & MII_SR_AUTONEG_COMPLETE); +} + static const VMStateDescription vmstate_e1000_mit_state = { .name = "e1000/mit_state", .version_id = 1, @@ -1541,6 +1554,30 @@ static const VMStateDescription vmstate_e1000_tx_tso_state = { } }; +static const VMStateDescription vmstate_e1000_mit_timer = { + .name = "e1000/mit_timer", + .version_id = 1, + .minimum_version_id = 1, + .needed = e1000_mit_timer_needed, + .post_load = e1000_mit_timer_post_load, + .fields = (VMStateField[]) { + VMSTATE_TIMER_PTR(mit_timer, E1000State), + VMSTATE_UINT32(mit_ide, E1000State), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_e1000_autoneg_timer = { + .name = "e1000/autoneg_timer", + .version_id = 1, + .minimum_version_id = 1, + .needed = e1000_autoneg_timer_needed, + .fields = (VMStateField[]) { + VMSTATE_TIMER_PTR(autoneg_timer, E1000State), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_e1000 = { .name = "e1000", .version_id = 2, @@ -1622,6 +1659,8 @@ static const VMStateDescription vmstate_e1000 = { &vmstate_e1000_mit_state, &vmstate_e1000_full_mac_state, &vmstate_e1000_tx_tso_state, + &vmstate_e1000_mit_timer, + &vmstate_e1000_autoneg_timer, NULL } };
Setting timers randomly when vmstate is loaded breaks execution determinism. Therefore this patch allows saving mit and autoneg timers for e1000. It makes execution deterministic and allows snapshotting and reverse debugging in icount mode. Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru> --- hw/net/e1000.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 11 deletions(-)