Re: [Qemu-devel] [PULL 18/18] qapi: move RTC_CHANGE to the target schema

[Peter Maydell]

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