Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Corey Minyard]

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, address@hidden wrote:
> > > > From: Corey Minyard <address@hidden>
> > > >
> > > > Reset the contents to init data and reset the offset on a machine
> > > > reset.
> > > >
> > > > Signed-off-by: Corey Minyard <address@hidden>
> > > > ---
> > > >    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

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.

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;