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

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

[Dr. David Alan Gilbert (git)]

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

Hi Ed, Jason,
  This set of patches change the e1000 migration code to make
it easier to keep with compatibility with older versions in backwards
migration;  but I do need some advice whether I need to do more as well.

I think the first and second patch are fairly uncontrovercial and I
would like them for 2.12, since it'll make any future changes easier.
The third one changes the default behaviour, so again I'd prefer it but
lets see what you think.

My question however, without knowing the internals of the e1000, is
whether when ommitting the subsection, should the code in 2.12 be
changing the data it sends back in the main section of data?

Dave

  

Dr. David Alan Gilbert (3):
  e1000: Convert v3 fields to subsection
  e1000: wire new subsection to property
  e1000: Old machine types, turn new subsection off

 hw/net/e1000.c      | 46 ++++++++++++++++++++++++++++++++++------------
 include/hw/compat.h |  4 ++++
 2 files changed, 38 insertions(+), 12 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] 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 2/3] 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 24e9a4ab40..0562b8c547 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;
 } E1000State;
 
@@ -1407,6 +1409,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,
@@ -1437,6 +1446,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,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(tx.tso_props.ipcss, E1000State),
         VMSTATE_UINT8(tx.tso_props.ipcso, E1000State),
@@ -1661,6 +1671,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 3/3] 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 0/3] e1000 migration changes for 2.12

[Jason Wang]

On 2018年03月27日 19:34, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Hi Ed, Jason,
>    This set of patches change the e1000 migration code to make
> it easier to keep with compatibility with older versions in backwards
> migration;  but I do need some advice whether I need to do more as well.
>
> I think the first and second patch are fairly uncontrovercial and I
> would like them for 2.12, since it'll make any future changes easier.
> The third one changes the default behaviour, so again I'd prefer it but
> lets see what you think.

The patches looks good to me. So for the changes of default behavior, 
did you mean we can make the migration to older versions work?

>
> My question however, without knowing the internals of the e1000, is
> whether when ommitting the subsection, should the code in 2.12 be
> changing the data it sends back in the main section of data?

I'm not sure I get the meaning here. But it looks to me turning it off 
for old machine types makes sense, otherwise, management need to set it 
explicitly.

Thanks

>
> Dave
>
>    
>
> Dr. David Alan Gilbert (3):
>    e1000: Convert v3 fields to subsection
>    e1000: wire new subsection to property
>    e1000: Old machine types, turn new subsection off
>
>   hw/net/e1000.c      | 46 ++++++++++++++++++++++++++++++++++------------
>   include/hw/compat.h |  4 ++++
>   2 files changed, 38 insertions(+), 12 deletions(-)
>

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

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Hi Ed, Jason,
>   This set of patches change the e1000 migration code to make
> it easier to keep with compatibility with older versions in backwards
> migration;  but I do need some advice whether I need to do more as well.
>
> I think the first and second patch are fairly uncontrovercial and I
> would like them for 2.12, since it'll make any future changes easier.
> The third one changes the default behaviour, so again I'd prefer it but
> lets see what you think.
>
> My question however, without knowing the internals of the e1000, is
> whether when ommitting the subsection, should the code in 2.12 be
> changing the data it sends back in the main section of data?
>
> Dave

Reviewed-by: Juan Quintela <quintela@redhat.com>

For the whole series.

I would have merged patch 1 & 2, but as you are the one coding them.

Later, Juan.

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

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> 
> On 2018年03月27日 19:34, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Hi Ed, Jason,
> >    This set of patches change the e1000 migration code to make
> > it easier to keep with compatibility with older versions in backwards
> > migration;  but I do need some advice whether I need to do more as well.
> > 
> > I think the first and second patch are fairly uncontrovercial and I
> > would like them for 2.12, since it'll make any future changes easier.
> > The third one changes the default behaviour, so again I'd prefer it but
> > lets see what you think.
> 
> The patches looks good to me. So for the changes of default behavior, did
> you mean we can make the migration to older versions work?

Right.

> > My question however, without knowing the internals of the e1000, is
> > whether when ommitting the subsection, should the code in 2.12 be
> > changing the data it sends back in the main section of data?
> 
> I'm not sure I get the meaning here. But it looks to me turning it off for
> old machine types makes sense, otherwise, management need to set it
> explicitly.

OK, let me expand the question a bit.
If I understand correctly the d6244b description, there's two pieces of
state (TSO and non-TSO) where there used to be only one.
Now, with the new format we migrate both pieces of state, but lets think
about what happens if we have to migrate only one piece.

a) 2.11->2.11 migrated the only piece it knew about; so I guess the only
problem was really the UDP corruption mentioned in the commit.

b) 2.11->2.12 - it receives the wrongly merged piece of state and puts it
in - well which of the two states does it load it into?  What's the
effect of that?

c) 2.12(+my mod)->2.11 ok, so 2.12 will have filled in both sets of state
internally; but now it's only going to send one of them over to 2.11 -
which one gets sent to 2.11? Is it the one that 2.11 is expecting?

d) 2.12(+my mod)->2.12(+my mod) with an old machine type, again we're only
going to send one set of data (same as c) - but what's 2.12 going to
make of the one piece of state received?

(b) is an existing question.

Dave

> Thanks
> 
> > 
> > Dave
> > 
> > 
> > Dr. David Alan Gilbert (3):
> >    e1000: Convert v3 fields to subsection
> >    e1000: wire new subsection to property
> >    e1000: Old machine types, turn new subsection off
> > 
> >   hw/net/e1000.c      | 46 ++++++++++++++++++++++++++++++++++------------
> >   include/hw/compat.h |  4 ++++
> >   2 files changed, 38 insertions(+), 12 deletions(-)
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Paolo Bonzini]

On 27/03/2018 13:34, Dr. David Alan Gilbert (git) wrote:
> 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 \
> 

Unfortunately it's not that easy, because on old machine types
tx.tso_props was stored in tx.props.  So if the subsection is absent you
have to migrate either tx.tso_props or tx.props, depending on s->tx.cptse.

Likewise if you migrate from older versions: if s->tx.props.tse &&
s->tx.cptse, you have to copy s->tx.props to s->tx.tso_props and clear
s->tx.props.  My understanding is that s->tx.tso_props.tse will be 1 if
and only if the source sent s->tx.tso_props.

This seems most easily done with a new field (e.g. vmstate_fixed_props)
that is written in pre_save and set in post_load.

Paolo

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 27/03/2018 13:34, Dr. David Alan Gilbert (git) wrote:
> > 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 \
> > 
> 
> Unfortunately it's not that easy, because on old machine types
> tx.tso_props was stored in tx.props.

OK, this pretty much sounds like the answer to the question I asked on
the cover letter.

> So if the subsection is absent you
> have to migrate either tx.tso_props or tx.props, depending on s->tx.cptse.

Do you mean when sending you have to decide which set to send in the
non-subsection data?  And with cptse true that means use tso_props?

> Likewise if you migrate from older versions: if s->tx.props.tse &&
> s->tx.cptse, you have to copy s->tx.props to s->tx.tso_props and clear
> s->tx.props.


I don't see any equivalent code in the existing non-subsection postload to
do this; so I'm guessing there are some cases of 2.11->2.12 that will
break at the moment?

> My understanding is that s->tx.tso_props.tse will be 1 if
> and only if the source sent s->tx.tso_props.

I don't see anything in the current code that migrates tso_props.tse -
where does it come from?

> This seems most easily done with a new field (e.g. vmstate_fixed_props)
> that is written in pre_save and set in post_load.

It might need a VMSTATE_WITH_TMP to be able to do the saving part;
when saving we can't change the current state when migrating
to an old destination in case the migration fails and we just continue.

How would I test this lot?

Dave

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

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

[Paolo Bonzini]

On 27/03/2018 18:47, Dr. David Alan Gilbert wrote:
>> So if the subsection is absent you
>> have to migrate either tx.tso_props or tx.props, depending on s->tx.cptse.
> Do you mean when sending you have to decide which set to send in the
> non-subsection data?  And with cptse true that means use tso_props?

Yes.

>> Likewise if you migrate from older versions: if s->tx.props.tse &&
>> s->tx.cptse, you have to copy s->tx.props to s->tx.tso_props and clear
>> s->tx.props.
> 
> I don't see any equivalent code in the existing non-subsection postload to
> do this; so I'm guessing there are some cases of 2.11->2.12 that will
> break at the moment?

Yes, I think so.

>> My understanding is that s->tx.tso_props.tse will be 1 if
>> and only if the source sent s->tx.tso_props.
> I don't see anything in the current code that migrates tso_props.tse -
> where does it come from?

Ouch... The tse field is more or less dead in current code AFAICS, but
it was used in the previous version.  What's the best way then to find
if the subsection was transmitted?  Do we have anything like a post_load
callback in the subsection itself?

To find out which "props" to transmit to older QEMU, you can add a
tp->use_tso_for_migration = tp->cptse just before "if (!(txd_lower &
E1000_TXD_CMD_EOP))" in process_tx_desc...

>> This seems most easily done with a new field (e.g. vmstate_fixed_props)
>> that is written in pre_save and set in post_load.
> It might need a VMSTATE_WITH_TMP to be able to do the saving part;
> when saving we can't change the current state when migrating
> to an old destination in case the migration fails and we just continue.

Perhaps you can just copy props/tso_props to a new field, and change all the

        VMSTATE_UINT8(tx.props.ipcss, E1000State),
        VMSTATE_UINT8(tx.props.ipcso, E1000State),
        VMSTATE_UINT16(tx.props.ipcse, E1000State),

to

        VMSTATE_UINT8(tx_legacy_vmstate_props.ipcss, E1000State),
	...

and then add tx.props to the subsection together with tso.props.

New->old migration will place tx_legacy_vmstate_props in tx.props on the
destination; new->new will realize the subsection was transmitted and
ignore the tx_legacy_vmstate_props; old->new will not find data from the
subsection and copy the tx_legacy_vmstate_props into one of tx.props and
tx.tso_props.

Paolo

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 27/03/2018 18:47, Dr. David Alan Gilbert wrote:
> >> So if the subsection is absent you
> >> have to migrate either tx.tso_props or tx.props, depending on s->tx.cptse.
> > Do you mean when sending you have to decide which set to send in the
> > non-subsection data?  And with cptse true that means use tso_props?
> 
> Yes.
> 
> >> Likewise if you migrate from older versions: if s->tx.props.tse &&
> >> s->tx.cptse, you have to copy s->tx.props to s->tx.tso_props and clear
> >> s->tx.props.
> > 
> > I don't see any equivalent code in the existing non-subsection postload to
> > do this; so I'm guessing there are some cases of 2.11->2.12 that will
> > break at the moment?
> 
> Yes, I think so.

OK, so we'd better fix that for 2.12.

> >> My understanding is that s->tx.tso_props.tse will be 1 if
> >> and only if the source sent s->tx.tso_props.
> > I don't see anything in the current code that migrates tso_props.tse -
> > where does it come from?
> 
> Ouch... The tse field is more or less dead in current code AFAICS, but
> it was used in the previous version.  What's the best way then to find
> if the subsection was transmitted?  Do we have anything like a post_load
> callback in the subsection itself?

Yes, if I just add a .post_load field in the subsection it should get
called.

> To find out which "props" to transmit to older QEMU, you can add a
> tp->use_tso_for_migration = tp->cptse just before "if (!(txd_lower &
> E1000_TXD_CMD_EOP))" in process_tx_desc...

OK.

> >> This seems most easily done with a new field (e.g. vmstate_fixed_props)
> >> that is written in pre_save and set in post_load.
> > It might need a VMSTATE_WITH_TMP to be able to do the saving part;
> > when saving we can't change the current state when migrating
> > to an old destination in case the migration fails and we just continue.
> 
> Perhaps you can just copy props/tso_props to a new field, and change all the
> 
>         VMSTATE_UINT8(tx.props.ipcss, E1000State),
>         VMSTATE_UINT8(tx.props.ipcso, E1000State),
>         VMSTATE_UINT16(tx.props.ipcse, E1000State),
> 
> to
> 
>         VMSTATE_UINT8(tx_legacy_vmstate_props.ipcss, E1000State),
> 	...
> 
> and then add tx.props to the subsection together with tso.props.
> 
> New->old migration will place tx_legacy_vmstate_props in tx.props on the
> destination; new->new will realize the subsection was transmitted and
> ignore the tx_legacy_vmstate_props; old->new will not find data from the
> subsection and copy the tx_legacy_vmstate_props into one of tx.props and
> tx.tso_props.

Yeh, adding a legacy_props field should do it; although we never need
to transmit more than 2 copies.  

I'll look at this more tomorrow; I am a bit worried about testing it
though.

Dave


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

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

[Paolo Bonzini]

On 27/03/2018 20:01, Dr. David Alan Gilbert wrote:
>> New->old migration will place tx_legacy_vmstate_props in tx.props on the
>> destination; new->new will realize the subsection was transmitted and
>> ignore the tx_legacy_vmstate_props; old->new will not find data from the
>> subsection and copy the tx_legacy_vmstate_props into one of tx.props and
>> tx.tso_props.
> Yeh, adding a legacy_props field should do it; although we never need
> to transmit more than 2 copies.  
> 
> I'll look at this more tomorrow; I am a bit worried about testing it
> though.

Yes, I'm also afraid that we can't really do much better than careful
code review.

Paolo

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

[Ed Swierk]

On Tue, Mar 27, 2018 at 7:26 AM, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> If I understand correctly the d6244b description, there's two pieces of
> state (TSO and non-TSO) where there used to be only one.
> Now, with the new format we migrate both pieces of state, but lets think
> about what happens if we have to migrate only one piece.
>
> a) 2.11->2.11 migrated the only piece it knew about; so I guess the only
> problem was really the UDP corruption mentioned in the commit.

Right.

> b) 2.11->2.12 - it receives the wrongly merged piece of state and puts it
> in - well which of the two states does it load it into?  What's the
> effect of that?

The best we can do is copy the old props into both props and
tso_props. Copying it into just props means that e1000 would suddenly
find all zero offsets in tso_props, screwing up an ongoing TCP session
using TSO in a Windows guest. Conversely copying into just tso_props
would screw up an ongoing UDP session using non-TSO offload in a
Windows guest. Copying means we do no worse than the buggy 2.11
behavior, until both TSO and non-TSO contexts are refreshed and
everything is fine. (For Linux guests it doesn't matter since Linux
always seems to refresh both TSO and non-TSO contexts before every tx
data descriptor.)

> c) 2.12(+my mod)->2.11 ok, so 2.12 will have filled in both sets of state
> internally; but now it's only going to send one of them over to 2.11 -
> which one gets sent to 2.11? Is it the one that 2.11 is expecting?

This case is trickier. We want to copy into the old props whichever
one of props and tso_props was updated most recently by a non-TSO or
TSO context descriptor. Always copying one or the other risks screwing
up an ongoing session in a Windows guest by using outdated offsets.
(Again there's no problem for Linux guests.)

Probably the easiest solution is to add yet another flag (which
doesn't itself get migrated) that's set when tso_props is updated and
cleared when props is updated.

> d) 2.12(+my mod)->2.12(+my mod) with an old machine type, again we're only
> going to send one set of data (same as c) - but what's 2.12 going to
> make of the one piece of state received?

This is the same as (b) I think.

--Ed

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

[Ed Swierk]

On Tue, Mar 27, 2018 at 10:28 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 27/03/2018 18:47, Dr. David Alan Gilbert wrote:
>>> So if the subsection is absent you
>>> have to migrate either tx.tso_props or tx.props, depending on s->tx.cptse.
>> Do you mean when sending you have to decide which set to send in the
>> non-subsection data?  And with cptse true that means use tso_props?
>
> Yes.
>
>>> Likewise if you migrate from older versions: if s->tx.props.tse &&
>>> s->tx.cptse, you have to copy s->tx.props to s->tx.tso_props and clear
>>> s->tx.props.
>>
>> I don't see any equivalent code in the existing non-subsection postload to
>> do this; so I'm guessing there are some cases of 2.11->2.12 that will
>> break at the moment?
>
> Yes, I think so.
>
>>> My understanding is that s->tx.tso_props.tse will be 1 if
>>> and only if the source sent s->tx.tso_props.
>> I don't see anything in the current code that migrates tso_props.tse -
>> where does it come from?
>
> Ouch... The tse field is more or less dead in current code AFAICS, but
> it was used in the previous version.  What's the best way then to find
> if the subsection was transmitted?  Do we have anything like a post_load
> callback in the subsection itself?

The TSE flag in the cmd_and_length field of the context descriptor is
useful only as an indication of which context is being updated: TSO
(tso_props) or non-TSO (props). There is no reason to store it or
migrate it, and all prior uses of the stored field were based on an
incorrect understanding of its meaning. Now props.tse is always 0, and
tso_props.tse is always 1 after the first TSO context is processed.

> To find out which "props" to transmit to older QEMU, you can add a
> tp->use_tso_for_migration = tp->cptse just before "if (!(txd_lower &
> E1000_TXD_CMD_EOP))" in process_tx_desc...

tp->cptse only indicates whether the current tx data descriptor should
be segmented using parameters from the last TSO context descriptor.
It's perfectly legal for the guest to set up a TSO context and then
use it for some but not all subsequent data descriptors. tp->cptse
doesn't help in deciding what to migrate.

Whether to migrate props or tso_props back to 2.11 should be instead
based on which was updated last by a context descriptor. Something
like

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);
        tp->use_tso_for_migration = 1;
        tp->tso_frames = 0;
    } else {
        e1000x_read_tx_ctx_descr(xp, &tp->props);
        tp->use_tso_for_migration = 0;
    }
    return;

--Ed

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

[Jason Wang]

On 2018年03月27日 22:26, Dr. David Alan Gilbert wrote:
> * Jason Wang (jasowang@redhat.com) wrote:
>>
>> On 2018年03月27日 19:34, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> Hi Ed, Jason,
>>>     This set of patches change the e1000 migration code to make
>>> it easier to keep with compatibility with older versions in backwards
>>> migration;  but I do need some advice whether I need to do more as well.
>>>
>>> I think the first and second patch are fairly uncontrovercial and I
>>> would like them for 2.12, since it'll make any future changes easier.
>>> The third one changes the default behaviour, so again I'd prefer it but
>>> lets see what you think.
>> The patches looks good to me. So for the changes of default behavior, did
>> you mean we can make the migration to older versions work?
> Right.
>
>>> My question however, without knowing the internals of the e1000, is
>>> whether when ommitting the subsection, should the code in 2.12 be
>>> changing the data it sends back in the main section of data?
>> I'm not sure I get the meaning here. But it looks to me turning it off for
>> old machine types makes sense, otherwise, management need to set it
>> explicitly.
> OK, let me expand the question a bit.
> If I understand correctly the d6244b description, there's two pieces of
> state (TSO and non-TSO) where there used to be only one.
> Now, with the new format we migrate both pieces of state, but lets think
> about what happens if we have to migrate only one piece.
>
> a) 2.11->2.11 migrated the only piece it knew about; so I guess the only
> problem was really the UDP corruption mentioned in the commit.
>
> b) 2.11->2.12 - it receives the wrongly merged piece of state and puts it
> in - well which of the two states does it load it into?  What's the
> effect of that?

So I think what we can do in this case is keep the (buggy) behavior 
here. E.g put the state into both props and tso_props.

>
> c) 2.12(+my mod)->2.11 ok, so 2.12 will have filled in both sets of state
> internally; but now it's only going to send one of them over to 2.11 -
> which one gets sent to 2.11? Is it the one that 2.11 is expecting?

Then we can keep the behavior of 2.11 when migrate_tso_props (probably 
need a better name) is false we will use props for all contexts.

>
> d) 2.12(+my mod)->2.12(+my mod) with an old machine type, again we're only
> going to send one set of data (same as c) - but what's 2.12 going to
> make of the one piece of state received?

So if we do like above, guest will see buggy device after migration. 
(But do we really care this case?).

Thanks

>
> (b) is an existing question.
>
> Dave
>
>> Thanks
>>
>>> Dave
>>>
>>>
>>> Dr. David Alan Gilbert (3):
>>>     e1000: Convert v3 fields to subsection
>>>     e1000: wire new subsection to property
>>>     e1000: Old machine types, turn new subsection off
>>>
>>>    hw/net/e1000.c      | 46 ++++++++++++++++++++++++++++++++++------------
>>>    include/hw/compat.h |  4 ++++
>>>    2 files changed, 38 insertions(+), 12 deletions(-)
>>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK