Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Peter Maydell]

On 9 November 2018 at 14:56, Corey Minyard <address@hidden> wrote:
> On 11/8/18 12:03 PM, Peter Maydell wrote:
>>
>> On 8 November 2018 at 17:58, Corey Minyard <address@hidden> 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