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

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

Posted by Eric Auger 2 years, 7 months ago

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(-)

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);
-- 
2.26.3

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

Posted by Peter Maydell 2 years, 7 months ago

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

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

Posted by Peter Maydell 2 years, 5 months ago

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

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

Posted by Paolo Bonzini 2 years, 5 months ago

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

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

Posted by Peter Maydell 2 years, 5 months ago

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