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

[Corey Minyard <minyard@acm.org>]

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

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.

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