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

[Markus Armbruster]

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é?