Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature

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

On Thu, Feb 13, 2025 at 07:14:30PM +0100, Peter Hilber wrote:
> On Tue, Feb 11, 2025 at 01:33:42PM +0100, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Jan 23, 2025 at 11:16:15AM +0100, Peter Hilber wrote:
> > > Add the normative statements for the alarm feature added previously.
> > > 
> > > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > > ---
> > > 
> > > Notes:
> > >     v7:
> > >     
> > >     - Remove inadvertent v6 changes, which should have been part of
> > >       preceding patches.
> > >     
> > >     v4:
> > >     
> > >     - Update normative statements to match v4 changes to non-normative
> > >       statements (patch 3).
> > >     
> > >     - Driver should read clock to confirm that alarm has expired.
> > >     
> > >     - Formatting and wording improvements.
> > > 
> > >  device-types/rtc/description.tex        | 157 ++++++++++++++++++++++++
> > >  device-types/rtc/device-conformance.tex |   4 +
> > >  device-types/rtc/driver-conformance.tex |   2 +
> > >  3 files changed, 163 insertions(+)
> > > 
> > > diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
> > > index 47ad50cd95ca..32fed5f8620f 100644
> > > --- a/device-types/rtc/description.tex
> > > +++ b/device-types/rtc/description.tex
> > > @@ -32,6 +32,11 @@ \subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
> > >  VIRTIO_RTC_F_ALARM determines whether the device supports setting an
> > >  alarm for some of the clocks.
> > >  
> > > +\devicenormative{\subsubsection}{Feature bits}{Device Types / RTC Device / Feature bits}
> > > +
> > > +The device SHOULD offer VIRTIO_RTC_F_ALARM if the device can support
> > > +setting an alarm for any of its clocks.
> > 
> > I think you can remove `can`.
> > 
> 
> OK.
> 
> > > +
> > >  \subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
> > >  
> > >  There is no configuration data for the device.
> > > @@ -715,6 +720,12 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
> > >  this specification, the device MUST use the nanosecond as unit for
> > >  field \field{clock_reading}.
> > >  
> > > +If VIRTIO_RTC_F_ALARM was negotiated, and the device sent an alarm
> > > +notification N for clock C with alarm time A, the device MUST, for all
> > > +read requests of C which the driver marks as available after the device
> > > +marked N as used, return a \field{clock_reading} which does not precede
> > > +A, unless C stepped backwards to before A.
> > > +
> > 
> > I wonder if there is a simpler way to rewrite this paragraph. It is a
> > bit hard to follow.
> > 
> 
> Like this?
> 
>     If the device sent an alarm notification for clock C with alarm time A,
>     the device MUST, for all read requests of C which the driver marks as
>     available after the notification, return a \field{clock_reading} which
>     does not precede A (except if C stepped backwards to before A).
> 

I think it is better.

> > >  \subsubsection{Alarm Operation}\label{sec:Device Types / RTC Device / Device Operation / Alarm Operation}
> > >  
> > >  Through the optional alarm feature, the driver can set an alarm time. On
> > > @@ -828,6 +839,79 @@ \subsubsection{Alarm Operation}\label{sec:Device Types / RTC Device / Device Ope
> > >  To prevent the above issues, the driver also marks buffers in the alarmq
> > >  as available only after completing the above steps for all clocks.
> > >  
> > > +\devicenormative{\paragraph}{Alarm Operation}{Device Types / RTC Device / Device Operation / Alarm Operation}
> > > +
> > > +The device MAY retain both \field{driver_alarm_time} and
> > > +\field{alarm_enabled} of a clock across a device reset.
> > > +
> > > +If the device did not retain \field{driver_alarm_time} and
> > > +\field{alarm_enabled} of a clock across a device reset, the device MUST
> > > +initialize \field{driver_alarm_time} to 0.
> > > +
> > > +If the device did not retain \field{driver_alarm_time} and
> > > +\field{alarm_enabled} of a clock across a device reset, the device MUST
> > > +initialize \field{alarm_enabled} to false.
> > > +
> > > +While \field{alarm_enabled} for a clock is true, the device MUST set the
> > > +alarm time to \field{driver_alarm_time}.
> > > +
> > > +While \field{alarm_enabled} for a clock is false, the device MUST act as
> > > +if the alarm time was in the future.
> > > +
> > > +If VIRTIO_RTC_F_ALARM was negotiated, the device MUST support the alarm
> > > +messages, VIRTIO_RTC_REQ_READ_ALARM, VIRTIO_RTC_REQ_SET_ALARM,
> > > +VIRTIO_RTC_REQ_SET_ALARM_ENABLED, and VIRTIO_RTC_NOTIF_ALARM, for one or
> > > +more clocks.
> > > +
> > > +If VIRTIO_RTC_F_ALARM was not negotiated, the device MUST NOT support the
> > > +alarm messages.
> > > +
> > > +The device MUST set flag VIRTIO_RTC_FLAG_ALARM_CAP in \field{struct
> > > +virtio_rtc_resp_clock_cap.flags} if the respective clock supports alarm
> > > +messages, and clear the flag otherwise.
> > > +
> > > +The device MUST consider it an alarm expiration event when the
> > > +associated clock progresses (also: steps) from a time prior to the alarm
> > > +time to the alarm time, or to a time after the alarm time.
> > > +
> > > +The device MUST consider it an alarm expiration event when the
> > > +driver sets an alarm time which the associated clock has already reached
> > > +or passed.
> > > +
> > > +If the device retained \field{driver_alarm_time} and
> > > +\field{alarm_enabled} of a clock across a device reset, and the clock
> > > +has already reached or passed the alarm time, the device MUST consider
> > > +this device reset an alarm expiration event.
> > > +
> > > +If an alarm expiration event E happens, the device MUST start serving
> > > +the alarm expiration event E.
> > > +
> > > +If the driver successfully requests VIRTIO_RTC_REQ_SET_ALARM, or
> > > +VIRTIO_RTC_REQ_SET_ALARM_ENABLED, for clock C, the device MUST stop
> > > +serving any previous alarm expiration event for C before the device
> > > +marks the message as used.
> > 
> > What do you mean with serving?
> > 
> 
> "Serving an alarm expiration event" means 
> 
> - executing the corresponding alarm actions, and/or 
> 
> - waiting for alarmq notification to become possible.
> 
> That execution is defined in the third and fourth requirement quoted
> below. Maybe these requirements should be moved above the others. Would
> that suffice to clarify?
> 

Oh I see, thanks.

> 
> Thanks for the review,
> 
> Peter
> 
> > > +
> > > +If a clock C steps to a time previous to C's alarm time, the device MUST
> > > +stop serving any previous alarm expiration event for C, within a grace
> > > +period.
> > > +
> > > +If an alarm expiration event happens for clock C, the device MUST stop
> > > +serving any previous alarm expiration event for C, within a grace
> > > +period.
> > > +
> > > +If the device is currently serving an alarm expiration event E, the
> > > +device MUST send a single VIRTIO_RTC_NOTIF_ALARM notification for E, as
> > > +soon as an alarmq buffer is available for this purpose.
> > > +
> > > +While the device is serving an alarm expiration event, the device MAY
> > > +execute implementation-specific alarm actions.
> > > +
> > > +The device MAY ignore the device status when executing
> > > +implementation-specific alarm actions.
> > > +
> > > +The device MAY ignore whether VIRTIO_RTC_F_ALARM was negotiated when
> > > +executing implementation-specific alarm actions.
> 

I wonder if these last three requirements are needed or we can just drop
them.

Matias