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

[Markus Armbruster <armbru@redhat.com>]

Daniil Tatianin <d-tatianin@yandex-team.ru> writes:

> 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>

[...]

>>>> 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.

[...]

>>> 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?

Putting "mc146818" right into the command name is fine with me.
Rephrasing the doc comment to say "all MC146818 RTC devices" then makes
sense, and removes the need for a "Bug: clause".

With "mc146818" in the command name, I don't see the need for
"-broadcast".  The fact that it applies to all MC146818 RTCs feels like
detail to me.  In particular since there's usually exactly one.  Still
important enough to spell out in documentation, but I doub't it's
important enough to warrant a mention in the command name.

I have doubts on replacing the commands action "inject-irq" by the
action's purpose "force-sync".  What the guest does with the IRQ is
entirely up to guest software.  Common guest software sets the system
clock from the RTC hardware clock.  But it's really up to the guest.

What about mc146818-inject-irq?

>> (I'm trying to not spread the problems we already have with
>> @rtc-reset-reinjection).

Well, we are adding to them no matter how we name the command.  We're
just more honest about it :)

[...]