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

[Corey Minyard <tcminyard@gmail.com>]

On 11/27/18 4:54 AM, Philippe Mathieu-Daudé wrote:
> 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, minyard@acm.org wrote:
>>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>>
>>>>>> Reset the contents to init data and reset the offset on a machine
>>>>>> reset.
>>>>>>
>>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>>> ---
>>>>>>     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.

Good idea, I've done that.  Thanks for the reviews.  Is this a 
"reviewed-by"?


-corey

>> 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;
>>>>>>