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, miny...@acm.org wrote:
>>>>> From: Corey Minyard <cminy...@mvista.com>
>>>>>
>>>>> Reset the contents to init data and reset the offset on a machine
>>>>> reset.
>>>>>
>>>>> Signed-off-by: Corey Minyard <cminy...@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.
>
> 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;
>>>>>
>
