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

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

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

> +
>  \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.

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

> +
> +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.
> +
>  \paragraph{Alarm Control Requests}
>  
>  If VIRTIO_RTC_F_ALARM is negotiated,
> @@ -924,6 +1008,62 @@ \subsubsection{Alarm Operation}\label{sec:Device Types / RTC Device / Device Ope
>  \field{driver_alarm_time}.
>  \end{description}
>  
> +\drivernormative{\subparagraph}{Alarm Control Requests}{Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Control Requests}
> +
> +For VIRTIO_RTC_REQ_SET_ALARM and for any clock type listed in this
> +specification, the driver MUST use the nanosecond as unit for the
> +\field{alarm_time} field.
> +
> +\devicenormative{\subparagraph}{Alarm Control Requests}{Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Control Requests}
> +
> +If VIRTIO_RTC_F_ALARM was not negotiated, the device MUST set status
> +VIRTIO_RTC_S_ENODEV for VIRTIO_RTC_REQ_READ_ALARM,
> +VIRTIO_RTC_REQ_SET_ALARM, and VIRTIO_RTC_REQ_SET_ALARM_ENABLED.
> +
> +If the clock does not support alarm messages, the device MUST set status
> +VIRTIO_RTC_S_ENODEV for VIRTIO_RTC_REQ_READ_ALARM,
> +VIRTIO_RTC_REQ_SET_ALARM, and VIRTIO_RTC_REQ_SET_ALARM_ENABLED.
> +
> +For VIRTIO_RTC_REQ_READ_ALARM, the device MUST set field
> +\field{alarm_time} to \field{driver_alarm_time}.
> +
> +For VIRTIO_RTC_REQ_READ_ALARM, the device MUST set flag
> +VIRTIO_RTC_FLAG_ALARM_ENABLED in field \field{flags} if
> +\field{alarm_enabled} is true, and clear the flag otherwise.
> +
> +For VIRTIO_RTC_REQ_READ_ALARM and for any clock type listed in this
> +specification, the device MUST use the nanosecond as unit for the
> +\field{alarm_time} field.
> +
> +For VIRTIO_RTC_REQ_SET_ALARM, the device MUST accept any
> +\field{alarm_time} value.
> +
> +If the device sets status VIRTIO_RTC_S_OK for VIRTIO_RTC_REQ_SET_ALARM,
> +the device MUST set \field{driver_alarm_time} to the time
> +represented by field \field{alarm_time}.
> +
> +If the device sets status VIRTIO_RTC_S_OK for VIRTIO_RTC_REQ_SET_ALARM,
> +the device MUST set \field{alarm_enabled} to true if flag
> +VIRTIO_RTC_FLAG_ALARM_ENABLED is set in field \field{flags}.
> +
> +If the device sets status VIRTIO_RTC_S_OK for VIRTIO_RTC_REQ_SET_ALARM,
> +the device MUST set \field{alarm_enabled} to false if flag
> +VIRTIO_RTC_FLAG_ALARM_ENABLED is cleared in field \field{flags}.
> +
> +If the device sets status VIRTIO_RTC_S_OK for
> +VIRTIO_RTC_REQ_SET_ALARM_ENABLED, the device MUST set
> +\field{alarm_enabled} to true if flag VIRTIO_RTC_FLAG_ALARM_ENABLED is
> +set in field \field{flags}.
> +
> +If the device sets status VIRTIO_RTC_S_OK for
> +VIRTIO_RTC_REQ_SET_ALARM_ENABLED, the device MUST set
> +\field{alarm_enabled} to false if flag VIRTIO_RTC_FLAG_ALARM_ENABLED is
> +cleared in field \field{flags}.
> +
> +If the device sets status VIRTIO_RTC_S_OK for
> +VIRTIO_RTC_REQ_SET_ALARM_ENABLED, the device MUST retain
> +\field{driver_alarm_time}.
> +
>  \paragraph{Alarm Notifications}
>  
>  If the alarmq is present, the driver should make buffers available in
> @@ -959,3 +1099,20 @@ \subsubsection{Alarm Operation}\label{sec:Device Types / RTC Device / Device Ope
>  
>  \field{clock_id} identifies the expired alarm through its associated
>  clock.
> +
> +\drivernormative{\subparagraph}{Alarm Notifications}{Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Notifications}
> +
> +If VIRTIO_RTC_F_ALARM was negotiated, the driver SHOULD populate the
> +alarmq with buffers.
> +
> +The driver MUST allocate enough space for any alarmq message in the
> +device-writable part of an alarmq buffer.
> +
> +If the driver receives a VIRTIO_RTC_NOTIF_ALARM notification, the driver
> +SHOULD read the associated clock instead of assuming that the alarm time
> +which the driver set last has been reached.
> +
> +\devicenormative{\subparagraph}{Alarm Notifications}{Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Notifications}
> +
> +The device MUST NOT send a VIRTIO_RTC_NOTIF_ALARM notification for a
> +clock which does not support alarm messages.
> diff --git a/device-types/rtc/device-conformance.tex b/device-types/rtc/device-conformance.tex
> index 4303cd450542..705691a7319f 100644
> --- a/device-types/rtc/device-conformance.tex
> +++ b/device-types/rtc/device-conformance.tex
> @@ -3,7 +3,11 @@
>  An RTC device MUST conform to the following normative statements:
>  
>  \begin{itemize}
> +\item \ref{devicenormative:Device Types / RTC Device / Feature bits}
>  \item \ref{devicenormative:Device Types / RTC Device / Device Operation}
>  \item \ref{devicenormative:Device Types / RTC Device / Device Operation / Control Requests}
>  \item \ref{devicenormative:Device Types / RTC Device / Device Operation / Read Requests}
> +\item \ref{devicenormative:Device Types / RTC Device / Device Operation / Alarm Operation}
> +\item \ref{devicenormative:Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Control Requests}
> +\item \ref{devicenormative:Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Notifications}
>  \end{itemize}
> diff --git a/device-types/rtc/driver-conformance.tex b/device-types/rtc/driver-conformance.tex
> index 689c18d158d0..a87c4cde99c2 100644
> --- a/device-types/rtc/driver-conformance.tex
> +++ b/device-types/rtc/driver-conformance.tex
> @@ -6,4 +6,6 @@
>  \item \ref{drivernormative:Device Types / RTC Device / Device Operation}
>  \item \ref{drivernormative:Device Types / RTC Device / Device Operation / Control Requests}
>  \item \ref{drivernormative:Device Types / RTC Device / Device Operation / Read Requests}
> +\item \ref{drivernormative:Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Control Requests}
> +\item \ref{drivernormative:Device Types / RTC Device / Device Operation / Alarm Operation / Alarm Notifications}
>  \end{itemize}
> -- 
> 2.43.0
> 
>