From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42185) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRPsc-0000Ce-77 for qemu-devel@nongnu.org; Mon, 26 Nov 2018 18:01:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gRPsV-00077t-84 for qemu-devel@nongnu.org; Mon, 26 Nov 2018 18:01:25 -0500 Received: from mail-wm1-f66.google.com ([209.85.128.66]:50200) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gRPsU-00075m-UW for qemu-devel@nongnu.org; Mon, 26 Nov 2018 18:01:19 -0500 Received: by mail-wm1-f66.google.com with SMTP id 125so20001983wmh.0 for ; Mon, 26 Nov 2018 15:01:10 -0800 (PST) References: <20181126200435.23408-1-minyard@acm.org> <20181126200435.23408-17-minyard@acm.org> <192d9e99-a3e6-0ca3-7c23-bd5a0d2b44cb@acm.org> From: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Message-ID: Date: Tue, 27 Nov 2018 00:01:07 +0100 MIME-Version: 1.0 In-Reply-To: <192d9e99-a3e6-0ca3-7c23-bd5a0d2b44cb@acm.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: minyard@acm.org, qemu-devel@nongnu.org, "Dr . David Alan Gilbert" , Peter Maydell Cc: Paolo Bonzini , "Michael S . Tsirkin" , Corey Minyard 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 >>> >>> Reset the contents to init data and reset the offset on a machine >>> reset. >>> >>> Signed-off-by: Corey Minyard >>> --- >>>   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; >>> >