Re: [PATCH v7 3/4] virtio-rtc: Add alarm feature

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

On Thu, Feb 20, 2025 at 05:51:23PM +0100, Peter Hilber wrote:
> On Wed, Feb 19, 2025 at 05:08:35PM +0100, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Feb 13, 2025 at 07:13:47PM +0100, Peter Hilber wrote:
> > > On Tue, Feb 11, 2025 at 12:51:54PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > > On Thu, Jan 23, 2025 at 11:16:14AM +0100, Peter Hilber wrote:
> > > > > Add the VIRTIO_RTC_F_ALARM feature (without normative statements).
> > > > > 
> > > > > The intended use case is: A driver needs to react when an alarm time has
> > > > > been reached, but at alarm time, the driver may be in a sleep state or
> > > > > powered off. The alarm feature can resume and notify the driver in this
> > > > > case. Alarms may be retained across device resets (including reset on
> > > > > boot).
> > > > > 
> > > > > Peculiarities
> > > > > -------------
> > > > > 
> > > > > Unlike usual alarm clocks, a virtio-rtc alarm-capable clock may step
> > > > > autonomously at any time: An alarm may change back from "expired" to
> > > > > "not expired" before the driver has started processing an alarm
> > > > > notification.
> > > > > 
> > > > > To address the above, and the device resets, define "alarm expiration"
> > > > > in such a way that the driver always has a chance to react to an alarm,
> > > > > and make the device always responsible for notifying the driver about an
> > > > > alarm expiration.
> > > > > 
> > > > > The VIRTIO_RTC_REQ_SET_ALARM_ENABLED request is there so that the Linux
> > > > > ioctls RTC_AIE_ON and RTC_AIE_OFF only need to emit one request.
> > > > > 
> > > > > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > > > > ---
> > > > > 
> > > > > Notes:
> > > > >     v7:
> > > > >     
> > > > >     - Change flag numeric value due to removing leap second indication.
> > > > >     
> > > > >     v5:
> > > > >     
> > > > >     - Reformat.
> > > > >     
> > > > >     v4:
> > > > >     
> > > > >     - Change requirements so that driver can reset alarm to clean slate, and
> > > > >       document how driver can achieve this (Cornelia Hell, Jason Wang) [1].
> > > > >     
> > > > >     - Require device to support all expressible alarm times.
> > > > >     
> > > > >     - Formatting and wording improvements.
> > > > >     
> > > > >     [1] https://lore.kernel.org/all/2ae67401-a8f5-4686-9321-cb3105df594d@opensynergy.com/
> > > > > 
> 
> [...]
> 
> > > > > +An alarm expiration becomes obsolete
> > > > > +
> > > > > +\begin{itemize}
> > > > > +\item once the clock jumps backwards, before the alarm time, or
> > > > > +
> > > > > +\item once the driver sets an alarm time, or
> > > > > +
> > > > > +\item once another alarm expiration event happens.
> > > > > +\end{itemize}
> > > > > +
> > > > 
> > > > This is a minor comment, I think you can use `when` instead of `once` like
> > > > in the paragraph before.
> > > > 
> > > 
> > > OK.
> > > 
> > > > > +If an alarm expiration becomes obsolete, it is unspecified which alarm
> > > > > +actions the device executes for this alarm expiration, and the device
> > > > > +stops executing these alarm actions after a grace period.
> > > > 
> > > > What is a grace period? You mean that whatever the device does after
> > > > alarm expiration, the device has to STOP doing it after a grace period.
> > > > Am I right?
> > > > 
> > > 
> > > "[An] alarm expiration becomes obsolete" means that the device should no
> > > longer act according to the alarm (typically because the driver disabled
> > > the alarm, or for one of the other reasons listed in the above
> > > enumeration). In this case, the device must stop the alarm actions as
> > > soon as possible (within a finite grace period). 
> > > 
> > > Maybe I could rephrase like this?
> > > 
> > > 	If an alarm expiration becomes obsolete as per the above
> > > 	conditions, it is unspecified which alarm actions the device
> > > 	executes for this alarm expiration, and the device stops
> > > 	executing these alarm actions as soon as possible.
> > > 
> > 
I just meant that the paragraph above does not seem precise so I was
thinking that we could just drop it but I am OK to keep it too.

> > I wonder if we can just drop this and let the device implementation do
> > decide when an alarm is obsolete and what to do in that situation.
> > 
> 
> Are you referring to an obsolete alarm expiration, or to an obsolete
> alarm? (As for the alarm, I would say the device should only consider an
> alarm totally obsolete once the driver disables the alarm, acknowledging
> it.)

I am talking about when an alarm expiration becomes obsolete. I agree
regarding when an alarm becomes obsolete.

> 
> As for the alarm expiration obsoletion text above and the related
> requirements in patch 4, I think they are still required for the
> following:
> 
> 1. The requirement to obsolete alarm expirations when the driver sets a
>    new alarm makes it possible to prevent information leakage, as
>    discussed in [2], per the procedure listed elsewhere in this patch:
> 
> 	+Alarms set prior to reset may cause unwanted alarm expiration
> 	+notifications, and information leakage, after the reset. To prevent both
> 	+issues, the driver can do the following after the reset, for each clock
> 	+which supports alarm:
> 	+
> 	+\begin{enumerate}
> 	+\item Send a VIRTIO_RTC_REQ_SET_ALARM message, with \field{alarm_time}
> 	+        set to 0, and \field{flags} set to 0.
> 	+
> 	+\item Wait until the device marks the VIRTIO_RTC_REQ_SET_ALARM message
> 	+        as used, with status VIRTIO_RTC_S_OK.
> 	+\end{enumerate}
> 	+
> 	+To prevent the above issues, the driver also marks buffers in the alarmq
> 	+as available only after completing the above steps for all clocks.
> 
> 2. Without the requirements to obsolete alarm expirations, it might not
>    be clear how long the device needs to remember to send a notification
>    through the alarmq, in case there is no buffer available in the
>    alarmq.
> 
> > > > > +
> > > > > +The driver-visible settings of an alarm consist of two elements:
> > > > > +
> > > > > +\begin{itemize}
> > > > > +\item \field{driver_alarm_time}, a valid time for the corresponding
> > > > > +        clock, and
> > > > > +
> > > > > +\item \field{alarm_enabled}, a boolean. While \field{alarm_enabled} is
> > > > > +        true, \field{driver_alarm_time} is the actual alarm time.
> > > > > +        While \field{alarm_enabled} is false, the device will act as if
> > > > > +        the alarm time was in the future, so that the alarm will not
> > > > > +        expire.
> > > > > +\end{itemize}
> > > > 
> > > > Is `alarm_enabled` a field that is device implementation specific?
> > > > 
> > > 
> > > No. The use of "\field{}" around alarm_enabled is for typographic
> > > purposes, not because it is supposed to correspond to a particular
> > > element in the device implementation. It is unspecified how the device
> > > implements the alarm feature.
> > > 
> > > The two elements mentioned above describe the state of the alarm in the
> > > device which the driver can set and get through the respective requests.
> > > 
> > > By overriding driver_alarm_time with an alarm time in an unreachable
> > > future if alarm_enabled is false, the spec does not need to consider the
> > > alarm_enabled state in most places. Most non-normative text and most
> > > requirements just need to refer to "alarm time reached", not to "alarm
> > > time reached and alarm enabled".
> > 
> > I see, are you suggesting to replace driver_alarm_time and alarm_enabled
> > occurrences?  
> > 
> > Matias
> > 
> 
> In this version, the occurrences have already been replaced in most
> cases, so to speak. alarm_enabled is only introduced towards the bottom
> of the "Alarm Operation" section. Most of the spec ignores
> alarm_enabled, since alarm_enabled == false has the same effect as alarm
> time == unreachable future. But since conflating alarm time and
> alarm_enabled creates confusion, I now think alarm_enabled should just
> be mentioned across all non-normative and normative sections instead.
> 
> (This version specifies that the driver sets driver_alarm_time and
> alarm_enabled. The spec synthesizes the applicable "alarm time" from
> both, for the purposes of specification: 
> 
> 	While alarm_enabled is true, driver_alarm_time is the actual
> 	alarm time. While alarm_enabled is false, the device will act as
> 	if the alarm time was in the future, so that the alarm will not
> 	expire.
> 
> This version does not require the device implementation to actually
> synthesize the "alarm time".)
> 
> But, as said, now I think I should just refer to alarm_enabled
> everywhere, and drop the driver_alarm_time and "alarm time" distinction.

You can do that and see how it looks in the next version. I think the
issue is just the use of the underscore in the name, which made me think
it is a variable. I do not have an strong opinion about it though.

Matias