e1000 migration changes for 2.12

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/net/e1000.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

hw/net/e1000.c +17 -2

index 4e606d4b2a..13a9494a8d 100644
@@ -130,6 +130,7 @@ typedef struct E1000State_st {
 #define E1000_FLAG_TSO (1 << E1000_FLAG_TSO_BIT)
     uint32_t compat_flags;
     bool received_tx_tso;
+    bool use_tso_for_migration;
     e1000x_txd_props mig_props;
 } E1000State;
 

@@ -622,9 +623,11 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
     if (dtype == E1000_TXD_CMD_DEXT) {    /* context descriptor */
         if (le32_to_cpu(xp->cmd_and_length) & E1000_TXD_CMD_TSE) {
             e1000x_read_tx_ctx_descr(xp, &tp->tso_props);
+            s->use_tso_for_migration = 1;
             tp->tso_frames = 0;
         } else {
             e1000x_read_tx_ctx_descr(xp, &tp->props);
+            s->use_tso_for_migration = 0;
         }
         return;
     } else if (dtype == (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D)) {

@@ -1366,7 +1369,20 @@ static int e1000_pre_save(void *opaque)
         s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE;
     }
 
-    s->mig_props = s->tx.props;
+    /* Decide which set of props to migrate in the main structure */
+    if (chkflag(TSO) || !s->use_tso_for_migration) {
+        /* Either we're migrating with the extra subsection, in which
+         * case the mig_props is always 'props' OR
+         * we've not got the subsection, but 'props' was the last
+         * updated.
+         */
+        s->mig_props = s->tx.props;
+    } else {
+        /* We're not using the subsection, and 'tso_props' was
+         * the last updated.
+         */
+        s->mig_props = s->tx.tso_props;
+    }
     return 0;
 }
 
--

2.14.3

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

Quoted 5 lines

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

But only after migration. Why not simply switch back to the old behavior if migrate_tso_props if false?

Thanks

Quoted 9 lines · * Jason Wang ([email protected]) wrote:

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

Quoted 7 lines

Quoted 5 lines

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

But only after migration. Why not simply switch back to the old behavior if migrate_tso_props if false?

Because:
1) We know it's a broken behaviour so it's better not to unfix it
2) The fix doesn't change guest visible behaviour other than actually sending the right packets; so there's no reason to make the fix itself dependent on the machine type.
3) Gating the fix itself on the flag is actually more complex and would need checking the flag in lots of places that are already pretty complex, rather than what this does which is just check it in one place at migration.

Dave

Quoted 2 lines

Thanks

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 17 lines · Dr. David Alan Gilbert

Quoted 9 lines · * Jason Wang ([email protected]) wrote:

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

Quoted 7 lines

Quoted 5 lines

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

But only after migration. Why not simply switch back to the old behavior if migrate_tso_props if false?

Quoted 9 lines

Because:
1) We know it's a broken behaviour so it's better not to unfix it
2) The fix doesn't change guest visible behaviour other than actually sending the right packets; so there's no reason to make the fix itself dependent on the machine type.
3) Gating the fix itself on the flag is actually more complex and would need checking the flag in lots of places that are already pretty complex, rather than what this does which is just check it in one place at migration.

It looks to me it was just something like:

    struct e1000x_txd_props *props = tp->cptse && chkflag(TSO) ? &tp->tso_props : &tp->props;

Btw, did this patch work when:

migrate A to B
migrate B to C

But there's no tx during B?

Thanks

Quoted 7 lines

Dave

Quoted 2 lines

Thanks

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 27 lines · * Jason Wang ([email protected]) wrote:

Quoted 25 lines · Dr. David Alan Gilbert

Quoted 8 lines · * Jason Wang ([email protected]) wrote:

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

Quoted 7 lines

Quoted 5 lines

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

But only after migration. Why not simply switch back to the old behavior if migrate_tso_props if false?

Because:
1) We know it's a broken behaviour so it's better not to unfix it
2) The fix doesn't change guest visible behaviour other than actually sending the right packets; so there's no reason to make the fix itself dependent on the machine type.
3) Gating the fix itself on the flag is actually more complex and would need checking the flag in lots of places that are already pretty complex, rather than what this does which is just check it in one place at migration.

Quoted 4 lines

It looks to me it was just something like:

    struct e1000x_txd_props *props = tp->cptse && chkflag(TSO) ?
&tp->tso_props : &tp->props;

is that the only thing that would change?

Quoted 7 lines

Btw, did this patch work when:

migrate A to B
migrate B to C

But there's no tx during B?

Hmm, good question; I only tried it keeping the stream alive during migration.
Lets see what happens.

For this code to be used we have to be running with an old machine type/property.
That means A->B will have either come from 2.11 or a 2.12 with this same patch.
But given patch 2 that duplicates on loading; that means A->B should end up with B having the same data in both sets of props, and thus it doesn't matter which set this patch picks.

Dave

Quoted 8 lines

Thanks

Quoted 6 lines

Dave

Quoted 2 lines

Thanks

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

From: "Dr. David Alan Gilbert" <[email protected]>

When we're using the subsection we migrate both
the 'props' and 'tso_props' data; when we're not using the subsection (to migrate to 2.11 or old machine types) we've got to choose what to migrate in the main structure.

If we're using the subsection migrate 'props' in the main structure. If we're not using the subsection then migrate the last one that changed, which gives behaviour similar to the old behaviour.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/net/e1000.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

hw/net/e1000.c +9 -1

index 4e606d4b2a..13a9494a8d 100644
@@ -130,6 +130,7 @@ typedef struct E1000State_st {
 #define E1000_FLAG_TSO (1 << E1000_FLAG_TSO_BIT)
     uint32_t compat_flags;
     bool received_tx_tso;
+    bool use_tso_for_migration;
     e1000x_txd_props mig_props;
 } E1000State;
 

@@ -622,9 +623,11 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
     if (dtype == E1000_TXD_CMD_DEXT) {    /* context descriptor */
         if (le32_to_cpu(xp->cmd_and_length) & E1000_TXD_CMD_TSE) {
             e1000x_read_tx_ctx_descr(xp, &tp->tso_props);
+            s->use_tso_for_migration = 1;
             tp->tso_frames = 0;
         } else {
             e1000x_read_tx_ctx_descr(xp, &tp->props);
+            s->use_tso_for_migration = 0;
         }
         return;
     } else if (dtype == (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D)) {

@@ -1366,7 +1369,20 @@ static int e1000_pre_save(void *opaque)
         s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE;
     }
 
-    s->mig_props = s->tx.props;
+    /* Decide which set of props to migrate in the main structure */
+    if (chkflag(TSO) || !s->use_tso_for_migration) {
+        /* Either we're migrating with the extra subsection, in which
+         * case the mig_props is always 'props' OR
+         * we've not got the subsection, but 'props' was the last
+         * updated.

From: "Dr. David Alan Gilbert" <[email protected]>

Hi,
This set of patches change the e1000 migration code to make
it easier to keep with compatibility with older versions in backwards migration.

I think the first 3 patches are fairly uncontrovercial and I would like them for 2.12; it would be nice to have the lot since changing them after we've shipped is much more difficult.

v2
Ed and Paolo answered my question that I asked in the cover letter;
and I think I've followed the advice - although my testing has been very light. The new patches do two things:
a) When we receive a stream without the subsection we duplicate the
received pops state into both props and tso_props.
b) When we send without the subsection we decide which set to send
in the main part of the state based on which state was last changed.

Dave

Dr. David Alan Gilbert (6):
e1000: Convert v3 fields to subsection
e1000: Dupe offload data on reading old stream
e1000: wire new subsection to property
e1000: Migrate props via a temporary structure
e1000: Choose which set of props to migrate
e1000: Old machine types, turn new subsection off

  hw/net/e1000.c      | 103 ++++++++++++++++++++++++++++++++++++++++------------
  include/hw/compat.h |   4 ++
  2 files changed, 84 insertions(+), 23 deletions(-)