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

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

On 29/4/24 11:43, Daniil Tatianin wrote:
> On 4/29/24 12:40 PM, Philippe Mathieu-Daudé wrote:
> 
>> On 29/4/24 11:34, Daniil Tatianin wrote:
>>> On 4/29/24 11:51 AM, 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.
>>>>>
>>>>> 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
>>>>>
>>>>> ---
>>>>>   hw/rtc/mc146818rtc.c         | 20 ++++++++++++++++++++
>>>>>   include/hw/rtc/mc146818rtc.h |  1 +
>>>>>   qapi/misc-target.json        | 16 ++++++++++++++++
>>>>>   3 files changed, 37 insertions(+)
>>>>>
>>>>> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>>>>> index f4c1869232..8501b55cbd 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(Error **errp)
>>>>> +{
>>>>> +    MC146818RtcState *s;
>>>>> +
>>>>> +    /*
>>>>> +     * See:
>>>>> +     * 
>>>>> https://www.kernel.org/doc/Documentation/virtual/kvm/timekeeping.txt
>>>>> +     */
>>>>> +    QLIST_FOREACH(s, &rtc_devices, link) {
>>>>> +        s->cmos_data[RTC_REG_B] |= REG_B_UIE;
>>>>> +        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..6cd9761d80 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(Error **errp);
>>>>>   #endif /* HW_RTC_MC146818RTC_H */
>>>>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>>>>> index 4e0a6492a9..d84a5d07a2 100644
>>>>> --- a/qapi/misc-target.json
>>>>> +++ b/qapi/misc-target.json
>>>>> @@ -19,6 +19,22 @@
>>>>>   { 'command': 'rtc-reset-reinjection',
>>>>>     'if': 'TARGET_I386' }
>>>>> +##
>>>>> +# @rtc-inject-irq:
>>>>> +#
>>>>> +# Inject an RTC interrupt.
>>>> Your cover letter explains what this could be good for.  Would it make
>>>> sense to explain it here, too?
>>>
>>> Sure, sounds useful. I'll add a description in the next version.
>>
>> Please also see my comments on the previous patch:
>> https://lore.kernel.org/qemu-devel/11c78645-e87b-4a43-8191-a73540c364a9@linaro.org/
>>
> I think this makes sense, but there's already a similar command, which 
> doesn't do it. Should that one be changed as well then? Or do we only 
> change the interface for this one?

Better to not follow a short sighted interface. If you can, start
with a correct one. Help fixing broken interface is certainly
welcomed, but that shouldn't block adding your new command.

Regards,

Phil.