[Qemu-devel] [PATCH v2 0/6] e1000 migration changes for 2.12

[Qemu-devel] [PATCH v2 0/6] e1000 migration changes for 2.12

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/6] e1000: Convert v3 fields to subsection

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

A bunch of new TSO fields were introduced by d62644b4 and this bumped
the VMState version; however it's easier for those trying to keep
backwards migration compatibility if these fields are added in a
subsection instead.

Move the new fields to a subsection.

Since this was added after 2.11, this change will only affect
compatbility with 2.12-rc0.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/e1000.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index c7f1695f57..24e9a4ab40 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1433,9 +1433,29 @@ static const VMStateDescription vmstate_e1000_full_mac_state = {
     }
 };
 
+static const VMStateDescription vmstate_e1000_tx_tso_state = {
+    .name = "e1000/tx_tso_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(tx.tso_props.ipcss, E1000State),
+        VMSTATE_UINT8(tx.tso_props.ipcso, E1000State),
+        VMSTATE_UINT16(tx.tso_props.ipcse, E1000State),
+        VMSTATE_UINT8(tx.tso_props.tucss, E1000State),
+        VMSTATE_UINT8(tx.tso_props.tucso, E1000State),
+        VMSTATE_UINT16(tx.tso_props.tucse, E1000State),
+        VMSTATE_UINT32(tx.tso_props.paylen, E1000State),
+        VMSTATE_UINT8(tx.tso_props.hdr_len, E1000State),
+        VMSTATE_UINT16(tx.tso_props.mss, E1000State),
+        VMSTATE_INT8(tx.tso_props.ip, E1000State),
+        VMSTATE_INT8(tx.tso_props.tcp, E1000State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_e1000 = {
     .name = "e1000",
-    .version_id = 3,
+    .version_id = 2,
     .minimum_version_id = 1,
     .pre_save = e1000_pre_save,
     .post_load = e1000_post_load,
@@ -1508,22 +1528,12 @@ static const VMStateDescription vmstate_e1000 = {
         VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, RA, 32),
         VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, MTA, 128),
         VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, VFTA, 128),
-        VMSTATE_UINT8_V(tx.tso_props.ipcss, E1000State, 3),
-        VMSTATE_UINT8_V(tx.tso_props.ipcso, E1000State, 3),
-        VMSTATE_UINT16_V(tx.tso_props.ipcse, E1000State, 3),
-        VMSTATE_UINT8_V(tx.tso_props.tucss, E1000State, 3),
-        VMSTATE_UINT8_V(tx.tso_props.tucso, E1000State, 3),
-        VMSTATE_UINT16_V(tx.tso_props.tucse, E1000State, 3),
-        VMSTATE_UINT32_V(tx.tso_props.paylen, E1000State, 3),
-        VMSTATE_UINT8_V(tx.tso_props.hdr_len, E1000State, 3),
-        VMSTATE_UINT16_V(tx.tso_props.mss, E1000State, 3),
-        VMSTATE_INT8_V(tx.tso_props.ip, E1000State, 3),
-        VMSTATE_INT8_V(tx.tso_props.tcp, E1000State, 3),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_e1000_mit_state,
         &vmstate_e1000_full_mac_state,
+        &vmstate_e1000_tx_tso_state,
         NULL
     }
 };
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/6] e1000: Dupe offload data on reading old stream

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Old QEMUs only had one set of offload data;  when we only receive
one lot, dupe the received data - that should give us about the
same bug level as the old version.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/e1000.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 24e9a4ab40..d399ce3e4f 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -127,6 +127,7 @@ typedef struct E1000State_st {
 #define E1000_FLAG_MIT (1 << E1000_FLAG_MIT_BIT)
 #define E1000_FLAG_MAC (1 << E1000_FLAG_MAC_BIT)
     uint32_t compat_flags;
+    bool received_tx_tso;
 } E1000State;
 
 #define chkflag(x)     (s->compat_flags & E1000_FLAG_##x)
@@ -1390,6 +1391,20 @@ static int e1000_post_load(void *opaque, int version_id)
                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
     }
 
+    if (!s->received_tx_tso) {
+        /* We received only one set of offload data (tx.props)
+         * and haven't got tx.tso_props.  The best we can do
+         * is dupe the data.
+         */
+        s->tx.tso_props = s->tx.props;
+    }
+    return 0;
+}
+
+static int e1000_tx_tso_post_load(void *opaque, int version_id)
+{
+    E1000State *s = opaque;
+    s->received_tx_tso = true;
     return 0;
 }
 
@@ -1437,6 +1452,7 @@ static const VMStateDescription vmstate_e1000_tx_tso_state = {
     .name = "e1000/tx_tso_state",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = e1000_tx_tso_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(tx.tso_props.ipcss, E1000State),
         VMSTATE_UINT8(tx.tso_props.ipcso, E1000State),
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/6] e1000: wire new subsection to property

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Wire the new subsection from the previous commit to a property
so we can turn it off easily.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/e1000.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d399ce3e4f..bb8ee2acb0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -123,9 +123,11 @@ typedef struct E1000State_st {
 #define E1000_FLAG_AUTONEG_BIT 0
 #define E1000_FLAG_MIT_BIT 1
 #define E1000_FLAG_MAC_BIT 2
+#define E1000_FLAG_TSO_BIT 3
 #define E1000_FLAG_AUTONEG (1 << E1000_FLAG_AUTONEG_BIT)
 #define E1000_FLAG_MIT (1 << E1000_FLAG_MIT_BIT)
 #define E1000_FLAG_MAC (1 << E1000_FLAG_MAC_BIT)
+#define E1000_FLAG_TSO (1 << E1000_FLAG_TSO_BIT)
     uint32_t compat_flags;
     bool received_tx_tso;
 } E1000State;
@@ -1422,6 +1424,13 @@ static bool e1000_full_mac_needed(void *opaque)
     return chkflag(MAC);
 }
 
+static bool e1000_tso_state_needed(void *opaque)
+{
+    E1000State *s = opaque;
+
+    return chkflag(TSO);
+}
+
 static const VMStateDescription vmstate_e1000_mit_state = {
     .name = "e1000/mit_state",
     .version_id = 1,
@@ -1452,6 +1461,7 @@ static const VMStateDescription vmstate_e1000_tx_tso_state = {
     .name = "e1000/tx_tso_state",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = e1000_tso_state_needed,
     .post_load = e1000_tx_tso_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(tx.tso_props.ipcss, E1000State),
@@ -1677,6 +1687,8 @@ static Property e1000_properties[] = {
                     compat_flags, E1000_FLAG_MIT_BIT, true),
     DEFINE_PROP_BIT("extra_mac_registers", E1000State,
                     compat_flags, E1000_FLAG_MAC_BIT, true),
+    DEFINE_PROP_BIT("migrate_tso_props", E1000State,
+                    compat_flags, E1000_FLAG_TSO_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 4/6] e1000: Migrate props via a temporary structure

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Swing the tx.props out via a temporary structure, so in future patches
we can select what we're going to send.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/e1000.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bb8ee2acb0..4e606d4b2a 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -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;
+    e1000x_txd_props mig_props;
 } E1000State;
 
 #define chkflag(x)     (s->compat_flags & E1000_FLAG_##x)
@@ -1365,6 +1366,7 @@ static int e1000_pre_save(void *opaque)
         s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE;
     }
 
+    s->mig_props = s->tx.props;
     return 0;
 }
 
@@ -1393,12 +1395,13 @@ static int e1000_post_load(void *opaque, int version_id)
                   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)
          * and haven't got tx.tso_props.  The best we can do
          * is dupe the data.
          */
-        s->tx.tso_props = s->tx.props;
+        s->tx.tso_props = s->mig_props;
     }
     return 0;
 }
@@ -1496,20 +1499,20 @@ static const VMStateDescription vmstate_e1000 = {
         VMSTATE_UINT16(eecd_state.bitnum_out, E1000State),
         VMSTATE_UINT16(eecd_state.reading, E1000State),
         VMSTATE_UINT32(eecd_state.old_eecd, E1000State),
-        VMSTATE_UINT8(tx.props.ipcss, E1000State),
-        VMSTATE_UINT8(tx.props.ipcso, E1000State),
-        VMSTATE_UINT16(tx.props.ipcse, E1000State),
-        VMSTATE_UINT8(tx.props.tucss, E1000State),
-        VMSTATE_UINT8(tx.props.tucso, E1000State),
-        VMSTATE_UINT16(tx.props.tucse, E1000State),
-        VMSTATE_UINT32(tx.props.paylen, E1000State),
-        VMSTATE_UINT8(tx.props.hdr_len, E1000State),
-        VMSTATE_UINT16(tx.props.mss, E1000State),
+        VMSTATE_UINT8(mig_props.ipcss, E1000State),
+        VMSTATE_UINT8(mig_props.ipcso, E1000State),
+        VMSTATE_UINT16(mig_props.ipcse, E1000State),
+        VMSTATE_UINT8(mig_props.tucss, E1000State),
+        VMSTATE_UINT8(mig_props.tucso, E1000State),
+        VMSTATE_UINT16(mig_props.tucse, E1000State),
+        VMSTATE_UINT32(mig_props.paylen, E1000State),
+        VMSTATE_UINT8(mig_props.hdr_len, E1000State),
+        VMSTATE_UINT16(mig_props.mss, E1000State),
         VMSTATE_UINT16(tx.size, E1000State),
         VMSTATE_UINT16(tx.tso_frames, E1000State),
         VMSTATE_UINT8(tx.sum_needed, E1000State),
-        VMSTATE_INT8(tx.props.ip, E1000State),
-        VMSTATE_INT8(tx.props.tcp, E1000State),
+        VMSTATE_INT8(mig_props.ip, E1000State),
+        VMSTATE_INT8(mig_props.tcp, E1000State),
         VMSTATE_BUFFER(tx.header, E1000State),
         VMSTATE_BUFFER(tx.data, E1000State),
         VMSTATE_UINT16_ARRAY(eeprom_data, E1000State, 64),
-- 
2.14.3

[Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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 <dgilbert@redhat.com>
---
 hw/net/e1000.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 4e606d4b2a..13a9494a8d 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -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

[Qemu-devel] [PATCH v2 6/6] e1000: Old machine types, turn new subsection off

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Turn the newly added subsection off for old machine types

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/hw/compat.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index bc9e3a6627..13242b831a 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -14,6 +14,10 @@
         .driver   = "vhost-user-blk-pci",\
         .property = "vectors",\
         .value    = "2",\
+    },{\
+        .driver   = "e1000",\
+        .property = "migrate_tso_props",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_10 \
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Ed Swierk]

On Wed, Mar 28, 2018 at 9:36 AM, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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 <dgilbert@redhat.com>

Acked-by: Ed Swierk <eswierk@gmail.com>

> ---
>  hw/net/e1000.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 4e606d4b2a..13a9494a8d 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -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
>

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Jason Wang]

On 2018年03月29日 00:36, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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.
>
>

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

Thanks

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> 
> On 2018年03月29日 00:36, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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.
> > 
> > 
> 
> 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
> Thanks
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Jason Wang]

On 2018年03月29日 16:08, Dr. David Alan Gilbert wrote:
> * Jason Wang (jasowang@redhat.com) wrote:
>>
>> On 2018年03月29日 00:36, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> 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.
>>>
>>>
>> 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.

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

>
> Dave
>> Thanks
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> 
> On 2018年03月29日 16:08, Dr. David Alan Gilbert wrote:
> > * Jason Wang (jasowang@redhat.com) wrote:
> > > 
> > > On 2018年03月29日 00:36, Dr. David Alan Gilbert (git) wrote:
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > 
> > > > 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.
> > > > 
> > > > 
> > > 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.
> 
> 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?

> 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

> Thanks
> 
> > 
> > Dave
> > > Thanks
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Jason Wang]

On 2018年03月29日 16:44, Dr. David Alan Gilbert wrote:
> * Jason Wang (jasowang@redhat.com) wrote:
>>
>> On 2018年03月29日 16:08, Dr. David Alan Gilbert wrote:
>>> * Jason Wang (jasowang@redhat.com) wrote:
>>>> On 2018年03月29日 00:36, Dr. David Alan Gilbert (git) wrote:
>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>
>>>>> 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.
>>>>>
>>>>>
>>>> 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.
>> 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?

Looks like and we can avoid using tricks like patch 4.

>
>> 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.

Yes it is. I think I would agree with your idea now consider it was 
slightly better than using the old behavior.

Thanks

>
> Dave
>
>> Thanks
>>
>>> Dave
>>>> Thanks
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [Qemu-devel] [PATCH v2 0/6] e1000 migration changes for 2.12

[Jason Wang]

On 2018年03月29日 00:36, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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(-)
>

Reviewed-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH v2 5/6] e1000: Choose which set of props to migrate

[Paolo Bonzini]

On 28/03/2018 18:36, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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 <dgilbert@redhat.com>
> ---
>  hw/net/e1000.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 4e606d4b2a..13a9494a8d 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -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;
>  }

Looks good, thanks!

Paolo