[PATCH v2 0/3] qapi: Move RTC_CHANGE back out of target schema

[PATCH v2 0/3] qapi: Move RTC_CHANGE back out of target schema

[Peter Maydell]

This patchset moves RTC_CHANGE back to misc.json, effectively
reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
the target schema.  That change was an attempt to make the event
target-specific to improve introspection, but the event isn't really
target-specific: it's machine or device specific.  Putting RTC_CHANGE
in the target schema with an ifdef list reduces maintainability (by
adding an if: list with a long list of targets that needs to be
manually updated as architectures are added or removed or as new
devices gain the RTC_CHANGE functionality) and increases compile time
(by preventing RTC devices which emit the event from being "compile
once" rather than "compile once per target", because
qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
"compile once" files.)

Patch 2 fixes some minor documentation issues for the RTC_CHANGE
event, noticed during development of the patchset.

Patch 3 makes the pl031 a build-once file again, which was the
initial motivation for the series.

Changes v1->v2:
 * patch 1 needs to change the #include line for pl031.c, now that
   the change to make that device emit RTC_CHANGE has landed upstream
 * patch 2 now also notes in the RTC_CHANGE documentation that
   not all devices/systems will emit the event (suggested by Markus)
 * patch 3 is new

My default assumption is that this series will go in via the
qapi tree; let me know if you'd prefer me to take it via
target-arm instead.

thanks
-- PMM

Peter Maydell (3):
  qapi: Move RTC_CHANGE back out of target schema
  qapi: Document some missing details of RTC_CHANGE event
  hw/rtc: Compile pl031 once-only

 qapi/misc-target.json | 33 ---------------------------------
 qapi/misc.json        | 24 ++++++++++++++++++++++++
 hw/ppc/spapr_rtc.c    |  2 +-
 hw/rtc/mc146818rtc.c  |  2 +-
 hw/rtc/pl031.c        |  2 +-
 hw/rtc/meson.build    |  2 +-
 6 files changed, 28 insertions(+), 37 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] qapi: Move RTC_CHANGE back out of target schema

[Peter Maydell]

This commit effectively reverts commit 183e4281a30962, which moved
the RTC_CHANGE event to the target schema.  That change was an
attempt to make the event target-specific to improve introspection,
but the event isn't really target-specific: it's machine or device
specific.  Putting RTC_CHANGE in the target schema with an ifdef list
reduces maintainability (by adding an if: list with a long list of
targets that needs to be manually updated as architectures are added
or removed or as new devices gain the RTC_CHANGE functionality) and
increases compile time (by preventing RTC devices which emit the
event from being "compile once" rather than "compile once per
target", because qapi-events-misc-target.h uses TARGET_* ifdefs,
which are poisoned in "compile once" files.)

Move RTC_CHANGE back to misc.json.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Greg Kurz <groug@kaod.org>
---
 qapi/misc-target.json | 33 ---------------------------------
 qapi/misc.json        | 22 ++++++++++++++++++++++
 hw/ppc/spapr_rtc.c    |  2 +-
 hw/rtc/mc146818rtc.c  |  2 +-
 hw/rtc/pl031.c        |  2 +-
 5 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4bc45d24741..036c5e4a914 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -2,39 +2,6 @@
 # vim: filetype=python
 #
 
-##
-# @RTC_CHANGE:
-#
-# Emitted when the guest changes the RTC time.
-#
-# @offset: offset between base RTC clock (as specified by -rtc base), and
-#          new RTC clock value
-#
-# Note: This event is rate-limited.
-#
-# Since: 0.13
-#
-# Example:
-#
-# <-   { "event": "RTC_CHANGE",
-#        "data": { "offset": 78 },
-#        "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
-#
-##
-{ 'event': 'RTC_CHANGE',
-  'data': { 'offset': 'int' },
-  'if': { 'any': [ 'TARGET_ALPHA',
-                   'TARGET_ARM',
-                   'TARGET_HPPA',
-                   'TARGET_I386',
-                   'TARGET_MIPS',
-                   'TARGET_MIPS64',
-                   'TARGET_PPC',
-                   'TARGET_PPC64',
-                   'TARGET_S390X',
-                   'TARGET_SH4',
-                   'TARGET_SPARC' ] } }
-
 ##
 # @rtc-reset-reinjection:
 #
diff --git a/qapi/misc.json b/qapi/misc.json
index e8054f415b2..7a70eaa3ffc 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -527,3 +527,25 @@
  'data': { '*option': 'str' },
  'returns': ['CommandLineOptionInfo'],
  'allow-preconfig': true }
+
+##
+# @RTC_CHANGE:
+#
+# Emitted when the guest changes the RTC time.
+#
+# @offset: offset between base RTC clock (as specified by -rtc base), and
+#          new RTC clock value
+#
+# Note: This event is rate-limited.
+#
+# Since: 0.13
+#
+# Example:
+#
+# <-   { "event": "RTC_CHANGE",
+#        "data": { "offset": 78 },
+#        "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
+#
+##
+{ 'event': 'RTC_CHANGE',
+  'data': { 'offset': 'int' } }
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 94a5510e4eb..79677cf5503 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -32,7 +32,7 @@
 #include "hw/ppc/spapr.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
-#include "qapi/qapi-events-misc-target.h"
+#include "qapi/qapi-events-misc.h"
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index e61a0cced4c..57c514e15c5 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -40,7 +40,7 @@
 #include "hw/rtc/mc146818rtc_regs.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
-#include "qapi/qapi-events-misc-target.h"
+#include "qapi/qapi-events-misc.h"
 #include "qapi/visitor.h"
 #include "hw/rtc/mc146818rtc_regs.h"
 
diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c
index 38d9d3c2f38..60167c778f2 100644
--- a/hw/rtc/pl031.c
+++ b/hw/rtc/pl031.c
@@ -24,7 +24,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "trace.h"
-#include "qapi/qapi-events-misc-target.h"
+#include "qapi/qapi-events-misc.h"
 
 #define RTC_DR      0x00    /* Data read register */
 #define RTC_MR      0x04    /* Match register */
-- 
2.25.1

Re: [PATCH v2 1/3] qapi: Move RTC_CHANGE back out of target schema

[Philippe Mathieu-Daudé]

On 21/2/22 20:21, Peter Maydell wrote:
> This commit effectively reverts commit 183e4281a30962, which moved
> the RTC_CHANGE event to the target schema.  That change was an
> attempt to make the event target-specific to improve introspection,
> but the event isn't really target-specific: it's machine or device
> specific.  Putting RTC_CHANGE in the target schema with an ifdef list
> reduces maintainability (by adding an if: list with a long list of
> targets that needs to be manually updated as architectures are added
> or removed or as new devices gain the RTC_CHANGE functionality) and
> increases compile time (by preventing RTC devices which emit the
> event from being "compile once" rather than "compile once per
> target", because qapi-events-misc-target.h uses TARGET_* ifdefs,
> which are poisoned in "compile once" files.)
> 
> Move RTC_CHANGE back to misc.json.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Acked-by: Greg Kurz <groug@kaod.org>
> ---
>   qapi/misc-target.json | 33 ---------------------------------
>   qapi/misc.json        | 22 ++++++++++++++++++++++
>   hw/ppc/spapr_rtc.c    |  2 +-
>   hw/rtc/mc146818rtc.c  |  2 +-
>   hw/rtc/pl031.c        |  2 +-
>   5 files changed, 25 insertions(+), 36 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 2/3] qapi: Document some missing details of RTC_CHANGE event

[Peter Maydell]

The RTC_CHANGE event's documentation is missing some details:
 * the offset argument is in units of seconds
 * it isn't guaranteed that the RTC will implement the event

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
v1->v2: add the "RTC might not implement this" note
---
 qapi/misc.json | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 7a70eaa3ffc..0ab235e41f7 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -533,10 +533,12 @@
 #
 # Emitted when the guest changes the RTC time.
 #
-# @offset: offset between base RTC clock (as specified by -rtc base), and
-#          new RTC clock value
+# @offset: offset in seconds between base RTC clock (as specified
+#          by -rtc base), and new RTC clock value
 #
 # Note: This event is rate-limited.
+#       It is not guaranteed that the RTC in the system implements
+#       this event, or even that the system has an RTC at all.
 #
 # Since: 0.13
 #
-- 
2.25.1

Re: [PATCH v2 2/3] qapi: Document some missing details of RTC_CHANGE event

[Philippe Mathieu-Daudé]

On 21/2/22 20:21, Peter Maydell wrote:
> The RTC_CHANGE event's documentation is missing some details:
>   * the offset argument is in units of seconds
>   * it isn't guaranteed that the RTC will implement the event
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: add the "RTC might not implement this" note
> ---
>   qapi/misc.json | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 2/3] qapi: Document some missing details of RTC_CHANGE event

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> The RTC_CHANGE event's documentation is missing some details:
>  * the offset argument is in units of seconds
>  * it isn't guaranteed that the RTC will implement the event
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: add the "RTC might not implement this" note
> ---
>  qapi/misc.json | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 7a70eaa3ffc..0ab235e41f7 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -533,10 +533,12 @@
>  #
>  # Emitted when the guest changes the RTC time.
>  #
> -# @offset: offset between base RTC clock (as specified by -rtc base), and
> -#          new RTC clock value
> +# @offset: offset in seconds between base RTC clock (as specified
> +#          by -rtc base), and new RTC clock value
>  #
>  # Note: This event is rate-limited.
> +#       It is not guaranteed that the RTC in the system implements
> +#       this event, or even that the system has an RTC at all.
>  #
>  # Since: 0.13
>  #

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 3/3] hw/rtc: Compile pl031 once-only

[Peter Maydell]

Now that the RTC_CHANGE event is no longer target-specific,
we can move the pl031 back to a compile-once source file
rather than a compile-per-target one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/rtc/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
index 8fd8d8f9a71..7cecdee5ddb 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'))
-specific_ss.add(when: 'CONFIG_PL031', if_true: files('pl031.c'))
+softmmu_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'))
-- 
2.25.1

Re: [PATCH v2 3/3] hw/rtc: Compile pl031 once-only

[Philippe Mathieu-Daudé]

On 21/2/22 20:21, Peter Maydell wrote:
> Now that the RTC_CHANGE event is no longer target-specific,
> we can move the pl031 back to a compile-once source file
> rather than a compile-per-target one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/rtc/meson.build | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 0/3] qapi: Move RTC_CHANGE back out of target schema

[Eric Auger]

Hi Peter,

On 2/21/22 8:21 PM, Peter Maydell wrote:
> This patchset moves RTC_CHANGE back to misc.json, effectively
> reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
> the target schema.  That change was an attempt to make the event
> target-specific to improve introspection, but the event isn't really
> target-specific: it's machine or device specific.  Putting RTC_CHANGE
> in the target schema with an ifdef list reduces maintainability (by
> adding an if: list with a long list of targets that needs to be
> manually updated as architectures are added or removed or as new
> devices gain the RTC_CHANGE functionality) and increases compile time
> (by preventing RTC devices which emit the event from being "compile
> once" rather than "compile once per target", because
> qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
> "compile once" files.)
> 
> Patch 2 fixes some minor documentation issues for the RTC_CHANGE
> event, noticed during development of the patchset.
> 
> Patch 3 makes the pl031 a build-once file again, which was the
> initial motivation for the series.
> 
> Changes v1->v2:
>  * patch 1 needs to change the #include line for pl031.c, now that
>    the change to make that device emit RTC_CHANGE has landed upstream
>  * patch 2 now also notes in the RTC_CHANGE documentation that
>    not all devices/systems will emit the event (suggested by Markus)
>  * patch 3 is new

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

Thanks!

Eric
> 
> My default assumption is that this series will go in via the
> qapi tree; let me know if you'd prefer me to take it via
> target-arm instead.
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   qapi: Move RTC_CHANGE back out of target schema
>   qapi: Document some missing details of RTC_CHANGE event
>   hw/rtc: Compile pl031 once-only
> 
>  qapi/misc-target.json | 33 ---------------------------------
>  qapi/misc.json        | 24 ++++++++++++++++++++++++
>  hw/ppc/spapr_rtc.c    |  2 +-
>  hw/rtc/mc146818rtc.c  |  2 +-
>  hw/rtc/pl031.c        |  2 +-
>  hw/rtc/meson.build    |  2 +-
>  6 files changed, 28 insertions(+), 37 deletions(-)
> 

[PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Markus Armbruster]

Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
the RTC supports the event).  What if there's more than one RTC?
Which one changed?  New @qom-path identifies it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
RFC because it's compile-tested only.  Worthwhile?  Let me know what you
think.

 qapi/misc.json       | 4 +++-
 hw/ppc/spapr_rtc.c   | 4 +++-
 hw/rtc/mc146818rtc.c | 3 ++-
 hw/rtc/pl031.c       | 3 ++-
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 0ab235e41f..b83cc39029 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -536,6 +536,8 @@
 # @offset: offset in seconds between base RTC clock (as specified
 #          by -rtc base), and new RTC clock value
 #
+# @qom-path: path to the RTC object in the QOM tree
+#
 # Note: This event is rate-limited.
 #       It is not guaranteed that the RTC in the system implements
 #       this event, or even that the system has an RTC at all.
@@ -550,4 +552,4 @@
 #
 ##
 { 'event': 'RTC_CHANGE',
-  'data': { 'offset': 'int' } }
+  'data': { 'offset': 'int', 'qom-path': 'str' } }
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 79677cf550..d55b4b0c50 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -97,6 +97,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, SpaprMachineState *spapr,
                                  uint32_t nret, target_ulong rets)
 {
     SpaprRtcState *rtc = &spapr->rtc;
+    g_autofree const char *qom_path = NULL;
     struct tm tm;
     time_t new_s;
     int64_t host_ns;
@@ -120,7 +121,8 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, SpaprMachineState *spapr,
     }
 
     /* Generate a monitor event for the change */
-    qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
+    qom_path = object_get_canonical_path(OBJECT(rtc));
+    qapi_event_send_rtc_change(qemu_timedate_diff(&tm), qom_path);
 
     host_ns = qemu_clock_get_ns(rtc_clock);
 
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 57c514e15c..ac9a60c90e 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -611,12 +611,13 @@ static void rtc_get_time(RTCState *s, struct tm *tm)
 static void rtc_set_time(RTCState *s)
 {
     struct tm tm;
+    g_autofree const char *qom_path = object_get_canonical_path(OBJECT(s));
 
     rtc_get_time(s, &tm);
     s->base_rtc = mktimegm(&tm);
     s->last_update = qemu_clock_get_ns(rtc_clock);
 
-    qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
+    qapi_event_send_rtc_change(qemu_timedate_diff(&tm), qom_path);
 }
 
 static void rtc_set_cmos(RTCState *s, const struct tm *tm)
diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c
index 60167c778f..b01d0e75d1 100644
--- a/hw/rtc/pl031.c
+++ b/hw/rtc/pl031.c
@@ -138,12 +138,13 @@ static void pl031_write(void * opaque, hwaddr offset,
 
     switch (offset) {
     case RTC_LR: {
+        g_autofree const char *qom_path = object_get_canonical_path(opaque);
         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));
+        qapi_event_send_rtc_change(qemu_timedate_diff(&tm), qom_path);
 
         pl031_set_alarm(s);
         break;
-- 
2.35.1

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Philippe Mathieu-Daudé]

Hi Markus,

On 22/2/22 13:02, Markus Armbruster wrote:
> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
> the RTC supports the event).  What if there's more than one RTC?

w.r.t. RTC, a machine having multiple RTC devices is silly...

Assuming one wants to emulate that; shouldn't all QMP events have a
qom-path by default? Or have a generic "event-from-multiple-sources"
flag which automatically add this field?

> Which one changed?  New @qom-path identifies it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> RFC because it's compile-tested only.  Worthwhile?  Let me know what you
> think.
> 
>   qapi/misc.json       | 4 +++-
>   hw/ppc/spapr_rtc.c   | 4 +++-
>   hw/rtc/mc146818rtc.c | 3 ++-
>   hw/rtc/pl031.c       | 3 ++-
>   4 files changed, 10 insertions(+), 4 deletions(-)

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Peter Maydell]

On Tue, 22 Feb 2022 at 12:56, Philippe Mathieu-Daudé
<philippe.mathieu.daude@gmail.com> wrote:
> On 22/2/22 13:02, Markus Armbruster wrote:
> > Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
> > the RTC supports the event).  What if there's more than one RTC?
>
> w.r.t. RTC, a machine having multiple RTC devices is silly...

I don't think we have any examples in the tree currently, but
I bet real hardware like that does exist: the most plausible
thing would be a board where there's an RTC built into the SoC
but the board designers put an external RTC on the board (perhaps
because it was better/more accurate/easier to make battery-backed).

In fact, here's an old bug report from a user trying to get
their Debian system to use the battery-backed RTC as the
"real" one rather than the non-battery-backed RTC device
that's also part of the arm board they're using:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785445

-- PMM

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Philippe Mathieu-Daudé]

On 22/2/22 14:06, Peter Maydell wrote:
> On Tue, 22 Feb 2022 at 12:56, Philippe Mathieu-Daudé
> <philippe.mathieu.daude@gmail.com> wrote:
>> On 22/2/22 13:02, Markus Armbruster wrote:
>>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
>>> the RTC supports the event).  What if there's more than one RTC?
>>
>> w.r.t. RTC, a machine having multiple RTC devices is silly...
> 
> I don't think we have any examples in the tree currently, but
> I bet real hardware like that does exist: the most plausible
> thing would be a board where there's an RTC built into the SoC
> but the board designers put an external RTC on the board (perhaps
> because it was better/more accurate/easier to make battery-backed).
> 
> In fact, here's an old bug report from a user trying to get
> their Debian system to use the battery-backed RTC as the
> "real" one rather than the non-battery-backed RTC device
> that's also part of the arm board they're using:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785445

OK, thanks for this pointer.

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Cédric Le Goater]

On 2/22/22 14:06, Peter Maydell wrote:
> On Tue, 22 Feb 2022 at 12:56, Philippe Mathieu-Daudé
> <philippe.mathieu.daude@gmail.com> wrote:
>> On 22/2/22 13:02, Markus Armbruster wrote:
>>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
>>> the RTC supports the event).  What if there's more than one RTC?
>>
>> w.r.t. RTC, a machine having multiple RTC devices is silly...
> 
> I don't think we have any examples in the tree currently, but
> I bet real hardware like that does exist: the most plausible
> thing would be a board where there's an RTC built into the SoC
> but the board designers put an external RTC on the board (perhaps
> because it was better/more accurate/easier to make battery-backed).

Yes. like Aspeed machines.

C.


> 
> In fact, here's an old bug report from a user trying to get
> their Debian system to use the battery-backed RTC as the
> "real" one rather than the non-battery-backed RTC device
> that's also part of the arm board they're using:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785445
> 
> -- PMM

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Markus Armbruster]

Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes:

> Hi Markus,
>
> On 22/2/22 13:02, Markus Armbruster wrote:
>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
>> the RTC supports the event).  What if there's more than one RTC?
>
> w.r.t. RTC, a machine having multiple RTC devices is silly...
>
> Assuming one wants to emulate that; shouldn't all QMP events have a
> qom-path by default? Or have a generic "event-from-multiple-sources"
> flag which automatically add this field?

Not all events originate from a device, or even a QOM object.

The ones that do could all use a qom-path member, I guess.

[...]

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Philippe Mathieu-Daudé]

On 22/2/22 16:04, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes:
> 
>> Hi Markus,
>>
>> On 22/2/22 13:02, Markus Armbruster wrote:
>>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
>>> the RTC supports the event).  What if there's more than one RTC?
>>
>> w.r.t. RTC, a machine having multiple RTC devices is silly...
>>
>> Assuming one wants to emulate that; shouldn't all QMP events have a
>> qom-path by default? Or have a generic "event-from-multiple-sources"
>> flag which automatically add this field?
> 
> Not all events originate from a device, or even a QOM object.

OK.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> The ones that do could all use a qom-path member, I guess.
> 
> [...]
> 

Re: [PATCH RFC 4/4] rtc: Have event RTC_CHANGE identify the RTC by QOM path

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and
> the RTC supports the event).  What if there's more than one RTC?
> Which one changed?  New @qom-path identifies it.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> RFC because it's compile-tested only.  Worthwhile?  Let me know what you
> think.

Passes manual testing with mc146818rtc.

Re: [PATCH v2 0/3] qapi: Move RTC_CHANGE back out of target schema

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> This patchset moves RTC_CHANGE back to misc.json, effectively
> reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
> the target schema.  That change was an attempt to make the event
> target-specific to improve introspection, but the event isn't really
> target-specific: it's machine or device specific.  Putting RTC_CHANGE
> in the target schema with an ifdef list reduces maintainability (by
> adding an if: list with a long list of targets that needs to be
> manually updated as architectures are added or removed or as new
> devices gain the RTC_CHANGE functionality) and increases compile time
> (by preventing RTC devices which emit the event from being "compile
> once" rather than "compile once per target", because
> qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
> "compile once" files.)
>
> Patch 2 fixes some minor documentation issues for the RTC_CHANGE
> event, noticed during development of the patchset.
>
> Patch 3 makes the pl031 a build-once file again, which was the
> initial motivation for the series.

Queued including my PATCH 4.  Happy to unqueue it if there are
objections, or the entire thing if you'd rather take it through the ARM
tree.  Thanks!