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

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

Hi Daniil,

On 21/5/24 10:08, Daniil Tatianin wrote:
> Could you please take a look at this revision? I think I've taken 
> everyone's feedback into account.

Sorry for the delay, I missed your patch since you didn't Cc me
(Markus asked me to look at this).

Thanks for addressing the previous requests.

> Thank you!
> 
> On 5/14/24 9:57 AM, Daniil Tatianin wrote:
>> ping :)
>> 06.05.2024, 11:34, "Daniil Tatianin" <d-tatianin@yandex-team.ru>:
>>
>>     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
>>
>>     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.
>>
>>     ---
>>      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..2b3754f5c6 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

This doesn't pass the checkpatch script because it isn't QEMU coding
style:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#use-the-qemu-coding-style

>>     + 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' }
>>

Your new command doesn't make my life harder than the current
'rtc-reset-reinjection' command, so if this is useful to you,
I'm OK to:
Acked-by: Philippe Mathieu-Daudé <philmd@linaro.org>
(assuming the comment style is fixed).

I'll see later how to deal with that with heterogeneous
emulation.

Regards,

Phil.

>>     +##
>>     +# @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.
>>     +#
>>     +# Since: 9.1
>>     +#
>>     +# Example:
>>     +#
>>     +# -> { "execute": "rtc-inject-irq-broadcast" }
>>     +# <- { "return": {} }
>>     +#
>>     +##
>>     +{ 'command': 'rtc-inject-irq-broadcast',
>>     + 'if': 'TARGET_I386' }
>>     +
>>      ##
>>      # @SevState:
>>      #
>>
>>     --
>>     2.34.1
>>