Hi On Fri, Sep 24, 2021 at 4:21 PM Markus Armbruster <arm...@redhat.com> wrote:
> Peter Maydell <peter.mayd...@linaro.org> writes: > > > On Mon, 18 Feb 2019 at 14:19, Markus Armbruster <arm...@redhat.com> > wrote: > >> > >> From: Marc-André Lureau <marcandre.lur...@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.lur...@redhat.com> > >> Reviewed-by: Markus Armbruster <arm...@redhat.com> > >> Signed-off-by: Markus Armbruster <arm...@redhat.com> > >> Message-Id: <20190214152251.2073-19-arm...@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.