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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 29/5/24 16:34, Markus Armbruster wrote:
> Daniil Tatianin <d-tatianin@yandex-team.ru> writes:
> 
>> On 5/29/24 4:39 PM, Philippe Mathieu-Daudé wrote:
>>
>>> On 29/5/24 14:43, Daniil Tatianin wrote:
>>>> 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/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.
>>>
>>> Please help me understand your workflow. Your management layer call
>>> @stop and @cont QMP commands, is that right?
>>
>> Yes, that is correct.
>>
>>> @cont will emit a @RESUME event.
>>>
>>> If we could listen to QAPI events from C code, we could have the
>>> mc146818rtc device automatically sync on VM resume, and no need for
>>> this async command.
>>
>> Perhaps? I'm not sure how that would be implemented, but let's see what Markus has to say.
> 
> You can't listen on an event in QEMU itself.  You can only hook into the
> place that generates the event.

Apparently "qemu/notify.h" could be use for QAPI events (currently
only used by migration). Big change, to be discussed later.

> The RESUME event is sent from vm_prepare_start() in system/cpus.c.

Good spot, it is where we call synchronize_pre_resume() for vCPUs,
which is exactly what Daniil wants for RTC devices.

I'd rather we call here rtc_synchronize_pre_resume(), which would
mostly be qmp_rtc_inject_irq_broadcast() content, without using QMP
at all.

But for back-compat we need some CLI option "sync-rtc-on-resume"
default to false. Preferably a mc146818rtc property to KISS.

That would solve Daniil problem and make Markus/myself happier.

Paolo, any objection?

Regards,

Phil.