[3/3] i2c: Add vmstate handling to the smbus eeprom

Commit Message

Corey Minyard Nov. 7, 2018, 3:54 p.m. UTC

From: Corey Minyard <[e-mail]>

This was if the eeprom is accessed during a state transfer, the
transfer will be reliable.

Signed-off-by: Corey Minyard <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Michael S. Tsirkin <[e-mail]>
Cc: Dr. David Alan Gilbert <[e-mail]>
---
 hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Peter Maydell Nov. 8, 2018, 2:08 p.m. UTC | #1

On 7 November 2018 at 15:54,  <[e-mail]> wrote:
> From: Corey Minyard <[e-mail]>
>
> This was if the eeprom is accessed during a state transfer, the
> transfer will be reliable.
>
> Signed-off-by: Corey Minyard <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Michael S. Tsirkin <[e-mail]>
> Cc: Dr. David Alan Gilbert <[e-mail]>
> ---
>  hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..d4430b0c5d 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -29,6 +29,8 @@
>
>  //#define DEBUG
>
> +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"

The part of this patch which is adding the usual QOM
macros is good, but we should provide also the
cast-macro (the one that's an OBJECT_CHECK(...)).
And this should be separate from adding the vmstate.

> +
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
>      void *data;
> @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
>      return eeprom_receive_byte(dev);
>  }
>
> +static const VMStateDescription vmstate_smbus_eeprom = {
> +    .name = TYPE_SMBUS_EEPROM_DEVICE,

Generally we don't use the TYPE_FOO constant for the vmstate
name, because we can usually freely change the TYPE_FOO string without
breaking back-compat, but we can't change the vmstate name.
So we decouple them to make it more obvious when a change might
be changing the migration on-the-wire format.

> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

This doesn't do anything for migration of the actual data contents.
The current users of this device (hw/arm/aspeed.c and the
smbus_eeprom_init() function) doesn't do anything
to migrate the contents. For that matter, "user of the device
passes a pointer to a random lump of memory via a device property"
is a bit funky as an interface. The device should allocate its
own memory and migrate it, and the user should just pass the
initial required contents (default being "zero-initialize",
which is what everybody except the mips_fulong2e, mips_malta
and sam460ex want).

Does this also break migration from an old QEMU to a new one?
(For Aspeed that's probably ok, but we should flag it up in the
commit message if so. x86 uses need care...)

> +
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
>      SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
> @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      sc->write_data = eeprom_write_data;
>      sc->read_data = eeprom_read_data;
>      dc->props = smbus_eeprom_properties;
> +    dc->vmsd = &vmstate_smbus_eeprom;
>      /* Reason: pointer property "data" */
>      dc->user_creatable = false;
>  }
>
>  static const TypeInfo smbus_eeprom_info = {
> -    .name          = "smbus-eeprom",
> +    .name          = TYPE_SMBUS_EEPROM_DEVICE,
>      .parent        = TYPE_SMBUS_DEVICE,
>      .instance_size = sizeof(SMBusEEPROMDevice),
>      .class_init    = smbus_eeprom_class_initfn,
> --
> 2.17.1

thanks
-- PMM

Corey Minyard Nov. 8, 2018, 5:58 p.m. UTC | #2

On 11/8/18 8:08 AM, Peter Maydell wrote:
> On 7 November 2018 at 15:54,  <[e-mail]> wrote:
>> From: Corey Minyard <[e-mail]>
>>
>> This was if the eeprom is accessed during a state transfer, the
>> transfer will be reliable.
>>
>> Signed-off-by: Corey Minyard <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Michael S. Tsirkin <[e-mail]>
>> Cc: Dr. David Alan Gilbert <[e-mail]>
>> ---
>>   hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index f18aa3de35..d4430b0c5d 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -29,6 +29,8 @@
>>
>>   //#define DEBUG
>>
>> +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
> The part of this patch which is adding the usual QOM
> macros is good, but we should provide also the
> cast-macro (the one that's an OBJECT_CHECK(...)).
> And this should be separate from adding the vmstate.
>
>> +
>>   typedef struct SMBusEEPROMDevice {
>>       SMBusDevice smbusdev;
>>       void *data;
>> @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
>>       return eeprom_receive_byte(dev);
>>   }
>>
>> +static const VMStateDescription vmstate_smbus_eeprom = {
>> +    .name = TYPE_SMBUS_EEPROM_DEVICE,
> Generally we don't use the TYPE_FOO constant for the vmstate
> name, because we can usually freely change the TYPE_FOO string without
> breaking back-compat, but we can't change the vmstate name.
> So we decouple them to make it more obvious when a change might
> be changing the migration on-the-wire format.
>

Ok, I've updated the code to use the standard type name and cast method
(in a separate patch) and I fixed the vmstate name.  It is better, you are
right.


>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
>> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> This doesn't do anything for migration of the actual data contents.
> The current users of this device (hw/arm/aspeed.c and the
> smbus_eeprom_init() function) doesn't do anything
> to migrate the contents. For that matter, "user of the device
> passes a pointer to a random lump of memory via a device property"
> is a bit funky as an interface. The device should allocate its
> own memory and migrate it, and the user should just pass the
> initial required contents (default being "zero-initialize",
> which is what everybody except the mips_fulong2e, mips_malta
> and sam460ex want).


I debated on this, and it depends on what the eeprom is used for.  If
it's a DRAM eeprom, it shouldn't need to be transferred.
If it's something where the host stores data for later use, you do.
Since it wasn't being cloned before, it probably won't matter now.

But even in the DRAM eeprom case, it shouldn't matter if it gets
transferred.  So it's probably safe to just transfer it.  Easy enough
to add.


>
> Does this also break migration from an old QEMU to a new one?
> (For Aspeed that's probably ok, but we should flag it up in the
> commit message if so. x86 uses need care...)
>
There is no transfer before, so I don't see why it would break anything.
Am I missing something?

-corey

>> +
>>   static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>   {
>>       SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
>> @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>       sc->write_data = eeprom_write_data;
>>       sc->read_data = eeprom_read_data;
>>       dc->props = smbus_eeprom_properties;
>> +    dc->vmsd = &vmstate_smbus_eeprom;
>>       /* Reason: pointer property "data" */
>>       dc->user_creatable = false;
>>   }
>>
>>   static const TypeInfo smbus_eeprom_info = {
>> -    .name          = "smbus-eeprom",
>> +    .name          = TYPE_SMBUS_EEPROM_DEVICE,
>>       .parent        = TYPE_SMBUS_DEVICE,
>>       .instance_size = sizeof(SMBusEEPROMDevice),
>>       .class_init    = smbus_eeprom_class_initfn,
>> --
>> 2.17.1
> thanks
> -- PMM

Peter Maydell Nov. 8, 2018, 6:03 p.m. UTC | #3

On 8 November 2018 at 17:58, Corey Minyard <[e-mail]> wrote:
> On 11/8/18 8:08 AM, Peter Maydell wrote:
>> This doesn't do anything for migration of the actual data contents.
>> The current users of this device (hw/arm/aspeed.c and the
>> smbus_eeprom_init() function) doesn't do anything
>> to migrate the contents. For that matter, "user of the device
>> passes a pointer to a random lump of memory via a device property"
>> is a bit funky as an interface. The device should allocate its
>> own memory and migrate it, and the user should just pass the
>> initial required contents (default being "zero-initialize",
>> which is what everybody except the mips_fulong2e, mips_malta
>> and sam460ex want).

> I debated on this, and it depends on what the eeprom is used for.  If
> it's a DRAM eeprom, it shouldn't need to be transferred.

It's guest-visible data; the guest can write it and read it back.
So it needs to be migrated. Otherwise behaviour after migration
will not be the same as if the guest had never migrated.

>> Does this also break migration from an old QEMU to a new one?
>> (For Aspeed that's probably ok, but we should flag it up in the
>> commit message if so. x86 uses need care...)
>>
> There is no transfer before, so I don't see why it would break anything.
> Am I missing something?

I forget what the behaviour is where the source QEMU didn't
have a vmstate at all but the destination QEMU does expect
one. David can remind me...

thanks
- PMM

Corey Minyard Nov. 9, 2018, 2:56 p.m. UTC | #4

On 11/8/18 12:03 PM, Peter Maydell wrote:
> On 8 November 2018 at 17:58, Corey Minyard <[e-mail]> wrote:
>> On 11/8/18 8:08 AM, Peter Maydell wrote:
>>> This doesn't do anything for migration of the actual data contents.
>>> The current users of this device (hw/arm/aspeed.c and the
>>> smbus_eeprom_init() function) doesn't do anything
>>> to migrate the contents. For that matter, "user of the device
>>> passes a pointer to a random lump of memory via a device property"
>>> is a bit funky as an interface. The device should allocate its
>>> own memory and migrate it, and the user should just pass the
>>> initial required contents (default being "zero-initialize",
>>> which is what everybody except the mips_fulong2e, mips_malta
>>> and sam460ex want).
>> I debated on this, and it depends on what the eeprom is used for.  If
>> it's a DRAM eeprom, it shouldn't need to be transferred.
> It's guest-visible data; the guest can write it and read it back.
> So it needs to be migrated. Otherwise behaviour after migration
> will not be the same as if the guest had never migrated.


I looked at adding it, but I ran into an issue.  The value is a

DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data)

and that means the data has to be void *, but to transfer it it must be 
a uint8_t *.
The pointer property seems to be a cheap hack, I'm not sure what it will 
take
to fix it.

I was hoping this would be easy.  I guess transferring the eeprom is not
that important, the state of pm_smbus is a lot more critical.  But it would
be nice to fix it since I am messing with that code.

Thanks for your help.

-corey

>>> Does this also break migration from an old QEMU to a new one?
>>> (For Aspeed that's probably ok, but we should flag it up in the
>>> commit message if so. x86 uses need care...)
>>>
>> There is no transfer before, so I don't see why it would break anything.
>> Am I missing something?
> I forget what the behaviour is where the source QEMU didn't
> have a vmstate at all but the destination QEMU does expect
> one. David can remind me...
>
> thanks
> - PMM

Peter Maydell Nov. 9, 2018, 3:02 p.m. UTC | #5

On 9 November 2018 at 14:56, Corey Minyard <[e-mail]> wrote:
> On 11/8/18 12:03 PM, Peter Maydell wrote:
>>
>> On 8 November 2018 at 17:58, Corey Minyard <[e-mail]> wrote:
>>>
>>> On 11/8/18 8:08 AM, Peter Maydell wrote:
>>>>
>>>> This doesn't do anything for migration of the actual data contents.
>>>> The current users of this device (hw/arm/aspeed.c and the
>>>> smbus_eeprom_init() function) doesn't do anything
>>>> to migrate the contents. For that matter, "user of the device
>>>> passes a pointer to a random lump of memory via a device property"
>>>> is a bit funky as an interface. The device should allocate its
>>>> own memory and migrate it, and the user should just pass the
>>>> initial required contents (default being "zero-initialize",
>>>> which is what everybody except the mips_fulong2e, mips_malta
>>>> and sam460ex want).
>>>
>>> I debated on this, and it depends on what the eeprom is used for.  If
>>> it's a DRAM eeprom, it shouldn't need to be transferred.
>>
>> It's guest-visible data; the guest can write it and read it back.
>> So it needs to be migrated. Otherwise behaviour after migration
>> will not be the same as if the guest had never migrated.
>
>
>
> I looked at adding it, but I ran into an issue.  The value is a
>
> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data)
>
> and that means the data has to be void *, but to transfer it it must be a
> uint8_t *.
> The pointer property seems to be a cheap hack, I'm not sure what it will
> take
> to fix it.

The data provided by the caller is only the initialization
data. So I think the device should create its own memory
to copy that init data into, and migrate that ram, not
wherever the initialization data lives. (Currently
this "copy the data into our own ram" happens in the
smbus_eeprom_init() wrapper, but we should do it in the
device realize function instead.)

Also there seem to be unresolved questions about what happens
on reset -- should the EEPROM revert back to the initial
contents? We don't do that at the moment, but this breaks
the usual QEMU pattern that reset is as if you'd just
completely restarted QEMU.

thanks
-- PMM

Corey Minyard Nov. 9, 2018, 5:19 p.m. UTC | #6

On 11/9/18 9:02 AM, Peter Maydell wrote:
> On 9 November 2018 at 14:56, Corey Minyard <[e-mail]> wrote:
>> On 11/8/18 12:03 PM, Peter Maydell wrote:
>>> On 8 November 2018 at 17:58, Corey Minyard <[e-mail]> wrote:
>>>> On 11/8/18 8:08 AM, Peter Maydell wrote:
>>>>> This doesn't do anything for migration of the actual data contents.
>>>>> The current users of this device (hw/arm/aspeed.c and the
>>>>> smbus_eeprom_init() function) doesn't do anything
>>>>> to migrate the contents. For that matter, "user of the device
>>>>> passes a pointer to a random lump of memory via a device property"
>>>>> is a bit funky as an interface. The device should allocate its
>>>>> own memory and migrate it, and the user should just pass the
>>>>> initial required contents (default being "zero-initialize",
>>>>> which is what everybody except the mips_fulong2e, mips_malta
>>>>> and sam460ex want).
>>>> I debated on this, and it depends on what the eeprom is used for.  If
>>>> it's a DRAM eeprom, it shouldn't need to be transferred.
>>> It's guest-visible data; the guest can write it and read it back.
>>> So it needs to be migrated. Otherwise behaviour after migration
>>> will not be the same as if the guest had never migrated.
>>
>>
>> I looked at adding it, but I ran into an issue.  The value is a
>>
>> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data)
>>
>> and that means the data has to be void *, but to transfer it it must be a
>> uint8_t *.
>> The pointer property seems to be a cheap hack, I'm not sure what it will
>> take
>> to fix it.
> The data provided by the caller is only the initialization
> data. So I think the device should create its own memory
> to copy that init data into, and migrate that ram, not
> wherever the initialization data lives. (Currently
> this "copy the data into our own ram" happens in the
> smbus_eeprom_init() wrapper, but we should do it in the
> device realize function instead.)

That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"?
I don't know the value of creating a properly like this, since the user
can't set it and can't see it.  Plus the comments in the code say that
it should be gotten rid of at some point.

But if I store off the initialization data and keep the actual data in
a separate memory created by the realize function, that should work
find.


>
> Also there seem to be unresolved questions about what happens
> on reset -- should the EEPROM revert back to the initial
> contents? We don't do that at the moment, but this breaks
> the usual QEMU pattern that reset is as if you'd just
> completely restarted QEMU.

I would consider this part of the hardware, like data on a disk drive,
so it seem better to leave it alone after a reset.  But it's not quite
the same.  So I'm not sure.

Thanks,

-corey


> thanks
> -- PMM

Peter Maydell Nov. 9, 2018, 5:53 p.m. UTC | #7

On 9 November 2018 at 17:19, Corey Minyard <[e-mail]> wrote:
> On 11/9/18 9:02 AM, Peter Maydell wrote:
>> The data provided by the caller is only the initialization
>> data. So I think the device should create its own memory
>> to copy that init data into, and migrate that ram, not
>> wherever the initialization data lives. (Currently
>> this "copy the data into our own ram" happens in the
>> smbus_eeprom_init() wrapper, but we should do it in the
>> device realize function instead.)
>
>
> That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"?
> I don't know the value of creating a properly like this, since the user
> can't set it and can't see it.  Plus the comments in the code say that
> it should be gotten rid of at some point.
>
> But if I store off the initialization data and keep the actual data in
> a separate memory created by the realize function, that should work
> find.

Well, you still have to pass the init data to the device
somehow, so I think a pointer property is not a terrible
way to do that.

>> Also there seem to be unresolved questions about what happens
>> on reset -- should the EEPROM revert back to the initial
>> contents? We don't do that at the moment, but this breaks
>> the usual QEMU pattern that reset is as if you'd just
>> completely restarted QEMU.
>
>
> I would consider this part of the hardware, like data on a disk drive,
> so it seem better to leave it alone after a reset.  But it's not quite
> the same.  So I'm not sure.

That would require us to support backing it properly with a block
device, like the pflash flash devices, I think. (This would
be the long term way to be able to dump the pointer property,
in favour of a block backend property.)

thanks
-- PMM

Dr. David Alan Gilbert Nov. 12, 2018, 5:28 p.m. UTC | #8

* Peter Maydell ([e-mail]) wrote:
> On 8 November 2018 at 17:58, Corey Minyard <[e-mail]> wrote:
> > On 11/8/18 8:08 AM, Peter Maydell wrote:
> >> This doesn't do anything for migration of the actual data contents.
> >> The current users of this device (hw/arm/aspeed.c and the
> >> smbus_eeprom_init() function) doesn't do anything
> >> to migrate the contents. For that matter, "user of the device
> >> passes a pointer to a random lump of memory via a device property"
> >> is a bit funky as an interface. The device should allocate its
> >> own memory and migrate it, and the user should just pass the
> >> initial required contents (default being "zero-initialize",
> >> which is what everybody except the mips_fulong2e, mips_malta
> >> and sam460ex want).
> 
> > I debated on this, and it depends on what the eeprom is used for.  If
> > it's a DRAM eeprom, it shouldn't need to be transferred.
> 
> It's guest-visible data; the guest can write it and read it back.
> So it needs to be migrated. Otherwise behaviour after migration
> will not be the same as if the guest had never migrated.
> 
> >> Does this also break migration from an old QEMU to a new one?
> >> (For Aspeed that's probably ok, but we should flag it up in the
> >> commit message if so. x86 uses need care...)
> >>
> > There is no transfer before, so I don't see why it would break anything.
> > Am I missing something?
> 
> I forget what the behaviour is where the source QEMU didn't
> have a vmstate at all but the destination QEMU does expect
> one. David can remind me...

If it's a separate device then you tend to get away with it - there's no
check that all devices receive their state, so it should work.
This does break backwards migration though - migrating to an older
qemu with the existing machine type will complain it's received a 
device which it doesn't understand.
You should be able to add a .needed to the device
so it's only sent for new machine types.
(Which is what I said in August, when I also asked about the data)

Dave

> thanks
> - PMM
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Dr. David Alan Gilbert Nov. 12, 2018, 5:38 p.m. UTC | #9

* Peter Maydell ([e-mail]) wrote:
> On 9 November 2018 at 17:19, Corey Minyard <[e-mail]> wrote:
> > On 11/9/18 9:02 AM, Peter Maydell wrote:
> >> The data provided by the caller is only the initialization
> >> data. So I think the device should create its own memory
> >> to copy that init data into, and migrate that ram, not
> >> wherever the initialization data lives. (Currently
> >> this "copy the data into our own ram" happens in the
> >> smbus_eeprom_init() wrapper, but we should do it in the
> >> device realize function instead.)
> >
> >
> > That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"?
> > I don't know the value of creating a properly like this, since the user
> > can't set it and can't see it.  Plus the comments in the code say that
> > it should be gotten rid of at some point.
> >
> > But if I store off the initialization data and keep the actual data in
> > a separate memory created by the realize function, that should work
> > find.
> 
> Well, you still have to pass the init data to the device
> somehow, so I think a pointer property is not a terrible
> way to do that.
> 
> >> Also there seem to be unresolved questions about what happens
> >> on reset -- should the EEPROM revert back to the initial
> >> contents? We don't do that at the moment, but this breaks
> >> the usual QEMU pattern that reset is as if you'd just
> >> completely restarted QEMU.
> >
> >
> > I would consider this part of the hardware, like data on a disk drive,
> > so it seem better to leave it alone after a reset.  But it's not quite
> > the same.  So I'm not sure.
> 
> That would require us to support backing it properly with a block
> device, like the pflash flash devices, I think. (This would
> be the long term way to be able to dump the pointer property,
> in favour of a block backend property.)

There's a number of separate questions:

a) Can the guest write to the EEPROM or are we just treating it as a
ROM?
b) If a guest writes to the EEPROM and then resets does the data stay
there [I'd expect so, it's an EEPROM]
c) It the guest writes to the EEPROM and then quits qemu and restarts
does the data stay there?
d) If the guest is migrated does it keep the data it's written - I'd say
it must because otherwise it would get confused.

(c) is where it starts looking like a pflash - which does get a bit
messy.

Dave


> thanks
> -- PMM
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Peter Maydell Nov. 12, 2018, 5:41 p.m. UTC | #10

On 12 November 2018 at 17:38, Dr. David Alan Gilbert
<[e-mail]> wrote:
> * Peter Maydell ([e-mail]) wrote:
>> That would require us to support backing it properly with a block
>> device, like the pflash flash devices, I think. (This would
>> be the long term way to be able to dump the pointer property,
>> in favour of a block backend property.)
>
> There's a number of separate questions:
>
> a) Can the guest write to the EEPROM or are we just treating it as a
> ROM?
> b) If a guest writes to the EEPROM and then resets does the data stay
> there [I'd expect so, it's an EEPROM]
> c) It the guest writes to the EEPROM and then quits qemu and restarts
> does the data stay there?
> d) If the guest is migrated does it keep the data it's written - I'd say
> it must because otherwise it would get confused.
>
> (c) is where it starts looking like a pflash - which does get a bit
> messy.

My view is that the answers to b and c should always be the same:
resetting QEMU with a system reset should look the same as if
you stopped QEMU and restarted it. (Our only kind of system
reset is an emulation of a hard powercycle.)

In this case, the answer to "a" is currently "yes, it can write to it",
though whether any guest takes advantage of that is a different
question.

thanks
-- PMM

Patch

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index f18aa3de35..d4430b0c5d 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -29,6 +29,8 @@ 
 
 //#define DEBUG
 
+#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
+
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
     void *data;
@@ -97,6 +99,17 @@  static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
     return eeprom_receive_byte(dev);
 }
 
+static const VMStateDescription vmstate_smbus_eeprom = {
+    .name = TYPE_SMBUS_EEPROM_DEVICE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
+        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
     SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
@@ -121,12 +134,13 @@  static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     sc->write_data = eeprom_write_data;
     sc->read_data = eeprom_read_data;
     dc->props = smbus_eeprom_properties;
+    dc->vmsd = &vmstate_smbus_eeprom;
     /* Reason: pointer property "data" */
     dc->user_creatable = false;
 }
 
 static const TypeInfo smbus_eeprom_info = {
-    .name          = "smbus-eeprom",
+    .name          = TYPE_SMBUS_EEPROM_DEVICE,
     .parent        = TYPE_SMBUS_DEVICE,
     .instance_size = sizeof(SMBusEEPROMDevice),
     .class_init    = smbus_eeprom_class_initfn,