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

[Philippe Mathieu-Daudé]

(+Peter who has more experience on such design).

On 29/4/24 13:32, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > Hi Daniil, Markus,
> >
> > On 26/4/24 10:39, 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.

What is a "serverless-type workload"?

> > > > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > ---
> > > >    hw/rtc/mc146818rtc.c         | 15 +++++++++++++++
> > > >    include/hw/rtc/mc146818rtc.h |  1 +
> > > >    qapi/misc-target.json        | 16 ++++++++++++++++
> > > >    3 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> > > > index f4c1869232..6980a78d5f 100644
> > > > --- a/hw/rtc/mc146818rtc.c
> > > > +++ b/hw/rtc/mc146818rtc.c
> > > > @@ -116,6 +116,21 @@ void qmp_rtc_reset_reinjection(Error **errp)
> > > >        }
> > > >    }
> > > >    +void qmp_rtc_notify(Error **errp)
> > > > +{
> > > > +    MC146818RtcState *s;
> > > > +
> > > > +    /*
> > > > +     * See:
> > > > +     * https://www.kernel.org/doc/Documentation/virtual/kvm/timekeeping.txt

What part of this document explains why this change is required?
I probably missed it. Explaining it here briefly would be more
useful.

> > > > +     */
> > > > +    QLIST_FOREACH(s, &rtc_devices, link) {
> > > > +        s->cmos_data[RTC_REG_B] |= REG_B_UIE;
                                      // Update-ended interrupt enable

> > > > +        s->cmos_data[RTC_REG_C] |= REG_C_IRQF | REG_C_UF;
                                      // interrupt request flag
                                      //           update interrupt flag

> > > > +        qemu_irq_raise(s->irq);
> > > > +    }
> > > > +}
> > > > +
> > > Note for later: qmp_rtc_notify() works on all realized mc146818rtc
> > > devices.  Other kinds of RTC devices are silently ignored.  Just like
> > > qmp_rtc_reset_reinjection().
> >
> > IMO to avoid any future ambiguity (in heterogeneous machines), this
> > command must take a QOM device path (or a list of) and only notify
> > those.
>
> Let's compare:
>
> • With QOM path:
>
>    · You need to know the machine's RTC device(s).
>
>      Unfortunately, this is bothersome, as the QOM path is not stable.

But we'll need more of that with dynamic machines...

>      For Q35, it's generally "/machine/unattached/device[N]/rtc", but N
>      varies with configuration (TCG N=2, KVM N=3 for me), and it might
>      vary with machine type version.  That's because the machine code
>      creates ICH9-LPC without a proper name.  We do that a lot.  I hate
>      it.
>
>      Likewise for i440FX with PIIX3 instead of ICH9-LPC.
>
>      For isapc, it's /machine/unattached/device[3].  I suspect the 3
>      isn't reliable there, either.
>
>      microvm doesn't seem to have an RTC by default.
>
>    · If the device so named doesn't support IRQ inject, the command
>      should fail.

Yes, why the management app would want to run this command if there
are not RTC on the machine?

>    · Could be generalized to non-RTC devices when that's useful.
>
> • Broadcast:
>
>    · You don't need to know the machine's RTC device(s).
>
>    · If there are multiple RTC devices that support IRQ inject, we inject
>      for each of them.  There is no way to select specific RTCs.
>
>    · If there is no RTC device that supports IRQ inject, the command does
>      nothing silently.
>
>      I don't like silent failures.  It could be made to fail instead.
>
> If it wasn't for the unstable QOM path problem, I'd advise against
> the broadcast interface.
>
> Thoughts?

Something bugs me in this patch but I couldn't figure out what I am
missing. The issue is when migrated VM is restored. I don't get why
the behavior depends on an external decision (via external management
transport). Don't we have post_load() hooks for such tuning?
This device implements it in rtc_post_load().

Regards,

Phil.

PD: BTW tomorrow community call could be a good opportunity to discuss
this.