| Message ID | 20220221192123.749970-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | qapi: Move RTC_CHANGE back out of target schema | expand |
Hi Peter, On 2/21/22 8:21 PM, Peter Maydell wrote: > This patchset moves RTC_CHANGE back to misc.json, effectively > reverting commit 183e4281a30962, which moved the RTC_CHANGE event to > the target schema. That change was an attempt to make the event > target-specific to improve introspection, but the event isn't really > target-specific: it's machine or device specific. Putting RTC_CHANGE > in the target schema with an ifdef list reduces maintainability (by > adding an if: list with a long list of targets that needs to be > manually updated as architectures are added or removed or as new > devices gain the RTC_CHANGE functionality) and increases compile time > (by preventing RTC devices which emit the event from being "compile > once" rather than "compile once per target", because > qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in > "compile once" files.) > > Patch 2 fixes some minor documentation issues for the RTC_CHANGE > event, noticed during development of the patchset. > > Patch 3 makes the pl031 a build-once file again, which was the > initial motivation for the series. > > Changes v1->v2: > * patch 1 needs to change the #include line for pl031.c, now that > the change to make that device emit RTC_CHANGE has landed upstream > * patch 2 now also notes in the RTC_CHANGE documentation that > not all devices/systems will emit the event (suggested by Markus) > * patch 3 is new Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks! Eric > > My default assumption is that this series will go in via the > qapi tree; let me know if you'd prefer me to take it via > target-arm instead. > > thanks > -- PMM > > Peter Maydell (3): > qapi: Move RTC_CHANGE back out of target schema > qapi: Document some missing details of RTC_CHANGE event > hw/rtc: Compile pl031 once-only > > qapi/misc-target.json | 33 --------------------------------- > qapi/misc.json | 24 ++++++++++++++++++++++++ > hw/ppc/spapr_rtc.c | 2 +- > hw/rtc/mc146818rtc.c | 2 +- > hw/rtc/pl031.c | 2 +- > hw/rtc/meson.build | 2 +- > 6 files changed, 28 insertions(+), 37 deletions(-) >
Hi Markus, On 22/2/22 13:02, Markus Armbruster wrote: > Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and > the RTC supports the event). What if there's more than one RTC? w.r.t. RTC, a machine having multiple RTC devices is silly... Assuming one wants to emulate that; shouldn't all QMP events have a qom-path by default? Or have a generic "event-from-multiple-sources" flag which automatically add this field? > Which one changed? New @qom-path identifies it. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > RFC because it's compile-tested only. Worthwhile? Let me know what you > think. > > qapi/misc.json | 4 +++- > hw/ppc/spapr_rtc.c | 4 +++- > hw/rtc/mc146818rtc.c | 3 ++- > hw/rtc/pl031.c | 3 ++- > 4 files changed, 10 insertions(+), 4 deletions(-)
On Tue, 22 Feb 2022 at 12:56, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > On 22/2/22 13:02, Markus Armbruster wrote: > > Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and > > the RTC supports the event). What if there's more than one RTC? > > w.r.t. RTC, a machine having multiple RTC devices is silly... I don't think we have any examples in the tree currently, but I bet real hardware like that does exist: the most plausible thing would be a board where there's an RTC built into the SoC but the board designers put an external RTC on the board (perhaps because it was better/more accurate/easier to make battery-backed). In fact, here's an old bug report from a user trying to get their Debian system to use the battery-backed RTC as the "real" one rather than the non-battery-backed RTC device that's also part of the arm board they're using: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785445 -- PMM
Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes: > Hi Markus, > > On 22/2/22 13:02, Markus Armbruster wrote: >> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and >> the RTC supports the event). What if there's more than one RTC? > > w.r.t. RTC, a machine having multiple RTC devices is silly... > > Assuming one wants to emulate that; shouldn't all QMP events have a > qom-path by default? Or have a generic "event-from-multiple-sources" > flag which automatically add this field? Not all events originate from a device, or even a QOM object. The ones that do could all use a qom-path member, I guess. [...]
On 22/2/22 16:04, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes: > >> Hi Markus, >> >> On 22/2/22 13:02, Markus Armbruster wrote: >>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and >>> the RTC supports the event). What if there's more than one RTC? >> >> w.r.t. RTC, a machine having multiple RTC devices is silly... >> >> Assuming one wants to emulate that; shouldn't all QMP events have a >> qom-path by default? Or have a generic "event-from-multiple-sources" >> flag which automatically add this field? > > Not all events originate from a device, or even a QOM object. OK. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > The ones that do could all use a qom-path member, I guess. > > [...] >
On 22/2/22 14:06, Peter Maydell wrote: > On Tue, 22 Feb 2022 at 12:56, Philippe Mathieu-Daudé > <philippe.mathieu.daude@gmail.com> wrote: >> On 22/2/22 13:02, Markus Armbruster wrote: >>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and >>> the RTC supports the event). What if there's more than one RTC? >> >> w.r.t. RTC, a machine having multiple RTC devices is silly... > > I don't think we have any examples in the tree currently, but > I bet real hardware like that does exist: the most plausible > thing would be a board where there's an RTC built into the SoC > but the board designers put an external RTC on the board (perhaps > because it was better/more accurate/easier to make battery-backed). > > In fact, here's an old bug report from a user trying to get > their Debian system to use the battery-backed RTC as the > "real" one rather than the non-battery-backed RTC device > that's also part of the arm board they're using: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785445 OK, thanks for this pointer.
Markus Armbruster <armbru@redhat.com> writes: > Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and > the RTC supports the event). What if there's more than one RTC? > Which one changed? New @qom-path identifies it. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > RFC because it's compile-tested only. Worthwhile? Let me know what you > think. Passes manual testing with mc146818rtc.
On 2/22/22 14:06, Peter Maydell wrote: > On Tue, 22 Feb 2022 at 12:56, Philippe Mathieu-Daudé > <philippe.mathieu.daude@gmail.com> wrote: >> On 22/2/22 13:02, Markus Armbruster wrote: >>> Event RTC_CHANGE is "emitted when the guest changes the RTC time" (and >>> the RTC supports the event). What if there's more than one RTC? >> >> w.r.t. RTC, a machine having multiple RTC devices is silly... > > I don't think we have any examples in the tree currently, but > I bet real hardware like that does exist: the most plausible > thing would be a board where there's an RTC built into the SoC > but the board designers put an external RTC on the board (perhaps > because it was better/more accurate/easier to make battery-backed). Yes. like Aspeed machines. C. > > In fact, here's an old bug report from a user trying to get > their Debian system to use the battery-backed RTC as the > "real" one rather than the non-battery-backed RTC device > that's also part of the arm board they're using: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785445 > > -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > This patchset moves RTC_CHANGE back to misc.json, effectively > reverting commit 183e4281a30962, which moved the RTC_CHANGE event to > the target schema. That change was an attempt to make the event > target-specific to improve introspection, but the event isn't really > target-specific: it's machine or device specific. Putting RTC_CHANGE > in the target schema with an ifdef list reduces maintainability (by > adding an if: list with a long list of targets that needs to be > manually updated as architectures are added or removed or as new > devices gain the RTC_CHANGE functionality) and increases compile time > (by preventing RTC devices which emit the event from being "compile > once" rather than "compile once per target", because > qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in > "compile once" files.) > > Patch 2 fixes some minor documentation issues for the RTC_CHANGE > event, noticed during development of the patchset. > > Patch 3 makes the pl031 a build-once file again, which was the > initial motivation for the series. Queued including my PATCH 4. Happy to unqueue it if there are objections, or the entire thing if you'd rather take it through the ARM tree. Thanks!