Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines

[Corey Minyard]

On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote:
> * Corey Minyard (address@hidden) wrote:
> > On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote:
> > > * address@hidden (address@hidden) wrote:
> > > > From: Corey Minyard <address@hidden>
> > > >
> > > > 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 <address@hidden>
> > > > Cc: Eduardo Habkost <address@hidden>
> > > > Cc: Marcel Apfelbaum <address@hidden>
> > > > ---
> > > >    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 / address@hidden / Manchester, UK
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK