From: Marc-André Lureau <marcandre.lureau@redhat.com>
A few targets don't emit RTC_CHANGE, we could restrict the event to
the tagets that do emit it.
Note: There is a lot more of events & commands that we could restrict
to capable targets, with the cost of some additional complexity, but
the benefit of added correctness and better introspection.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190214152251.2073-19-armbru@redhat.com>
---
hw/ppc/spapr_rtc.c | 2 +-
hw/timer/mc146818rtc.c | 2 +-
qapi/misc.json | 23 -----------------------
qapi/target.json | 23 +++++++++++++++++++++++
4 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index cd049f389d..eb95a7077d 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -31,7 +31,7 @@
#include "sysemu/sysemu.h"
#include "hw/ppc/spapr.h"
#include "qapi/error.h"
-#include "qapi/qapi-events-misc.h"
+#include "qapi/qapi-events-target.h"
#include "qemu/cutils.h"
void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index bc1862b6fc..513f105e62 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -32,7 +32,7 @@
#include "hw/timer/mc146818rtc.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-target.h"
-#include "qapi/qapi-events-misc.h"
+#include "qapi/qapi-events-target.h"
#include "qapi/visitor.h"
#include "exec/address-spaces.h"
diff --git a/qapi/misc.json b/qapi/misc.json
index 98f59f828a..8b3ca4fdd3 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -2876,29 +2876,6 @@
{ 'event': 'ACPI_DEVICE_OST',
'data': { 'info': 'ACPIOSTInfo' } }
-##
-# @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 that value will be different depending
-# on clock chosen to drive RTC (specified by -rtc clock).
-#
-# Note: This event is rate-limited.
-#
-# Since: 0.13.0
-#
-# Example:
-#
-# <- { "event": "RTC_CHANGE",
-# "data": { "offset": 78 },
-# "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
-#
-##
-{ 'event': 'RTC_CHANGE',
- 'data': { 'offset': 'int' } }
-
##
# @ReplayMode:
#
diff --git a/qapi/target.json b/qapi/target.json
index 5c41a0aee7..da7b4be51e 100644
--- a/qapi/target.json
+++ b/qapi/target.json
@@ -7,6 +7,29 @@
{ 'include': 'misc.json' }
+##
+# @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.0
+#
+# Example:
+#
+# <- { "event": "RTC_CHANGE",
+# "data": { "offset": 78 },
+# "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
+#
+##
+{ 'event': 'RTC_CHANGE',
+ 'data': { 'offset': 'int' },
+ 'if': 'defined(TARGET_ALPHA) || defined(TARGET_ARM) || defined(TARGET_HPPA) || defined(TARGET_I386) || defined(TARGET_MIPS) || defined(TARGET_MIPS64) || defined(TARGET_MOXIE) || defined(TARGET_PPC) || defined(TARGET_PPC64) || defined(TARGET_S390X) || defined(TARGET_SH4) || defined(TARGET_SPARC)' }
+
##
# @rtc-reset-reinjection:
#
--
2.17.2
On Mon, 18 Feb 2019 at 14:19, Markus Armbruster <armbru@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> A few targets don't emit RTC_CHANGE, we could restrict the event to
> the tagets that do emit it.
>
> Note: There is a lot more of events & commands that we could restrict
> to capable targets, with the cost of some additional complexity, but
> the benefit of added correctness and better introspection.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <20190214152251.2073-19-armbru@redhat.com>
Hi; I've just run into this starting from Eric's patch to add
RTC_CHANGE event support to the pl031 RTC. It seems kind of
awkward to me:
> diff --git a/qapi/target.json b/qapi/target.json
> index 5c41a0aee7..da7b4be51e 100644
> --- a/qapi/target.json
> +++ b/qapi/target.json
> @@ -7,6 +7,29 @@
>
> { 'include': 'misc.json' }
>
> +##
> +# @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.0
> +#
> +# Example:
> +#
> +# <- { "event": "RTC_CHANGE",
> +# "data": { "offset": 78 },
> +# "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
> +#
> +##
> +{ 'event': 'RTC_CHANGE',
> + 'data': { 'offset': 'int' },
> + 'if': 'defined(TARGET_ALPHA) || defined(TARGET_ARM) || defined(TARGET_HPPA) || defined(TARGET_I386) || defined(TARGET_MIPS) || defined(TARGET_MIPS64) || defined(TARGET_MOXIE) || defined(TARGET_PPC) || defined(TARGET_PPC64) || defined(TARGET_S390X) || defined(TARGET_SH4) || defined(TARGET_SPARC)' }
> +
Now we have a massive list of TARGET if conditions. As a general
principle if we can avoid long TARGET if-lists we should, because
it is yet another thing that needs updating when a new target
is added. In this case any new architecture that can handle an
ISA device would need to update this list. I pretty much guarantee
nobody is going to remember to do that.
It also doesn't actually usefully tell the thing on the other
end whether it can expect to see RTC_CHANGE events, because
whether it will actually get them depends not on the target
architecture but on the specific combination of machine type
and plugged-in devices. If there's a need for the other end of
the QMP connection to tell in advance whether it's going to get
RTC_CHANGE events then we should probably have an event or
something for that, and make all rtc-change aware RTC devices
cause QMP to send that event on startup (or otherwise register
themselves as being present).
It also means that now rtc devices that emit this event need to
change in meson.build from softmmu_ss to specific_ss, because the
qapi_event_send_rtc_change() prototype is in the generated
qapi/qapi-events-misc-target.h header, and that header uses
TARGET_* defines which are poisoned for softmmu compiles.
So instead of being able to build the RTC device once for
all targets, we need to build it over and over again for
each target.
Could we reconsider this change? It seems to me to be adding
complexity and build time and I don't really see the advantage
that compensates for that.
-- PMM
On 23/09/21 15:14, Peter Maydell wrote: > > It also means that now rtc devices that emit this event need to > change in meson.build from softmmu_ss to specific_ss, because the > qapi_event_send_rtc_change() prototype is in the generated > qapi/qapi-events-misc-target.h header, and that header uses > TARGET_* defines which are poisoned for softmmu compiles. > So instead of being able to build the RTC device once for > all targets, we need to build it over and over again for > each target. > > Could we reconsider this change? It seems to me to be adding > complexity and build time and I don't really see the advantage > that compensates for that. I agree, RTC_CHANGE is part of the management API. If only some devices implement it, that's only laziness and not an intrinsic property of the event. In some sense, lack of support for RTC_CHANGE is an incomplete transition and this patch is making it harder to complete it. Paolo
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 18 Feb 2019 at 14:19, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> A few targets don't emit RTC_CHANGE, we could restrict the event to
>> the tagets that do emit it.
>>
>> Note: There is a lot more of events & commands that we could restrict
>> to capable targets, with the cost of some additional complexity, but
>> the benefit of added correctness and better introspection.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Message-Id: <20190214152251.2073-19-armbru@redhat.com>
>
> Hi; I've just run into this starting from Eric's patch to add
> RTC_CHANGE event support to the pl031 RTC. It seems kind of
> awkward to me:
>
>> diff --git a/qapi/target.json b/qapi/target.json
>> index 5c41a0aee7..da7b4be51e 100644
>> --- a/qapi/target.json
>> +++ b/qapi/target.json
>> @@ -7,6 +7,29 @@
>>
>> { 'include': 'misc.json' }
>>
>> +##
>> +# @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.0
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "RTC_CHANGE",
>> +# "data": { "offset": 78 },
>> +# "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>> +#
>> +##
>> +{ 'event': 'RTC_CHANGE',
>> + 'data': { 'offset': 'int' },
>> + 'if': 'defined(TARGET_ALPHA) || defined(TARGET_ARM) || defined(TARGET_HPPA) || defined(TARGET_I386) || defined(TARGET_MIPS) || defined(TARGET_MIPS64) || defined(TARGET_MOXIE) || defined(TARGET_PPC) || defined(TARGET_PPC64) || defined(TARGET_S390X) || defined(TARGET_SH4) || defined(TARGET_SPARC)' }
>> +
>
> Now we have a massive list of TARGET if conditions. As a general
> principle if we can avoid long TARGET if-lists we should, because
> it is yet another thing that needs updating when a new target
> is added.
On the other hand, we really want to have the schema reflect
target-specifity, to make it visible in introspection, as the commit
message says. However, ...
> In this case any new architecture that can handle an
> ISA device would need to update this list. I pretty much guarantee
> nobody is going to remember to do that.
>
> It also doesn't actually usefully tell the thing on the other
> end whether it can expect to see RTC_CHANGE events, because
> whether it will actually get them depends not on the target
> architecture but on the specific combination of machine type
> and plugged-in devices. If there's a need for the other end of
> the QMP connection to tell in advance whether it's going to get
> RTC_CHANGE events then we should probably have an event or
> something for that, and make all rtc-change aware RTC devices
> cause QMP to send that event on startup (or otherwise register
> themselves as being present).
>
> It also means that now rtc devices that emit this event need to
> change in meson.build from softmmu_ss to specific_ss, because the
> qapi_event_send_rtc_change() prototype is in the generated
> qapi/qapi-events-misc-target.h header, and that header uses
> TARGET_* defines which are poisoned for softmmu compiles.
> So instead of being able to build the RTC device once for
> all targets, we need to build it over and over again for
> each target.
... this isn't really *target*-specific, it's *device*-specific: some
devices implement the event, some don't.
Ideally, we'd just fix that.
If we can't, we should document which devices don't implement it. Since
users typically get these devices indirectly via machine type, we should
also document which machine types are affected.
We may need to make the "RTC_CHANGE not implemented" bit observable in
QMP somehow. I'd ignore that until we have a use case.
> Could we reconsider this change? It seems to me to be adding
> complexity and build time and I don't really see the advantage
> that compensates for that.
No objection. Marc-André?
On Fri, 24 Sept 2021 at 13:21, Markus Armbruster <armbru@redhat.com> wrote: > ... this isn't really *target*-specific, it's *device*-specific: some > devices implement the event, some don't. > > Ideally, we'd just fix that. Would you want to tell the far end "this machine simply does not have an RTC device at all (because the hardware it's emulating doesn't have one) and so you won't get RTC_CHANGE events" ? A good first step for getting more devices to implement the RTC_CHANGE support would be if there was any documentation on how to do it. The JSON schema says the offset should be "offset between base RTC clock (as specified by -rtc base), and new RTC clock value", but there aren't any hints (either there or elsewhere) as to how a device is supposed to determine that value, and there's no documentation of what the behaviour or intent is of the qemu_timedate_diff() function that the existing implementations use to calculate the offset. Side note: probably the JSON schema should document the units for 'offset'. Code inspection suggests it wants seconds. Side side note: the JSON event doesn't seem to contemplate the possibility that a machine might have more than one RTC... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Fri, 24 Sept 2021 at 13:21, Markus Armbruster <armbru@redhat.com> wrote:
>> ... this isn't really *target*-specific, it's *device*-specific: some
>> devices implement the event, some don't.
>>
>> Ideally, we'd just fix that.
>
> Would you want to tell the far end "this machine simply does
> not have an RTC device at all (because the hardware it's emulating
> doesn't have one) and so you won't get RTC_CHANGE events" ?
Well, RTC_CHANGE is "Emitted when the guest changes the RTC time." If
the guest doesn't *have* an RTC...
> A good first step for getting more devices to implement the
> RTC_CHANGE support would be if there was any documentation on how
> to do it. The JSON schema says the offset should be "offset between
> base RTC clock (as specified by -rtc base), and new RTC clock value",
> but there aren't any hints (either there or elsewhere) as to how a
> device is supposed to determine that value, and there's no
> documentation of what the behaviour or intent is of the
> qemu_timedate_diff() function that the existing implementations
> use to calculate the offset.
RTC_CHANGE is from the bad old times, I'm afraid:
commit 80cd34787fc0fc31b1a341c7b8d8e729c1b6ea58
Author: Luiz Capitulino <lcapitulino@redhat.com>
Date: Thu Feb 25 12:11:44 2010 -0300
QMP: Introduce RTC_CHANGE event
Emitted whenever the RTC time changes.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
No hint at why or how to use it.
I figure this is the matching libvirt commit:
commit 32e6ac9c2601e19715d18f743cf805a3466d3385
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Thu Mar 18 18:28:15 2010 +0000
Add support for an explicit RTC change event
This introduces a new event type
VIR_DOMAIN_EVENT_ID_RTC_CHANGE
This event includes the new UTC offset measured in seconds.
Thus there is a new callback definition for this event type
typedef void (*virConnectDomainEventRTCChangeCallback)(virConnectPtr conn,
virDomainPtr dom,
long long utcoffset,
void *opaque);
If the guest XML configuration for the <clock> is set to
offset='variable', then the XML will automatically be
updated with the new UTC offset value. This ensures that
during migration/save/restore the new offset is preserved.
* daemon/remote.c: Dispatch RTC change events to client
* examples/domain-events/events-c/event-test.c: Watch for
RTC change events
* include/libvirt/libvirt.h.in: Define new RTC change event ID
and callback signature
* src/conf/domain_event.c, src/conf/domain_event.h,
src/libvirt_private.syms: Extend API to handle RTC change events
* src/qemu/qemu_driver.c: Connect to the QEMU monitor event
for RTC changes and emit a libvirt RTC change event
* src/remote/remote_driver.c: Receive and dispatch RTC change
events to application
* src/remote/remote_protocol.x: Wire protocol definition for
RTC change events
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h,
src/qemu/qemu_monitor_json.c: Watch for RTC_CHANGE event
from QEMU monitor
Suggests it might be needed for migration.
How today's libvirt uses RTC_CHANGE would be good to know. I don't have
the time to ferret it out myself. Daniel, do you know? If not, who
else might?
> Side note: probably the JSON schema should document the units
> for 'offset'. Code inspection suggests it wants seconds.
Doc bug, patch would be lovely.
> Side side note: the JSON event doesn't seem to contemplate
> the possibility that a machine might have more than one RTC...
Right. It clearly needs an additional member @qom-path identifying the
RTC device.
On Fri, Sep 24, 2021 at 03:35:52PM +0200, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Fri, 24 Sept 2021 at 13:21, Markus Armbruster <armbru@redhat.com> wrote: > >> ... this isn't really *target*-specific, it's *device*-specific: some > >> devices implement the event, some don't. > >> > >> Ideally, we'd just fix that. > > > > Would you want to tell the far end "this machine simply does > > not have an RTC device at all (because the hardware it's emulating > > doesn't have one) and so you won't get RTC_CHANGE events" ? > > Well, RTC_CHANGE is "Emitted when the guest changes the RTC time." If > the guest doesn't *have* an RTC... > > > A good first step for getting more devices to implement the > > RTC_CHANGE support would be if there was any documentation on how > > to do it. The JSON schema says the offset should be "offset between > > base RTC clock (as specified by -rtc base), and new RTC clock value", > > but there aren't any hints (either there or elsewhere) as to how a > > device is supposed to determine that value, and there's no > > documentation of what the behaviour or intent is of the > > qemu_timedate_diff() function that the existing implementations > > use to calculate the offset. > > RTC_CHANGE is from the bad old times, I'm afraid: > > commit 80cd34787fc0fc31b1a341c7b8d8e729c1b6ea58 > Author: Luiz Capitulino <lcapitulino@redhat.com> > Date: Thu Feb 25 12:11:44 2010 -0300 > > QMP: Introduce RTC_CHANGE event > > Emitted whenever the RTC time changes. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > No hint at why or how to use it. > > I figure this is the matching libvirt commit: > > commit 32e6ac9c2601e19715d18f743cf805a3466d3385 > Author: Daniel P. Berrangé <berrange@redhat.com> > Date: Thu Mar 18 18:28:15 2010 +0000 > > Add support for an explicit RTC change event > > This introduces a new event type > > VIR_DOMAIN_EVENT_ID_RTC_CHANGE > > This event includes the new UTC offset measured in seconds. > Thus there is a new callback definition for this event type > > typedef void (*virConnectDomainEventRTCChangeCallback)(virConnectPtr conn, > virDomainPtr dom, > long long utcoffset, > void *opaque); > > If the guest XML configuration for the <clock> is set to > offset='variable', then the XML will automatically be > updated with the new UTC offset value. This ensures that > during migration/save/restore the new offset is preserved. > > * daemon/remote.c: Dispatch RTC change events to client > * examples/domain-events/events-c/event-test.c: Watch for > RTC change events > * include/libvirt/libvirt.h.in: Define new RTC change event ID > and callback signature > * src/conf/domain_event.c, src/conf/domain_event.h, > src/libvirt_private.syms: Extend API to handle RTC change events > * src/qemu/qemu_driver.c: Connect to the QEMU monitor event > for RTC changes and emit a libvirt RTC change event > * src/remote/remote_driver.c: Receive and dispatch RTC change > events to application > * src/remote/remote_protocol.x: Wire protocol definition for > RTC change events > * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h, > src/qemu/qemu_monitor_json.c: Watch for RTC_CHANGE event > from QEMU monitor > > Suggests it might be needed for migration. > > How today's libvirt uses RTC_CHANGE would be good to know. I don't have > the time to ferret it out myself. Daniel, do you know? If not, who > else might? Libvirt puts it into an event and emits it to client applications. When booting a guest it is possible to give an RTC offset for the guest. If you shut it off and want to cold boot it again later, you really want to preserve the RTC offset that the guest might have updated. Thus an application like oVirt listens for the RTC change event and remembers this offset for subsequent boots. This is especially important for Windows because they keep the RTC in localtime, so twice a year they change the RTC and you want to keep track of that. > > Side side note: the JSON event doesn't seem to contemplate > > the possibility that a machine might have more than one RTC... > > Right. It clearly needs an additional member @qom-path identifying the > RTC device. Are there (mainstream) machines with more than one RTC ? The original use case only cared about x86 machines and my knowledge of other targets is minimal. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 24 Sept 2021 at 15:43, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Sep 24, 2021 at 03:35:52PM +0200, Markus Armbruster wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > Side side note: the JSON event doesn't seem to contemplate > > > the possibility that a machine might have more than one RTC... > > > > Right. It clearly needs an additional member @qom-path identifying the > > RTC device. > > Are there (mainstream) machines with more than one RTC ? Probably not many. The most likely case is probably "SoC has a crappy RTC and the board designer put a better one on the board". https://www.mail-archive.com/tech@openbsd.org/msg45128.html has a mention of some rockchip-based board like that. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 24 Sept 2021 at 15:43, Daniel P. Berrangé <berrange@redhat.com> wrote: >> >> On Fri, Sep 24, 2021 at 03:35:52PM +0200, Markus Armbruster wrote: >> > Peter Maydell <peter.maydell@linaro.org> writes: >> > > Side side note: the JSON event doesn't seem to contemplate >> > > the possibility that a machine might have more than one RTC... >> > >> > Right. It clearly needs an additional member @qom-path identifying the >> > RTC device. >> >> Are there (mainstream) machines with more than one RTC ? > > Probably not many. The most likely case is probably "SoC has > a crappy RTC and the board designer put a better one on the board". > https://www.mail-archive.com/tech@openbsd.org/msg45128.html > has a mention of some rockchip-based board like that. Adding @qom-path is the right thing to do. Simply ignore it when you know the VM has just one RTC, or that the guest will use just one.
Hi
On Fri, Sep 24, 2021 at 4:21 PM Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 18 Feb 2019 at 14:19, Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> A few targets don't emit RTC_CHANGE, we could restrict the event to
> >> the tagets that do emit it.
> >>
> >> Note: There is a lot more of events & commands that we could restrict
> >> to capable targets, with the cost of some additional complexity, but
> >> the benefit of added correctness and better introspection.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> Message-Id: <20190214152251.2073-19-armbru@redhat.com>
> >
> > Hi; I've just run into this starting from Eric's patch to add
> > RTC_CHANGE event support to the pl031 RTC. It seems kind of
> > awkward to me:
> >
> >> diff --git a/qapi/target.json b/qapi/target.json
> >> index 5c41a0aee7..da7b4be51e 100644
> >> --- a/qapi/target.json
> >> +++ b/qapi/target.json
> >> @@ -7,6 +7,29 @@
> >>
> >> { 'include': 'misc.json' }
> >>
> >> +##
> >> +# @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.0
> >> +#
> >> +# Example:
> >> +#
> >> +# <- { "event": "RTC_CHANGE",
> >> +# "data": { "offset": 78 },
> >> +# "timestamp": { "seconds": 1267020223, "microseconds": 435656
> } }
> >> +#
> >> +##
> >> +{ 'event': 'RTC_CHANGE',
> >> + 'data': { 'offset': 'int' },
> >> + 'if': 'defined(TARGET_ALPHA) || defined(TARGET_ARM) ||
> defined(TARGET_HPPA) || defined(TARGET_I386) || defined(TARGET_MIPS) ||
> defined(TARGET_MIPS64) || defined(TARGET_MOXIE) || defined(TARGET_PPC) ||
> defined(TARGET_PPC64) || defined(TARGET_S390X) || defined(TARGET_SH4) ||
> defined(TARGET_SPARC)' }
> >> +
> >
> > Now we have a massive list of TARGET if conditions. As a general
> > principle if we can avoid long TARGET if-lists we should, because
> > it is yet another thing that needs updating when a new target
> > is added.
>
> On the other hand, we really want to have the schema reflect
> target-specifity, to make it visible in introspection, as the commit
> message says. However, ...
>
> > In this case any new architecture that can handle an
> > ISA device would need to update this list. I pretty much guarantee
> > nobody is going to remember to do that.
> >
> > It also doesn't actually usefully tell the thing on the other
> > end whether it can expect to see RTC_CHANGE events, because
> > whether it will actually get them depends not on the target
> > architecture but on the specific combination of machine type
> > and plugged-in devices. If there's a need for the other end of
> > the QMP connection to tell in advance whether it's going to get
> > RTC_CHANGE events then we should probably have an event or
> > something for that, and make all rtc-change aware RTC devices
> > cause QMP to send that event on startup (or otherwise register
> > themselves as being present).
> >
> > It also means that now rtc devices that emit this event need to
> > change in meson.build from softmmu_ss to specific_ss, because the
> > qapi_event_send_rtc_change() prototype is in the generated
> > qapi/qapi-events-misc-target.h header, and that header uses
> > TARGET_* defines which are poisoned for softmmu compiles.
> > So instead of being able to build the RTC device once for
> > all targets, we need to build it over and over again for
> > each target.
>
> ... this isn't really *target*-specific, it's *device*-specific: some
> devices implement the event, some don't.
>
> Ideally, we'd just fix that.
>
> If we can't, we should document which devices don't implement it. Since
> users typically get these devices indirectly via machine type, we should
> also document which machine types are affected.
>
> We may need to make the "RTC_CHANGE not implemented" bit observable in
> QMP somehow. I'd ignore that until we have a use case.
>
> > Could we reconsider this change? It seems to me to be adding
> > complexity and build time and I don't really see the advantage
> > that compensates for that.
>
> No objection. Marc-André?
>
Not at this point. As hinted in the commit note, the patch was more of an
RFC, I wasn't so sure about it either.
On Fri, 24 Sept 2021 at 13:29, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > On Fri, Sep 24, 2021 at 4:21 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> > Could we reconsider this change? It seems to me to be adding >> > complexity and build time and I don't really see the advantage >> > that compensates for that. >> >> No objection. Marc-André? > > > Not at this point. As hinted in the commit note, the patch was more of an RFC, I wasn't so sure about it either. Thanks. I'll send out a patch that rolls back this change, probably this afternoon. -- PMM
© 2016 - 2026 Red Hat, Inc.