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