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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4719 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 6338 bytes --]