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, [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. >>> >>> 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é <[email protected]> > > > -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; >>>>>>> >
