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, [email protected] wrote:
>>> From: Corey Minyard <[email protected]>
>>>
>>> Reset the contents to init data and reset the offset on a machine
>>> reset.
>>>
>>> Signed-off-by: Corey Minyard <[email protected]>
>>> ---
>>> 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.
> 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.
>
> -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;
>>>
>
