From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57325) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRcxD-0006fg-Jb for qemu-devel@nongnu.org; Tue, 27 Nov 2018 07:59:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gRcx9-0005vh-L9 for qemu-devel@nongnu.org; Tue, 27 Nov 2018 07:59:03 -0500 Received: from mail-ot1-x343.google.com ([2607:f8b0:4864:20::343]:41309) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gRcx9-0005v9-GZ for qemu-devel@nongnu.org; Tue, 27 Nov 2018 07:58:59 -0500 Received: by mail-ot1-x343.google.com with SMTP id u16so19975548otk.8 for ; Tue, 27 Nov 2018 04:58:59 -0800 (PST) Reply-To: minyard@acm.org References: <20181126200435.23408-1-minyard@acm.org> <20181126200435.23408-17-minyard@acm.org> <192d9e99-a3e6-0ca3-7c23-bd5a0d2b44cb@acm.org> <663ea18c-4028-6eff-d69d-c8870effffa1@acm.org> <285b12c9-af5b-f4fa-0c8a-f73e57dd9c75@redhat.com> From: Corey Minyard Message-ID: <521bc02a-c1ea-267f-6629-817d41bcc987@gmail.com> Date: Tue, 27 Nov 2018 06:58:56 -0600 MIME-Version: 1.0 In-Reply-To: <285b12c9-af5b-f4fa-0c8a-f73e57dd9c75@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , minyard@acm.org, qemu-devel@nongnu.org, "Dr . David Alan Gilbert" , Peter Maydell Cc: Paolo Bonzini , Corey Minyard , "Michael S . Tsirkin" 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, 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. >> >> 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"? -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; >>>>>>