| Message ID | 20250321081255.30564-1-heinrich.schuchardt@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] goldfish_rtc: keep time offset when resetting | expand |
On Fri, Mar 21, 2025 at 1:43 PM Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > Currently resetting leads to resynchronizing the Goldfish RTC with the > system clock of the host. In real hardware an RTC reset would not change > the wall time. Other RTCs like pl031 do not show this behavior. > > Move the synchronization of the RTC with the system clock to the instance > realization. > > Reported-by: Frederik Du Toit Lotter <fred.lotter@canonical.com> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> LGTM. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > hw/rtc/goldfish_rtc.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c > index 0f1b53e0e4..203a343511 100644 > --- a/hw/rtc/goldfish_rtc.c > +++ b/hw/rtc/goldfish_rtc.c > @@ -239,15 +239,8 @@ static const VMStateDescription goldfish_rtc_vmstate = { > static void goldfish_rtc_reset(DeviceState *dev) > { > GoldfishRTCState *s = GOLDFISH_RTC(dev); > - struct tm tm; > > timer_del(s->timer); > - > - qemu_get_timedate(&tm, 0); > - s->tick_offset = mktimegm(&tm); > - s->tick_offset *= NANOSECONDS_PER_SECOND; > - s->tick_offset -= qemu_clock_get_ns(rtc_clock); > - s->tick_offset_vmstate = 0; > s->alarm_next = 0; > s->alarm_running = 0; > s->irq_pending = 0; > @@ -258,6 +251,7 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) > { > SysBusDevice *dev = SYS_BUS_DEVICE(d); > GoldfishRTCState *s = GOLDFISH_RTC(d); > + struct tm tm; > > memory_region_init_io(&s->iomem, OBJECT(s), > &goldfish_rtc_ops[s->big_endian], s, > @@ -267,6 +261,12 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) > sysbus_init_irq(dev, &s->irq); > > s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); > + > + qemu_get_timedate(&tm, 0); > + s->tick_offset = mktimegm(&tm); > + s->tick_offset *= NANOSECONDS_PER_SECOND; > + s->tick_offset -= qemu_clock_get_ns(rtc_clock); > + s->tick_offset_vmstate = 0; > } > > static const Property goldfish_rtc_properties[] = { > -- > 2.48.1 > >
Hi Heinrich, On 21/3/25 09:12, Heinrich Schuchardt wrote: > Currently resetting leads to resynchronizing the Goldfish RTC with the > system clock of the host. In real hardware an RTC reset would not change > the wall time. Other RTCs like pl031 do not show this behavior. > > Move the synchronization of the RTC with the system clock to the instance > realization. > Cc: qemu-stable@nongnu.org Fixes: 9a5b40b8427 ("hw: rtc: Add Goldfish RTC device") > Reported-by: Frederik Du Toit Lotter <fred.lotter@canonical.com> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > hw/rtc/goldfish_rtc.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c > index 0f1b53e0e4..203a343511 100644 > --- a/hw/rtc/goldfish_rtc.c > +++ b/hw/rtc/goldfish_rtc.c > @@ -239,15 +239,8 @@ static const VMStateDescription goldfish_rtc_vmstate = { > static void goldfish_rtc_reset(DeviceState *dev) > { > GoldfishRTCState *s = GOLDFISH_RTC(dev); > - struct tm tm; > > timer_del(s->timer); > - > - qemu_get_timedate(&tm, 0); > - s->tick_offset = mktimegm(&tm); > - s->tick_offset *= NANOSECONDS_PER_SECOND; > - s->tick_offset -= qemu_clock_get_ns(rtc_clock); > - s->tick_offset_vmstate = 0; > s->alarm_next = 0; > s->alarm_running = 0; > s->irq_pending = 0; > @@ -258,6 +251,7 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) > { > SysBusDevice *dev = SYS_BUS_DEVICE(d); > GoldfishRTCState *s = GOLDFISH_RTC(d); > + struct tm tm; > > memory_region_init_io(&s->iomem, OBJECT(s), > &goldfish_rtc_ops[s->big_endian], s, > @@ -267,6 +261,12 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) > sysbus_init_irq(dev, &s->irq); > > s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); > + > + qemu_get_timedate(&tm, 0); > + s->tick_offset = mktimegm(&tm); > + s->tick_offset *= NANOSECONDS_PER_SECOND; > + s->tick_offset -= qemu_clock_get_ns(rtc_clock); OK > + s->tick_offset_vmstate = 0; This last line is pointless. Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > }
On 21.03.25 17:08, Philippe Mathieu-Daudé wrote: > Hi Heinrich, > > On 21/3/25 09:12, Heinrich Schuchardt wrote: >> Currently resetting leads to resynchronizing the Goldfish RTC with the >> system clock of the host. In real hardware an RTC reset would not change >> the wall time. Other RTCs like pl031 do not show this behavior. >> >> Move the synchronization of the RTC with the system clock to the instance >> realization. >> > > Cc: qemu-stable@nongnu.org > Fixes: 9a5b40b8427 ("hw: rtc: Add Goldfish RTC device") > >> Reported-by: Frederik Du Toit Lotter <fred.lotter@canonical.com> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> hw/rtc/goldfish_rtc.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c >> index 0f1b53e0e4..203a343511 100644 >> --- a/hw/rtc/goldfish_rtc.c >> +++ b/hw/rtc/goldfish_rtc.c >> @@ -239,15 +239,8 @@ static const VMStateDescription >> goldfish_rtc_vmstate = { >> static void goldfish_rtc_reset(DeviceState *dev) >> { >> GoldfishRTCState *s = GOLDFISH_RTC(dev); >> - struct tm tm; >> timer_del(s->timer); >> - >> - qemu_get_timedate(&tm, 0); >> - s->tick_offset = mktimegm(&tm); >> - s->tick_offset *= NANOSECONDS_PER_SECOND; >> - s->tick_offset -= qemu_clock_get_ns(rtc_clock); >> - s->tick_offset_vmstate = 0; >> s->alarm_next = 0; >> s->alarm_running = 0; >> s->irq_pending = 0; >> @@ -258,6 +251,7 @@ static void goldfish_rtc_realize(DeviceState *d, >> Error **errp) >> { >> SysBusDevice *dev = SYS_BUS_DEVICE(d); >> GoldfishRTCState *s = GOLDFISH_RTC(d); >> + struct tm tm; >> memory_region_init_io(&s->iomem, OBJECT(s), >> &goldfish_rtc_ops[s->big_endian], s, >> @@ -267,6 +261,12 @@ static void goldfish_rtc_realize(DeviceState *d, >> Error **errp) >> sysbus_init_irq(dev, &s->irq); >> s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); >> + >> + qemu_get_timedate(&tm, 0); >> + s->tick_offset = mktimegm(&tm); >> + s->tick_offset *= NANOSECONDS_PER_SECOND; >> + s->tick_offset -= qemu_clock_get_ns(rtc_clock); > > OK > >> + s->tick_offset_vmstate = 0; > > This last line is pointless. Otherwise: > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Thanks for reviewing. Is the DeviceState structure fill with 0x00 when allocated? Best regards Heinrich > >> } >
On 21/3/25 17:10, Heinrich Schuchardt wrote: > On 21.03.25 17:08, Philippe Mathieu-Daudé wrote: >> Hi Heinrich, >> >> On 21/3/25 09:12, Heinrich Schuchardt wrote: >>> Currently resetting leads to resynchronizing the Goldfish RTC with the >>> system clock of the host. In real hardware an RTC reset would not change >>> the wall time. Other RTCs like pl031 do not show this behavior. >>> >>> Move the synchronization of the RTC with the system clock to the >>> instance >>> realization. >>> >> >> Cc: qemu-stable@nongnu.org >> Fixes: 9a5b40b8427 ("hw: rtc: Add Goldfish RTC device") >> >>> Reported-by: Frederik Du Toit Lotter <fred.lotter@canonical.com> >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>> --- >>> hw/rtc/goldfish_rtc.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c >>> index 0f1b53e0e4..203a343511 100644 >>> --- a/hw/rtc/goldfish_rtc.c >>> +++ b/hw/rtc/goldfish_rtc.c >>> @@ -239,15 +239,8 @@ static const VMStateDescription >>> goldfish_rtc_vmstate = { >>> static void goldfish_rtc_reset(DeviceState *dev) >>> { >>> GoldfishRTCState *s = GOLDFISH_RTC(dev); >>> - struct tm tm; >>> timer_del(s->timer); >>> - >>> - qemu_get_timedate(&tm, 0); >>> - s->tick_offset = mktimegm(&tm); >>> - s->tick_offset *= NANOSECONDS_PER_SECOND; >>> - s->tick_offset -= qemu_clock_get_ns(rtc_clock); >>> - s->tick_offset_vmstate = 0; >>> s->alarm_next = 0; >>> s->alarm_running = 0; >>> s->irq_pending = 0; >>> @@ -258,6 +251,7 @@ static void goldfish_rtc_realize(DeviceState *d, >>> Error **errp) >>> { >>> SysBusDevice *dev = SYS_BUS_DEVICE(d); >>> GoldfishRTCState *s = GOLDFISH_RTC(d); >>> + struct tm tm; >>> memory_region_init_io(&s->iomem, OBJECT(s), >>> &goldfish_rtc_ops[s->big_endian], s, >>> @@ -267,6 +261,12 @@ static void goldfish_rtc_realize(DeviceState *d, >>> Error **errp) >>> sysbus_init_irq(dev, &s->irq); >>> s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); >>> + >>> + qemu_get_timedate(&tm, 0); >>> + s->tick_offset = mktimegm(&tm); >>> + s->tick_offset *= NANOSECONDS_PER_SECOND; >>> + s->tick_offset -= qemu_clock_get_ns(rtc_clock); >> >> OK >> >>> + s->tick_offset_vmstate = 0; >> >> This last line is pointless. Otherwise: >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Thanks for reviewing. Is the DeviceState structure fill with 0x00 when > allocated? The QOM hierarchy of this device is: GOLDFISH_RTC -> SYS_BUS_DEVICE -> DEVICE -> OBJECT When objects are created, object_new() ends up calling object_initialize_with_type() which calls memset(0). Objects initialized "in place" via object_initialize_child() also end calling object_initialize() -> object_initialize_with_type() then memset(0). So yes, QOM-based objects have their state zero-initialized. Regards, Phil.
diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c index 0f1b53e0e4..203a343511 100644 --- a/hw/rtc/goldfish_rtc.c +++ b/hw/rtc/goldfish_rtc.c @@ -239,15 +239,8 @@ static const VMStateDescription goldfish_rtc_vmstate = { static void goldfish_rtc_reset(DeviceState *dev) { GoldfishRTCState *s = GOLDFISH_RTC(dev); - struct tm tm; timer_del(s->timer); - - qemu_get_timedate(&tm, 0); - s->tick_offset = mktimegm(&tm); - s->tick_offset *= NANOSECONDS_PER_SECOND; - s->tick_offset -= qemu_clock_get_ns(rtc_clock); - s->tick_offset_vmstate = 0; s->alarm_next = 0; s->alarm_running = 0; s->irq_pending = 0; @@ -258,6 +251,7 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) { SysBusDevice *dev = SYS_BUS_DEVICE(d); GoldfishRTCState *s = GOLDFISH_RTC(d); + struct tm tm; memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_rtc_ops[s->big_endian], s, @@ -267,6 +261,12 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) sysbus_init_irq(dev, &s->irq); s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); + + qemu_get_timedate(&tm, 0); + s->tick_offset = mktimegm(&tm); + s->tick_offset *= NANOSECONDS_PER_SECOND; + s->tick_offset -= qemu_clock_get_ns(rtc_clock); + s->tick_offset_vmstate = 0; } static const Property goldfish_rtc_properties[] = {
Currently resetting leads to resynchronizing the Goldfish RTC with the system clock of the host. In real hardware an RTC reset would not change the wall time. Other RTCs like pl031 do not show this behavior. Move the synchronization of the RTC with the system clock to the instance realization. Reported-by: Frederik Du Toit Lotter <fred.lotter@canonical.com> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- hw/rtc/goldfish_rtc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)