| Message ID | 20181115192446.17187-7-minyard@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Fix/add vmstate handling in some I2C code | expand |
* minyard@acm.org (minyard@acm.org) wrote: > From: Corey Minyard <cminyard@mvista.com> > > Migration capability is being added for pm_smbus and SMBus devices. > This change will allow backwards compatibility to be kept when > migrating back to an old qemu version. Add a bool to the machine > class tho keep smbus migration from happening. Future changes > will use this. So this is also going to have to be in the 3.0 and 3.1 machine options and maybe some other architectures? Dave > Signed-off-by: Corey Minyard <cminyard@mvista.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > --- > hw/i386/pc_piix.c | 1 + > hw/i386/pc_q35.c | 1 + > include/hw/boards.h | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index cb28227cc3..3d1ccb1af1 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m) > pc_i440fx_3_0_machine_options(m); > m->is_default = 0; > m->alias = NULL; > + m->smbus_no_migration_support = true; > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > } > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 90e88c9b28..0c6fca6a40 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m) > { > pc_q35_3_0_machine_options(m); > m->alias = NULL; > + m->smbus_no_migration_support = true; > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > } > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index f82f28468b..65314fbe2a 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -207,6 +207,7 @@ struct MachineClass { > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, > int nb_nodes, ram_addr_t size); > bool ignore_boot_device_suffixes; > + bool smbus_no_migration_support; > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > DeviceState *dev); > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote: > * minyard@acm.org (minyard@acm.org) wrote: >> From: Corey Minyard <cminyard@mvista.com> >> >> Migration capability is being added for pm_smbus and SMBus devices. >> This change will allow backwards compatibility to be kept when >> migrating back to an old qemu version. Add a bool to the machine >> class tho keep smbus migration from happening. Future changes >> will use this. > So this is also going to have to be in the 3.0 and 3.1 machine options > and maybe some other architectures? I'm not sure why it would need to be in both the 3.0 and 3.1 machine options. If this goes in, I'm assuming it's for 3.1 and it should only need to be there. That's if it's too late for 3.0. The pm_smbus device is also used by mips fulong. From what I can tell, that machine doesn't have any concept of backwards compatibility, so it seemed save to no worry about it there. If it weren't for that, this bool could go into the PC specific structure. Thanks, -corey > Dave > >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >> --- >> hw/i386/pc_piix.c | 1 + >> hw/i386/pc_q35.c | 1 + >> include/hw/boards.h | 1 + >> 3 files changed, 3 insertions(+) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index cb28227cc3..3d1ccb1af1 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m) >> pc_i440fx_3_0_machine_options(m); >> m->is_default = 0; >> m->alias = NULL; >> + m->smbus_no_migration_support = true; >> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); >> } >> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >> index 90e88c9b28..0c6fca6a40 100644 >> --- a/hw/i386/pc_q35.c >> +++ b/hw/i386/pc_q35.c >> @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m) >> { >> pc_q35_3_0_machine_options(m); >> m->alias = NULL; >> + m->smbus_no_migration_support = true; >> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); >> } >> >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index f82f28468b..65314fbe2a 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -207,6 +207,7 @@ struct MachineClass { >> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, >> int nb_nodes, ram_addr_t size); >> bool ignore_boot_device_suffixes; >> + bool smbus_no_migration_support; >> >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >> DeviceState *dev); >> -- >> 2.17.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Corey Minyard (minyard@acm.org) wrote: > On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote: > > * minyard@acm.org (minyard@acm.org) wrote: > > > From: Corey Minyard <cminyard@mvista.com> > > > > > > Migration capability is being added for pm_smbus and SMBus devices. > > > This change will allow backwards compatibility to be kept when > > > migrating back to an old qemu version. Add a bool to the machine > > > class tho keep smbus migration from happening. Future changes > > > will use this. > > So this is also going to have to be in the 3.0 and 3.1 machine options > > and maybe some other architectures? > > I'm not sure why it would need to be in both the 3.0 and 3.1 machine > options. > If this goes in, I'm assuming it's for 3.1 and it should only need to be > there. > That's if it's too late for 3.0. You're out by 1-2 releases; 3.0 shipped in August, 3.1 is in freeze at the moment (hence you've missed it unless some part of this is critical); so this will go in during the 4.0 release. > The pm_smbus device is also used by mips fulong. From what I can tell, that > machine doesn't have any concept of backwards compatibility, so it seemed > save to no worry about it there. If it weren't for that, this bool could go > into the Yeh that's fine. What about the eeprom's are they used more widely? Dave > PC specific structure. > > Thanks, > > -corey > > > Dave > > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > > > Cc: Eduardo Habkost <ehabkost@redhat.com> > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > > > --- > > > hw/i386/pc_piix.c | 1 + > > > hw/i386/pc_q35.c | 1 + > > > include/hw/boards.h | 1 + > > > 3 files changed, 3 insertions(+) > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > > index cb28227cc3..3d1ccb1af1 100644 > > > --- a/hw/i386/pc_piix.c > > > +++ b/hw/i386/pc_piix.c > > > @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m) > > > pc_i440fx_3_0_machine_options(m); > > > m->is_default = 0; > > > m->alias = NULL; > > > + m->smbus_no_migration_support = true; > > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > } > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > > index 90e88c9b28..0c6fca6a40 100644 > > > --- a/hw/i386/pc_q35.c > > > +++ b/hw/i386/pc_q35.c > > > @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m) > > > { > > > pc_q35_3_0_machine_options(m); > > > m->alias = NULL; > > > + m->smbus_no_migration_support = true; > > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > } > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > index f82f28468b..65314fbe2a 100644 > > > --- a/include/hw/boards.h > > > +++ b/include/hw/boards.h > > > @@ -207,6 +207,7 @@ struct MachineClass { > > > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, > > > int nb_nodes, ram_addr_t size); > > > bool ignore_boot_device_suffixes; > > > + bool smbus_no_migration_support; > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > > DeviceState *dev); > > > -- > > > 2.17.1 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote: > * Corey Minyard (minyard@acm.org) wrote: >> On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote: >>> * minyard@acm.org (minyard@acm.org) wrote: >>>> From: Corey Minyard <cminyard@mvista.com> >>>> >>>> Migration capability is being added for pm_smbus and SMBus devices. >>>> This change will allow backwards compatibility to be kept when >>>> migrating back to an old qemu version. Add a bool to the machine >>>> class tho keep smbus migration from happening. Future changes >>>> will use this. >>> So this is also going to have to be in the 3.0 and 3.1 machine options >>> and maybe some other architectures? >> I'm not sure why it would need to be in both the 3.0 and 3.1 machine >> options. >> If this goes in, I'm assuming it's for 3.1 and it should only need to be >> there. >> That's if it's too late for 3.0. > You're out by 1-2 releases; 3.0 shipped in August, 3.1 is in freeze at > the moment (hence you've missed it unless some part of this is > critical); so this will go in during the 4.0 release. Yes, you are right, this is for 4.0. > >> The pm_smbus device is also used by mips fulong. From what I can tell, that >> machine doesn't have any concept of backwards compatibility, so it seemed >> save to no worry about it there. If it weren't for that, this bool could go >> into the > Yeh that's fine. > > What about the eeprom's are they used more widely? The only other I2C EEPROM I see is hw/nvram/eeprom_at24c.c. That device is only configured for PPC. It has a block device backend, too. This is a little annoying because smbus_eeprom.c emulates an at24c02. It would be nice to only have one of these. I can add it to that file, too, it wouldn't be hard to do and I could put it into the x86 config to test. It would be nicer to combine the two; smbus_eeprom seems like a hack to me. at24c.c is done as an SMBus slave, its done as a raw I2C device, which is IMHO is more correct since the at24cxx devices are not SMBus devices, they are I2C devices. In fact, smbus_eeprom.c is the only thing currently using the smbus_slave code. The IPMI interface on SMBus that I have pending uses the smbus_slave code, too, so perhaps there's value, though it's no big deal either way. It would be nice to get this right the first time and not hack it up in the future. Thanks, -corey > > Dave > >> PC specific structure. >> >> Thanks, >> >> -corey >> >>> Dave >>> >>>> Signed-off-by: Corey Minyard <cminyard@mvista.com> >>>> Cc: Eduardo Habkost <ehabkost@redhat.com> >>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >>>> --- >>>> hw/i386/pc_piix.c | 1 + >>>> hw/i386/pc_q35.c | 1 + >>>> include/hw/boards.h | 1 + >>>> 3 files changed, 3 insertions(+) >>>> >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>> index cb28227cc3..3d1ccb1af1 100644 >>>> --- a/hw/i386/pc_piix.c >>>> +++ b/hw/i386/pc_piix.c >>>> @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m) >>>> pc_i440fx_3_0_machine_options(m); >>>> m->is_default = 0; >>>> m->alias = NULL; >>>> + m->smbus_no_migration_support = true; >>>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); >>>> } >>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >>>> index 90e88c9b28..0c6fca6a40 100644 >>>> --- a/hw/i386/pc_q35.c >>>> +++ b/hw/i386/pc_q35.c >>>> @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m) >>>> { >>>> pc_q35_3_0_machine_options(m); >>>> m->alias = NULL; >>>> + m->smbus_no_migration_support = true; >>>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); >>>> } >>>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>>> index f82f28468b..65314fbe2a 100644 >>>> --- a/include/hw/boards.h >>>> +++ b/include/hw/boards.h >>>> @@ -207,6 +207,7 @@ struct MachineClass { >>>> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, >>>> int nb_nodes, ram_addr_t size); >>>> bool ignore_boot_device_suffixes; >>>> + bool smbus_no_migration_support; >>>> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >>>> DeviceState *dev); >>>> -- >>>> 2.17.1 >>>> >>> -- >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Corey Minyard (minyard@acm.org) wrote: > On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote: > > * Corey Minyard (minyard@acm.org) wrote: > > > On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote: > > > > * minyard@acm.org (minyard@acm.org) wrote: > > > > > From: Corey Minyard <cminyard@mvista.com> > > > > > > > > > > Migration capability is being added for pm_smbus and SMBus devices. > > > > > This change will allow backwards compatibility to be kept when > > > > > migrating back to an old qemu version. Add a bool to the machine > > > > > class tho keep smbus migration from happening. Future changes > > > > > will use this. > > > > So this is also going to have to be in the 3.0 and 3.1 machine options > > > > and maybe some other architectures? > > > I'm not sure why it would need to be in both the 3.0 and 3.1 machine > > > options. > > > If this goes in, I'm assuming it's for 3.1 and it should only need to be > > > there. > > > That's if it's too late for 3.0. > > You're out by 1-2 releases; 3.0 shipped in August, 3.1 is in freeze at > > the moment (hence you've missed it unless some part of this is > > critical); so this will go in during the 4.0 release. > > Yes, you are right, this is for 4.0. > > > > > > > The pm_smbus device is also used by mips fulong. From what I can tell, that > > > machine doesn't have any concept of backwards compatibility, so it seemed > > > save to no worry about it there. If it weren't for that, this bool could go > > > into the > > Yeh that's fine. > > > > What about the eeprom's are they used more widely? > > The only other I2C EEPROM I see is hw/nvram/eeprom_at24c.c. That device is > only > configured for PPC. It has a block device backend, too. This is a little > annoying > because smbus_eeprom.c emulates an at24c02. It would be nice to only have > one of these. Added in David and Laurent for odd-ppc stuff; if I'm reading it right it's not used by one of the versioned machines. > I can add it to that file, too, it wouldn't be hard to do and I could put it > into the > x86 config to test. It would be nicer to combine the two; smbus_eeprom > seems > like a hack to me. at24c.c is done as an SMBus slave, its done as a raw I2C > device, which is IMHO is more correct since the at24cxx devices are not > SMBus > devices, they are I2C devices. > > In fact, smbus_eeprom.c is the only thing currently using the smbus_slave > code. > The IPMI interface on SMBus that I have pending uses the smbus_slave code, > too, > so perhaps there's value, though it's no big deal either way. It would be > nice to get > this right the first time and not hack it up in the future. Dave > > Thanks, > > -corey > > > > > > Dave > > > > > PC specific structure. > > > > > > Thanks, > > > > > > -corey > > > > > > > Dave > > > > > > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > > > > > Cc: Eduardo Habkost <ehabkost@redhat.com> > > > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > > > > > --- > > > > > hw/i386/pc_piix.c | 1 + > > > > > hw/i386/pc_q35.c | 1 + > > > > > include/hw/boards.h | 1 + > > > > > 3 files changed, 3 insertions(+) > > > > > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > > > > index cb28227cc3..3d1ccb1af1 100644 > > > > > --- a/hw/i386/pc_piix.c > > > > > +++ b/hw/i386/pc_piix.c > > > > > @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m) > > > > > pc_i440fx_3_0_machine_options(m); > > > > > m->is_default = 0; > > > > > m->alias = NULL; > > > > > + m->smbus_no_migration_support = true; > > > > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > > > } > > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > > > > index 90e88c9b28..0c6fca6a40 100644 > > > > > --- a/hw/i386/pc_q35.c > > > > > +++ b/hw/i386/pc_q35.c > > > > > @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m) > > > > > { > > > > > pc_q35_3_0_machine_options(m); > > > > > m->alias = NULL; > > > > > + m->smbus_no_migration_support = true; > > > > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > > > } > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > > index f82f28468b..65314fbe2a 100644 > > > > > --- a/include/hw/boards.h > > > > > +++ b/include/hw/boards.h > > > > > @@ -207,6 +207,7 @@ struct MachineClass { > > > > > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, > > > > > int nb_nodes, ram_addr_t size); > > > > > bool ignore_boot_device_suffixes; > > > > > + bool smbus_no_migration_support; > > > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > > > > DeviceState *dev); > > > > > -- > > > > > 2.17.1 > > > > > > > > > -- > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 27/11/2018 19:14, Dr. David Alan Gilbert wrote: > * Corey Minyard (minyard@acm.org) wrote: >> On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote: >>> * Corey Minyard (minyard@acm.org) wrote: >>>> On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote: >>>>> * minyard@acm.org (minyard@acm.org) wrote: >>>>>> From: Corey Minyard <cminyard@mvista.com> ... >>>> The pm_smbus device is also used by mips fulong. From what I can tell, that >>>> machine doesn't have any concept of backwards compatibility, so it seemed >>>> save to no worry about it there. If it weren't for that, this bool could go >>>> into the >>> Yeh that's fine. >>> >>> What about the eeprom's are they used more widely? >> >> The only other I2C EEPROM I see is hw/nvram/eeprom_at24c.c. That device is >> only >> configured for PPC. It has a block device backend, too. This is a little >> annoying >> because smbus_eeprom.c emulates an at24c02. It would be nice to only have >> one of these. > > Added in David and Laurent for odd-ppc stuff; if I'm reading it right > it's not used by one of the versioned machines. Yes, it seems unused. It has been introduced for the mvme31100 machine emulation, by: commit 5d8424dbd3e8335ea3d57f64eaa603c8fc80706f Author: Michael Davidsaver <mdavidsaver@gmail.com> Date: Sun Nov 19 21:24:17 2017 -0600 nvram: add AT24Cx i2c eeprom Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> from the series: [00/12] Add MVME3100 PPC SBC https://patchwork.ozlabs.org/cover/839400/ [12/12] tests: add mvme3100-test [11/12] ppc: add mvme3100 machine [10/12] timer: add ds1375 RTC [09/12] nvram: add AT24Cx i2c eeprom [08/12] e500: add mpc8540 i2c controller to ccsr [07/12] qtest: add e500_i2c_create() [06/12] i2c: add mpc8540 i2c controller [05/12] e500: name openpic and pci host bridge [04/12] e500: additional CCSR registers [03/12] e500: note possible bug with host bridge [02/12] e500: consolidate mpc8540 guts with e500-ccsr [01/12] e500: add board config options https://patchwork.ozlabs.org/project/qemu-devel/list/?series=14303 But the patch adding the machine type has never been commited: [11/12] ppc: add mvme3100 machine https://patchwork.ozlabs.org/patch/839407/ Thanks, Laurent
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index cb28227cc3..3d1ccb1af1 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m) pc_i440fx_3_0_machine_options(m); m->is_default = 0; m->alias = NULL; + m->smbus_no_migration_support = true; SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); } diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 90e88c9b28..0c6fca6a40 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m) { pc_q35_3_0_machine_options(m); m->alias = NULL; + m->smbus_no_migration_support = true; SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); } diff --git a/include/hw/boards.h b/include/hw/boards.h index f82f28468b..65314fbe2a 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -207,6 +207,7 @@ struct MachineClass { void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, int nb_nodes, ram_addr_t size); bool ignore_boot_device_suffixes; + bool smbus_no_migration_support; HotplugHandler *(*get_hotplug_handler)(MachineState *machine, DeviceState *dev);