diff mbox series

[v2] hw/rtc/pl031: Send RTC_CHANGE QMP event

Message ID 20210920122535.269988-1-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] hw/rtc/pl031: Send RTC_CHANGE QMP event | expand

Commit Message

Eric Auger Sept. 20, 2021, 12:25 p.m. UTC
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 which is reported corresponds to the difference between the guest
reference time and the reference time kept in softmmu/rtc.c.

For instance adding 20s to the guest RTC value will report 20. Adding
an extra 20s to the guest RTC value will report 20 + 20 = 40.

The inclusion of qapi/qapi-types-misc-target.h in hw/rtl/pl031.c
require to compile the PL031 with specific_ss.add() to avoid
./qapi/qapi-types-misc-target.h:18:13: error: attempt to use poisoned
"TARGET_<ARCH>".

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---

v1 -> v2:
- Use Peter's implementation and remove subsection

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"

and compared with x86 behavior.
---
 hw/rtc/meson.build |  2 +-
 hw/rtc/pl031.c     | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Peter Maydell Sept. 23, 2021, 1:29 p.m. UTC | #1
On Mon, 20 Sept 2021 at 13:25, 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 which is reported corresponds to the difference between the guest
> reference time and the reference time kept in softmmu/rtc.c.
>
> For instance adding 20s to the guest RTC value will report 20. Adding
> an extra 20s to the guest RTC value will report 20 + 20 = 40.
>
> The inclusion of qapi/qapi-types-misc-target.h in hw/rtl/pl031.c
> require to compile the PL031 with specific_ss.add() to avoid
> ./qapi/qapi-types-misc-target.h:18:13: error: attempt to use poisoned
> "TARGET_<ARCH>".
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Thanks. This looks plausible to me (well, it would ;-)) but
I would appreciate review from Paolo or somebody else who
understands the rtc_change feature and handling.

> ---
>
> v1 -> v2:
> - Use Peter's implementation and remove subsection
>
> 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"
>
> and compared with x86 behavior.
> ---
>  hw/rtc/meson.build |  2 +-
>  hw/rtc/pl031.c     | 10 +++++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> 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..e7ced90b025 100644
> --- a/hw/rtc/pl031.c
> +++ b/hw/rtc/pl031.c
> @@ -24,6 +24,7 @@
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "trace.h"
> +#include "qapi/qapi-events-misc-target.h"
>
>  #define RTC_DR      0x00    /* Data read register */
>  #define RTC_MR      0x04    /* Match register */
> @@ -136,10 +137,17 @@ static void pl031_write(void * opaque, hwaddr offset,
>      trace_pl031_write(offset, value);
>
>      switch (offset) {
> -    case RTC_LR:
> +    case RTC_LR: {
> +        struct tm tm;
> +
>          s->tick_offset += value - pl031_get_count(s);
> +
> +        qemu_get_timedate(&tm, s->tick_offset);
> +        qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
> +
>          pl031_set_alarm(s);
>          break;
> +    }
>      case RTC_MR:
>          s->mr = value;
>          pl031_set_alarm(s);

-- PMM
Peter Maydell Nov. 1, 2021, 4:04 p.m. UTC | #2
On Thu, 23 Sept 2021 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 20 Sept 2021 at 13:25, 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 which is reported corresponds to the difference between the guest
> > reference time and the reference time kept in softmmu/rtc.c.
> >
> > For instance adding 20s to the guest RTC value will report 20. Adding
> > an extra 20s to the guest RTC value will report 20 + 20 = 40.
> >
> > The inclusion of qapi/qapi-types-misc-target.h in hw/rtl/pl031.c
> > require to compile the PL031 with specific_ss.add() to avoid
> > ./qapi/qapi-types-misc-target.h:18:13: error: attempt to use poisoned
> > "TARGET_<ARCH>".
> >
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Thanks. This looks plausible to me (well, it would ;-)) but
> I would appreciate review from Paolo or somebody else who
> understands the rtc_change feature and handling.

Ping? Review from somebody who understands rtc_change would
still be nice...

-- PMM
Paolo Bonzini Nov. 15, 2021, 12:48 p.m. UTC | #3
On 11/1/21 17:04, Peter Maydell wrote:
> On Thu, 23 Sept 2021 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Mon, 20 Sept 2021 at 13:25, 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 which is reported corresponds to the difference between the guest
>>> reference time and the reference time kept in softmmu/rtc.c.
>>>
>>> For instance adding 20s to the guest RTC value will report 20. Adding
>>> an extra 20s to the guest RTC value will report 20 + 20 = 40.
>>>
>>> The inclusion of qapi/qapi-types-misc-target.h in hw/rtl/pl031.c
>>> require to compile the PL031 with specific_ss.add() to avoid
>>> ./qapi/qapi-types-misc-target.h:18:13: error: attempt to use poisoned
>>> "TARGET_<ARCH>".
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Thanks. This looks plausible to me (well, it would ;-)) but
>> I would appreciate review from Paolo or somebody else who
>> understands the rtc_change feature and handling.
> 
> Ping? Review from somebody who understands rtc_change would
> still be nice...

The change looks good to me (sorry I missed this v2).  x86 also has some 
logic in the migration post-load, that might end up sending the event. 
However, that's best done separately after understanding and documenting 
exactly what x86 is doing.

Paolo
Peter Maydell Nov. 15, 2021, 5:40 p.m. UTC | #4
On Mon, 15 Nov 2021 at 12:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/1/21 17:04, Peter Maydell wrote:
> > On Thu, 23 Sept 2021 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Mon, 20 Sept 2021 at 13:25, 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 which is reported corresponds to the difference between the guest
> >>> reference time and the reference time kept in softmmu/rtc.c.
> >>>
> >>> For instance adding 20s to the guest RTC value will report 20. Adding
> >>> an extra 20s to the guest RTC value will report 20 + 20 = 40.
> >>>
> >>> The inclusion of qapi/qapi-types-misc-target.h in hw/rtl/pl031.c
> >>> require to compile the PL031 with specific_ss.add() to avoid
> >>> ./qapi/qapi-types-misc-target.h:18:13: error: attempt to use poisoned
> >>> "TARGET_<ARCH>".
> >>>
> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >>
> >> Thanks. This looks plausible to me (well, it would ;-)) but
> >> I would appreciate review from Paolo or somebody else who
> >> understands the rtc_change feature and handling.
> >
> > Ping? Review from somebody who understands rtc_change would
> > still be nice...
>
> The change looks good to me (sorry I missed this v2).  x86 also has some
> logic in the migration post-load, that might end up sending the event.
> However, that's best done separately after understanding and documenting
> exactly what x86 is doing.

Thanks; I've applied this to target-arm.next for 6.2.

-- PMM
diff mbox series

Patch

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..e7ced90b025 100644
--- a/hw/rtc/pl031.c
+++ b/hw/rtc/pl031.c
@@ -24,6 +24,7 @@ 
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "trace.h"
+#include "qapi/qapi-events-misc-target.h"
 
 #define RTC_DR      0x00    /* Data read register */
 #define RTC_MR      0x04    /* Match register */
@@ -136,10 +137,17 @@  static void pl031_write(void * opaque, hwaddr offset,
     trace_pl031_write(offset, value);
 
     switch (offset) {
-    case RTC_LR:
+    case RTC_LR: {
+        struct tm tm;
+
         s->tick_offset += value - pl031_get_count(s);
+
+        qemu_get_timedate(&tm, s->tick_offset);
+        qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
+
         pl031_set_alarm(s);
         break;
+    }
     case RTC_MR:
         s->mr = value;
         pl031_set_alarm(s);