Re: [PATCH v4] mc146818rtc: add a way to generate RTC interrupts via QMP

[Daniil Tatianin]

On 5/29/24 3:36 PM, Philippe Mathieu-Daudé wrote:

> On 29/5/24 14:03, Markus Armbruster wrote:
> > Daniil Tatianin <d-tatianin@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é <philmd@linaro.org>
> > > Signed-off-by: Daniil Tatianin <d-tatianin@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?

Hmm, maybe it would be helpful for people who don't know what serverless 
means.

How about:
    This is useful after a long stop-const pause, which is common for 
serverless-type workloads,
    e.g. stopping/resuming the VM on every HTTP request to an endpoint, 
which might involve
    a long pause in between the requests, causing time drift in the guest.

> > 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 like this if Markus is okay with that. If we go with this, would it 
make sense to drop the "Bug" clause?

> (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:
> > >   #