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

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

* 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

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

* 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

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

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