| Message ID | 20181126200435.23408-17-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix/add vmstate handling in some I2C code | expand |
Hi Corey, On 26/11/18 21:04, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > Reset the contents to init data and reset the offset on a machine > reset. > > Signed-off-by: Corey Minyard <cminyard@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? > eeprom->offset = 0; This is correct, the offset reset belongs to the reset() function. 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; >
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 <cminyard@mvista.com> >> >> Reset the contents to init data and reset the offset on a machine >> reset. >> >> Signed-off-by: Corey Minyard <cminyard@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 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. -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; >>
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 <cminyard@mvista.com> >>> >>> Reset the contents to init data and reset the offset on a machine >>> reset. >>> >>> Signed-off-by: Corey Minyard <cminyard@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; >>> >
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 <cminyard@mvista.com> >>>> >>>> Reset the contents to init data and reset the offset on a machine >>>> reset. >>>> >>>> Signed-off-by: Corey Minyard <cminyard@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. 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; >>>>
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 <cminyard@mvista.com> >>>>> >>>>> Reset the contents to init data and reset the offset on a machine >>>>> reset. >>>>> >>>>> Signed-off-by: Corey Minyard <cminyard@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. > > > 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. > > 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; >>>>> >
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 <cminyard@mvista.com> >>>>>> >>>>>> Reset the contents to init data and reset the offset on a machine >>>>>> reset. >>>>>> >>>>>> Signed-off-by: Corey Minyard <cminyard@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. >> >> 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; >>>>>>
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, minyard@acm.org wrote: >>>>>>> From: Corey Minyard <cminyard@mvista.com> >>>>>>> >>>>>>> Reset the contents to init data and reset the offset on a machine >>>>>>> reset. >>>>>>> >>>>>>> Signed-off-by: Corey Minyard <cminyard@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. >>> >>> 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é <philmd@redhat.com> > > > -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; >>>>>>> >
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); @@ -115,6 +115,11 @@ static void smbus_eeprom_realize(DeviceState *dev, Error **errp) eeprom->offset = 0; } +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;