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

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

* 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

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

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

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