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

[Philippe Mathieu-Daudé]

On 27/11/18 13:58, Corey Minyard wrote:
> 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, 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.
> 
> Good idea, I've done that.  Thanks for the reviews.  Is this a
> "reviewed-by"?

Why not ;)

With a comment:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

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