| Message ID | 1477509718-6969-3-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
> On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > The user (or management software) is responsible for keeping the > same configuration on both sides while migrating. Remove the > configuration validation code at e1000e_post_load, and the > unnecessary subsys_used/subsys_ven_used fields. Migration with different subsys/subsys_ven may have a really cumbersome consequences that are very hard to debug, so I believe such a verification may save a lot of time, however it’s a matter of convention. Jason, is there specific convention for cases like this? > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/net/e1000e.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > index df24e55..a932620 100644 > --- a/hw/net/e1000e.c > +++ b/hw/net/e1000e.c > @@ -66,9 +66,6 @@ typedef struct E1000EState { > uint16_t subsys_ven; > uint16_t subsys; > > - uint16_t subsys_ven_used; > - uint16_t subsys_used; > - > bool disable_vnet; > > E1000ECore core; > @@ -424,9 +421,6 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp) > pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_ven); > pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, s->subsys); > > - s->subsys_ven_used = s->subsys_ven; > - s->subsys_used = s->subsys; > - > /* Define IO/MMIO regions */ > memory_region_init_io(&s->mmio, OBJECT(s), &mmio_ops, s, > "e1000e-mmio", E1000E_MMIO_SIZE); > @@ -531,14 +525,6 @@ static int e1000e_post_load(void *opaque, int version_id) > > trace_e1000e_cb_post_load(); > > - if ((s->subsys != s->subsys_used) || > - (s->subsys_ven != s->subsys_ven_used)) { > - fprintf(stderr, > - "ERROR: Cannot migrate while device properties " > - "(subsys/subsys_ven) differ"); > - return -1; > - } > - > return e1000e_core_post_load(&s->core); > } > > -- > 2.7.4 >
On Thu, Oct 27, 2016 at 09:42:53AM +0300, Dmitry Fleytman wrote: > > > On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > The user (or management software) is responsible for keeping the > > same configuration on both sides while migrating. Remove the > > configuration validation code at e1000e_post_load, and the > > unnecessary subsys_used/subsys_ven_used fields. > > Migration with different subsys/subsys_ven may have a > really cumbersome consequences that are very hard to debug, > so I believe such a verification may save a lot of time, > however it’s a matter of convention. The same issues apply to almost every other property on every other device. But the convention is to not expect devices to validate configuration changes on migration. (It would be nice to have a more generic mechanism that detects configuration mismatch on migration, though. Having each device manually checking each of its properties wouldn't work).
* Eduardo Habkost (ehabkost@redhat.com) wrote: > On Thu, Oct 27, 2016 at 09:42:53AM +0300, Dmitry Fleytman wrote: > > > > > On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > The user (or management software) is responsible for keeping the > > > same configuration on both sides while migrating. Remove the > > > configuration validation code at e1000e_post_load, and the > > > unnecessary subsys_used/subsys_ven_used fields. > > > > Migration with different subsys/subsys_ven may have a > > really cumbersome consequences that are very hard to debug, > > so I believe such a verification may save a lot of time, > > however it’s a matter of convention. > > The same issues apply to almost every other property on every > other device. But the convention is to not expect devices to > validate configuration changes on migration. > > (It would be nice to have a more generic mechanism that detects > configuration mismatch on migration, though. Having each device > manually checking each of its properties wouldn't work). Without knowing the e1000e code, isn't the easy way to do this to change the: VMSTATE_UINT16(subsys, E1000EState), VMSTATE_UINT16(subsys_ven, E1000EState), to VMSTATE_UINT16_EQUAL(subsys, E1000EState), VMSTATE_UINT16_EQUAL(subsys_ven, E1000EState), and the migration code will flag if they don't match for you. Dave > > -- > Eduardo > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Oct 27, 2016 at 01:22:29PM +0100, Dr. David Alan Gilbert wrote: > * Eduardo Habkost (ehabkost@redhat.com) wrote: > > On Thu, Oct 27, 2016 at 09:42:53AM +0300, Dmitry Fleytman wrote: > > > > > > > On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > The user (or management software) is responsible for keeping the > > > > same configuration on both sides while migrating. Remove the > > > > configuration validation code at e1000e_post_load, and the > > > > unnecessary subsys_used/subsys_ven_used fields. > > > > > > Migration with different subsys/subsys_ven may have a > > > really cumbersome consequences that are very hard to debug, > > > so I believe such a verification may save a lot of time, > > > however it’s a matter of convention. > > > > The same issues apply to almost every other property on every > > other device. But the convention is to not expect devices to > > validate configuration changes on migration. > > > > (It would be nice to have a more generic mechanism that detects > > configuration mismatch on migration, though. Having each device > > manually checking each of its properties wouldn't work). > > Without knowing the e1000e code, isn't the easy way to do this > to change the: > > VMSTATE_UINT16(subsys, E1000EState), > VMSTATE_UINT16(subsys_ven, E1000EState), > to > > VMSTATE_UINT16_EQUAL(subsys, E1000EState), > VMSTATE_UINT16_EQUAL(subsys_ven, E1000EState), > > and the migration code will flag if they don't match for you. That's very useful. I didn't know about it. I will submit a new version of the series that uses VMSTATE_UINT16_EQUAL.
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index df24e55..a932620 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -66,9 +66,6 @@ typedef struct E1000EState { uint16_t subsys_ven; uint16_t subsys; - uint16_t subsys_ven_used; - uint16_t subsys_used; - bool disable_vnet; E1000ECore core; @@ -424,9 +421,6 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp) pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_ven); pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, s->subsys); - s->subsys_ven_used = s->subsys_ven; - s->subsys_used = s->subsys; - /* Define IO/MMIO regions */ memory_region_init_io(&s->mmio, OBJECT(s), &mmio_ops, s, "e1000e-mmio", E1000E_MMIO_SIZE); @@ -531,14 +525,6 @@ static int e1000e_post_load(void *opaque, int version_id) trace_e1000e_cb_post_load(); - if ((s->subsys != s->subsys_used) || - (s->subsys_ven != s->subsys_ven_used)) { - fprintf(stderr, - "ERROR: Cannot migrate while device properties " - "(subsys/subsys_ven) differ"); - return -1; - } - return e1000e_core_post_load(&s->core); }
The user (or management software) is responsible for keeping the same configuration on both sides while migrating. Remove the configuration validation code at e1000e_post_load, and the unnecessary subsys_used/subsys_ven_used fields. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/net/e1000e.c | 14 -------------- 1 file changed, 14 deletions(-)