On 29/5/24 14:03, Markus Armbruster wrote:
Daniil Tatianin <d-tatia...@yandex-team.ru> writes:
This can be used to force-synchronize the time in guest after a long
stop-cont pause, which can be useful for serverless-type workload.
Also add a comment to highlight the fact that this (and one other QMP
command) only works for the MC146818 RTC controller.
Acked-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Signed-off-by: Daniil Tatianin <d-tatia...@yandex-team.ru>
---
Changes since v0:
- Rename to rtc-inject-irq to match other similar API
- Add a comment to highlight that this only works for the I386 RTC
Changes since v1:
- Added a description below the QMP command to explain how it can be
used and what it does.
Changes since v2:
- Add a 'broadcast' suffix.
- Change the comments to explain the flags we're setting.
- Change the command description to fix styling & explain that it's a broadcast
command.
Changes since v3:
- Fix checkpatch complaints about usage of C99 comments
---
hw/rtc/mc146818rtc.c | 20 ++++++++++++++++++++
include/hw/rtc/mc146818rtc.h | 1 +
qapi/misc-target.json | 19 +++++++++++++++++++
3 files changed, 40 insertions(+)
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 3379f92748..96ecd43036 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -107,6 +107,11 @@ static void rtc_coalesced_timer_update(MC146818RtcState *s)
static QLIST_HEAD(, MC146818RtcState) rtc_devices =
QLIST_HEAD_INITIALIZER(rtc_devices);
+/*
+ * NOTE:
+ * The two QMP functions below are _only_ implemented for the MC146818.
+ * All other RTC devices ignore this.
+ */
void qmp_rtc_reset_reinjection(Error **errp)
{
MC146818RtcState *s;
@@ -116,6 +121,21 @@ void qmp_rtc_reset_reinjection(Error **errp)
}
}
+void qmp_rtc_inject_irq_broadcast(Error **errp)
+{
+ MC146818RtcState *s;
+
+ QLIST_FOREACH(s, &rtc_devices, link) {
+ /* Update-ended interrupt enable */
+ s->cmos_data[RTC_REG_B] |= REG_B_UIE;
+
+ /* Interrupt request flag | update interrupt flag */
+ s->cmos_data[RTC_REG_C] |= REG_C_IRQF | REG_C_UF;
+
+ qemu_irq_raise(s->irq);
+ }
+}
+
static bool rtc_policy_slew_deliver_irq(MC146818RtcState *s)
{
kvm_reset_irq_delivered();
diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
index 97cec0b3e8..e9dd0f9c72 100644
--- a/include/hw/rtc/mc146818rtc.h
+++ b/include/hw/rtc/mc146818rtc.h
@@ -56,5 +56,6 @@ MC146818RtcState *mc146818_rtc_init(ISABus *bus, int
base_year,
void mc146818rtc_set_cmos_data(MC146818RtcState *s, int addr, int val);
int mc146818rtc_get_cmos_data(MC146818RtcState *s, int addr);
void qmp_rtc_reset_reinjection(Error **errp);
+void qmp_rtc_inject_irq_broadcast(Error **errp);
#endif /* HW_RTC_MC146818RTC_H */
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4e0a6492a9..7d388a3753 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -19,6 +19,25 @@
{ 'command': 'rtc-reset-reinjection',
'if': 'TARGET_I386' }
+##
+# @rtc-inject-irq-broadcast:
+#
+# Inject an RTC interrupt for all existing RTCs on the system.
+# The interrupt forces the guest to synchronize the time with RTC.
+# This is useful after a long stop-cont pause, which is common for
+# serverless-type workload.
In previous version you said:
> This isn't really related to migration though. Serverless is based
> on constantly stopping and resuming the VM on e.g. every HTTP
> request to an endpoint.
Which made some sense. Maybe mention HTTP? And point to that use case
(possibly with QMP commands) in the commit description?
Make that "workloads".
"For all existing RTCs" is a lie. It's really just all mc146818s. The
command works as documented only as long as the VM has no other RTCs.
@rtc-mc146818-force-sync-broadcast sounds clearer & safer;
IIUC the command goal, mentioning IRQ injection is irrelevant
(implementation detail).
(I'm trying to not spread the problems we already have with
@rtc-reset-reinjection).
+#
+# Since: 9.1
+#
+# Example:
+#
+# -> { "execute": "rtc-inject-irq-broadcast" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'rtc-inject-irq-broadcast',
+ 'if': 'TARGET_I386' }
The conditional kind-of-sort-of ensures "VM has no other RTCs":
TARGET_I386 compiles only this file in hw/rtc/, and therefore can't have
other RTCs (unless they're hiding in some other directory). Brittle.
When we move to single binary, we will compile in other RTCs. How can
we ensure "VM has no nother RTCs" then? What if one of these other RTCs
can be added with -device or device_add?
When this falls apart because the VM does have other RTCs, it can only
do so silently: the command can't tell us for which RTCs it actually
injected an interrupt.
Documentation making promises the implementation doesn't actually
deliver can only end in tears. The only reason I'm not rejecting this
patch out of hand is the existing and similarly broken
rtc-reset-reinjection.
I'm willing to reluctantly accept it with honest documentation.
Perhaps: "Bug: RTCs other than mc146818rtc are silently ignored."
Much, much better would be an interface that's actually usable with
multiple RTCs. We'd have to talk how interrupt injection could be used
with such a machine.
Anything less will likely need to be replaced later on.
+
##
# @SevState:
#
