e1000 migration changes for 2.12
cover e1000 migration changes for 2.12
From: "Dr. David Alan Gilbert" <[email protected]>
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(-)
Signature · 1 line
2.14.3
PATCH 1/3 e1000: Convert v3 fields to subsection
From: "Dr. David Alan Gilbert" <[email protected]>
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.
--- hw/net/e1000.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)
hw/net/e1000.c +22 -12
@@ -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 } };
Signature · 1 line
2.14.3
PATCH 2/3 e1000: wire new subsection to property
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Wire the new subsection from the previous commit to a property so we can turn it off easily.
--- hw/net/e1000.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
hw/net/e1000.c +12 -0
@@ -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(), };
Signature · 1 line
2.14.3
PATCH 3/3 e1000: Old machine types, turn new subsection off
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Turn the newly added subsection off for old machine types
--- include/hw/compat.h | 4 ++++ 1 file changed, 4 insertions(+)
include/hw/compat.h +4 -0
@@ -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 \
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH 3/3] e1000: Old machine types, turn new subsection off
Quoted 15 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Turn the newly added subsection off for old machine types
Signed-off-by Dr. David Alan Gilbert <[email protected]>--- include/hw/compat.h | 4 ++++ 1 file changed, 4 insertions(+)include/hw/compat.h +0
-0
include/hw/compat.h line 23
Quoted 10 lines
include/hw/compat.h +4
-0@@ -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
Quoted 24 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 23 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Turn the newly added subsection off for old machine types
Signed-off-by Dr. David Alan Gilbert <[email protected]>--- include/hw/compat.h | 4 ++++ 1 file changed, 4 insertions(+)include/hw/compat.h +4
-0@@ -14,6 +14,10 @@ .driver = "vhost-user-blk-pci",\ .property = "vectors",\ .value = "2",\ + },{\ + .driver = "e1000",\ + .property = "migrate_tso_props",\ + .value = "off",\ },
Quoted 5 lines
include/hw/compat.h line 23
Quoted 2 lines
include/hw/compat.h +0
-0#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.
Quoted 2 lines
So if the subsection is absent you
have to migrate either tx.tso_props or tx.props, depending ons->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?
Quoted 3 lines
Likewise if you migrate from older versions: if
s->tx.props.tse&&s->tx.cptse, you have to copys->tx.propstos->tx.tso_propsand clears->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?
Quoted 2 lines
My understanding is that
s->tx.tso_props.tsewill be 1 if and only if the source sents->tx.tso_props.
I don't see anything in the current code that migrates tso_props.tse - where does it come from?
Quoted 2 lines
This seems most easily done with a new field (e.g.
vmstate_fixed_props) that is written inpre_saveand set inpost_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
Quoted 1 line
Paolo
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 3/3] e1000: Old machine types, turn new subsection off
Quoted 4 lines · Dr. David Alan Gilbert
Quoted 2 lines
So if the subsection is absent you
have to migrate either tx.tso_props or tx.props, depending ons->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.
Quoted 7 lines
Quoted 3 lines
Likewise if you migrate from older versions: if
s->tx.props.tse&&s->tx.cptse, you have to copys->tx.propstos->tx.tso_propsand clears->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.
Quoted 4 lines
Quoted 2 lines
My understanding is that
s->tx.tso_props.tsewill be 1 if and only if the source sents->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...
Quoted 5 lines
Quoted 2 lines
This seems most easily done with a new field (e.g.
vmstate_fixed_props) that is written inpre_saveand set inpost_load.It might need a
VMSTATE_WITH_TMPto 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
Quoted 12 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 4 lines · Dr. David Alan Gilbert
Quoted 2 lines
So if the subsection is absent you
have to migrate either tx.tso_props or tx.props, depending ons->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.
Quoted 4 lines
Quoted 3 lines
Likewise if you migrate from older versions: if
s->tx.props.tse&&s->tx.cptse, you have to copys->tx.propstos->tx.tso_propsand clears->tx.props.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
My understanding is that
s->tx.tso_props.tsewill be 1 if and only if the source sents->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_loadcallback in the subsection itself?
Yes, if I just add a .post_load field in the subsection it should get called.
Quoted 3 lines
To find out which "props" to transmit to older QEMU, you can add a
tp->use_tso_for_migration=tp->cptsejust before "if (!(txd_lower &E1000_TXD_CMD_EOP))" inprocess_tx_desc...
OK.
Quoted 17 lines
Quoted 5 lines
Quoted 2 lines
This seems most easily done with a new field (e.g.
vmstate_fixed_props) that is written inpre_saveand set inpost_load.It might need a
VMSTATE_WITH_TMPto 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), ...
Quoted 7 lines
and then add tx.props to the subsection together with tso.props.
New->oldmigration will placetx_legacy_vmstate_propsin tx.props on the destination;new->newwill realize the subsection was transmitted and ignore thetx_legacy_vmstate_props;old->newwill not find data from the subsection and copy thetx_legacy_vmstate_propsinto 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
Quoted 1 line
Paolo
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 3/3] e1000: Old machine types, turn new subsection off
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 5 lines
New->oldmigration will placetx_legacy_vmstate_propsin tx.props on the destination;new->newwill realize the subsection was transmitted and ignore thetx_legacy_vmstate_props;old->newwill not find data from the subsection and copy thetx_legacy_vmstate_propsinto one of tx.props and tx.tso_props.Yeh, adding a
legacy_propsfield 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 3/3] e1000: Old machine types, turn new subsection off
In reply to Paolo BonziniQuoted 20 lines · Paolo Bonzini
Quoted 4 lines · Dr. David Alan Gilbert
Quoted 2 lines
So if the subsection is absent you
have to migrate either tx.tso_props or tx.props, depending ons->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.
Quoted 7 lines
Quoted 3 lines
Likewise if you migrate from older versions: if
s->tx.props.tse&&s->tx.cptse, you have to copys->tx.propstos->tx.tso_propsand clears->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.
Quoted 2 lines
Quoted 2 lines
My understanding is that
s->tx.tso_props.tsewill be 1 if and only if the source sents->tx.tso_props.
Quoted 7 lines
Quoted 2 lines
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_loadcallback 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.
Quoted 3 lines
To find out which "props" to transmit to older QEMU, you can add a
tp->use_tso_for_migration=tp->cptsejust before "if (!(txd_lower &E1000_TXD_CMD_EOP))" inprocess_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
Quoted 11 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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?
Quoted 4 lines
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
Quoted 14 lines
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 offhw/net/e1000.c | 46 ++++++++++++++++++++++++++++++++++------------ include/hw/compat.h | 4 ++++ 2 files changed, 38 insertions(+), 12 deletions(-)
Quoted 10 lines · * Jason Wang ([email protected]) wrote:
Quoted 7 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
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
Quoted 16 lines
Thanks
Quoted 13 lines
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 offhw/net/e1000.c | 46 ++++++++++++++++++++++++++++++++++------------ include/hw/compat.h | 4 ++++ 2 files changed, 38 insertions(+), 12 deletions(-)
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 7 lines · Dr. David Alan Gilbert
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.
Quoted 3 lines
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.)
Quoted 3 lines
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.
Quoted 3 lines
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
Quoted 30 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]>
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.Quoted 6 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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
Quoted 21 lines
(b) is an existing question.
Dave
Quoted 14 lines
Thanks
Quoted 12 lines
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 offhw/net/e1000.c | 46 ++++++++++++++++++++++++++++++++++------------ include/hw/compat.h | 4 ++++ 2 files changed, 38 insertions(+), 12 deletions(-)--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 12 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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.
Quoted 5 lines
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
For the whole series.
I would have merged patch 1 & 2, but as you are the one coding them.
Later, Juan.