Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Philippe Mathieu-Daudé]

On 27/11/18 0:58, Corey Minyard wrote:
> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>> On 26/11/18 23:41, Corey Minyard wrote:
>>> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>>>> Hi Corey,
>>>>
>>>> On 26/11/18 21:04, address@hidden wrote:
>>>>> From: Corey Minyard <address@hidden>
>>>>>
>>>>> Reset the contents to init data and reset the offset on a machine
>>>>> reset.
>>>>>
>>>>> Signed-off-by: Corey Minyard <address@hidden>
>>>>> ---
>>>>>    hw/i2c/smbus_eeprom.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>>>> index a0dcadbd60..de3a492df4 100644
>>>>> --- a/hw/i2c/smbus_eeprom.c
>>>>> +++ b/hw/i2c/smbus_eeprom.c
>>>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>>>> vmstate_smbus_eeprom = {
>>>>>        }
>>>>>    };
>>>>>    -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>>>>    {
>>>>>        SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>>>    
>>>> 'git diff -U4' also shows this line:
>>>>
>>>>          memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>>>
>>>> I don't think this is correct.
>>>>
>>>> One test I'd like to have is a machine booting, updating the EPROM then
>>>> rebooting calling hw reset() to use the new values (BIOS use this).
>>>>
>>>> With this patch this won't work, you'll restore the EPROM content on
>>>> each machine reset.
>>>>
>>>> I'd move the memcpy() call to the realize() function.
>>>>
>>>> What do you think?
>>> There was some debate on this in the earlier patch set.  The general
>>> principle
>> Hmm I missed it and can't find it (quick basic search). I only find
>> references about VMState.
> 
> 
> It starts at
> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html

Thank you, very helpful.

> 
> The patch set was fairly different at that point.
> 
> 
>>> is that a reset is the same as starting up qemu from scratch, so I added
>>> this
>>> code based on that principle.  But I'm not really sure.
>>>
>>>>>        eeprom->offset = 0;
>>>> This is correct, the offset reset belongs to the reset() function.
>>> Actually, on a real system, a hardware reset will generally not
>>> affect the
>>> eeprom current offset register.  So if we don't take the above code,
>>> then
>>> IMHO this is wrong, too.
>> Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle
>> would.
>>
>> Maybe we can argue QEMU system reset doesn't work correctly yet to use
>> this feature. Personally I wouldn't expect the EEPROM content be be
>> reset after a reset, but maybe I should rely on a block backend for a
>> such feature, and not the current simple approach.
>>
> Yeah, it was mentioned that to do this correctly would require a block
> backend.
> I'll let others comment on the correctness of this, I guess.  It's a
> separate patch
> so it can be easily dropped.

Since modelling eeprom data retention on hardware reset isn't the goal
of your series, we can have a consensus, adding a comment explaining why
we choose this simpler way, and eeprom retention simulation requieres
more work with block backend.

> 
> The current code is far too broken for anyone to be using it, so we
> won't be
> breaking any current users, I don't think.
> 
> -corey
> 
>>> -corey
>>>
>>>
>>>> Regards,
>>>>
>>>> Phil.
>>>>
>>>>>    }
>>>>>    +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>>> +{
>>>>> +    smbus_eeprom_reset(dev);
>>>>> +}
>>>>> +
>>>>>    static Property smbus_eeprom_properties[] = {
>>>>>        DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>>>>        DEFINE_PROP_END_OF_LIST(),
>>>>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
>>>>> *klass, void *data)
>>>>>        SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>>>>          dc->realize = smbus_eeprom_realize;
>>>>> +    dc->reset = smbus_eeprom_reset;
>>>>>        sc->receive_byte = eeprom_receive_byte;
>>>>>        sc->write_data = eeprom_write_data;
>>>>>        dc->props = smbus_eeprom_properties;
>>>>>
>