The PL031 currently is not able to report guest RTC change to the QMP
monitor as opposed to mc146818 or spapr RTCs. This patch adds the call
to qapi_event_send_rtc_change() when the Load Register is written. The
value that is reported corresponds to the difference between the new
RTC value and the RTC value elapsed since the base.

For instance adding 20s to the guest RTC value will report 20:
{'timestamp': {'seconds': 1631189494, 'microseconds': 16932},
'event': 'RTC_CHANGE', 'data': {'offset': 20}}

Adding another extra 20s to the guest RTC value will report 40:
{'timestamp': {'seconds': 1631189498, 'microseconds': 9708},
'event': 'RTC_CHANGE', 'data': {'offset': 40}}

To compute the offset we need to track the origin tick_offset (the one
computed at init time). So we need to migrate that field, which is done
in a dedicated subsection. The migration of this subsection is disabled
for machine types less or equal than 6.1.

After migration, adding an extra 20s on the destination returns 60:
{'timestamp': {'seconds': 1631189522, 'microseconds': 13081},
'event': 'RTC_CHANGE', 'data': {'offset': 60}}

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

Tested with the following script run on guest:

  #!/bin/sh
  old=$(hwclock --show | cut -f1-7 -d' ')
  oldabs=$(date +%s -d "$old")
  newabs=$(expr $oldabs + $1)
  new=$(date -d @"$newabs")
  echo Old: $oldabs $old
  echo New: $newabs $new
  hwclock --set --date "$new"

This was tested with both -rtc base=2010-12-03T01:02:00 and base=utc
qemu options. As far as I can see the reported value match what
is observed on x86 (except on x86 the values are not exactly the one
used on guest, ie. 18 for instance instead of 20).

Without migrating the original tick_offset (ie. original_tick_offset
taking the destination init tick_offset value), a delta is observed. I
don't know whether it is a bug. At firt glance I had the
impression it worked without the migration scheme but this delta
urged me to migrate the original_offset too.
---
 hw/core/machine.c      |  4 +++-
 hw/rtc/meson.build     |  2 +-
 hw/rtc/pl031.c         | 25 +++++++++++++++++++++++++
 include/hw/rtc/pl031.h |  2 ++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 067f42b528f..e93cc4ab39d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -37,7 +37,9 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
 
-GlobalProperty hw_compat_6_1[] = {};
+GlobalProperty hw_compat_6_1[] = {
+    { "pl031", "migrate-original-tick-offset", "false" },
+};
 const size_t hw_compat_6_1_len = G_N_ELEMENTS(hw_compat_6_1);
 
 GlobalProperty hw_compat_6_0[] = {
diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
index 7cecdee5ddb..8fd8d8f9a71 100644
--- a/hw/rtc/meson.build
+++ b/hw/rtc/meson.build
@@ -2,7 +2,7 @@
 softmmu_ss.add(when: 'CONFIG_DS1338', if_true: files('ds1338.c'))
 softmmu_ss.add(when: 'CONFIG_M41T80', if_true: files('m41t80.c'))
 softmmu_ss.add(when: 'CONFIG_M48T59', if_true: files('m48t59.c'))
-softmmu_ss.add(when: 'CONFIG_PL031', if_true: files('pl031.c'))
+specific_ss.add(when: 'CONFIG_PL031', if_true: files('pl031.c'))
 softmmu_ss.add(when: 'CONFIG_TWL92230', if_true: files('twl92230.c'))
 softmmu_ss.add(when: ['CONFIG_ISA_BUS', 'CONFIG_M48T59'], if_true: files('m48t59-isa.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-rtc.c'))
diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c
index 2bbb2062ac8..51dc14559c5 100644
--- a/hw/rtc/pl031.c
+++ b/hw/rtc/pl031.c
@@ -24,6 +24,8 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "trace.h"
+#include "qemu/timer.h"
+#include "qapi/qapi-events-misc-target.h"
 
 #define RTC_DR      0x00    /* Data read register */
 #define RTC_MR      0x04    /* Match register */
@@ -138,6 +140,7 @@ static void pl031_write(void * opaque, hwaddr offset,
     switch (offset) {
     case RTC_LR:
         s->tick_offset += value - pl031_get_count(s);
+        qapi_event_send_rtc_change(s->tick_offset - s->original_tick_offset);
         pl031_set_alarm(s);
         break;
     case RTC_MR:
@@ -190,6 +193,7 @@ static void pl031_init(Object *obj)
     qemu_get_timedate(&tm, 0);
     s->tick_offset = mktimegm(&tm) -
         qemu_clock_get_ns(rtc_clock) / NANOSECONDS_PER_SECOND;
+    s->original_tick_offset = s->tick_offset;
 
     s->timer = timer_new_ns(rtc_clock, pl031_interrupt, s);
 }
@@ -287,6 +291,24 @@ static const VMStateDescription vmstate_pl031_tick_offset = {
     }
 };
 
+static bool pl031_original_tick_offset_needed(void *opaque)
+{
+    PL031State *s = opaque;
+
+    return s->migrate_original_tick_offset;
+}
+
+static const VMStateDescription vmstate_pl031_original_tick_offset = {
+    .name = "pl031/original-tick-offset",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pl031_original_tick_offset_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(original_tick_offset, PL031State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_pl031 = {
     .name = "pl031",
     .version_id = 1,
@@ -305,6 +327,7 @@ static const VMStateDescription vmstate_pl031 = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_pl031_tick_offset,
+        &vmstate_pl031_original_tick_offset,
         NULL
     }
 };
@@ -320,6 +343,8 @@ static Property pl031_properties[] = {
      */
     DEFINE_PROP_BOOL("migrate-tick-offset",
                      PL031State, migrate_tick_offset, true),
+    DEFINE_PROP_BOOL("migrate-original-tick-offset",
+                     PL031State, migrate_original_tick_offset, true),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/rtc/pl031.h b/include/hw/rtc/pl031.h
index 9fd4be1abba..e1a12753d7d 100644
--- a/include/hw/rtc/pl031.h
+++ b/include/hw/rtc/pl031.h
@@ -37,6 +37,8 @@ struct PL031State {
     uint32_t tick_offset;
     bool tick_offset_migrated;
     bool migrate_tick_offset;
+    uint32_t original_tick_offset;
+    bool migrate_original_tick_offset;
 
     uint32_t mr;
     uint32_t lr;
-- 
2.26.3

On Thu, 9 Sept 2021 at 13:24, Eric Auger <eric.auger@redhat.com> wrote:
>
> The PL031 currently is not able to report guest RTC change to the QMP
> monitor as opposed to mc146818 or spapr RTCs. This patch adds the call
> to qapi_event_send_rtc_change() when the Load Register is written. The
> value that is reported corresponds to the difference between the new
> RTC value and the RTC value elapsed since the base.
>
> For instance adding 20s to the guest RTC value will report 20:
> {'timestamp': {'seconds': 1631189494, 'microseconds': 16932},
> 'event': 'RTC_CHANGE', 'data': {'offset': 20}}
>
> Adding another extra 20s to the guest RTC value will report 40:
> {'timestamp': {'seconds': 1631189498, 'microseconds': 9708},
> 'event': 'RTC_CHANGE', 'data': {'offset': 40}}
>
> To compute the offset we need to track the origin tick_offset (the one
> computed at init time). So we need to migrate that field, which is done
> in a dedicated subsection. The migration of this subsection is disabled
> for machine types less or equal than 6.1.
>
> After migration, adding an extra 20s on the destination returns 60:
> {'timestamp': {'seconds': 1631189522, 'microseconds': 13081},
> 'event': 'RTC_CHANGE', 'data': {'offset': 60}}
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


> @@ -138,6 +140,7 @@ static void pl031_write(void * opaque, hwaddr offset,
>      switch (offset) {
>      case RTC_LR:
>          s->tick_offset += value - pl031_get_count(s);
> +        qapi_event_send_rtc_change(s->tick_offset - s->original_tick_offset);
>          pl031_set_alarm(s);
>          break;
>      case RTC_MR:

None of the other users of qapi_event_send_rtc_change()
seem to have to track the baseline time like this. Shouldn't
this be doing something involving using qemu_ref_timedate()
as the baseline that it uses to figure out the change value ?
(The other users do this via qemu_timedate_diff() but since we
start with a value-in-seconds we might want to expose some other
API in softmmu/rtc.c.)

thanks
-- PMM

Hi Peter,
On 9/16/21 3:32 PM, Peter Maydell wrote:
> On Thu, 9 Sept 2021 at 13:24, Eric Auger <eric.auger@redhat.com> wrote:
>> The PL031 currently is not able to report guest RTC change to the QMP
>> monitor as opposed to mc146818 or spapr RTCs. This patch adds the call
>> to qapi_event_send_rtc_change() when the Load Register is written. The
>> value that is reported corresponds to the difference between the new
>> RTC value and the RTC value elapsed since the base.
>>
>> For instance adding 20s to the guest RTC value will report 20:
>> {'timestamp': {'seconds': 1631189494, 'microseconds': 16932},
>> 'event': 'RTC_CHANGE', 'data': {'offset': 20}}
>>
>> Adding another extra 20s to the guest RTC value will report 40:
>> {'timestamp': {'seconds': 1631189498, 'microseconds': 9708},
>> 'event': 'RTC_CHANGE', 'data': {'offset': 40}}
>>
>> To compute the offset we need to track the origin tick_offset (the one
>> computed at init time). So we need to migrate that field, which is done
>> in a dedicated subsection. The migration of this subsection is disabled
>> for machine types less or equal than 6.1.
>>
>> After migration, adding an extra 20s on the destination returns 60:
>> {'timestamp': {'seconds': 1631189522, 'microseconds': 13081},
>> 'event': 'RTC_CHANGE', 'data': {'offset': 60}}
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
>> @@ -138,6 +140,7 @@ static void pl031_write(void * opaque, hwaddr offset,
>>      switch (offset) {
>>      case RTC_LR:
>>          s->tick_offset += value - pl031_get_count(s);
>> +        qapi_event_send_rtc_change(s->tick_offset - s->original_tick_offset);
>>          pl031_set_alarm(s);
>>          break;
>>      case RTC_MR:
> None of the other users of qapi_event_send_rtc_change()
> seem to have to track the baseline time like this. Shouldn't
> this be doing something involving using qemu_ref_timedate()
> as the baseline that it uses to figure out the change value ?
> (The other users do this via qemu_timedate_diff() but since we
> start with a value-in-seconds we might want to expose some other
> API in softmmu/rtc.c.)

I struggled understanding the various kinds of clocks modeled in qemu
and the PL031 implementation. Both devices calling
qapi_event_send_rtc_change() seem to store the base rtc in their state
(mc146818rtc.c cmos data or spapr_rtc tas_ld(args, )) and then
effectivelly call qemu_timedate_diff() on this base rtc value. I did not
figure to do the equivalent with the pl031. I will further investigate
how I can mimic their implementation though.

Thanks

Eric

>
> thanks
> -- PMM
>

On Thu, 16 Sept 2021 at 18:19, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Peter,
> On 9/16/21 3:32 PM, Peter Maydell wrote:
> > None of the other users of qapi_event_send_rtc_change()
> > seem to have to track the baseline time like this. Shouldn't
> > this be doing something involving using qemu_ref_timedate()
> > as the baseline that it uses to figure out the change value ?
> > (The other users do this via qemu_timedate_diff() but since we
> > start with a value-in-seconds we might want to expose some other
> > API in softmmu/rtc.c.)
>
> I struggled understanding the various kinds of clocks modeled in qemu
> and the PL031 implementation. Both devices calling
> qapi_event_send_rtc_change() seem to store the base rtc in their state
> (mc146818rtc.c cmos data or spapr_rtc tas_ld(args, )) and then
> effectivelly call qemu_timedate_diff() on this base rtc value. I did not
> figure to do the equivalent with the pl031. I will further investigate
> how I can mimic their implementation though.

mc146818rtc.c calls qapi_event_send_rtc_change() in rtc_set_time().
It looks to me like that is just "when the guest writes to an
RTC register such that the guest RTC time has changed, use
qemu_timedate_diff() to find out the delta between that and what
the softmmu/rtc.c clock has as its baseline time, and then pass
that delta in seconds to qapi_event_send_rtc_change()".
This RTC has a lot of separate day/month/year registers, so the
implementation's idea of "current guest RTC setting" is a
complicated thing that it fills in into a struct tm, and which
qemu_timedate_diff() then parses back out into a "seconds" value.

spapr_rtc() is a hypercall implementation, so the guest passes it
a complete set of year/month/day/etc values all at once to set the time.

pl031 is a lot simpler as it is essentially just a count of
time in seconds, which we set up as "seconds since the Unix epoch".
But the basic principle is the same: the device contains the state
that tells you what it thinks the guest RTC value is now, and the
value we want to pass qapi_event_send_rtc_change() is the
difference between that and the reference time kept in
softmmu/rtc.c.

I think what you want is probably:

    struct tm tm;

    qemu_get_timedate(&tm, s->tick_offset);
    qapi_event_send_rtc_change(qemu_timedate_diff(&tm));

But I'm not 100% sure. I also feel like this is doing a
roundtrip from seconds to struct tm to seconds again, which
seems a bit silly (but it might matter if the rtc is configured
to 'localtime'? At any rate it's not completely clear that
it's always a no-op roundtrip).

I've cc'd Paolo who might be a bit more familiar with the rtc code
than I am.

-- PMM

Hi Peter,

On 9/16/21 8:24 PM, Peter Maydell wrote:
> On Thu, 16 Sept 2021 at 18:19, Eric Auger <eric.auger@redhat.com> wrote:
>> Hi Peter,
>> On 9/16/21 3:32 PM, Peter Maydell wrote:
>>> None of the other users of qapi_event_send_rtc_change()
>>> seem to have to track the baseline time like this. Shouldn't
>>> this be doing something involving using qemu_ref_timedate()
>>> as the baseline that it uses to figure out the change value ?
>>> (The other users do this via qemu_timedate_diff() but since we
>>> start with a value-in-seconds we might want to expose some other
>>> API in softmmu/rtc.c.)
>> I struggled understanding the various kinds of clocks modeled in qemu
>> and the PL031 implementation. Both devices calling
>> qapi_event_send_rtc_change() seem to store the base rtc in their state
>> (mc146818rtc.c cmos data or spapr_rtc tas_ld(args, )) and then
>> effectivelly call qemu_timedate_diff() on this base rtc value. I did not
>> figure to do the equivalent with the pl031. I will further investigate
>> how I can mimic their implementation though.
> mc146818rtc.c calls qapi_event_send_rtc_change() in rtc_set_time().
> It looks to me like that is just "when the guest writes to an
> RTC register such that the guest RTC time has changed, use
> qemu_timedate_diff() to find out the delta between that and what
> the softmmu/rtc.c clock has as its baseline time, and then pass
> that delta in seconds to qapi_event_send_rtc_change()".
> This RTC has a lot of separate day/month/year registers, so the
> implementation's idea of "current guest RTC setting" is a
> complicated thing that it fills in into a struct tm, and which
> qemu_timedate_diff() then parses back out into a "seconds" value.
>
> spapr_rtc() is a hypercall implementation, so the guest passes it
> a complete set of year/month/day/etc values all at once to set the time.
>
> pl031 is a lot simpler as it is essentially just a count of
> time in seconds, which we set up as "seconds since the Unix epoch".
> But the basic principle is the same: the device contains the state
> that tells you what it thinks the guest RTC value is now, and the
> value we want to pass qapi_event_send_rtc_change() is the
> difference between that and the reference time kept in
> softmmu/rtc.c.
>
> I think what you want is probably:
>
>     struct tm tm;
>
>     qemu_get_timedate(&tm, s->tick_offset);
>     qapi_event_send_rtc_change(qemu_timedate_diff(&tm));

Thank you for the explanation. It works on my end and indeed looks
similar to what other rtc do.

I will respin accordingly adding your Sob.

Thanks

Eric
>
> But I'm not 100% sure. I also feel like this is doing a
> roundtrip from seconds to struct tm to seconds again, which
> seems a bit silly (but it might matter if the rtc is configured
> to 'localtime'? At any rate it's not completely clear that
> it's always a no-op roundtrip).
>
> I've cc'd Paolo who might be a bit more familiar with the rtc code
> than I am.
>
> -- PMM
>