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

Commit Message

Daniil Tatianin May 28, 2024, 7:22 a.m. UTC

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é <[e-mail]>
Signed-off-by: Daniil Tatianin <[e-mail]>
---

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(+)

Comments

Markus Armbruster May 29, 2024, 12:03 p.m. UTC | #1

Daniil Tatianin <[e-mail]> 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é <[e-mail]>
> Signed-off-by: Daniil Tatianin <[e-mail]>
> ---
>
> 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/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 3379f92748..96ecd43036 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 */
> +        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' }
>  
> +##
> +# @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.

> +#
> +# Since: 9.1
> +#
> +# Example:
> +#
> +#     -> { "execute": "rtc-inject-irq-broadcast" }
> +#     <- { "return": {} }
> +#
> +##
> +{ 'command': 'rtc-inject-irq-broadcast',
> +  'if': 'TARGET_I386' }

The conditional kind-of-sort-of ensures "VM has no other RTCs":
TARGET_I386 compiles only this file in hw/rtc/, and therefore can't have
other RTCs (unless they're hiding in some other directory).  Brittle.

When we move to single binary, we will compile in other RTCs.  How can
we ensure "VM has no nother RTCs" then?  What if one of these other RTCs
can be added with -device or device_add?

When this falls apart because the VM does have other RTCs, it can only
do so silently: the command can't tell us for which RTCs it actually
injected an interrupt.

Documentation making promises the implementation doesn't actually
deliver can only end in tears.  The only reason I'm not rejecting this
patch out of hand is the existing and similarly broken
rtc-reset-reinjection.

I'm willing to reluctantly accept it with honest documentation.
Perhaps: "Bug: RTCs other than mc146818rtc are silently ignored."

Much, much better would be an interface that's actually usable with
multiple RTCs.  We'd have to talk how interrupt injection could be used
with such a machine.

Anything less will likely need to be replaced later on.

> +
>  ##
>  # @SevState:
>  #

Daniil Tatianin May 29, 2024, 12:31 p.m. UTC | #2

Thanks for the review Markus!

I will fix the wording and add a "Bug:" clause for the next revision.

On 5/29/24 3:03 PM, Markus Armbruster wrote:
> Daniil Tatianin <[e-mail]> 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é <[e-mail]>
>> Signed-off-by: Daniil Tatianin <[e-mail]>
>> ---
>>
>> 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/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>> index 3379f92748..96ecd43036 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 */
>> +        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' }
>>   
>> +##
>> +# @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.
>
>> +#
>> +# Since: 9.1
>> +#
>> +# Example:
>> +#
>> +#     -> { "execute": "rtc-inject-irq-broadcast" }
>> +#     <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'rtc-inject-irq-broadcast',
>> +  'if': 'TARGET_I386' }
> The conditional kind-of-sort-of ensures "VM has no other RTCs":
> TARGET_I386 compiles only this file in hw/rtc/, and therefore can't have
> other RTCs (unless they're hiding in some other directory).  Brittle.
>
> When we move to single binary, we will compile in other RTCs.  How can
> we ensure "VM has no nother RTCs" then?  What if one of these other RTCs
> can be added with -device or device_add?
>
> When this falls apart because the VM does have other RTCs, it can only
> do so silently: the command can't tell us for which RTCs it actually
> injected an interrupt.
>
> Documentation making promises the implementation doesn't actually
> deliver can only end in tears.  The only reason I'm not rejecting this
> patch out of hand is the existing and similarly broken
> rtc-reset-reinjection.
>
> I'm willing to reluctantly accept it with honest documentation.
> Perhaps: "Bug: RTCs other than mc146818rtc are silently ignored."
>
> Much, much better would be an interface that's actually usable with
> multiple RTCs.  We'd have to talk how interrupt injection could be used
> with such a machine.
>
> Anything less will likely need to be replaced later on.
>
>> +
>>   ##
>>   # @SevState:
>>   #

Philippe Mathieu-Daudé May 29, 2024, 12:36 p.m. UTC | #3

On 29/5/24 14:03, Markus Armbruster wrote:
> Daniil Tatianin <[e-mail]> 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é <[e-mail]>
>> Signed-off-by: Daniil Tatianin <[e-mail]>
>> ---
>>
>> 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/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>> index 3379f92748..96ecd43036 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 */
>> +        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' }
>>   
>> +##
>> +# @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?

> 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'm trying to not spread the problems we already have with
@rtc-reset-reinjection).

>> +#
>> +# Since: 9.1
>> +#
>> +# Example:
>> +#
>> +#     -> { "execute": "rtc-inject-irq-broadcast" }
>> +#     <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'rtc-inject-irq-broadcast',
>> +  'if': 'TARGET_I386' }
> 
> The conditional kind-of-sort-of ensures "VM has no other RTCs":
> TARGET_I386 compiles only this file in hw/rtc/, and therefore can't have
> other RTCs (unless they're hiding in some other directory).  Brittle.
> 
> When we move to single binary, we will compile in other RTCs.  How can
> we ensure "VM has no nother RTCs" then?  What if one of these other RTCs
> can be added with -device or device_add?
> 
> When this falls apart because the VM does have other RTCs, it can only
> do so silently: the command can't tell us for which RTCs it actually
> injected an interrupt.
> 
> Documentation making promises the implementation doesn't actually
> deliver can only end in tears.  The only reason I'm not rejecting this
> patch out of hand is the existing and similarly broken
> rtc-reset-reinjection.
> 
> I'm willing to reluctantly accept it with honest documentation.
> Perhaps: "Bug: RTCs other than mc146818rtc are silently ignored."
> 
> Much, much better would be an interface that's actually usable with
> multiple RTCs.  We'd have to talk how interrupt injection could be used
> with such a machine.
> 
> Anything less will likely need to be replaced later on.
> 
>> +
>>   ##
>>   # @SevState:
>>   #
>

Daniil Tatianin May 29, 2024, 12:43 p.m. UTC | #4

On 5/29/24 3:36 PM, Philippe Mathieu-Daudé wrote:

> On 29/5/24 14:03, Markus Armbruster wrote:
>> Daniil Tatianin <[e-mail]> 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é <[e-mail]>
>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>> ---
>>>
>>> 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/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>>> index 3379f92748..96ecd43036 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 */
>>> +        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' }
>>>   +##
>>> +# @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.

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

> (I'm trying to not spread the problems we already have with
> @rtc-reset-reinjection).
>
>>> +#
>>> +# Since: 9.1
>>> +#
>>> +# Example:
>>> +#
>>> +#     -> { "execute": "rtc-inject-irq-broadcast" }
>>> +#     <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'rtc-inject-irq-broadcast',
>>> +  'if': 'TARGET_I386' }
>>
>> The conditional kind-of-sort-of ensures "VM has no other RTCs":
>> TARGET_I386 compiles only this file in hw/rtc/, and therefore can't have
>> other RTCs (unless they're hiding in some other directory). Brittle.
>>
>> When we move to single binary, we will compile in other RTCs. How can
>> we ensure "VM has no nother RTCs" then?  What if one of these other RTCs
>> can be added with -device or device_add?
>>
>> When this falls apart because the VM does have other RTCs, it can only
>> do so silently: the command can't tell us for which RTCs it actually
>> injected an interrupt.
>>
>> Documentation making promises the implementation doesn't actually
>> deliver can only end in tears.  The only reason I'm not rejecting this
>> patch out of hand is the existing and similarly broken
>> rtc-reset-reinjection.
>>
>> I'm willing to reluctantly accept it with honest documentation.
>> Perhaps: "Bug: RTCs other than mc146818rtc are silently ignored."
>>
>> Much, much better would be an interface that's actually usable with
>> multiple RTCs.  We'd have to talk how interrupt injection could be used
>> with such a machine.
>>
>> Anything less will likely need to be replaced later on.
>>
>>> +
>>>   ##
>>>   # @SevState:
>>>   #
>>
>

Philippe Mathieu-Daudé May 29, 2024, 1:39 p.m. UTC | #5

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 <[e-mail]> 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é <[e-mail]>
>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>> ---
>>>>
>>>> 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?

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

I'll let our QAPI expert enlighten me on this :)

Daniil Tatianin May 29, 2024, 1:51 p.m. UTC | #6

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 <[e-mail]> 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é <[e-mail]>
>>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>>> ---
>>>>>
>>>>> 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.

>
> I'll let our QAPI expert enlighten me on this :)

Markus Armbruster May 29, 2024, 2:28 p.m. UTC | #7

Daniil Tatianin <[e-mail]> writes:

> On 5/29/24 3:36 PM, Philippe Mathieu-Daudé wrote:
>
>> On 29/5/24 14:03, Markus Armbruster wrote:
>>> Daniil Tatianin <[e-mail]> 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é <[e-mail]>
>>>> Signed-off-by: Daniil Tatianin <[e-mail]>

[...]

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

[...]

Markus Armbruster May 29, 2024, 2:34 p.m. UTC | #8

Daniil Tatianin <[e-mail]> 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 <[e-mail]> 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é <[e-mail]>
>>>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>>>> ---
>>>>>>
>>>>>> 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.

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

>> I'll let our QAPI expert enlighten me on this :)

Philippe Mathieu-Daudé May 29, 2024, 3:27 p.m. UTC | #9

On 29/5/24 16:34, Markus Armbruster wrote:
> Daniil Tatianin <[e-mail]> 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 <[e-mail]> 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é <[e-mail]>
>>>>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.

Daniil Tatianin June 17, 2024, 8:27 a.m. UTC | #10

On 5/29/24 6:27 PM, Philippe Mathieu-Daudé wrote:

> On 29/5/24 16:34, Markus Armbruster wrote:
>> Daniil Tatianin <[e-mail]> 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 <[e-mail]> 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é <[e-mail]>
>>>>>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> 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?
>
Hey there!

Since Paolo never replied I'm going to take that as a no then. Is 
everyone else okay with this idea?

If there are no objections I'm going to try and implement this.

Thanks!

> Regards,
>
> Phil.

Daniil Tatianin July 17, 2024, 7:57 a.m. UTC | #11

On 5/29/24 6:27 PM, Philippe Mathieu-Daudé wrote:

> On 29/5/24 16:34, Markus Armbruster wrote:
>> Daniil Tatianin <[e-mail]> 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 <[e-mail]> 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é <[e-mail]>
>>>>>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> 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.

So I started looking into this, and I'm a bit unsure about what we want 
this API to look like. What I mean is there isn't a generic RTC 
abstraction in QEMU, likewise there isn't an "RTC" global variable you 
can easily use to hook up some sort of API or ops-like functions like 
cpu_accel does.

One simple solution I'm seeing is making an mc146818-specific API like 
mc146818rtc_synchronize_pre_resume(), and call that directly wrapped 
inside an ifdef CONFIG_MC146818RTC inside system/cpus.c. We can then 
check the sync-on-resume property inside of that helper and optionally 
just return from it if it's not set.

Any objections on this approach? Is there a better way to do this?

Thanks!
> Paolo, any objection?
>
> Regards,
>
> Phil.

Daniil Tatianin July 22, 2024, 11:31 a.m. UTC | #12

Ping :)

Any ideas on this?

On 7/17/24 10:57 AM, Daniil Tatianin wrote:
> On 5/29/24 6:27 PM, Philippe Mathieu-Daudé wrote:
>
>> On 29/5/24 16:34, Markus Armbruster wrote:
>>> Daniil Tatianin <[e-mail]> 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 <[e-mail]> 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é <[e-mail]>
>>>>>>>>> Signed-off-by: Daniil Tatianin <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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.
>
> So I started looking into this, and I'm a bit unsure about what we 
> want this API to look like. What I mean is there isn't a generic RTC 
> abstraction in QEMU, likewise there isn't an "RTC" global variable you 
> can easily use to hook up some sort of API or ops-like functions like 
> cpu_accel does.
>
> One simple solution I'm seeing is making an mc146818-specific API like 
> mc146818rtc_synchronize_pre_resume(), and call that directly wrapped 
> inside an ifdef CONFIG_MC146818RTC inside system/cpus.c. We can then 
> check the sync-on-resume property inside of that helper and optionally 
> just return from it if it's not set.
>
> Any objections on this approach? Is there a better way to do this?
>
> Thanks!
>> Paolo, any objection?
>>
>> Regards,
>>
>> Phil.

Patch

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 3379f92748..96ecd43036 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 */
+        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' }
 
+##
+# @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:
 #