* [PATCH v7 1/4] virtio-rtc: Add initial device specification
2025-01-23 10:16 [PATCH v7 0/4] virtio-rtc: Add device specification Peter Hilber
@ 2025-01-23 10:16 ` Peter Hilber
2025-02-10 13:52 ` Matias Ezequiel Vara Larsen
2025-01-23 10:16 ` [PATCH v7 2/4] virtio-rtc: Add initial normative statements Peter Hilber
` (2 subsequent siblings)
3 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-01-23 10:16 UTC (permalink / raw)
To: virtio-comment
Cc: Cornelia Huck, Parav Pandit, Jason Wang, David Woodhouse,
Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri, Peter Hilber
The virtio-rtc device provides information about current time through
one or more clocks. As such, it is a Real-Time Clock (RTC) device.
The normative statements for this device follow in the next patch.
For this device, there is an RFC Linux kernel driver which is being
upstreamed, and a proprietary device implementation.
Miscellaneous
-------------
The spec does not specify how a driver should interpret clock readings,
esp. also not how to perform clock synchronization.
The device uses the "Timer/Clock" device id which is already part of the
specification. This device id was registered a long time ago and should
be unused according to the author's information. The name "RTC" was
determined to be the best for a device which focuses on current time.
Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
---
Notes:
v7:
- Remove leap second and performance indications from struct
virtio_rtc_resp_read_cross. Remove backing definitions.
- Add wording change which was previously mistakenly placed in last
patch.
v6:
- Make leap second status information optional if the clock smears (or
might smear) leap seconds.
- Do not use union for leap second indication.
- Improve wording.
- Refer to the new POSIX.1-2024 for UTC epoch definition.
v5:
- Change structure and wording to support adding shared memory like
vmclock [8].
- Add dedicated clock types for UTC leap second smearing (David
Woodhouse).
- Extend leap second indications.
- Split UTC-TAI offset and fractional offset due to smearing (David
Woodhouse).
- Remove requirement that TAI offset must not be a whole second while
clock is being smeared.
- Align bit widths, and some names, with '[RFC PATCH v4] ptp: Add
vDSO-style vmclock support' [8].
- Replace VIRTIO_RTC_SUBTYPE_ by VIRTIO_RTC_SMEAR_.
- For Arm Generic Timer, only support Virtual Count Register (David
Woodhouse).
- Rename MONO clock to MONOTONIC clock.
v4:
- Drop distinction of Arm Generic Timer virtual and physical counter [7].
- Add requirement that device should assume that driver reads clock from
first vCPU (David Woodhouse) [6].
- Formatting and wording improvements.
v3:
- Address comments from Parav Pandit.
- Split off normative requirements into a second commit [2].
- Merge readq and controlq into requestq [3].
- Don't guard cross-timestamping with feature bit [3].
- Pad request headers to 64 bit [2].
- Rename Virtio status codes to match UNIX error names [2].
- Avoid Virtio status code clashes with net controlq ack values.
- Reword to refer more to "requests", rather than "messages" [2].
- Rephrase some sentences [2].
- Use integer data types without "__" prefixes [2].
- Reduce clock id width to 16 bits [5].
- Make VIRTIO_RTC_FLAG_CROSS_CAP a bit mask rather than a bit index.
v2:
- Address comments from Cornelia Huck.
- Add VIRTIO_RTC_M_CROSS_CAP message [1].
- Fix various minor issues and improve wording [1].
- Add several clarifications regarding device error statuses.
[1] https://lists.oasis-open.org/archives/virtio-comment/202304/msg00523.html
[2] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mffb93800fea11d6dda9e151078abedd6ff1c0f1e
[3] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#m94efd0aa9b9c2b96a246b79ef8bfc3bf64ebe791
[4] https://lore.kernel.org/lkml/20230630171052.985577-1-peter.hilber@opensynergy.com/T/#m65fa1d715933360498c4e33d7225e4220215a9d6
[5] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469048c5d8b62
[6] https://lore.kernel.org/lkml/d796d9a5-8eda-4528-a6d8-1c4eba24aa1e@opensynergy.com/
[7] https://lore.kernel.org/all/20231218064253.9734-2-peter.hilber@opensynergy.com/
[8] https://lore.kernel.org/lkml/20240708092924.1473461-1-dwmw2@infradead.org/
content.tex | 3 +-
device-types/rtc/description.tex | 426 +++++++++++++++++++++++++++++++
introduction.tex | 6 +
3 files changed, 434 insertions(+), 1 deletion(-)
create mode 100644 device-types/rtc/description.tex
diff --git a/content.tex b/content.tex
index 67b1bf3c35ab..b1d93a8aebb4 100644
--- a/content.tex
+++ b/content.tex
@@ -684,7 +684,7 @@ \chapter{Device Types}\label{sec:Device Types}
\hline
16 & GPU device \\
\hline
-17 & Timer/Clock device \\
+17 & RTC (Timer/Clock) device \\
\hline
18 & Input device \\
\hline
@@ -776,6 +776,7 @@ \chapter{Device Types}\label{sec:Device Types}
\input{device-types/pmem/description.tex}
\input{device-types/can/description.tex}
\input{device-types/spi/description.tex}
+\input{device-types/rtc/description.tex}
\chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
new file mode 100644
index 000000000000..aae015690b26
--- /dev/null
+++ b/device-types/rtc/description.tex
@@ -0,0 +1,426 @@
+\section{RTC Device}\label{sec:Device Types / RTC Device}
+
+The RTC (Real Time Clock) device provides information about current
+time. The device can provide different clocks, e.g.\ for the UTC or TAI
+time standards, or for physical time elapsed since some past epoch. The
+driver can read the clocks with simple or more accurate methods.
+
+\subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
+
+17
+
+\subsection{Virtqueues}\label{sec:Device Types / RTC Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+The driver enqueues requests to the requestq.
+
+\subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
+
+No device-specific feature bits are defined yet.
+
+\subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
+
+There is no configuration data for the device.
+
+\subsection{Device Initialization}\label{sec:Device Types / RTC Device / Device Initialization}
+
+The device determines the set of clocks. The device can provide zero or
+more clocks.
+
+\subsection{Device Operation}\label{sec:Device Types / RTC Device / Device Operation}
+
+For the requestq, the driver sends a message with a request, and
+receives the response in the device-writable part of the message. The
+requestq uses common request and response headers.
+
+\begin{lstlisting}
+/* common request header */
+struct virtio_rtc_req_head {
+ le16 msg_type;
+ u8 reserved[6];
+};
+
+/* common response header */
+struct virtio_rtc_resp_head {
+ u8 status;
+ u8 reserved[7];
+};
+\end{lstlisting}
+
+The \field{msg_type} field identifies the message type.
+
+The \field{status} field indicates whether the device successfully
+executed the request. The device sets the \field{status} field to one of
+the following values:
+
+\begin{lstlisting}
+#define VIRTIO_RTC_S_OK 0
+#define VIRTIO_RTC_S_EOPNOTSUPP 2
+#define VIRTIO_RTC_S_ENODEV 3
+#define VIRTIO_RTC_S_EINVAL 4
+#define VIRTIO_RTC_S_EIO 5
+\end{lstlisting}
+
+VIRTIO_RTC_S_OK indicates that the device successfully executed the
+request.
+
+If \field{status} is not VIRTIO_RTC_S_OK, the value of other response
+fields is undefined.
+
+VIRTIO_RTC_S_EOPNOTSUPP indicates that the device could not execute the
+specific request due to an implementation limitation. The device also
+returns status VIRTIO_RTC_S_EOPNOTSUPP for requests with unknown values
+in the fields \field{msg_type} or \field{hw_counter}.
+
+VIRTIO_RTC_S_ENODEV indicates that the \field{clock_id} field value
+supplied with the request does not identify a clock.
+
+VIRTIO_RTC_S_EINVAL indicates that
+
+\begin{itemize}
+\item the driver request values are not allowed by the specification or
+\item the device read-only part of the message, or device write-only
+ part of the message, is too small.
+\end{itemize}
+
+VIRTIO_RTC_S_EIO indicates that the device did not execute the request
+due to an error which was not caused by invalid input from the driver.
+
+All \field{reserved} fields are written as zero, and their value is
+ignored.
+
+The set of clocks does not change after feature negotiation completion,
+until device reset. The set of clocks should not change on device reset
+either (similar to negotiated features). Clock identifiers are
+zero-based, dense indices. All fields named \field{clock_id} contain
+clock identifiers.
+
+\subsubsection{Common Definitions}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions}
+
+This section makes common definitions.
+
+\paragraph{Clock Types}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Clock Types}
+
+The following clock types are defined:
+
+\begin{lstlisting}
+#define VIRTIO_RTC_CLOCK_UTC 0
+#define VIRTIO_RTC_CLOCK_TAI 1
+#define VIRTIO_RTC_CLOCK_MONOTONIC 2
+#define VIRTIO_RTC_CLOCK_UTC_SMEARED 3
+#define VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED 4
+\end{lstlisting}
+
+\begin{description}
+
+\item[VIRTIO_RTC_CLOCK_UTC] uses the UTC (Coordinated Universal Time)
+ time standard. This clock uses the time epoch of January 1,
+ 1970, 00:00 UTC. This is the same epoch as \emph{Unix time}. The
+ clock's seconds since the epoch are related to UTC time as
+ defined by \hyperref[intro:EPOCH]{EPOCH}.
+
+ This clock observes positive and negative leap seconds as
+ announced by standard bodies. At the start of leap seconds, the
+ clock steps accordingly.
+
+\item[VIRTIO_RTC_CLOCK_TAI] uses the TAI (International Atomic Time)
+ time standard. This clock uses the time epoch of January 1,
+ 1970, 00:00 TAI.
+
+\item[VIRTIO_RTC_CLOCK_MONOTONIC] uses monotonic physical time (SI
+ seconds subdivisions) since some unspecified epoch. The epoch is
+ before or during device reset.
+
+\item[VIRTIO_RTC_CLOCK_UTC_SMEARED] deviates from the UTC standard by
+ smearing time in the vicinity of a leap second. This avoids
+ clock steps due to UTC leap seconds. Otherwise, this clock is
+ similar to VIRTIO_RTC_CLOCK_UTC.
+
+\item[VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED] This clock
+
+\begin{itemize}
+\item may deviate from the UTC standard by smearing time in the vicinity
+ of a leap second (similar to VIRTIO_RTC_CLOCK_UTC_SMEARED), or
+
+\item may step at the start of leap seconds like VIRTIO_RTC_CLOCK_UTC.
+\end{itemize}
+
+A clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED can change this
+behavior for every leap second.
+
+\end{description}
+
+In the following, \emph{UTC-like clock} designates any clock of type
+VIRTIO_RTC_CLOCK_UTC, VIRTIO_RTC_CLOCK_UTC_SMEARED, or
+VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED.
+
+Additional clock types may be standardized in the future.
+Implementation-specific definitions of clock types are not recommended
+and use ids between 0xF0 and 0xFF.
+
+\paragraph{Smearing Variants}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Smearing Variants}
+
+Leap second \emph{smearing variants} describe the deviation from the UTC
+standard in the vicinity of a leap second. The following smearing
+variants are currently defined:
+
+\begin{lstlisting}
+#define VIRTIO_RTC_SMEAR_UNSPECIFIED 0
+#define VIRTIO_RTC_SMEAR_NOON_LINEAR 1
+#define VIRTIO_RTC_SMEAR_UTC_SLS 2
+\end{lstlisting}
+
+\begin{description}
+
+ \item[VIRTIO_RTC_SMEAR_UNSPECIFIED] means that it is unspecified
+ how time is smeared in the vicinity of leap seconds.
+
+ \item[VIRTIO_RTC_SMEAR_NOON_LINEAR] specifies a linear smear
+ from noon prior to the leap second until noon after the
+ leap second.
+
+ \item[VIRTIO_RTC_SMEAR_UTC_SLS] specifies a linear smear as per
+ the \hyperref[intro:UTC-SLS]{UTC-SLS} proposal.
+
+\end{description}
+
+Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED always behave according to a
+smearing variant. The smearing variant does not change over the clock's
+lifetime.
+
+For clocks of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, it is unspecified
+whether leap seconds are smeared, and how leap seconds are smeared.
+
+Additional smearing variants may be standardized in the future.
+Implementation-specific definitions of smearing variants are not
+recommended and use ids greater than or equal to 0xF0.
+
+In the following, \emph{leap smearing clock} designates
+
+\begin{itemize}
+
+\item any clock of type VIRTIO_RTC_CLOCK_UTC_SMEARED
+
+\item any clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED at any time
+ when the clock is smearing a leap second.
+
+\end{itemize}
+
+\paragraph{Hardware Counters}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}
+
+The following hardware counter identifiers are specified:
+
+\begin{lstlisting}
+/* Arm Generic Timer Counter-timer Virtual Count Register (CNTVCT_EL0) */
+#define VIRTIO_RTC_COUNTER_ARM_VCT 0
+/* x86 Time-Stamp Counter */
+#define VIRTIO_RTC_COUNTER_X86_TSC 1
+/* Invalid */
+#define VIRTIO_RTC_COUNTER_INVALID 0xFF
+\end{lstlisting}
+
+Additional hardware counter identifiers may be standardized in the
+future. Implementation-specific hardware counter identifiers are not
+recommended and have values between 0xF0 and 0xFE.
+
+\subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Operation / Control Requests}
+
+Through \emph{control requests}, the driver requests information about
+the device capabilities. The driver enqueues control requests in the
+requestq.
+
+\begin{description}
+
+\item[VIRTIO_RTC_REQ_CFG] discovers the number of clocks.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_CFG 0x1000 /* message type */
+
+struct virtio_rtc_req_cfg {
+ struct virtio_rtc_req_head head;
+ /* no request params */
+};
+
+struct virtio_rtc_resp_cfg {
+ struct virtio_rtc_resp_head head;
+ le16 num_clocks;
+ u8 reserved[6];
+};
+\end{lstlisting}
+
+Field \field{num_clocks} contains the number of clocks. A device
+provides zero or more clocks. Valid clock ids are those smaller than
+\field{num_clocks}.
+
+\item[VIRTIO_RTC_REQ_CLOCK_CAP] discovers the capabilities of the clock
+identified by the \field{clock_id} field.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_CLOCK_CAP 0x1001 /* message type */
+
+struct virtio_rtc_req_clock_cap {
+ struct virtio_rtc_req_head head;
+ le16 clock_id;
+ u8 reserved[6];
+};
+
+struct virtio_rtc_resp_clock_cap {
+ struct virtio_rtc_resp_head head;
+ u8 type;
+ u8 leap_second_smearing;
+ u8 reserved[6];
+};
+\end{lstlisting}
+
+The \field{type} field identifies the clock type. A device provides
+zero or more clocks for a clock type.
+
+Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED indicate the \emph{smearing
+variant} through field \field{leap_second_smearing}. All other clocks
+set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
+
+\item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
+cross-timestamping for a particular pair of clock and hardware counter.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_CROSS_CAP 0x1002 /* message type */
+
+struct virtio_rtc_req_cross_cap {
+ struct virtio_rtc_req_head head;
+ le16 clock_id;
+ u8 hw_counter;
+ u8 reserved[5];
+};
+
+
+struct virtio_rtc_resp_cross_cap {
+ struct virtio_rtc_resp_head head;
+#define VIRTIO_RTC_FLAG_CROSS_CAP (1 << 0)
+ u8 flags;
+ u8 reserved[7];
+};
+\end{lstlisting}
+
+The \field{clock_id} field identifies the clock, and the
+\field{hw_counter} field identifies the hardware counter, for which
+cross-timestamp support is probed. The device sets flag
+VIRTIO_RTC_FLAG_CROSS_CAP in the \field{flags} field if the clock
+supports cross-timestamping for the particular clock and hardware
+counter, and clears the flag otherwise.
+
+\end{description}
+
+\subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Operation / Read Requests}
+
+Through \emph{read requests}, the driver requests clock readings from
+the device. The driver enqueues read requests in the requestq. The
+device obtains device-side clock readings and forwards these clock
+readings to the driver. The driver may enhance and interpret the clock
+readings through methods which are beyond the scope of this
+specification.
+
+Once DRIVER_OK has been set, the device should support reading every
+clock, even when a clock may yet have to be aligned to reference time
+sources.
+
+In general,
+
+\begin{itemize}
+\item clocks may jump backwards or forward, and
+\item the clock frequency may change. Clocks may be \emph{slewed},
+ i.e.\ clocks may run at a frequency other than their current
+ best frequency estimate.
+\end{itemize}
+
+As long as a clock does not jump backwards, the driver clock readings
+through read request responses increase monotonically:
+
+\begin{itemize}
+\item As long as a clock does not jump backwards in-between device-side
+ clock readings, the driver-side readings for that clock increase
+ monotonically as well, in the order in which the driver
+ marks read requests as available.
+
+\item The device marks read requests for the same clock as used in
+ the order in which the messages were marked as available.
+\end{itemize}
+
+For a clock of type VIRTIO_RTC_CLOCK_MONOTONIC, the device always returns
+monotonically increasing clock readings through read request responses.
+
+The unit of all \field{clock_reading} fields is 1
+nanosecond.\footnote{For time epochs in year 1970 or later, this means
+that time until at least year 2553 can be represented in the \field{le64
+clock_reading} fields.}
+
+\begin{description}
+
+\item[VIRTIO_RTC_REQ_READ] reads the clock identified by the
+\field{clock_id} field. The device supports this message for every
+clock.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_READ 0x0001 /* message type */
+
+struct virtio_rtc_req_read {
+ struct virtio_rtc_req_head head;
+ le16 clock_id;
+ u8 reserved[6];
+};
+
+struct virtio_rtc_resp_read {
+ struct virtio_rtc_resp_head head;
+ le64 clock_reading;
+};
+\end{lstlisting}
+
+\field{clock_reading} is a device-side clock reading obtained after the
+message was marked as available.
+
+\item[VIRTIO_RTC_REQ_READ_CROSS] returns a cross-timestamp for the clock
+identified by the \field{clock_id} field.\footnote{Cross-timestamping
+is similar to the ptp_kvm mechanism in the Linux kernel.} This message
+may yield better performance than using VIRTIO_RTC_REQ_READ.
+
+The driver can determine whether the device supports
+VIRTIO_RTC_REQ_READ_CROSS for a specific clock and \field{hw_counter}
+through VIRTIO_RTC_REQ_CROSS_CAP.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_READ_CROSS 0x0002 /* message type */
+
+struct virtio_rtc_req_read_cross {
+ struct virtio_rtc_req_head head;
+ le16 clock_id;
+ u8 hw_counter;
+ u8 reserved[5];
+};
+
+struct virtio_rtc_resp_read_cross {
+ struct virtio_rtc_resp_head head;
+ le64 clock_reading;
+ le64 counter_cycles;
+};
+\end{lstlisting}
+
+The \field{hw_counter} field specifies the hardware counter for which
+the driver requests a cross-timestamp.
+
+Cross-timestamping returns a \field{clock_reading}, and an associated
+hardware counter value, \field{counter_cycles}. The
+\field{counter_cycles} field is the approximate or precise value which
+the driver would have read at the \field{clock_reading} time instant
+from the hardware counter identified by \field{hw_counter}. How the
+device determines the value which the driver would have seen is beyond
+the scope of this specification. In case hardware counter reads differ
+among CPUs used by the driver, the device should assume that the driver
+reads the hardware counter from the CPU which the driver enumerates as
+the first.
+
+The hardware counter identifiers are defined in
+\ref{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}.
+
+\end{description}
diff --git a/introduction.tex b/introduction.tex
index e60298a4d78b..34f9485bc393 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -168,6 +168,12 @@ \section{Normative References}\label{sec:Normative References}
Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
14, RFC 8174, DOI 10.17487/RFC8174, May 2017
\newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
+ \phantomsection\label{intro:EPOCH}\textbf{[EPOCH]} &
+ POSIX.1-2024, Base Definitions, Seconds Since the Epoch
+ \newline\url{https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap04.html#tag_04_19}\\
+ \phantomsection\label{intro:UTC-SLS}\textbf{[UTC-SLS]} &
+ UTC with Smoothed Leap Seconds (UTC-SLS)
+ \newline\url{https://www.cl.cam.ac.uk/~mgk25/time/utc-sls/}\\
\end{longtable}
\section{Non-Normative References}
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v7 1/4] virtio-rtc: Add initial device specification
2025-01-23 10:16 ` [PATCH v7 1/4] virtio-rtc: Add initial " Peter Hilber
@ 2025-02-10 13:52 ` Matias Ezequiel Vara Larsen
2025-02-13 18:12 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-10 13:52 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Jan 23, 2025 at 11:16:12AM +0100, Peter Hilber wrote:
> The virtio-rtc device provides information about current time through
> one or more clocks. As such, it is a Real-Time Clock (RTC) device.
>
> The normative statements for this device follow in the next patch.
>
> For this device, there is an RFC Linux kernel driver which is being
> upstreamed, and a proprietary device implementation.
>
> Miscellaneous
> -------------
>
> The spec does not specify how a driver should interpret clock readings,
> esp. also not how to perform clock synchronization.
>
> The device uses the "Timer/Clock" device id which is already part of the
> specification. This device id was registered a long time ago and should
> be unused according to the author's information. The name "RTC" was
> determined to be the best for a device which focuses on current time.
>
> Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> ---
>
> Notes:
> v7:
>
> - Remove leap second and performance indications from struct
> virtio_rtc_resp_read_cross. Remove backing definitions.
>
> - Add wording change which was previously mistakenly placed in last
> patch.
>
> v6:
>
> - Make leap second status information optional if the clock smears (or
> might smear) leap seconds.
>
> - Do not use union for leap second indication.
>
> - Improve wording.
>
> - Refer to the new POSIX.1-2024 for UTC epoch definition.
>
> v5:
>
> - Change structure and wording to support adding shared memory like
> vmclock [8].
>
> - Add dedicated clock types for UTC leap second smearing (David
> Woodhouse).
>
> - Extend leap second indications.
>
> - Split UTC-TAI offset and fractional offset due to smearing (David
> Woodhouse).
>
> - Remove requirement that TAI offset must not be a whole second while
> clock is being smeared.
>
> - Align bit widths, and some names, with '[RFC PATCH v4] ptp: Add
> vDSO-style vmclock support' [8].
>
> - Replace VIRTIO_RTC_SUBTYPE_ by VIRTIO_RTC_SMEAR_.
>
> - For Arm Generic Timer, only support Virtual Count Register (David
> Woodhouse).
>
> - Rename MONO clock to MONOTONIC clock.
>
> v4:
>
> - Drop distinction of Arm Generic Timer virtual and physical counter [7].
>
> - Add requirement that device should assume that driver reads clock from
> first vCPU (David Woodhouse) [6].
>
> - Formatting and wording improvements.
>
> v3:
>
> - Address comments from Parav Pandit.
>
> - Split off normative requirements into a second commit [2].
>
> - Merge readq and controlq into requestq [3].
>
> - Don't guard cross-timestamping with feature bit [3].
>
> - Pad request headers to 64 bit [2].
>
> - Rename Virtio status codes to match UNIX error names [2].
>
> - Avoid Virtio status code clashes with net controlq ack values.
>
> - Reword to refer more to "requests", rather than "messages" [2].
>
> - Rephrase some sentences [2].
>
> - Use integer data types without "__" prefixes [2].
>
> - Reduce clock id width to 16 bits [5].
>
> - Make VIRTIO_RTC_FLAG_CROSS_CAP a bit mask rather than a bit index.
>
> v2:
>
> - Address comments from Cornelia Huck.
>
> - Add VIRTIO_RTC_M_CROSS_CAP message [1].
>
> - Fix various minor issues and improve wording [1].
>
> - Add several clarifications regarding device error statuses.
>
> [1] https://lists.oasis-open.org/archives/virtio-comment/202304/msg00523.html
> [2] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mffb93800fea11d6dda9e151078abedd6ff1c0f1e
> [3] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#m94efd0aa9b9c2b96a246b79ef8bfc3bf64ebe791
> [4] https://lore.kernel.org/lkml/20230630171052.985577-1-peter.hilber@opensynergy.com/T/#m65fa1d715933360498c4e33d7225e4220215a9d6
> [5] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469048c5d8b62
> [6] https://lore.kernel.org/lkml/d796d9a5-8eda-4528-a6d8-1c4eba24aa1e@opensynergy.com/
> [7] https://lore.kernel.org/all/20231218064253.9734-2-peter.hilber@opensynergy.com/
> [8] https://lore.kernel.org/lkml/20240708092924.1473461-1-dwmw2@infradead.org/
>
> content.tex | 3 +-
> device-types/rtc/description.tex | 426 +++++++++++++++++++++++++++++++
> introduction.tex | 6 +
> 3 files changed, 434 insertions(+), 1 deletion(-)
> create mode 100644 device-types/rtc/description.tex
>
> diff --git a/content.tex b/content.tex
> index 67b1bf3c35ab..b1d93a8aebb4 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -684,7 +684,7 @@ \chapter{Device Types}\label{sec:Device Types}
> \hline
> 16 & GPU device \\
> \hline
> -17 & Timer/Clock device \\
> +17 & RTC (Timer/Clock) device \\
> \hline
> 18 & Input device \\
> \hline
> @@ -776,6 +776,7 @@ \chapter{Device Types}\label{sec:Device Types}
> \input{device-types/pmem/description.tex}
> \input{device-types/can/description.tex}
> \input{device-types/spi/description.tex}
> +\input{device-types/rtc/description.tex}
>
> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
> diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
> new file mode 100644
> index 000000000000..aae015690b26
> --- /dev/null
> +++ b/device-types/rtc/description.tex
> @@ -0,0 +1,426 @@
> +\section{RTC Device}\label{sec:Device Types / RTC Device}
> +
> +The RTC (Real Time Clock) device provides information about current
> +time. The device can provide different clocks, e.g.\ for the UTC or TAI
> +time standards, or for physical time elapsed since some past epoch. The
> +driver can read the clocks with simple or more accurate methods.
> +
> +\subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
> +
> +17
> +
> +\subsection{Virtqueues}\label{sec:Device Types / RTC Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +The driver enqueues requests to the requestq.
> +
> +\subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
> +
> +No device-specific feature bits are defined yet.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
> +
> +There is no configuration data for the device.
> +
> +\subsection{Device Initialization}\label{sec:Device Types / RTC Device / Device Initialization}
> +
> +The device determines the set of clocks. The device can provide zero or
> +more clocks.
> +
> +\subsection{Device Operation}\label{sec:Device Types / RTC Device / Device Operation}
> +
> +For the requestq, the driver sends a message with a request, and
> +receives the response in the device-writable part of the message. The
> +requestq uses common request and response headers.
> +
> +\begin{lstlisting}
> +/* common request header */
> +struct virtio_rtc_req_head {
> + le16 msg_type;
> + u8 reserved[6];
> +};
> +
> +/* common response header */
> +struct virtio_rtc_resp_head {
> + u8 status;
> + u8 reserved[7];
> +};
> +\end{lstlisting}
> +
> +The \field{msg_type} field identifies the message type.
> +
> +The \field{status} field indicates whether the device successfully
> +executed the request. The device sets the \field{status} field to one of
> +the following values:
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_S_OK 0
> +#define VIRTIO_RTC_S_EOPNOTSUPP 2
> +#define VIRTIO_RTC_S_ENODEV 3
> +#define VIRTIO_RTC_S_EINVAL 4
> +#define VIRTIO_RTC_S_EIO 5
> +\end{lstlisting}
> +
> +VIRTIO_RTC_S_OK indicates that the device successfully executed the
> +request.
> +
> +If \field{status} is not VIRTIO_RTC_S_OK, the value of other response
> +fields is undefined.
> +
> +VIRTIO_RTC_S_EOPNOTSUPP indicates that the device could not execute the
> +specific request due to an implementation limitation. The device also
> +returns status VIRTIO_RTC_S_EOPNOTSUPP for requests with unknown values
> +in the fields \field{msg_type} or \field{hw_counter}.
> +
> +VIRTIO_RTC_S_ENODEV indicates that the \field{clock_id} field value
> +supplied with the request does not identify a clock.
> +
> +VIRTIO_RTC_S_EINVAL indicates that
> +
> +\begin{itemize}
> +\item the driver request values are not allowed by the specification or
> +\item the device read-only part of the message, or device write-only
> + part of the message, is too small.
> +\end{itemize}
> +
> +VIRTIO_RTC_S_EIO indicates that the device did not execute the request
> +due to an error which was not caused by invalid input from the driver.
> +
> +All \field{reserved} fields are written as zero, and their value is
> +ignored.
> +
> +The set of clocks does not change after feature negotiation completion,
> +until device reset. The set of clocks should not change on device reset
> +either (similar to negotiated features). Clock identifiers are
> +zero-based, dense indices. All fields named \field{clock_id} contain
> +clock identifiers.
> +
> +\subsubsection{Common Definitions}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions}
> +
> +This section makes common definitions.
> +
> +\paragraph{Clock Types}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Clock Types}
> +
> +The following clock types are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_CLOCK_UTC 0
> +#define VIRTIO_RTC_CLOCK_TAI 1
> +#define VIRTIO_RTC_CLOCK_MONOTONIC 2
> +#define VIRTIO_RTC_CLOCK_UTC_SMEARED 3
> +#define VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED 4
> +\end{lstlisting}
> +
> +\begin{description}
> +
> +\item[VIRTIO_RTC_CLOCK_UTC] uses the UTC (Coordinated Universal Time)
> + time standard. This clock uses the time epoch of January 1,
> + 1970, 00:00 UTC. This is the same epoch as \emph{Unix time}. The
> + clock's seconds since the epoch are related to UTC time as
> + defined by \hyperref[intro:EPOCH]{EPOCH}.
> +
> + This clock observes positive and negative leap seconds as
> + announced by standard bodies. At the start of leap seconds, the
> + clock steps accordingly.
> +
> +\item[VIRTIO_RTC_CLOCK_TAI] uses the TAI (International Atomic Time)
> + time standard. This clock uses the time epoch of January 1,
> + 1970, 00:00 TAI.
> +
> +\item[VIRTIO_RTC_CLOCK_MONOTONIC] uses monotonic physical time (SI
> + seconds subdivisions) since some unspecified epoch. The epoch is
> + before or during device reset.
> +
> +\item[VIRTIO_RTC_CLOCK_UTC_SMEARED] deviates from the UTC standard by
> + smearing time in the vicinity of a leap second. This avoids
> + clock steps due to UTC leap seconds. Otherwise, this clock is
> + similar to VIRTIO_RTC_CLOCK_UTC.
> +
> +\item[VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED] This clock
> +
> +\begin{itemize}
> +\item may deviate from the UTC standard by smearing time in the vicinity
> + of a leap second (similar to VIRTIO_RTC_CLOCK_UTC_SMEARED), or
> +
> +\item may step at the start of leap seconds like VIRTIO_RTC_CLOCK_UTC.
> +\end{itemize}
> +
> +A clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED can change this
> +behavior for every leap second.
> +
> +\end{description}
> +
> +In the following, \emph{UTC-like clock} designates any clock of type
> +VIRTIO_RTC_CLOCK_UTC, VIRTIO_RTC_CLOCK_UTC_SMEARED, or
> +VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED.
> +
> +Additional clock types may be standardized in the future.
> +Implementation-specific definitions of clock types are not recommended
> +and use ids between 0xF0 and 0xFF.
Do you mean that ids between 0xF0 and 0xFF are not recommended?
> +
> +\paragraph{Smearing Variants}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Smearing Variants}
> +
> +Leap second \emph{smearing variants} describe the deviation from the UTC
> +standard in the vicinity of a leap second. The following smearing
> +variants are currently defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_SMEAR_UNSPECIFIED 0
> +#define VIRTIO_RTC_SMEAR_NOON_LINEAR 1
> +#define VIRTIO_RTC_SMEAR_UTC_SLS 2
> +\end{lstlisting}
> +
> +\begin{description}
> +
> + \item[VIRTIO_RTC_SMEAR_UNSPECIFIED] means that it is unspecified
> + how time is smeared in the vicinity of leap seconds.
> +
> + \item[VIRTIO_RTC_SMEAR_NOON_LINEAR] specifies a linear smear
> + from noon prior to the leap second until noon after the
> + leap second.
> +
> + \item[VIRTIO_RTC_SMEAR_UTC_SLS] specifies a linear smear as per
> + the \hyperref[intro:UTC-SLS]{UTC-SLS} proposal.
> +
> +\end{description}
> +
> +Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED always behave according to a
> +smearing variant. The smearing variant does not change over the clock's
> +lifetime.
> +
> +For clocks of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, it is unspecified
> +whether leap seconds are smeared, and how leap seconds are smeared.
> +
> +Additional smearing variants may be standardized in the future.
> +Implementation-specific definitions of smearing variants are not
> +recommended and use ids greater than or equal to 0xF0.
Like the question above, It is not clear to me if `ids greater than or
equal to 0xF0` are not recommended.
> +
> +In the following, \emph{leap smearing clock} designates
> +
> +\begin{itemize}
> +
> +\item any clock of type VIRTIO_RTC_CLOCK_UTC_SMEARED
> +
> +\item any clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED at any time
> + when the clock is smearing a leap second.
> +
> +\end{itemize}
> +
> +\paragraph{Hardware Counters}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}
> +
> +The following hardware counter identifiers are specified:
> +
> +\begin{lstlisting}
> +/* Arm Generic Timer Counter-timer Virtual Count Register (CNTVCT_EL0) */
> +#define VIRTIO_RTC_COUNTER_ARM_VCT 0
> +/* x86 Time-Stamp Counter */
> +#define VIRTIO_RTC_COUNTER_X86_TSC 1
> +/* Invalid */
> +#define VIRTIO_RTC_COUNTER_INVALID 0xFF
> +\end{lstlisting}
> +
> +Additional hardware counter identifiers may be standardized in the
> +future. Implementation-specific hardware counter identifiers are not
> +recommended and have values between 0xF0 and 0xFE.
> +
> +\subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Operation / Control Requests}
> +
> +Through \emph{control requests}, the driver requests information about
> +the device capabilities. The driver enqueues control requests in the
> +requestq.
> +
> +\begin{description}
> +
> +\item[VIRTIO_RTC_REQ_CFG] discovers the number of clocks.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_CFG 0x1000 /* message type */
> +
> +struct virtio_rtc_req_cfg {
> + struct virtio_rtc_req_head head;
> + /* no request params */
> +};
> +
> +struct virtio_rtc_resp_cfg {
> + struct virtio_rtc_resp_head head;
> + le16 num_clocks;
> + u8 reserved[6];
> +};
> +\end{lstlisting}
> +
> +Field \field{num_clocks} contains the number of clocks. A device
> +provides zero or more clocks. Valid clock ids are those smaller than
> +\field{num_clocks}.
> +
> +\item[VIRTIO_RTC_REQ_CLOCK_CAP] discovers the capabilities of the clock
> +identified by the \field{clock_id} field.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_CLOCK_CAP 0x1001 /* message type */
> +
> +struct virtio_rtc_req_clock_cap {
> + struct virtio_rtc_req_head head;
> + le16 clock_id;
> + u8 reserved[6];
> +};
> +
> +struct virtio_rtc_resp_clock_cap {
> + struct virtio_rtc_resp_head head;
> + u8 type;
> + u8 leap_second_smearing;
> + u8 reserved[6];
> +};
> +\end{lstlisting}
> +
> +The \field{type} field identifies the clock type. A device provides
> +zero or more clocks for a clock type.
> +
> +Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED indicate the \emph{smearing
> +variant} through field \field{leap_second_smearing}. All other clocks
> +set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
> +
> +\item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
> +cross-timestamping for a particular pair of clock and hardware counter.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_CROSS_CAP 0x1002 /* message type */
> +
> +struct virtio_rtc_req_cross_cap {
> + struct virtio_rtc_req_head head;
> + le16 clock_id;
> + u8 hw_counter;
> + u8 reserved[5];
> +};
> +
> +
> +struct virtio_rtc_resp_cross_cap {
> + struct virtio_rtc_resp_head head;
> +#define VIRTIO_RTC_FLAG_CROSS_CAP (1 << 0)
> + u8 flags;
> + u8 reserved[7];
> +};
> +\end{lstlisting}
> +
> +The \field{clock_id} field identifies the clock, and the
> +\field{hw_counter} field identifies the hardware counter, for which
> +cross-timestamp support is probed. The device sets flag
> +VIRTIO_RTC_FLAG_CROSS_CAP in the \field{flags} field if the clock
> +supports cross-timestamping for the particular clock and hardware
> +counter, and clears the flag otherwise.
> +
> +\end{description}
> +
> +\subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Operation / Read Requests}
> +
> +Through \emph{read requests}, the driver requests clock readings from
> +the device. The driver enqueues read requests in the requestq. The
> +device obtains device-side clock readings and forwards these clock
> +readings to the driver. The driver may enhance and interpret the clock
> +readings through methods which are beyond the scope of this
> +specification.
> +
> +Once DRIVER_OK has been set, the device should support reading every
> +clock, even when a clock may yet have to be aligned to reference time
> +sources.
> +
> +In general,
> +
> +\begin{itemize}
> +\item clocks may jump backwards or forward, and
> +\item the clock frequency may change. Clocks may be \emph{slewed},
> + i.e.\ clocks may run at a frequency other than their current
> + best frequency estimate.
> +\end{itemize}
> +
> +As long as a clock does not jump backwards, the driver clock readings
> +through read request responses increase monotonically:
> +
> +\begin{itemize}
> +\item As long as a clock does not jump backwards in-between device-side
> + clock readings, the driver-side readings for that clock increase
> + monotonically as well, in the order in which the driver
> + marks read requests as available.
> +
> +\item The device marks read requests for the same clock as used in
> + the order in which the messages were marked as available.
> +\end{itemize}
> +
Should it not be `MUST` here?
> +For a clock of type VIRTIO_RTC_CLOCK_MONOTONIC, the device always returns
> +monotonically increasing clock readings through read request responses.
> +
> +The unit of all \field{clock_reading} fields is 1
> +nanosecond.\footnote{For time epochs in year 1970 or later, this means
> +that time until at least year 2553 can be represented in the \field{le64
> +clock_reading} fields.}
> +
> +\begin{description}
> +
> +\item[VIRTIO_RTC_REQ_READ] reads the clock identified by the
> +\field{clock_id} field. The device supports this message for every
> +clock.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_READ 0x0001 /* message type */
> +
> +struct virtio_rtc_req_read {
> + struct virtio_rtc_req_head head;
> + le16 clock_id;
> + u8 reserved[6];
> +};
> +
> +struct virtio_rtc_resp_read {
> + struct virtio_rtc_resp_head head;
> + le64 clock_reading;
> +};
> +\end{lstlisting}
> +
> +\field{clock_reading} is a device-side clock reading obtained after the
> +message was marked as available.
> +
> +\item[VIRTIO_RTC_REQ_READ_CROSS] returns a cross-timestamp for the clock
> +identified by the \field{clock_id} field.\footnote{Cross-timestamping
> +is similar to the ptp_kvm mechanism in the Linux kernel.} This message
> +may yield better performance than using VIRTIO_RTC_REQ_READ.
> +
> +The driver can determine whether the device supports
> +VIRTIO_RTC_REQ_READ_CROSS for a specific clock and \field{hw_counter}
> +through VIRTIO_RTC_REQ_CROSS_CAP.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_READ_CROSS 0x0002 /* message type */
> +
> +struct virtio_rtc_req_read_cross {
> + struct virtio_rtc_req_head head;
> + le16 clock_id;
> + u8 hw_counter;
> + u8 reserved[5];
> +};
> +
> +struct virtio_rtc_resp_read_cross {
> + struct virtio_rtc_resp_head head;
> + le64 clock_reading;
> + le64 counter_cycles;
> +};
> +\end{lstlisting}
> +
> +The \field{hw_counter} field specifies the hardware counter for which
> +the driver requests a cross-timestamp.
> +
> +Cross-timestamping returns a \field{clock_reading}, and an associated
> +hardware counter value, \field{counter_cycles}. The
> +\field{counter_cycles} field is the approximate or precise value which
> +the driver would have read at the \field{clock_reading} time instant
> +from the hardware counter identified by \field{hw_counter}. How the
> +device determines the value which the driver would have seen is beyond
> +the scope of this specification. In case hardware counter reads differ
> +among CPUs used by the driver, the device should assume that the driver
> +reads the hardware counter from the CPU which the driver enumerates as
> +the first.
> +
> +The hardware counter identifiers are defined in
> +\ref{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}.
> +
> +\end{description}
> diff --git a/introduction.tex b/introduction.tex
> index e60298a4d78b..34f9485bc393 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -168,6 +168,12 @@ \section{Normative References}\label{sec:Normative References}
> Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> 14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> + \phantomsection\label{intro:EPOCH}\textbf{[EPOCH]} &
> + POSIX.1-2024, Base Definitions, Seconds Since the Epoch
> + \newline\url{https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap04.html#tag_04_19}\\
> + \phantomsection\label{intro:UTC-SLS}\textbf{[UTC-SLS]} &
> + UTC with Smoothed Leap Seconds (UTC-SLS)
> + \newline\url{https://www.cl.cam.ac.uk/~mgk25/time/utc-sls/}\\
> \end{longtable}
>
> \section{Non-Normative References}
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 1/4] virtio-rtc: Add initial device specification
2025-02-10 13:52 ` Matias Ezequiel Vara Larsen
@ 2025-02-13 18:12 ` Peter Hilber
2025-02-19 12:45 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-13 18:12 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Mon, Feb 10, 2025 at 02:52:33PM +0100, Matias Ezequiel Vara Larsen wrote:
> On Thu, Jan 23, 2025 at 11:16:12AM +0100, Peter Hilber wrote:
> > The virtio-rtc device provides information about current time through
> > one or more clocks. As such, it is a Real-Time Clock (RTC) device.
> >
> > The normative statements for this device follow in the next patch.
> >
> > For this device, there is an RFC Linux kernel driver which is being
> > upstreamed, and a proprietary device implementation.
> >
> > Miscellaneous
> > -------------
> >
> > The spec does not specify how a driver should interpret clock readings,
> > esp. also not how to perform clock synchronization.
> >
> > The device uses the "Timer/Clock" device id which is already part of the
> > specification. This device id was registered a long time ago and should
> > be unused according to the author's information. The name "RTC" was
> > determined to be the best for a device which focuses on current time.
> >
> > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > ---
> >
> > Notes:
> > v7:
> >
> > - Remove leap second and performance indications from struct
> > virtio_rtc_resp_read_cross. Remove backing definitions.
> >
> > - Add wording change which was previously mistakenly placed in last
> > patch.
> >
> > v6:
> >
> > - Make leap second status information optional if the clock smears (or
> > might smear) leap seconds.
> >
> > - Do not use union for leap second indication.
> >
> > - Improve wording.
> >
> > - Refer to the new POSIX.1-2024 for UTC epoch definition.
> >
> > v5:
> >
> > - Change structure and wording to support adding shared memory like
> > vmclock [8].
> >
> > - Add dedicated clock types for UTC leap second smearing (David
> > Woodhouse).
> >
> > - Extend leap second indications.
> >
> > - Split UTC-TAI offset and fractional offset due to smearing (David
> > Woodhouse).
> >
> > - Remove requirement that TAI offset must not be a whole second while
> > clock is being smeared.
> >
> > - Align bit widths, and some names, with '[RFC PATCH v4] ptp: Add
> > vDSO-style vmclock support' [8].
> >
> > - Replace VIRTIO_RTC_SUBTYPE_ by VIRTIO_RTC_SMEAR_.
> >
> > - For Arm Generic Timer, only support Virtual Count Register (David
> > Woodhouse).
> >
> > - Rename MONO clock to MONOTONIC clock.
> >
> > v4:
> >
> > - Drop distinction of Arm Generic Timer virtual and physical counter [7].
> >
> > - Add requirement that device should assume that driver reads clock from
> > first vCPU (David Woodhouse) [6].
> >
> > - Formatting and wording improvements.
> >
> > v3:
> >
> > - Address comments from Parav Pandit.
> >
> > - Split off normative requirements into a second commit [2].
> >
> > - Merge readq and controlq into requestq [3].
> >
> > - Don't guard cross-timestamping with feature bit [3].
> >
> > - Pad request headers to 64 bit [2].
> >
> > - Rename Virtio status codes to match UNIX error names [2].
> >
> > - Avoid Virtio status code clashes with net controlq ack values.
> >
> > - Reword to refer more to "requests", rather than "messages" [2].
> >
> > - Rephrase some sentences [2].
> >
> > - Use integer data types without "__" prefixes [2].
> >
> > - Reduce clock id width to 16 bits [5].
> >
> > - Make VIRTIO_RTC_FLAG_CROSS_CAP a bit mask rather than a bit index.
> >
> > v2:
> >
> > - Address comments from Cornelia Huck.
> >
> > - Add VIRTIO_RTC_M_CROSS_CAP message [1].
> >
> > - Fix various minor issues and improve wording [1].
> >
> > - Add several clarifications regarding device error statuses.
> >
> > [1] https://lists.oasis-open.org/archives/virtio-comment/202304/msg00523.html
> > [2] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mffb93800fea11d6dda9e151078abedd6ff1c0f1e
> > [3] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#m94efd0aa9b9c2b96a246b79ef8bfc3bf64ebe791
> > [4] https://lore.kernel.org/lkml/20230630171052.985577-1-peter.hilber@opensynergy.com/T/#m65fa1d715933360498c4e33d7225e4220215a9d6
> > [5] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469048c5d8b62
> > [6] https://lore.kernel.org/lkml/d796d9a5-8eda-4528-a6d8-1c4eba24aa1e@opensynergy.com/
> > [7] https://lore.kernel.org/all/20231218064253.9734-2-peter.hilber@opensynergy.com/
> > [8] https://lore.kernel.org/lkml/20240708092924.1473461-1-dwmw2@infradead.org/
> >
> > content.tex | 3 +-
> > device-types/rtc/description.tex | 426 +++++++++++++++++++++++++++++++
> > introduction.tex | 6 +
> > 3 files changed, 434 insertions(+), 1 deletion(-)
> > create mode 100644 device-types/rtc/description.tex
> >
[...]
> > +
> > +\paragraph{Clock Types}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Clock Types}
> > +
> > +The following clock types are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_RTC_CLOCK_UTC 0
> > +#define VIRTIO_RTC_CLOCK_TAI 1
> > +#define VIRTIO_RTC_CLOCK_MONOTONIC 2
> > +#define VIRTIO_RTC_CLOCK_UTC_SMEARED 3
> > +#define VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED 4
> > +\end{lstlisting}
> > +
> > +\begin{description}
> > +
> > +\item[VIRTIO_RTC_CLOCK_UTC] uses the UTC (Coordinated Universal Time)
> > + time standard. This clock uses the time epoch of January 1,
> > + 1970, 00:00 UTC. This is the same epoch as \emph{Unix time}. The
> > + clock's seconds since the epoch are related to UTC time as
> > + defined by \hyperref[intro:EPOCH]{EPOCH}.
> > +
> > + This clock observes positive and negative leap seconds as
> > + announced by standard bodies. At the start of leap seconds, the
> > + clock steps accordingly.
> > +
> > +\item[VIRTIO_RTC_CLOCK_TAI] uses the TAI (International Atomic Time)
> > + time standard. This clock uses the time epoch of January 1,
> > + 1970, 00:00 TAI.
> > +
> > +\item[VIRTIO_RTC_CLOCK_MONOTONIC] uses monotonic physical time (SI
> > + seconds subdivisions) since some unspecified epoch. The epoch is
> > + before or during device reset.
> > +
> > +\item[VIRTIO_RTC_CLOCK_UTC_SMEARED] deviates from the UTC standard by
> > + smearing time in the vicinity of a leap second. This avoids
> > + clock steps due to UTC leap seconds. Otherwise, this clock is
> > + similar to VIRTIO_RTC_CLOCK_UTC.
> > +
> > +\item[VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED] This clock
> > +
> > +\begin{itemize}
> > +\item may deviate from the UTC standard by smearing time in the vicinity
> > + of a leap second (similar to VIRTIO_RTC_CLOCK_UTC_SMEARED), or
> > +
> > +\item may step at the start of leap seconds like VIRTIO_RTC_CLOCK_UTC.
> > +\end{itemize}
> > +
> > +A clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED can change this
> > +behavior for every leap second.
> > +
> > +\end{description}
> > +
> > +In the following, \emph{UTC-like clock} designates any clock of type
> > +VIRTIO_RTC_CLOCK_UTC, VIRTIO_RTC_CLOCK_UTC_SMEARED, or
> > +VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED.
> > +
> > +Additional clock types may be standardized in the future.
> > +Implementation-specific definitions of clock types are not recommended
> > +and use ids between 0xF0 and 0xFF.
>
> Do you mean that ids between 0xF0 and 0xFF are not recommended?
I mean that implementation-specific definitions must use this id range
(this is defined as a requirement in the patch with the normative
requirements). Is the following better?
Implementation-specific definitions of clock types are not
recommended. Implementation-specific definitions use ids between
0xF0 and 0xFF.
>
> > +
> > +\paragraph{Smearing Variants}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Smearing Variants}
> > +
> > +Leap second \emph{smearing variants} describe the deviation from the UTC
> > +standard in the vicinity of a leap second. The following smearing
> > +variants are currently defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_RTC_SMEAR_UNSPECIFIED 0
> > +#define VIRTIO_RTC_SMEAR_NOON_LINEAR 1
> > +#define VIRTIO_RTC_SMEAR_UTC_SLS 2
> > +\end{lstlisting}
> > +
> > +\begin{description}
> > +
> > + \item[VIRTIO_RTC_SMEAR_UNSPECIFIED] means that it is unspecified
> > + how time is smeared in the vicinity of leap seconds.
> > +
> > + \item[VIRTIO_RTC_SMEAR_NOON_LINEAR] specifies a linear smear
> > + from noon prior to the leap second until noon after the
> > + leap second.
> > +
> > + \item[VIRTIO_RTC_SMEAR_UTC_SLS] specifies a linear smear as per
> > + the \hyperref[intro:UTC-SLS]{UTC-SLS} proposal.
> > +
> > +\end{description}
> > +
> > +Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED always behave according to a
> > +smearing variant. The smearing variant does not change over the clock's
> > +lifetime.
> > +
> > +For clocks of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, it is unspecified
> > +whether leap seconds are smeared, and how leap seconds are smeared.
> > +
> > +Additional smearing variants may be standardized in the future.
> > +Implementation-specific definitions of smearing variants are not
> > +recommended and use ids greater than or equal to 0xF0.
>
> Like the question above, It is not clear to me if `ids greater than or
> equal to 0xF0` are not recommended.
>
Is the following better?
Implementation-specific definitions of smearing variants are not
recommended. Implementation-specific definitions use ids greater
than or equal to 0xF0.
> > +
> > +In the following, \emph{leap smearing clock} designates
> > +
> > +\begin{itemize}
> > +
> > +\item any clock of type VIRTIO_RTC_CLOCK_UTC_SMEARED
> > +
> > +\item any clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED at any time
> > + when the clock is smearing a leap second.
> > +
> > +\end{itemize}
> > +
> > +\paragraph{Hardware Counters}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}
> > +
> > +The following hardware counter identifiers are specified:
> > +
> > +\begin{lstlisting}
> > +/* Arm Generic Timer Counter-timer Virtual Count Register (CNTVCT_EL0) */
> > +#define VIRTIO_RTC_COUNTER_ARM_VCT 0
> > +/* x86 Time-Stamp Counter */
> > +#define VIRTIO_RTC_COUNTER_X86_TSC 1
> > +/* Invalid */
> > +#define VIRTIO_RTC_COUNTER_INVALID 0xFF
> > +\end{lstlisting}
> > +
> > +Additional hardware counter identifiers may be standardized in the
> > +future. Implementation-specific hardware counter identifiers are not
> > +recommended and have values between 0xF0 and 0xFE.
> > +
> > +\subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Operation / Control Requests}
> > +
> > +Through \emph{control requests}, the driver requests information about
> > +the device capabilities. The driver enqueues control requests in the
> > +requestq.
> > +
> > +\begin{description}
> > +
> > +\item[VIRTIO_RTC_REQ_CFG] discovers the number of clocks.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_RTC_REQ_CFG 0x1000 /* message type */
> > +
> > +struct virtio_rtc_req_cfg {
> > + struct virtio_rtc_req_head head;
> > + /* no request params */
> > +};
> > +
> > +struct virtio_rtc_resp_cfg {
> > + struct virtio_rtc_resp_head head;
> > + le16 num_clocks;
> > + u8 reserved[6];
> > +};
> > +\end{lstlisting}
> > +
> > +Field \field{num_clocks} contains the number of clocks. A device
> > +provides zero or more clocks. Valid clock ids are those smaller than
> > +\field{num_clocks}.
> > +
> > +\item[VIRTIO_RTC_REQ_CLOCK_CAP] discovers the capabilities of the clock
> > +identified by the \field{clock_id} field.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_RTC_REQ_CLOCK_CAP 0x1001 /* message type */
> > +
> > +struct virtio_rtc_req_clock_cap {
> > + struct virtio_rtc_req_head head;
> > + le16 clock_id;
> > + u8 reserved[6];
> > +};
> > +
> > +struct virtio_rtc_resp_clock_cap {
> > + struct virtio_rtc_resp_head head;
> > + u8 type;
> > + u8 leap_second_smearing;
> > + u8 reserved[6];
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{type} field identifies the clock type. A device provides
> > +zero or more clocks for a clock type.
> > +
> > +Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED indicate the \emph{smearing
> > +variant} through field \field{leap_second_smearing}. All other clocks
> > +set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
> > +
> > +\item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
> > +cross-timestamping for a particular pair of clock and hardware counter.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_RTC_REQ_CROSS_CAP 0x1002 /* message type */
> > +
> > +struct virtio_rtc_req_cross_cap {
> > + struct virtio_rtc_req_head head;
> > + le16 clock_id;
> > + u8 hw_counter;
> > + u8 reserved[5];
> > +};
> > +
> > +
> > +struct virtio_rtc_resp_cross_cap {
> > + struct virtio_rtc_resp_head head;
> > +#define VIRTIO_RTC_FLAG_CROSS_CAP (1 << 0)
> > + u8 flags;
> > + u8 reserved[7];
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{clock_id} field identifies the clock, and the
> > +\field{hw_counter} field identifies the hardware counter, for which
> > +cross-timestamp support is probed. The device sets flag
> > +VIRTIO_RTC_FLAG_CROSS_CAP in the \field{flags} field if the clock
> > +supports cross-timestamping for the particular clock and hardware
> > +counter, and clears the flag otherwise.
> > +
> > +\end{description}
> > +
> > +\subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Operation / Read Requests}
> > +
> > +Through \emph{read requests}, the driver requests clock readings from
> > +the device. The driver enqueues read requests in the requestq. The
> > +device obtains device-side clock readings and forwards these clock
> > +readings to the driver. The driver may enhance and interpret the clock
> > +readings through methods which are beyond the scope of this
> > +specification.
> > +
> > +Once DRIVER_OK has been set, the device should support reading every
> > +clock, even when a clock may yet have to be aligned to reference time
> > +sources.
> > +
> > +In general,
> > +
> > +\begin{itemize}
> > +\item clocks may jump backwards or forward, and
> > +\item the clock frequency may change. Clocks may be \emph{slewed},
> > + i.e.\ clocks may run at a frequency other than their current
> > + best frequency estimate.
> > +\end{itemize}
> > +
> > +As long as a clock does not jump backwards, the driver clock readings
> > +through read request responses increase monotonically:
> > +
> > +\begin{itemize}
> > +\item As long as a clock does not jump backwards in-between device-side
> > + clock readings, the driver-side readings for that clock increase
> > + monotonically as well, in the order in which the driver
> > + marks read requests as available.
> > +
> > +\item The device marks read requests for the same clock as used in
> > + the order in which the messages were marked as available.
> > +\end{itemize}
> > +
> Should it not be `MUST` here?
>
There are corresponding MUST requirements in the normative statements
patch. E.g. for the last enumeration item:
For any clock C, the device MUST mark all read requests reading
C as used in the total order in which the driver marked these
messages as available.
I understood that the prevailing preference is to avoid MUST outside of
the normative statements.
Thanks for the review,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 1/4] virtio-rtc: Add initial device specification
2025-02-13 18:12 ` Peter Hilber
@ 2025-02-19 12:45 ` Matias Ezequiel Vara Larsen
0 siblings, 0 replies; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-19 12:45 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Feb 13, 2025 at 07:12:39PM +0100, Peter Hilber wrote:
> On Mon, Feb 10, 2025 at 02:52:33PM +0100, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Jan 23, 2025 at 11:16:12AM +0100, Peter Hilber wrote:
> > > The virtio-rtc device provides information about current time through
> > > one or more clocks. As such, it is a Real-Time Clock (RTC) device.
> > >
> > > The normative statements for this device follow in the next patch.
> > >
> > > For this device, there is an RFC Linux kernel driver which is being
> > > upstreamed, and a proprietary device implementation.
> > >
> > > Miscellaneous
> > > -------------
> > >
> > > The spec does not specify how a driver should interpret clock readings,
> > > esp. also not how to perform clock synchronization.
> > >
> > > The device uses the "Timer/Clock" device id which is already part of the
> > > specification. This device id was registered a long time ago and should
> > > be unused according to the author's information. The name "RTC" was
> > > determined to be the best for a device which focuses on current time.
> > >
> > > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > > ---
> > >
> > > Notes:
> > > v7:
> > >
> > > - Remove leap second and performance indications from struct
> > > virtio_rtc_resp_read_cross. Remove backing definitions.
> > >
> > > - Add wording change which was previously mistakenly placed in last
> > > patch.
> > >
> > > v6:
> > >
> > > - Make leap second status information optional if the clock smears (or
> > > might smear) leap seconds.
> > >
> > > - Do not use union for leap second indication.
> > >
> > > - Improve wording.
> > >
> > > - Refer to the new POSIX.1-2024 for UTC epoch definition.
> > >
> > > v5:
> > >
> > > - Change structure and wording to support adding shared memory like
> > > vmclock [8].
> > >
> > > - Add dedicated clock types for UTC leap second smearing (David
> > > Woodhouse).
> > >
> > > - Extend leap second indications.
> > >
> > > - Split UTC-TAI offset and fractional offset due to smearing (David
> > > Woodhouse).
> > >
> > > - Remove requirement that TAI offset must not be a whole second while
> > > clock is being smeared.
> > >
> > > - Align bit widths, and some names, with '[RFC PATCH v4] ptp: Add
> > > vDSO-style vmclock support' [8].
> > >
> > > - Replace VIRTIO_RTC_SUBTYPE_ by VIRTIO_RTC_SMEAR_.
> > >
> > > - For Arm Generic Timer, only support Virtual Count Register (David
> > > Woodhouse).
> > >
> > > - Rename MONO clock to MONOTONIC clock.
> > >
> > > v4:
> > >
> > > - Drop distinction of Arm Generic Timer virtual and physical counter [7].
> > >
> > > - Add requirement that device should assume that driver reads clock from
> > > first vCPU (David Woodhouse) [6].
> > >
> > > - Formatting and wording improvements.
> > >
> > > v3:
> > >
> > > - Address comments from Parav Pandit.
> > >
> > > - Split off normative requirements into a second commit [2].
> > >
> > > - Merge readq and controlq into requestq [3].
> > >
> > > - Don't guard cross-timestamping with feature bit [3].
> > >
> > > - Pad request headers to 64 bit [2].
> > >
> > > - Rename Virtio status codes to match UNIX error names [2].
> > >
> > > - Avoid Virtio status code clashes with net controlq ack values.
> > >
> > > - Reword to refer more to "requests", rather than "messages" [2].
> > >
> > > - Rephrase some sentences [2].
> > >
> > > - Use integer data types without "__" prefixes [2].
> > >
> > > - Reduce clock id width to 16 bits [5].
> > >
> > > - Make VIRTIO_RTC_FLAG_CROSS_CAP a bit mask rather than a bit index.
> > >
> > > v2:
> > >
> > > - Address comments from Cornelia Huck.
> > >
> > > - Add VIRTIO_RTC_M_CROSS_CAP message [1].
> > >
> > > - Fix various minor issues and improve wording [1].
> > >
> > > - Add several clarifications regarding device error statuses.
> > >
> > > [1] https://lists.oasis-open.org/archives/virtio-comment/202304/msg00523.html
> > > [2] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mffb93800fea11d6dda9e151078abedd6ff1c0f1e
> > > [3] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#m94efd0aa9b9c2b96a246b79ef8bfc3bf64ebe791
> > > [4] https://lore.kernel.org/lkml/20230630171052.985577-1-peter.hilber@opensynergy.com/T/#m65fa1d715933360498c4e33d7225e4220215a9d6
> > > [5] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469048c5d8b62
> > > [6] https://lore.kernel.org/lkml/d796d9a5-8eda-4528-a6d8-1c4eba24aa1e@opensynergy.com/
> > > [7] https://lore.kernel.org/all/20231218064253.9734-2-peter.hilber@opensynergy.com/
> > > [8] https://lore.kernel.org/lkml/20240708092924.1473461-1-dwmw2@infradead.org/
> > >
> > > content.tex | 3 +-
> > > device-types/rtc/description.tex | 426 +++++++++++++++++++++++++++++++
> > > introduction.tex | 6 +
> > > 3 files changed, 434 insertions(+), 1 deletion(-)
> > > create mode 100644 device-types/rtc/description.tex
> > >
>
> [...]
>
> > > +
> > > +\paragraph{Clock Types}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Clock Types}
> > > +
> > > +The following clock types are defined:
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_RTC_CLOCK_UTC 0
> > > +#define VIRTIO_RTC_CLOCK_TAI 1
> > > +#define VIRTIO_RTC_CLOCK_MONOTONIC 2
> > > +#define VIRTIO_RTC_CLOCK_UTC_SMEARED 3
> > > +#define VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED 4
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +
> > > +\item[VIRTIO_RTC_CLOCK_UTC] uses the UTC (Coordinated Universal Time)
> > > + time standard. This clock uses the time epoch of January 1,
> > > + 1970, 00:00 UTC. This is the same epoch as \emph{Unix time}. The
> > > + clock's seconds since the epoch are related to UTC time as
> > > + defined by \hyperref[intro:EPOCH]{EPOCH}.
> > > +
> > > + This clock observes positive and negative leap seconds as
> > > + announced by standard bodies. At the start of leap seconds, the
> > > + clock steps accordingly.
> > > +
> > > +\item[VIRTIO_RTC_CLOCK_TAI] uses the TAI (International Atomic Time)
> > > + time standard. This clock uses the time epoch of January 1,
> > > + 1970, 00:00 TAI.
> > > +
> > > +\item[VIRTIO_RTC_CLOCK_MONOTONIC] uses monotonic physical time (SI
> > > + seconds subdivisions) since some unspecified epoch. The epoch is
> > > + before or during device reset.
> > > +
> > > +\item[VIRTIO_RTC_CLOCK_UTC_SMEARED] deviates from the UTC standard by
> > > + smearing time in the vicinity of a leap second. This avoids
> > > + clock steps due to UTC leap seconds. Otherwise, this clock is
> > > + similar to VIRTIO_RTC_CLOCK_UTC.
> > > +
> > > +\item[VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED] This clock
> > > +
> > > +\begin{itemize}
> > > +\item may deviate from the UTC standard by smearing time in the vicinity
> > > + of a leap second (similar to VIRTIO_RTC_CLOCK_UTC_SMEARED), or
> > > +
> > > +\item may step at the start of leap seconds like VIRTIO_RTC_CLOCK_UTC.
> > > +\end{itemize}
> > > +
> > > +A clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED can change this
> > > +behavior for every leap second.
> > > +
> > > +\end{description}
> > > +
> > > +In the following, \emph{UTC-like clock} designates any clock of type
> > > +VIRTIO_RTC_CLOCK_UTC, VIRTIO_RTC_CLOCK_UTC_SMEARED, or
> > > +VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED.
> > > +
> > > +Additional clock types may be standardized in the future.
> > > +Implementation-specific definitions of clock types are not recommended
> > > +and use ids between 0xF0 and 0xFF.
> >
> > Do you mean that ids between 0xF0 and 0xFF are not recommended?
>
> I mean that implementation-specific definitions must use this id range
> (this is defined as a requirement in the patch with the normative
> requirements). Is the following better?
>
> Implementation-specific definitions of clock types are not
> recommended. Implementation-specific definitions use ids between
> 0xF0 and 0xFF.
>
I think it is better.
> >
> > > +
> > > +\paragraph{Smearing Variants}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Smearing Variants}
> > > +
> > > +Leap second \emph{smearing variants} describe the deviation from the UTC
> > > +standard in the vicinity of a leap second. The following smearing
> > > +variants are currently defined:
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_RTC_SMEAR_UNSPECIFIED 0
> > > +#define VIRTIO_RTC_SMEAR_NOON_LINEAR 1
> > > +#define VIRTIO_RTC_SMEAR_UTC_SLS 2
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +
> > > + \item[VIRTIO_RTC_SMEAR_UNSPECIFIED] means that it is unspecified
> > > + how time is smeared in the vicinity of leap seconds.
> > > +
> > > + \item[VIRTIO_RTC_SMEAR_NOON_LINEAR] specifies a linear smear
> > > + from noon prior to the leap second until noon after the
> > > + leap second.
> > > +
> > > + \item[VIRTIO_RTC_SMEAR_UTC_SLS] specifies a linear smear as per
> > > + the \hyperref[intro:UTC-SLS]{UTC-SLS} proposal.
> > > +
> > > +\end{description}
> > > +
> > > +Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED always behave according to a
> > > +smearing variant. The smearing variant does not change over the clock's
> > > +lifetime.
> > > +
> > > +For clocks of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, it is unspecified
> > > +whether leap seconds are smeared, and how leap seconds are smeared.
> > > +
> > > +Additional smearing variants may be standardized in the future.
> > > +Implementation-specific definitions of smearing variants are not
> > > +recommended and use ids greater than or equal to 0xF0.
> >
> > Like the question above, It is not clear to me if `ids greater than or
> > equal to 0xF0` are not recommended.
> >
>
> Is the following better?
>
> Implementation-specific definitions of smearing variants are not
> recommended. Implementation-specific definitions use ids greater
> than or equal to 0xF0.
>
I think it is better.
> > > +
> > > +In the following, \emph{leap smearing clock} designates
> > > +
> > > +\begin{itemize}
> > > +
> > > +\item any clock of type VIRTIO_RTC_CLOCK_UTC_SMEARED
> > > +
> > > +\item any clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED at any time
> > > + when the clock is smearing a leap second.
> > > +
> > > +\end{itemize}
> > > +
> > > +\paragraph{Hardware Counters}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}
> > > +
> > > +The following hardware counter identifiers are specified:
> > > +
> > > +\begin{lstlisting}
> > > +/* Arm Generic Timer Counter-timer Virtual Count Register (CNTVCT_EL0) */
> > > +#define VIRTIO_RTC_COUNTER_ARM_VCT 0
> > > +/* x86 Time-Stamp Counter */
> > > +#define VIRTIO_RTC_COUNTER_X86_TSC 1
> > > +/* Invalid */
> > > +#define VIRTIO_RTC_COUNTER_INVALID 0xFF
> > > +\end{lstlisting}
> > > +
> > > +Additional hardware counter identifiers may be standardized in the
> > > +future. Implementation-specific hardware counter identifiers are not
> > > +recommended and have values between 0xF0 and 0xFE.
> > > +
> > > +\subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Operation / Control Requests}
> > > +
> > > +Through \emph{control requests}, the driver requests information about
> > > +the device capabilities. The driver enqueues control requests in the
> > > +requestq.
> > > +
> > > +\begin{description}
> > > +
> > > +\item[VIRTIO_RTC_REQ_CFG] discovers the number of clocks.
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_RTC_REQ_CFG 0x1000 /* message type */
> > > +
> > > +struct virtio_rtc_req_cfg {
> > > + struct virtio_rtc_req_head head;
> > > + /* no request params */
> > > +};
> > > +
> > > +struct virtio_rtc_resp_cfg {
> > > + struct virtio_rtc_resp_head head;
> > > + le16 num_clocks;
> > > + u8 reserved[6];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +Field \field{num_clocks} contains the number of clocks. A device
> > > +provides zero or more clocks. Valid clock ids are those smaller than
> > > +\field{num_clocks}.
> > > +
> > > +\item[VIRTIO_RTC_REQ_CLOCK_CAP] discovers the capabilities of the clock
> > > +identified by the \field{clock_id} field.
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_RTC_REQ_CLOCK_CAP 0x1001 /* message type */
> > > +
> > > +struct virtio_rtc_req_clock_cap {
> > > + struct virtio_rtc_req_head head;
> > > + le16 clock_id;
> > > + u8 reserved[6];
> > > +};
> > > +
> > > +struct virtio_rtc_resp_clock_cap {
> > > + struct virtio_rtc_resp_head head;
> > > + u8 type;
> > > + u8 leap_second_smearing;
> > > + u8 reserved[6];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{type} field identifies the clock type. A device provides
> > > +zero or more clocks for a clock type.
> > > +
> > > +Clocks of type VIRTIO_RTC_CLOCK_UTC_SMEARED indicate the \emph{smearing
> > > +variant} through field \field{leap_second_smearing}. All other clocks
> > > +set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
> > > +
> > > +\item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
> > > +cross-timestamping for a particular pair of clock and hardware counter.
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_RTC_REQ_CROSS_CAP 0x1002 /* message type */
> > > +
> > > +struct virtio_rtc_req_cross_cap {
> > > + struct virtio_rtc_req_head head;
> > > + le16 clock_id;
> > > + u8 hw_counter;
> > > + u8 reserved[5];
> > > +};
> > > +
> > > +
> > > +struct virtio_rtc_resp_cross_cap {
> > > + struct virtio_rtc_resp_head head;
> > > +#define VIRTIO_RTC_FLAG_CROSS_CAP (1 << 0)
> > > + u8 flags;
> > > + u8 reserved[7];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{clock_id} field identifies the clock, and the
> > > +\field{hw_counter} field identifies the hardware counter, for which
> > > +cross-timestamp support is probed. The device sets flag
> > > +VIRTIO_RTC_FLAG_CROSS_CAP in the \field{flags} field if the clock
> > > +supports cross-timestamping for the particular clock and hardware
> > > +counter, and clears the flag otherwise.
> > > +
> > > +\end{description}
> > > +
> > > +\subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Operation / Read Requests}
> > > +
> > > +Through \emph{read requests}, the driver requests clock readings from
> > > +the device. The driver enqueues read requests in the requestq. The
> > > +device obtains device-side clock readings and forwards these clock
> > > +readings to the driver. The driver may enhance and interpret the clock
> > > +readings through methods which are beyond the scope of this
> > > +specification.
> > > +
> > > +Once DRIVER_OK has been set, the device should support reading every
> > > +clock, even when a clock may yet have to be aligned to reference time
> > > +sources.
> > > +
> > > +In general,
> > > +
> > > +\begin{itemize}
> > > +\item clocks may jump backwards or forward, and
> > > +\item the clock frequency may change. Clocks may be \emph{slewed},
> > > + i.e.\ clocks may run at a frequency other than their current
> > > + best frequency estimate.
> > > +\end{itemize}
> > > +
> > > +As long as a clock does not jump backwards, the driver clock readings
> > > +through read request responses increase monotonically:
> > > +
> > > +\begin{itemize}
> > > +\item As long as a clock does not jump backwards in-between device-side
> > > + clock readings, the driver-side readings for that clock increase
> > > + monotonically as well, in the order in which the driver
> > > + marks read requests as available.
> > > +
> > > +\item The device marks read requests for the same clock as used in
> > > + the order in which the messages were marked as available.
> > > +\end{itemize}
> > > +
> > Should it not be `MUST` here?
> >
>
> There are corresponding MUST requirements in the normative statements
> patch. E.g. for the last enumeration item:
>
> For any clock C, the device MUST mark all read requests reading
> C as used in the total order in which the driver marked these
> messages as available.
>
> I understood that the prevailing preference is to avoid MUST outside of
> the normative statements.
>
>
I see, thanks for the explanation.
Matias
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v7 2/4] virtio-rtc: Add initial normative statements
2025-01-23 10:16 [PATCH v7 0/4] virtio-rtc: Add device specification Peter Hilber
2025-01-23 10:16 ` [PATCH v7 1/4] virtio-rtc: Add initial " Peter Hilber
@ 2025-01-23 10:16 ` Peter Hilber
2025-02-10 16:33 ` Matias Ezequiel Vara Larsen
2025-01-23 10:16 ` [PATCH v7 3/4] virtio-rtc: Add alarm feature Peter Hilber
2025-01-23 10:16 ` [PATCH v7 4/4] virtio-rtc: Add normative statements for " Peter Hilber
3 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-01-23 10:16 UTC (permalink / raw)
To: virtio-comment
Cc: Cornelia Huck, Parav Pandit, Jason Wang, David Woodhouse,
Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri, Peter Hilber
Add the normative statements for the initial device specification.
Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
---
Notes:
v7:
- Remove obsolete requirements for leap second indication.
v6:
- Update requirements to make leap second status information optional if
if the clock smears (or might smear) leap seconds.
- Allow indicating VIRTIO_RTC_SMEAR_UNSPECIFIED for
VIRTIO_RTC_CLOCK_SMEARED.
- Shorten some requirements by omitting redundant information.
v5:
- Update normative statements to match v5 changes to non-normative
statements (patch 1).
v4:
- Require driver to set unused flags to zero.
- Update normative statements to match v4 changes to non-normative
statements (patch 1).
- Improve formatting.
conformance.tex | 2 +
device-types/rtc/description.tex | 269 ++++++++++++++++++++++++
device-types/rtc/device-conformance.tex | 9 +
device-types/rtc/driver-conformance.tex | 9 +
4 files changed, 289 insertions(+)
create mode 100644 device-types/rtc/device-conformance.tex
create mode 100644 device-types/rtc/driver-conformance.tex
diff --git a/conformance.tex b/conformance.tex
index d92a2369488e..4ce86a525e84 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -162,6 +162,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
\input{device-types/pmem/driver-conformance.tex}
\input{device-types/can/driver-conformance.tex}
\input{device-types/spi/driver-conformance.tex}
+\input{device-types/rtc/driver-conformance.tex}
\conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
@@ -254,6 +255,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
\input{device-types/pmem/device-conformance.tex}
\input{device-types/can/device-conformance.tex}
\input{device-types/spi/device-conformance.tex}
+\input{device-types/rtc/device-conformance.tex}
\conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
A conformant implementation MUST be either transitional or
diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
index aae015690b26..2aefc22cb649 100644
--- a/device-types/rtc/description.tex
+++ b/device-types/rtc/description.tex
@@ -98,6 +98,111 @@ \subsection{Device Operation}\label{sec:Device Types / RTC Device / Device Opera
zero-based, dense indices. All fields named \field{clock_id} contain
clock identifiers.
+\drivernormative{\subsubsection}{Device Operation}{Device Types / RTC Device / Device Operation}
+
+If the \field{struct virtio_rtc_resp_head} field \field{status} is not
+VIRTIO_RTC_S_OK, the driver MUST NOT interpret response fields other
+than \field{status}.
+
+The driver MUST set \emph{reserved} fields in the device-readable part
+of the message to zero.
+
+The driver MUST set unnamed bits in \emph{flags} fields in the
+device-readable part of the message to zero.
+
+The driver MUST NOT interpret \emph{reserved} fields in the
+device-writable part of the message.
+
+The driver MUST NOT interpret unnamed bits in \emph{flags} fields in the
+device-writable part of the message.
+
+The driver MUST put the request into the device-readable part of the
+message.
+
+The driver MUST allocate enough space for the response in the
+device-writable part of a requestq message.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / RTC Device / Device Operation}
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_OK if the device successfully
+executed the request.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to a status other than VIRTIO_RTC_S_OK if the
+device did not successfully execute the request.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EOPNOTSUPP if the device could not
+execute the specific request due to an implementation limitation.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EOPNOTSUPP for a request with a
+value of the \field{msg_type} field which is not described in this
+specification.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EOPNOTSUPP for a request with a
+value of the \field{hw_counter} field which is neither described in this
+specification nor otherwise known to the implementation.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_ENODEV if the \field{clock_id}
+field value supplied with the request does not identify a clock.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EINVAL if the request values are
+inconsistent with the specification and if the inconsistence is not
+described by the requirements which stipulate status
+VIRTIO_RTC_S_EOPNOTSUPP or VIRTIO_RTC_S_ENODEV.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EINVAL if the device read-only part
+of the message is too small.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EINVAL if the device write-only
+part of the message is too small, unless the \field{status} field does
+not fit into the device write-only part.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST NOT set the
+\field{status} field if the \field{status} field does not fit into the
+device write-only part.
+
+For \field{struct virtio_rtc_resp_head}, the device MUST set the
+\field{status} field to VIRTIO_RTC_S_EIO if none of the previous
+requirements in this document stipulated another \field{status}.
+
+If the device read-only part of a message M is bigger than the size of
+the request specified for message M, the device MUST ignore the
+additional space.
+
+If the device write-only part of a message M is bigger than the size of
+the response specified for message M, the device MUST ignore the
+additional space.
+
+The device MUST NOT interpret \emph{reserved} fields in the
+device-readable part of the message.
+
+The device MUST set \emph{reserved} fields in the device-writable part
+of the message to zero.
+
+The device MUST set unnamed bits in \emph{flags} fields in the
+device-writable part of the message to zero.
+
+After feature negotiation completion the device MUST NOT change the set
+of clocks until device reset.
+
+The device SHOULD NOT change the set of clocks on a device reset after
+the first device reset.
+
+The device MUST use non-negative integers, which are smaller than the
+number of clocks, as clock identifiers.
+
+For a fixed request value V, the device SHOULD either, for every request
+with value V, always execute successfully, or, for every request with
+value V, always set a fixed \field{status} other than VIRTIO_RTC_S_OK.
+
\subsubsection{Common Definitions}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions}
This section makes common definitions.
@@ -313,6 +418,103 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
\end{description}
+\drivernormative{\paragraph}{Control Requests}{Device Types / RTC Device / Device Operation / Control Requests}
+
+For VIRTIO_RTC_REQ_CROSS_CAP, the driver MUST set \field{hw_counter} to
+one of the hardware counter identifiers defined in this specification,
+or to a value between 0xF0 and 0xFE.
+
+\devicenormative{\paragraph}{Control Requests}{Device Types / RTC Device / Device Operation / Control Requests}
+
+For any clock of type VIRTIO_RTC_CLOCK_UTC, the device MUST use the UTC
+time standard (Coordinated Universal Time).
+
+For any clock of type VIRTIO_RTC_CLOCK_UTC_SMEARED or
+VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, the device MUST use the UTC time
+standard, insofar as the following requirements do not say otherwise.
+
+For any UTC-like clock, the device MUST use the time epoch of January 1,
+1970, 00:00 UTC.
+
+For any UTC-like clock, the device MUST count seconds since the epoch
+according to \hyperref[intro:EPOCH]{EPOCH}.
+
+For any clock of type VIRTIO_RTC_CLOCK_UTC, the device MUST apply a
+positive leap second according to the UTC time standard by
+instantaneously stepping the clock backwards by 1 s at the start of the
+leap second.
+
+For any clock of type VIRTIO_RTC_CLOCK_UTC, the device MUST apply a
+negative leap second according to the UTC time standard by
+instantaneously stepping the clock forward by 1 s at the start of the
+leap second.
+
+For any leap smearing clock, the device MUST NOT step the clock due to a
+leap second.
+
+For any leap smearing clock, on a positive leap second, the device MUST
+slow down the clock during part of the day containing the leap second
+and/or part of the day after the leap second.
+
+For any leap smearing clock, on a negative leap second, the device MUST
+speed up the clock during part of the day containing the leap second
+and/or part of the day after the leap second.
+
+For any clock with smearing variant VIRTIO_RTC_SMEAR_NOON_LINEAR, on a
+leap second, the device MUST change the frequency of the clock exactly
+from noon prior to the leap second until noon after the leap second.
+
+For any clock with smearing variant VIRTIO_RTC_SMEAR_NOON_LINEAR, while
+changing the frequency of the clock due to a positive leap second, the
+device MUST decrease the frequency of the clock by $1/86400$.
+
+For any clock with smearing variant VIRTIO_RTC_SMEAR_NOON_LINEAR, while
+changing the frequency of the clock due to a negative leap second, the
+device MUST increase the frequency of the clock by $1/86400$.
+
+For any clock with smearing variant VIRTIO_RTC_SMEAR_UTC_SLS, on a leap
+second, the device MUST change the frequency of the clock exactly during
+the last 1000 seconds of the day with the leap second.
+
+For any clock with smearing variant VIRTIO_RTC_SMEAR_UTC_SLS, while
+changing the frequency of the clock due to a positive leap second, the
+device MUST decrease the frequency of the clock by 0.1\%.
+
+For any clock with smearing variant VIRTIO_RTC_SMEAR_UTC_SLS, while
+changing the frequency of the clock due to a negative leap second, the
+device MUST increase the frequency of the clock by 0.1\%.
+
+For any clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, the device MAY
+deviate from the UTC standard with respect to leap second introduction.
+
+For any clock of type VIRTIO_RTC_CLOCK_TAI, the device MUST use the TAI
+time standard (International Atomic Time).
+
+For any clock of type VIRTIO_RTC_CLOCK_TAI, the device MUST use the time
+epoch of January 1, 1970, 00:00 TAI.
+
+For any clock of type VIRTIO_RTC_CLOCK_MONOTONIC, the device MUST use SI
+seconds subdivisions.
+
+For any clock of type VIRTIO_RTC_CLOCK_MONOTONIC, the device MUST use an
+epoch at a time instant before or during device reset.
+
+For VIRTIO_RTC_REQ_CLOCK_CAP, and clock types other than
+VIRTIO_RTC_CLOCK_UTC_SMEARED, the device MUST set field
+\field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
+
+For VIRTIO_RTC_REQ_CLOCK_CAP, and clock type
+VIRTIO_RTC_CLOCK_UTC_SMEARED, the device MUST set field
+\field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED,
+VIRTIO_RTC_SMEAR_NOON_LINEAR, VIRTIO_RTC_SMEAR_UTC_SLS, or to a value
+greater than or equal to 0xF0.
+
+For a VIRTIO_RTC_REQ_CROSS_CAP message M, the device MUST set the
+VIRTIO_RTC_FLAG_CROSS_CAP flag in the \field{flags} field if the device
+would respond to a VIRTIO_RTC_REQ_READ_CROSS message with the same
+\field{hw_counter} and \field{clock_id} values as in M with status
+VIRTIO_RTC_S_OK, and clear the flag otherwise.
+
\subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Operation / Read Requests}
Through \emph{read requests}, the driver requests clock readings from
@@ -424,3 +626,70 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
\ref{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}.
\end{description}
+
+\drivernormative{\paragraph}{Read Requests}{Device Types / RTC Device / Device Operation / Read Requests}
+
+For VIRTIO_RTC_REQ_READ_CROSS, the driver MUST set \field{hw_counter} to
+one of the hardware counter identifiers defined in this specification,
+or to a value between 0xF0 and 0xFE.
+
+\devicenormative{\paragraph}{Read Requests}{Device Types / RTC Device / Device Operation / Read Requests}
+
+The device SHOULD continuously support reading of all clocks once
+DRIVER_OK has been set.
+
+For any clock C and read requests \emph{A} and \emph{B} which read C,
+\emph{A} being the message which the driver marks as available before
+\emph{B}, the device MUST obtain the \field{clock_reading} value for the
+message \emph{A} response before the \field{clock_reading} value for the
+message \emph{B} response, or the device MUST obtain the same
+\field{clock_reading} value for both \emph{A} and \emph{B}.
+
+For any clock C, the device MUST mark all read requests reading C as
+used in the total order in which the driver marked these messages as
+available.
+
+For any clock C of type VIRTIO_RTC_CLOCK_MONOTONIC and read requests
+\emph{A} and \emph{B} which read C, \emph{A} being the message which the
+driver marks as available before \emph{B}, the device MUST set the
+\field{clock_reading} value for the message \emph{B} response to a value
+greater than or equal to the \field{clock_reading} value for the message
+\emph{A} response.
+
+The device MUST support VIRTIO_RTC_REQ_READ for every clock.
+
+For VIRTIO_RTC_REQ_READ and for any clock type listed in this
+specification, the device MUST use the nanosecond as unit for field
+\field{clock_reading}.
+
+For read requests, the device MUST obtain the \field{clock_reading}
+response value after the read request is marked as available.
+
+For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set
+\field{counter_cycles} to a value which approximates the value which the
+driver would have read from the hardware counter identified by
+\field{hw_counter} at the time instant when the device read the
+\field{clock_reading} value.
+
+For VIRTIO_RTC_REQ_READ_CROSS, the device SHOULD assume that the driver
+reads the hardware counter identified by \field{hw_counter} through the
+CPU which the driver enumerates as the first.
+
+For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set \field{status} to a
+value other than VIRTIO_RTC_S_OK if the device cannot determine the
+approximate value which the driver would have read from the hardware
+counter identified by \field{hw_counter} at the time instant when the
+device read the \field{clock_reading} value.
+
+For any clock C, and VIRTIO_RTC_REQ_READ_CROSS messages \emph{A} and
+\emph{B} which read C and which specify the same hardware counter
+\emph{H} through field \field{hw_counter}, \emph{A} being the message
+which the driver marks as available before \emph{B}, the device MUST set
+the \field{counter_cycles} value for \emph{B} to a value which \emph{H}
+shows after the \field{counter_cycles} value of \emph{A}, or the device
+MUST set the same \field{counter_cycles} value for \emph{A} and
+\emph{B}.
+
+For VIRTIO_RTC_REQ_READ_CROSS and for any clock type listed in
+this specification, the device MUST use the nanosecond as unit for
+field \field{clock_reading}.
diff --git a/device-types/rtc/device-conformance.tex b/device-types/rtc/device-conformance.tex
new file mode 100644
index 000000000000..4303cd450542
--- /dev/null
+++ b/device-types/rtc/device-conformance.tex
@@ -0,0 +1,9 @@
+\conformance{\subsection}{RTC Device Conformance}\label{sec:Conformance / Device Conformance / RTC Device Conformance}
+
+An RTC device MUST conform to the following normative statements:
+
+\begin{itemize}
+\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}
+\end{itemize}
diff --git a/device-types/rtc/driver-conformance.tex b/device-types/rtc/driver-conformance.tex
new file mode 100644
index 000000000000..689c18d158d0
--- /dev/null
+++ b/device-types/rtc/driver-conformance.tex
@@ -0,0 +1,9 @@
+\conformance{\subsection}{RTC Driver Conformance}\label{sec:Conformance / Driver Conformance / RTC Driver Conformance}
+
+An RTC driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\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}
+\end{itemize}
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v7 2/4] virtio-rtc: Add initial normative statements
2025-01-23 10:16 ` [PATCH v7 2/4] virtio-rtc: Add initial normative statements Peter Hilber
@ 2025-02-10 16:33 ` Matias Ezequiel Vara Larsen
2025-02-13 18:13 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-10 16:33 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Jan 23, 2025 at 11:16:13AM +0100, Peter Hilber wrote:
> Add the normative statements for the initial device specification.
>
> Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> ---
>
> Notes:
> v7:
>
> - Remove obsolete requirements for leap second indication.
>
> v6:
>
> - Update requirements to make leap second status information optional if
> if the clock smears (or might smear) leap seconds.
>
> - Allow indicating VIRTIO_RTC_SMEAR_UNSPECIFIED for
> VIRTIO_RTC_CLOCK_SMEARED.
>
> - Shorten some requirements by omitting redundant information.
>
> v5:
>
> - Update normative statements to match v5 changes to non-normative
> statements (patch 1).
>
> v4:
>
> - Require driver to set unused flags to zero.
>
> - Update normative statements to match v4 changes to non-normative
> statements (patch 1).
>
> - Improve formatting.
>
> conformance.tex | 2 +
> device-types/rtc/description.tex | 269 ++++++++++++++++++++++++
> device-types/rtc/device-conformance.tex | 9 +
> device-types/rtc/driver-conformance.tex | 9 +
> 4 files changed, 289 insertions(+)
> create mode 100644 device-types/rtc/device-conformance.tex
> create mode 100644 device-types/rtc/driver-conformance.tex
>
> diff --git a/conformance.tex b/conformance.tex
> index d92a2369488e..4ce86a525e84 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -162,6 +162,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> \input{device-types/pmem/driver-conformance.tex}
> \input{device-types/can/driver-conformance.tex}
> \input{device-types/spi/driver-conformance.tex}
> +\input{device-types/rtc/driver-conformance.tex}
>
> \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
>
> @@ -254,6 +255,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> \input{device-types/pmem/device-conformance.tex}
> \input{device-types/can/device-conformance.tex}
> \input{device-types/spi/device-conformance.tex}
> +\input{device-types/rtc/device-conformance.tex}
>
> \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
> A conformant implementation MUST be either transitional or
> diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
> index aae015690b26..2aefc22cb649 100644
> --- a/device-types/rtc/description.tex
> +++ b/device-types/rtc/description.tex
> @@ -98,6 +98,111 @@ \subsection{Device Operation}\label{sec:Device Types / RTC Device / Device Opera
> zero-based, dense indices. All fields named \field{clock_id} contain
> clock identifiers.
>
> +\drivernormative{\subsubsection}{Device Operation}{Device Types / RTC Device / Device Operation}
> +
> +If the \field{struct virtio_rtc_resp_head} field \field{status} is not
> +VIRTIO_RTC_S_OK, the driver MUST NOT interpret response fields other
> +than \field{status}.
> +
> +The driver MUST set \emph{reserved} fields in the device-readable part
> +of the message to zero.
> +
> +The driver MUST set unnamed bits in \emph{flags} fields in the
> +device-readable part of the message to zero.
> +
> +The driver MUST NOT interpret \emph{reserved} fields in the
> +device-writable part of the message.
> +
> +The driver MUST NOT interpret unnamed bits in \emph{flags} fields in the
> +device-writable part of the message.
> +
> +The driver MUST put the request into the device-readable part of the
> +message.
> +
> +The driver MUST allocate enough space for the response in the
> +device-writable part of a requestq message.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / RTC Device / Device Operation}
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_OK if the device successfully
> +executed the request.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to a status other than VIRTIO_RTC_S_OK if the
> +device did not successfully execute the request.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EOPNOTSUPP if the device could not
> +execute the specific request due to an implementation limitation.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EOPNOTSUPP for a request with a
> +value of the \field{msg_type} field which is not described in this
> +specification.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EOPNOTSUPP for a request with a
> +value of the \field{hw_counter} field which is neither described in this
> +specification nor otherwise known to the implementation.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_ENODEV if the \field{clock_id}
> +field value supplied with the request does not identify a clock.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EINVAL if the request values are
> +inconsistent with the specification and if the inconsistence is not
> +described by the requirements which stipulate status
> +VIRTIO_RTC_S_EOPNOTSUPP or VIRTIO_RTC_S_ENODEV.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EINVAL if the device read-only part
> +of the message is too small.
What do you mean with `too small`?
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EINVAL if the device write-only
> +part of the message is too small, unless the \field{status} field does
> +not fit into the device write-only part.
> +
I think we can improve the wording by replacing `too small` with
something else.
> +For \field{struct virtio_rtc_resp_head}, the device MUST NOT set the
> +\field{status} field if the \field{status} field does not fit into the
> +device write-only part.
> +
> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> +\field{status} field to VIRTIO_RTC_S_EIO if none of the previous
> +requirements in this document stipulated another \field{status}.
> +
> +If the device read-only part of a message M is bigger than the size of
> +the request specified for message M, the device MUST ignore the
> +additional space.
> +
> +If the device write-only part of a message M is bigger than the size of
> +the response specified for message M, the device MUST ignore the
> +additional space.
> +
> +The device MUST NOT interpret \emph{reserved} fields in the
> +device-readable part of the message.
I wonder if last three requirements are needed.
> +
> +The device MUST set \emph{reserved} fields in the device-writable part
> +of the message to zero.
> +
> +The device MUST set unnamed bits in \emph{flags} fields in the
> +device-writable part of the message to zero.
> +
> +After feature negotiation completion the device MUST NOT change the set
> +of clocks until device reset.
> +
> +The device SHOULD NOT change the set of clocks on a device reset after
> +the first device reset.
> +
> +The device MUST use non-negative integers, which are smaller than the
> +number of clocks, as clock identifiers.
> +
> +For a fixed request value V, the device SHOULD either, for every request
> +with value V, always execute successfully, or, for every request with
> +value V, always set a fixed \field{status} other than VIRTIO_RTC_S_OK.
I am not familiar enough with rtc clocks, what does `request with value
V` mean ?
> +
> \subsubsection{Common Definitions}\label{sec:Device Types / RTC Device / Device Operation / Common Definitions}
>
> This section makes common definitions.
> @@ -313,6 +418,103 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
>
> \end{description}
>
> +\drivernormative{\paragraph}{Control Requests}{Device Types / RTC Device / Device Operation / Control Requests}
> +
> +For VIRTIO_RTC_REQ_CROSS_CAP, the driver MUST set \field{hw_counter} to
> +one of the hardware counter identifiers defined in this specification,
> +or to a value between 0xF0 and 0xFE.
> +
> +\devicenormative{\paragraph}{Control Requests}{Device Types / RTC Device / Device Operation / Control Requests}
> +
> +For any clock of type VIRTIO_RTC_CLOCK_UTC, the device MUST use the UTC
> +time standard (Coordinated Universal Time).
> +
> +For any clock of type VIRTIO_RTC_CLOCK_UTC_SMEARED or
> +VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, the device MUST use the UTC time
> +standard, insofar as the following requirements do not say otherwise.
> +
> +For any UTC-like clock, the device MUST use the time epoch of January 1,
> +1970, 00:00 UTC.
> +
> +For any UTC-like clock, the device MUST count seconds since the epoch
> +according to \hyperref[intro:EPOCH]{EPOCH}.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_UTC, the device MUST apply a
> +positive leap second according to the UTC time standard by
> +instantaneously stepping the clock backwards by 1 s at the start of the
> +leap second.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_UTC, the device MUST apply a
> +negative leap second according to the UTC time standard by
> +instantaneously stepping the clock forward by 1 s at the start of the
> +leap second.
> +
> +For any leap smearing clock, the device MUST NOT step the clock due to a
> +leap second.
> +
> +For any leap smearing clock, on a positive leap second, the device MUST
> +slow down the clock during part of the day containing the leap second
> +and/or part of the day after the leap second.
> +
> +For any leap smearing clock, on a negative leap second, the device MUST
> +speed up the clock during part of the day containing the leap second
> +and/or part of the day after the leap second.
> +
> +For any clock with smearing variant VIRTIO_RTC_SMEAR_NOON_LINEAR, on a
> +leap second, the device MUST change the frequency of the clock exactly
> +from noon prior to the leap second until noon after the leap second.
> +
> +For any clock with smearing variant VIRTIO_RTC_SMEAR_NOON_LINEAR, while
> +changing the frequency of the clock due to a positive leap second, the
> +device MUST decrease the frequency of the clock by $1/86400$.
> +
> +For any clock with smearing variant VIRTIO_RTC_SMEAR_NOON_LINEAR, while
> +changing the frequency of the clock due to a negative leap second, the
> +device MUST increase the frequency of the clock by $1/86400$.
> +
> +For any clock with smearing variant VIRTIO_RTC_SMEAR_UTC_SLS, on a leap
> +second, the device MUST change the frequency of the clock exactly during
> +the last 1000 seconds of the day with the leap second.
> +
> +For any clock with smearing variant VIRTIO_RTC_SMEAR_UTC_SLS, while
> +changing the frequency of the clock due to a positive leap second, the
> +device MUST decrease the frequency of the clock by 0.1\%.
> +
> +For any clock with smearing variant VIRTIO_RTC_SMEAR_UTC_SLS, while
> +changing the frequency of the clock due to a negative leap second, the
> +device MUST increase the frequency of the clock by 0.1\%.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, the device MAY
> +deviate from the UTC standard with respect to leap second introduction.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_TAI, the device MUST use the TAI
> +time standard (International Atomic Time).
> +
> +For any clock of type VIRTIO_RTC_CLOCK_TAI, the device MUST use the time
> +epoch of January 1, 1970, 00:00 TAI.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_MONOTONIC, the device MUST use SI
> +seconds subdivisions.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_MONOTONIC, the device MUST use an
> +epoch at a time instant before or during device reset.
> +
> +For VIRTIO_RTC_REQ_CLOCK_CAP, and clock types other than
> +VIRTIO_RTC_CLOCK_UTC_SMEARED, the device MUST set field
> +\field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
> +
> +For VIRTIO_RTC_REQ_CLOCK_CAP, and clock type
> +VIRTIO_RTC_CLOCK_UTC_SMEARED, the device MUST set field
> +\field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED,
> +VIRTIO_RTC_SMEAR_NOON_LINEAR, VIRTIO_RTC_SMEAR_UTC_SLS, or to a value
> +greater than or equal to 0xF0.
> +
> +For a VIRTIO_RTC_REQ_CROSS_CAP message M, the device MUST set the
> +VIRTIO_RTC_FLAG_CROSS_CAP flag in the \field{flags} field if the device
> +would respond to a VIRTIO_RTC_REQ_READ_CROSS message with the same
> +\field{hw_counter} and \field{clock_id} values as in M with status
> +VIRTIO_RTC_S_OK, and clear the flag otherwise.
> +
> \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Operation / Read Requests}
>
> Through \emph{read requests}, the driver requests clock readings from
> @@ -424,3 +626,70 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
> \ref{sec:Device Types / RTC Device / Device Operation / Common Definitions / Hardware Counters}.
>
> \end{description}
> +
> +\drivernormative{\paragraph}{Read Requests}{Device Types / RTC Device / Device Operation / Read Requests}
> +
> +For VIRTIO_RTC_REQ_READ_CROSS, the driver MUST set \field{hw_counter} to
> +one of the hardware counter identifiers defined in this specification,
> +or to a value between 0xF0 and 0xFE.
> +
> +\devicenormative{\paragraph}{Read Requests}{Device Types / RTC Device / Device Operation / Read Requests}
> +
> +The device SHOULD continuously support reading of all clocks once
> +DRIVER_OK has been set.
> +
> +For any clock C and read requests \emph{A} and \emph{B} which read C,
> +\emph{A} being the message which the driver marks as available before
> +\emph{B}, the device MUST obtain the \field{clock_reading} value for the
> +message \emph{A} response before the \field{clock_reading} value for the
> +message \emph{B} response, or the device MUST obtain the same
> +\field{clock_reading} value for both \emph{A} and \emph{B}.
Can't we just say that the device MUST processes the available ring as FIFO
queue?
> +
> +For any clock C, the device MUST mark all read requests reading C as
> +used in the total order in which the driver marked these messages as
> +available.
> +
> +For any clock C of type VIRTIO_RTC_CLOCK_MONOTONIC and read requests
> +\emph{A} and \emph{B} which read C, \emph{A} being the message which the
> +driver marks as available before \emph{B}, the device MUST set the
> +\field{clock_reading} value for the message \emph{B} response to a value
> +greater than or equal to the \field{clock_reading} value for the message
> +\emph{A} response.
See comment above.
> +
> +The device MUST support VIRTIO_RTC_REQ_READ for every clock.
> +
> +For VIRTIO_RTC_REQ_READ and for any clock type listed in this
> +specification, the device MUST use the nanosecond as unit for field
> +\field{clock_reading}.
In other parts of the document the word `field` is written after the
name of the field. I think we could just put it after the name
everywhere.
> +
> +For read requests, the device MUST obtain the \field{clock_reading}
> +response value after the read request is marked as available.
> +
> +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set
> +\field{counter_cycles} to a value which approximates the value which the
> +driver would have read from the hardware counter identified by
> +\field{hw_counter} at the time instant when the device read the
> +\field{clock_reading} value.
s/read/reads
> +
> +For VIRTIO_RTC_REQ_READ_CROSS, the device SHOULD assume that the driver
> +reads the hardware counter identified by \field{hw_counter} through the
> +CPU which the driver enumerates as the first.
> +
> +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set \field{status} to a
> +value other than VIRTIO_RTC_S_OK if the device cannot determine the
> +approximate value which the driver would have read from the hardware
> +counter identified by \field{hw_counter} at the time instant when the
> +device read the \field{clock_reading} value.
s/read/reads
> +
> +For any clock C, and VIRTIO_RTC_REQ_READ_CROSS messages \emph{A} and
> +\emph{B} which read C and which specify the same hardware counter
> +\emph{H} through field \field{hw_counter}, \emph{A} being the message
> +which the driver marks as available before \emph{B}, the device MUST set
> +the \field{counter_cycles} value for \emph{B} to a value which \emph{H}
> +shows after the \field{counter_cycles} value of \emph{A}, or the device
> +MUST set the same \field{counter_cycles} value for \emph{A} and
> +\emph{B}.
Can't we just say that the request queue follows a FIFO semantics?
> +
> +For VIRTIO_RTC_REQ_READ_CROSS and for any clock type listed in
> +this specification, the device MUST use the nanosecond as unit for
> +field \field{clock_reading}.
> diff --git a/device-types/rtc/device-conformance.tex b/device-types/rtc/device-conformance.tex
> new file mode 100644
> index 000000000000..4303cd450542
> --- /dev/null
> +++ b/device-types/rtc/device-conformance.tex
> @@ -0,0 +1,9 @@
> +\conformance{\subsection}{RTC Device Conformance}\label{sec:Conformance / Device Conformance / RTC Device Conformance}
> +
> +An RTC device MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\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}
> +\end{itemize}
> diff --git a/device-types/rtc/driver-conformance.tex b/device-types/rtc/driver-conformance.tex
> new file mode 100644
> index 000000000000..689c18d158d0
> --- /dev/null
> +++ b/device-types/rtc/driver-conformance.tex
> @@ -0,0 +1,9 @@
> +\conformance{\subsection}{RTC Driver Conformance}\label{sec:Conformance / Driver Conformance / RTC Driver Conformance}
> +
> +An RTC driver MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\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}
> +\end{itemize}
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 2/4] virtio-rtc: Add initial normative statements
2025-02-10 16:33 ` Matias Ezequiel Vara Larsen
@ 2025-02-13 18:13 ` Peter Hilber
2025-02-19 14:58 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-13 18:13 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Mon, Feb 10, 2025 at 05:33:12PM +0100, Matias Ezequiel Vara Larsen wrote:
> On Thu, Jan 23, 2025 at 11:16:13AM +0100, Peter Hilber wrote:
> > Add the normative statements for the initial device specification.
> >
> > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > ---
> >
> > Notes:
> > v7:
> >
> > - Remove obsolete requirements for leap second indication.
> >
> > v6:
> >
> > - Update requirements to make leap second status information optional if
> > if the clock smears (or might smear) leap seconds.
> >
> > - Allow indicating VIRTIO_RTC_SMEAR_UNSPECIFIED for
> > VIRTIO_RTC_CLOCK_SMEARED.
> >
> > - Shorten some requirements by omitting redundant information.
> >
> > v5:
> >
> > - Update normative statements to match v5 changes to non-normative
> > statements (patch 1).
> >
> > v4:
> >
> > - Require driver to set unused flags to zero.
> >
> > - Update normative statements to match v4 changes to non-normative
> > statements (patch 1).
> >
> > - Improve formatting.
> >
> > conformance.tex | 2 +
> > device-types/rtc/description.tex | 269 ++++++++++++++++++++++++
> > device-types/rtc/device-conformance.tex | 9 +
> > device-types/rtc/driver-conformance.tex | 9 +
> > 4 files changed, 289 insertions(+)
> > create mode 100644 device-types/rtc/device-conformance.tex
> > create mode 100644 device-types/rtc/driver-conformance.tex
> >
[...]
> > +
> > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > +\field{status} field to VIRTIO_RTC_S_EINVAL if the request values are
> > +inconsistent with the specification and if the inconsistence is not
> > +described by the requirements which stipulate status
> > +VIRTIO_RTC_S_EOPNOTSUPP or VIRTIO_RTC_S_ENODEV.
> > +
> > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > +\field{status} field to VIRTIO_RTC_S_EINVAL if the device read-only part
> > +of the message is too small.
>
> What do you mean with `too small`?
>
"Too small" means that the cumulative length of the buffers marked as
device-readable is less than the (minimum) length of the request which
is indicated by the message type in the header.
Maybe I could reword like this?
For \field{struct virtio_rtc_resp_head}, the device MUST set the
\field{status} field to VIRTIO_RTC_S_EINVAL if the request
specified in the request header through the \field{msg_type}
field does not fit into the device read-only part of the actual
message.
> > +
> > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > +\field{status} field to VIRTIO_RTC_S_EINVAL if the device write-only
> > +part of the message is too small, unless the \field{status} field does
> > +not fit into the device write-only part.
> > +
>
> I think we can improve the wording by replacing `too small` with
> something else.
>
Maybe I could reword like this?
For \field{struct virtio_rtc_resp_head}, the device MUST set the
\field{status} field to VIRTIO_RTC_S_EINVAL if the response
specified in the request header through the \field{msg_type}
field does not fit into the device write-only part of the actual
message, unless the \field{status} field does not fit into the
device write-only part.
> > +For \field{struct virtio_rtc_resp_head}, the device MUST NOT set the
> > +\field{status} field if the \field{status} field does not fit into the
> > +device write-only part.
> > +
> > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > +\field{status} field to VIRTIO_RTC_S_EIO if none of the previous
> > +requirements in this document stipulated another \field{status}.
> > +
> > +If the device read-only part of a message M is bigger than the size of
> > +the request specified for message M, the device MUST ignore the
> > +additional space.
> > +
> > +If the device write-only part of a message M is bigger than the size of
> > +the response specified for message M, the device MUST ignore the
> > +additional space.
> > +
> > +The device MUST NOT interpret \emph{reserved} fields in the
> > +device-readable part of the message.
>
> I wonder if last three requirements are needed.
>
I included them because in my understanding this is not specified in the
generic part of the spec, and because I tried to be *somewhat*
forward-compatible.
Do you think they are unneeded because they are self-evident? Then I
think they could be dropped indeed. However, the IOMMU device explicitly
requires the device to reject a request if some reserved field is not
zero, and to ignore some other reserved field.
> > +
> > +The device MUST set \emph{reserved} fields in the device-writable part
> > +of the message to zero.
> > +
> > +The device MUST set unnamed bits in \emph{flags} fields in the
> > +device-writable part of the message to zero.
> > +
> > +After feature negotiation completion the device MUST NOT change the set
> > +of clocks until device reset.
> > +
> > +The device SHOULD NOT change the set of clocks on a device reset after
> > +the first device reset.
> > +
> > +The device MUST use non-negative integers, which are smaller than the
> > +number of clocks, as clock identifiers.
> > +
> > +For a fixed request value V, the device SHOULD either, for every request
> > +with value V, always execute successfully, or, for every request with
> > +value V, always set a fixed \field{status} other than VIRTIO_RTC_S_OK.
>
> I am not familiar enough with rtc clocks, what does `request with value
> V` mean ?
>
I tried to avoid ambiguity by emulating the language used in semi-formal
proofs. "V" refers to any particular request, e.g. VIRTIO_REQ_READ with
clock_id 1. For any such particular request the device SHOULD either
always return success, or always return the same error code.
Maybe I could rephrase like this:
The device SHOULD always set the same \field{status} in response
to identical requests.
> > +
> > +For any clock C and read requests \emph{A} and \emph{B} which read C,
> > +\emph{A} being the message which the driver marks as available before
> > +\emph{B}, the device MUST obtain the \field{clock_reading} value for the
> > +message \emph{A} response before the \field{clock_reading} value for the
> > +message \emph{B} response, or the device MUST obtain the same
> > +\field{clock_reading} value for both \emph{A} and \emph{B}.
>
> Can't we just say that the device MUST processes the available ring as FIFO
> queue?
>
The spec mandates FIFO processing for all the readings *of the same
clock* by the above requirement (in conjunction with the below
requirement, if you care about the used order).
The ordering requirement only applies to readings of the same clock, so
that clocks which are slow to read (such as GPS clocks read over UART),
or future slow commands, do not necessarily stall other processing, such
as fast clock readings.
To guarantee that a clock never goes backwards (unless the clock is
stepped backwards), the spec explicitly specifies the order of
clock_reading values of successive clock readings. Just mandating FIFO
processing would not prohibit a clock from going backwards.
E.g. when hardware counters are not properly synchronized across a
multi-core device, the clock can go backwards when read on different
cores. The spec mandates that the device avoid this. By this, the
driver does not have to do mitigation.
> > +
> > +For any clock C, the device MUST mark all read requests reading C as
> > +used in the total order in which the driver marked these messages as
> > +available.
> > +
> > +For any clock C of type VIRTIO_RTC_CLOCK_MONOTONIC and read requests
> > +\emph{A} and \emph{B} which read C, \emph{A} being the message which the
> > +driver marks as available before \emph{B}, the device MUST set the
> > +\field{clock_reading} value for the message \emph{B} response to a value
> > +greater than or equal to the \field{clock_reading} value for the message
> > +\emph{A} response.
>
> See comment above.
>
> > +
> > +The device MUST support VIRTIO_RTC_REQ_READ for every clock.
> > +
> > +For VIRTIO_RTC_REQ_READ and for any clock type listed in this
> > +specification, the device MUST use the nanosecond as unit for field
> > +\field{clock_reading}.
>
> In other parts of the document the word `field` is written after the
> name of the field. I think we could just put it after the name
> everywhere.
>
OK.
> > +
> > +For read requests, the device MUST obtain the \field{clock_reading}
> > +response value after the read request is marked as available.
> > +
> > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set
> > +\field{counter_cycles} to a value which approximates the value which the
> > +driver would have read from the hardware counter identified by
> > +\field{hw_counter} at the time instant when the device read the
> > +\field{clock_reading} value.
>
> s/read/reads
>
"read" seems correct to me. The last "read" in the above sentence is
simple past tense, since the device sets the field after reading the
clock_reading value.
> > +
> > +For VIRTIO_RTC_REQ_READ_CROSS, the device SHOULD assume that the driver
> > +reads the hardware counter identified by \field{hw_counter} through the
> > +CPU which the driver enumerates as the first.
> > +
> > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set \field{status} to a
> > +value other than VIRTIO_RTC_S_OK if the device cannot determine the
> > +approximate value which the driver would have read from the hardware
> > +counter identified by \field{hw_counter} at the time instant when the
> > +device read the \field{clock_reading} value.
>
> s/read/reads
>
The last "read" above is simple past tense, too.
> > +
> > +For any clock C, and VIRTIO_RTC_REQ_READ_CROSS messages \emph{A} and
> > +\emph{B} which read C and which specify the same hardware counter
> > +\emph{H} through field \field{hw_counter}, \emph{A} being the message
> > +which the driver marks as available before \emph{B}, the device MUST set
> > +the \field{counter_cycles} value for \emph{B} to a value which \emph{H}
> > +shows after the \field{counter_cycles} value of \emph{A}, or the device
> > +MUST set the same \field{counter_cycles} value for \emph{A} and
> > +\emph{B}.
>
> Can't we just say that the request queue follows a FIFO semantics?
>
As discussed above, this would not guarantee that the counter_cycles
value does not decrease across reads (ignoring rollovers), e.g. if
successive READ_CROSS messages are processed on different cores whose
hardware counters are not properly synchronized.
Thanks for the review,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 2/4] virtio-rtc: Add initial normative statements
2025-02-13 18:13 ` Peter Hilber
@ 2025-02-19 14:58 ` Matias Ezequiel Vara Larsen
2025-02-20 16:36 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-19 14:58 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Feb 13, 2025 at 07:13:12PM +0100, Peter Hilber wrote:
> On Mon, Feb 10, 2025 at 05:33:12PM +0100, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Jan 23, 2025 at 11:16:13AM +0100, Peter Hilber wrote:
> > > Add the normative statements for the initial device specification.
> > >
> > > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > > ---
> > >
> > > Notes:
> > > v7:
> > >
> > > - Remove obsolete requirements for leap second indication.
> > >
> > > v6:
> > >
> > > - Update requirements to make leap second status information optional if
> > > if the clock smears (or might smear) leap seconds.
> > >
> > > - Allow indicating VIRTIO_RTC_SMEAR_UNSPECIFIED for
> > > VIRTIO_RTC_CLOCK_SMEARED.
> > >
> > > - Shorten some requirements by omitting redundant information.
> > >
> > > v5:
> > >
> > > - Update normative statements to match v5 changes to non-normative
> > > statements (patch 1).
> > >
> > > v4:
> > >
> > > - Require driver to set unused flags to zero.
> > >
> > > - Update normative statements to match v4 changes to non-normative
> > > statements (patch 1).
> > >
> > > - Improve formatting.
> > >
> > > conformance.tex | 2 +
> > > device-types/rtc/description.tex | 269 ++++++++++++++++++++++++
> > > device-types/rtc/device-conformance.tex | 9 +
> > > device-types/rtc/driver-conformance.tex | 9 +
> > > 4 files changed, 289 insertions(+)
> > > create mode 100644 device-types/rtc/device-conformance.tex
> > > create mode 100644 device-types/rtc/driver-conformance.tex
> > >
>
> [...]
>
> > > +
> > > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > > +\field{status} field to VIRTIO_RTC_S_EINVAL if the request values are
> > > +inconsistent with the specification and if the inconsistence is not
> > > +described by the requirements which stipulate status
> > > +VIRTIO_RTC_S_EOPNOTSUPP or VIRTIO_RTC_S_ENODEV.
> > > +
> > > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > > +\field{status} field to VIRTIO_RTC_S_EINVAL if the device read-only part
> > > +of the message is too small.
> >
> > What do you mean with `too small`?
> >
>
> "Too small" means that the cumulative length of the buffers marked as
> device-readable is less than the (minimum) length of the request which
> is indicated by the message type in the header.
>
Oh, I see, you mean there is a mismatch between the type of the request
and the corresponding size of device read-only part.
> Maybe I could reword like this?
>
> For \field{struct virtio_rtc_resp_head}, the device MUST set the
> \field{status} field to VIRTIO_RTC_S_EINVAL if the request
> specified in the request header through the \field{msg_type}
> field does not fit into the device read-only part of the actual
> message.
I think it is better.
>
> > > +
> > > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > > +\field{status} field to VIRTIO_RTC_S_EINVAL if the device write-only
> > > +part of the message is too small, unless the \field{status} field does
> > > +not fit into the device write-only part.
> > > +
> >
> > I think we can improve the wording by replacing `too small` with
> > something else.
> >
>
> Maybe I could reword like this?
>
> For \field{struct virtio_rtc_resp_head}, the device MUST set the
> \field{status} field to VIRTIO_RTC_S_EINVAL if the response
> specified in the request header through the \field{msg_type}
> field does not fit into the device write-only part of the actual
> message, unless the \field{status} field does not fit into the
> device write-only part.
>
I think it is better.
> > > +For \field{struct virtio_rtc_resp_head}, the device MUST NOT set the
> > > +\field{status} field if the \field{status} field does not fit into the
> > > +device write-only part.
> > > +
> > > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > > +\field{status} field to VIRTIO_RTC_S_EIO if none of the previous
> > > +requirements in this document stipulated another \field{status}.
> > > +
> > > +If the device read-only part of a message M is bigger than the size of
> > > +the request specified for message M, the device MUST ignore the
> > > +additional space.
> > > +
> > > +If the device write-only part of a message M is bigger than the size of
> > > +the response specified for message M, the device MUST ignore the
> > > +additional space.
> > > +
> > > +The device MUST NOT interpret \emph{reserved} fields in the
> > > +device-readable part of the message.
> >
> > I wonder if last three requirements are needed.
> >
>
> I included them because in my understanding this is not specified in the
> generic part of the spec, and because I tried to be *somewhat*
> forward-compatible.
I see.
>
> Do you think they are unneeded because they are self-evident? Then I
> think they could be dropped indeed. However, the IOMMU device explicitly
> requires the device to reject a request if some reserved field is not
> zero, and to ignore some other reserved field.
I think is OK to have a requirement that says that the device rejects a
request if reserved fields are not zero. For the other requirements, if
you drop them, they will be implementation-specific. What is the
drawback of doing this?
>
> > > +
> > > +The device MUST set \emph{reserved} fields in the device-writable part
> > > +of the message to zero.
> > > +
> > > +The device MUST set unnamed bits in \emph{flags} fields in the
> > > +device-writable part of the message to zero.
> > > +
> > > +After feature negotiation completion the device MUST NOT change the set
> > > +of clocks until device reset.
> > > +
> > > +The device SHOULD NOT change the set of clocks on a device reset after
> > > +the first device reset.
> > > +
> > > +The device MUST use non-negative integers, which are smaller than the
> > > +number of clocks, as clock identifiers.
> > > +
> > > +For a fixed request value V, the device SHOULD either, for every request
> > > +with value V, always execute successfully, or, for every request with
> > > +value V, always set a fixed \field{status} other than VIRTIO_RTC_S_OK.
> >
> > I am not familiar enough with rtc clocks, what does `request with value
> > V` mean ?
> >
>
> I tried to avoid ambiguity by emulating the language used in semi-formal
> proofs. "V" refers to any particular request, e.g. VIRTIO_REQ_READ with
> clock_id 1. For any such particular request the device SHOULD either
> always return success, or always return the same error code.
>
> Maybe I could rephrase like this:
>
> The device SHOULD always set the same \field{status} in response
> to identical requests.
>
Oh, I see. Do you think we could drop this requirement? It is not clear
to me if it is required.
> > > +
> > > +For any clock C and read requests \emph{A} and \emph{B} which read C,
> > > +\emph{A} being the message which the driver marks as available before
> > > +\emph{B}, the device MUST obtain the \field{clock_reading} value for the
> > > +message \emph{A} response before the \field{clock_reading} value for the
> > > +message \emph{B} response, or the device MUST obtain the same
> > > +\field{clock_reading} value for both \emph{A} and \emph{B}.
> >
> > Can't we just say that the device MUST processes the available ring as FIFO
> > queue?
> >
>
> The spec mandates FIFO processing for all the readings *of the same
> clock* by the above requirement (in conjunction with the below
> requirement, if you care about the used order).
>
> The ordering requirement only applies to readings of the same clock, so
> that clocks which are slow to read (such as GPS clocks read over UART),
> or future slow commands, do not necessarily stall other processing, such
> as fast clock readings.
>
> To guarantee that a clock never goes backwards (unless the clock is
> stepped backwards), the spec explicitly specifies the order of
> clock_reading values of successive clock readings. Just mandating FIFO
> processing would not prohibit a clock from going backwards.
>
> E.g. when hardware counters are not properly synchronized across a
> multi-core device, the clock can go backwards when read on different
> cores. The spec mandates that the device avoid this. By this, the
> driver does not have to do mitigation.
The example helped a lot. You mean that the device must ensure that
requests that are read in order from the available ring, their output
values must respect that order. In a multicore system, I think
measurements should base always on the same physical clock for a given
clock to prevent getting the value from the clock in which the request
is processed. Does it make sense?
>
> > > +
> > > +For any clock C, the device MUST mark all read requests reading C as
> > > +used in the total order in which the driver marked these messages as
> > > +available.
> > > +
> > > +For any clock C of type VIRTIO_RTC_CLOCK_MONOTONIC and read requests
> > > +\emph{A} and \emph{B} which read C, \emph{A} being the message which the
> > > +driver marks as available before \emph{B}, the device MUST set the
> > > +\field{clock_reading} value for the message \emph{B} response to a value
> > > +greater than or equal to the \field{clock_reading} value for the message
> > > +\emph{A} response.
> >
> > See comment above.
> >
> > > +
> > > +The device MUST support VIRTIO_RTC_REQ_READ for every clock.
> > > +
> > > +For VIRTIO_RTC_REQ_READ and for any clock type listed in this
> > > +specification, the device MUST use the nanosecond as unit for field
> > > +\field{clock_reading}.
> >
> > In other parts of the document the word `field` is written after the
> > name of the field. I think we could just put it after the name
> > everywhere.
> >
>
> OK.
>
> > > +
> > > +For read requests, the device MUST obtain the \field{clock_reading}
> > > +response value after the read request is marked as available.
> > > +
> > > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set
> > > +\field{counter_cycles} to a value which approximates the value which the
> > > +driver would have read from the hardware counter identified by
> > > +\field{hw_counter} at the time instant when the device read the
> > > +\field{clock_reading} value.
> >
> > s/read/reads
> >
>
> "read" seems correct to me. The last "read" in the above sentence is
> simple past tense, since the device sets the field after reading the
> clock_reading value.
>
I wonder if it is not `have read` them, I may be wrong and simple past
tense is OK.
> > > +
> > > +For VIRTIO_RTC_REQ_READ_CROSS, the device SHOULD assume that the driver
> > > +reads the hardware counter identified by \field{hw_counter} through the
> > > +CPU which the driver enumerates as the first.
> > > +
> > > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set \field{status} to a
> > > +value other than VIRTIO_RTC_S_OK if the device cannot determine the
> > > +approximate value which the driver would have read from the hardware
> > > +counter identified by \field{hw_counter} at the time instant when the
> > > +device read the \field{clock_reading} value.
> >
> > s/read/reads
> >
>
> The last "read" above is simple past tense, too.
>
> > > +
> > > +For any clock C, and VIRTIO_RTC_REQ_READ_CROSS messages \emph{A} and
> > > +\emph{B} which read C and which specify the same hardware counter
> > > +\emph{H} through field \field{hw_counter}, \emph{A} being the message
> > > +which the driver marks as available before \emph{B}, the device MUST set
> > > +the \field{counter_cycles} value for \emph{B} to a value which \emph{H}
> > > +shows after the \field{counter_cycles} value of \emph{A}, or the device
> > > +MUST set the same \field{counter_cycles} value for \emph{A} and
> > > +\emph{B}.
> >
> > Can't we just say that the request queue follows a FIFO semantics?
> >
>
> As discussed above, this would not guarantee that the counter_cycles
> value does not decrease across reads (ignoring rollovers), e.g. if
> successive READ_CROSS messages are processed on different cores whose
> hardware counters are not properly synchronized.
Got it. Is that not a wrong implementation? Because the device must base
on the same physical clock. It must not base on the clock in which the
request is processed. In other words, the device should ensure that
request to a given clock (virtual) are got always from the same physical
clock.
Matias
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 2/4] virtio-rtc: Add initial normative statements
2025-02-19 14:58 ` Matias Ezequiel Vara Larsen
@ 2025-02-20 16:36 ` Peter Hilber
2025-02-24 11:09 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-20 16:36 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Wed, Feb 19, 2025 at 03:58:41PM +0100, Matias Ezequiel Vara Larsen wrote:
> On Thu, Feb 13, 2025 at 07:13:12PM +0100, Peter Hilber wrote:
> > On Mon, Feb 10, 2025 at 05:33:12PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > On Thu, Jan 23, 2025 at 11:16:13AM +0100, Peter Hilber wrote:
> > > > Add the normative statements for the initial device specification.
> > > >
> > > > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > > > ---
> > > >
> > > > Notes:
> > > > v7:
> > > >
> > > > - Remove obsolete requirements for leap second indication.
> > > >
> > > > v6:
> > > >
> > > > - Update requirements to make leap second status information optional if
> > > > if the clock smears (or might smear) leap seconds.
> > > >
> > > > - Allow indicating VIRTIO_RTC_SMEAR_UNSPECIFIED for
> > > > VIRTIO_RTC_CLOCK_SMEARED.
> > > >
> > > > - Shorten some requirements by omitting redundant information.
> > > >
> > > > v5:
> > > >
> > > > - Update normative statements to match v5 changes to non-normative
> > > > statements (patch 1).
> > > >
> > > > v4:
> > > >
> > > > - Require driver to set unused flags to zero.
> > > >
> > > > - Update normative statements to match v4 changes to non-normative
> > > > statements (patch 1).
> > > >
> > > > - Improve formatting.
> > > >
> > > > conformance.tex | 2 +
> > > > device-types/rtc/description.tex | 269 ++++++++++++++++++++++++
> > > > device-types/rtc/device-conformance.tex | 9 +
> > > > device-types/rtc/driver-conformance.tex | 9 +
> > > > 4 files changed, 289 insertions(+)
> > > > create mode 100644 device-types/rtc/device-conformance.tex
> > > > create mode 100644 device-types/rtc/driver-conformance.tex
> > > >
[...]
> > > > +For \field{struct virtio_rtc_resp_head}, the device MUST NOT set the
> > > > +\field{status} field if the \field{status} field does not fit into the
> > > > +device write-only part.
> > > > +
> > > > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > > > +\field{status} field to VIRTIO_RTC_S_EIO if none of the previous
> > > > +requirements in this document stipulated another \field{status}.
> > > > +
> > > > +If the device read-only part of a message M is bigger than the size of
> > > > +the request specified for message M, the device MUST ignore the
> > > > +additional space.
> > > > +
> > > > +If the device write-only part of a message M is bigger than the size of
> > > > +the response specified for message M, the device MUST ignore the
> > > > +additional space.
> > > > +
> > > > +The device MUST NOT interpret \emph{reserved} fields in the
> > > > +device-readable part of the message.
> > >
> > > I wonder if last three requirements are needed.
> > >
> >
> > I included them because in my understanding this is not specified in the
> > generic part of the spec, and because I tried to be *somewhat*
> > forward-compatible.
>
> I see.
>
> >
> > Do you think they are unneeded because they are self-evident? Then I
> > think they could be dropped indeed. However, the IOMMU device explicitly
> > requires the device to reject a request if some reserved field is not
> > zero, and to ignore some other reserved field.
>
> I think is OK to have a requirement that says that the device rejects a
> request if reserved fields are not zero. For the other requirements, if
> you drop them, they will be implementation-specific. What is the
> drawback of doing this?
>
If the ignore-size-is-bigger requirements are dropped, it complicates
the driver handling of any message which is lengthened through feature
bits in the future: the driver would then need to size the
request/response according to the negotiated features, instead of using
sizeof.
As for ignoring reserved fields in the device-readable part
(effectively: in the request), there is arguably no good reason for the
driver to put data in the request which the device will not process
according to the negotiated feature bits.
But does the device actually need to check that reserved fields are
zero? I would say no. Depending on future additions, there could also be
multiple reserved fields per request.
So my suggestion would be to drop the last requirement and retain the
other two.
(For the response, adding extra information not negotiated by feature
bits should be unproblematic. I think I added the requirement on the
request for symmetry.)
> >
> > > > +
> > > > +The device MUST set \emph{reserved} fields in the device-writable part
> > > > +of the message to zero.
> > > > +
> > > > +The device MUST set unnamed bits in \emph{flags} fields in the
> > > > +device-writable part of the message to zero.
> > > > +
> > > > +After feature negotiation completion the device MUST NOT change the set
> > > > +of clocks until device reset.
> > > > +
> > > > +The device SHOULD NOT change the set of clocks on a device reset after
> > > > +the first device reset.
> > > > +
> > > > +The device MUST use non-negative integers, which are smaller than the
> > > > +number of clocks, as clock identifiers.
> > > > +
> > > > +For a fixed request value V, the device SHOULD either, for every request
> > > > +with value V, always execute successfully, or, for every request with
> > > > +value V, always set a fixed \field{status} other than VIRTIO_RTC_S_OK.
> > >
> > > I am not familiar enough with rtc clocks, what does `request with value
> > > V` mean ?
> > >
> >
> > I tried to avoid ambiguity by emulating the language used in semi-formal
> > proofs. "V" refers to any particular request, e.g. VIRTIO_REQ_READ with
> > clock_id 1. For any such particular request the device SHOULD either
> > always return success, or always return the same error code.
> >
> > Maybe I could rephrase like this:
> >
> > The device SHOULD always set the same \field{status} in response
> > to identical requests.
> >
>
> Oh, I see. Do you think we could drop this requirement? It is not clear
> to me if it is required.
>
This requirement says that the device SHOULD respond to requests in an
consistent way, i.e. it SHOULD not sporadically fail requests. If there
are sporadic failures, clock synchronization daemons such as chrony may
choose a suboptimal clock reading method, i.e. one without READ_CROSS.
OTOH there is another requirement
The device SHOULD continuously support reading of all clocks
once DRIVER_OK has been set.
I think the latter might suffice and the former could indeed be dropped.
> > > > +
> > > > +For any clock C and read requests \emph{A} and \emph{B} which read C,
> > > > +\emph{A} being the message which the driver marks as available before
> > > > +\emph{B}, the device MUST obtain the \field{clock_reading} value for the
> > > > +message \emph{A} response before the \field{clock_reading} value for the
> > > > +message \emph{B} response, or the device MUST obtain the same
> > > > +\field{clock_reading} value for both \emph{A} and \emph{B}.
> > >
> > > Can't we just say that the device MUST processes the available ring as FIFO
> > > queue?
> > >
> >
> > The spec mandates FIFO processing for all the readings *of the same
> > clock* by the above requirement (in conjunction with the below
> > requirement, if you care about the used order).
> >
> > The ordering requirement only applies to readings of the same clock, so
> > that clocks which are slow to read (such as GPS clocks read over UART),
> > or future slow commands, do not necessarily stall other processing, such
> > as fast clock readings.
> >
> > To guarantee that a clock never goes backwards (unless the clock is
> > stepped backwards), the spec explicitly specifies the order of
> > clock_reading values of successive clock readings. Just mandating FIFO
> > processing would not prohibit a clock from going backwards.
> >
> > E.g. when hardware counters are not properly synchronized across a
> > multi-core device, the clock can go backwards when read on different
> > cores. The spec mandates that the device avoid this. By this, the
> > driver does not have to do mitigation.
>
> The example helped a lot. You mean that the device must ensure that
> requests that are read in order from the available ring, their output
> values must respect that order. In a multicore system, I think
> measurements should base always on the same physical clock for a given
> clock to prevent getting the value from the clock in which the request
> is processed. Does it make sense?
>
I would not say that the device always needs to use the same physical
clock (counter). It suffices if the physical clocks are synchronized
good enough so that the clock never goes backwards between successive
requests.
> >
> > > > +
> > > > +For any clock C, the device MUST mark all read requests reading C as
> > > > +used in the total order in which the driver marked these messages as
> > > > +available.
> > > > +
> > > > +For any clock C of type VIRTIO_RTC_CLOCK_MONOTONIC and read requests
> > > > +\emph{A} and \emph{B} which read C, \emph{A} being the message which the
> > > > +driver marks as available before \emph{B}, the device MUST set the
> > > > +\field{clock_reading} value for the message \emph{B} response to a value
> > > > +greater than or equal to the \field{clock_reading} value for the message
> > > > +\emph{A} response.
> > >
> > > See comment above.
> > >
> > > > +
> > > > +The device MUST support VIRTIO_RTC_REQ_READ for every clock.
> > > > +
> > > > +For VIRTIO_RTC_REQ_READ and for any clock type listed in this
> > > > +specification, the device MUST use the nanosecond as unit for field
> > > > +\field{clock_reading}.
> > >
> > > In other parts of the document the word `field` is written after the
> > > name of the field. I think we could just put it after the name
> > > everywhere.
> > >
> >
> > OK.
> >
> > > > +
> > > > +For read requests, the device MUST obtain the \field{clock_reading}
> > > > +response value after the read request is marked as available.
> > > > +
> > > > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set
> > > > +\field{counter_cycles} to a value which approximates the value which the
> > > > +driver would have read from the hardware counter identified by
> > > > +\field{hw_counter} at the time instant when the device read the
> > > > +\field{clock_reading} value.
> > >
> > > s/read/reads
> > >
> >
> > "read" seems correct to me. The last "read" in the above sentence is
> > simple past tense, since the device sets the field after reading the
> > clock_reading value.
> >
>
> I wonder if it is not `have read` them, I may be wrong and simple past
> tense is OK.
>
Can somebody help? I looked at some grammar website and I still think
simple past is the appropriate tense, because "the device read the
clock_reading value" describes something which finished in the past.
> > > > +
> > > > +For VIRTIO_RTC_REQ_READ_CROSS, the device SHOULD assume that the driver
> > > > +reads the hardware counter identified by \field{hw_counter} through the
> > > > +CPU which the driver enumerates as the first.
> > > > +
> > > > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set \field{status} to a
> > > > +value other than VIRTIO_RTC_S_OK if the device cannot determine the
> > > > +approximate value which the driver would have read from the hardware
> > > > +counter identified by \field{hw_counter} at the time instant when the
> > > > +device read the \field{clock_reading} value.
> > >
> > > s/read/reads
> > >
> >
> > The last "read" above is simple past tense, too.
> >
> > > > +
> > > > +For any clock C, and VIRTIO_RTC_REQ_READ_CROSS messages \emph{A} and
> > > > +\emph{B} which read C and which specify the same hardware counter
> > > > +\emph{H} through field \field{hw_counter}, \emph{A} being the message
> > > > +which the driver marks as available before \emph{B}, the device MUST set
> > > > +the \field{counter_cycles} value for \emph{B} to a value which \emph{H}
> > > > +shows after the \field{counter_cycles} value of \emph{A}, or the device
> > > > +MUST set the same \field{counter_cycles} value for \emph{A} and
> > > > +\emph{B}.
> > >
> > > Can't we just say that the request queue follows a FIFO semantics?
> > >
> >
> > As discussed above, this would not guarantee that the counter_cycles
> > value does not decrease across reads (ignoring rollovers), e.g. if
> > successive READ_CROSS messages are processed on different cores whose
> > hardware counters are not properly synchronized.
>
> Got it. Is that not a wrong implementation? Because the device must base
> on the same physical clock. It must not base on the clock in which the
> request is processed. In other words, the device should ensure that
> request to a given clock (virtual) are got always from the same physical
> clock.
>
> Matias
>
I think it should be up to the implementation to use any suitable
physical clock and translation mechanism to virtual clock (if required).
It should be possible to implement the device as a program which can
migrate across cores. The spec should only mandate that the responses to
the driver are self-consistent (which relieves the driver of
post-processing).
Thanks for the reply,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 2/4] virtio-rtc: Add initial normative statements
2025-02-20 16:36 ` Peter Hilber
@ 2025-02-24 11:09 ` Matias Ezequiel Vara Larsen
0 siblings, 0 replies; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-24 11:09 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Feb 20, 2025 at 05:36:15PM +0100, Peter Hilber wrote:
> On Wed, Feb 19, 2025 at 03:58:41PM +0100, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Feb 13, 2025 at 07:13:12PM +0100, Peter Hilber wrote:
> > > On Mon, Feb 10, 2025 at 05:33:12PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > > On Thu, Jan 23, 2025 at 11:16:13AM +0100, Peter Hilber wrote:
> > > > > Add the normative statements for the initial device specification.
> > > > >
> > > > > Signed-off-by: Peter Hilber <quic_philber@quicinc.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > v7:
> > > > >
> > > > > - Remove obsolete requirements for leap second indication.
> > > > >
> > > > > v6:
> > > > >
> > > > > - Update requirements to make leap second status information optional if
> > > > > if the clock smears (or might smear) leap seconds.
> > > > >
> > > > > - Allow indicating VIRTIO_RTC_SMEAR_UNSPECIFIED for
> > > > > VIRTIO_RTC_CLOCK_SMEARED.
> > > > >
> > > > > - Shorten some requirements by omitting redundant information.
> > > > >
> > > > > v5:
> > > > >
> > > > > - Update normative statements to match v5 changes to non-normative
> > > > > statements (patch 1).
> > > > >
> > > > > v4:
> > > > >
> > > > > - Require driver to set unused flags to zero.
> > > > >
> > > > > - Update normative statements to match v4 changes to non-normative
> > > > > statements (patch 1).
> > > > >
> > > > > - Improve formatting.
> > > > >
> > > > > conformance.tex | 2 +
> > > > > device-types/rtc/description.tex | 269 ++++++++++++++++++++++++
> > > > > device-types/rtc/device-conformance.tex | 9 +
> > > > > device-types/rtc/driver-conformance.tex | 9 +
> > > > > 4 files changed, 289 insertions(+)
> > > > > create mode 100644 device-types/rtc/device-conformance.tex
> > > > > create mode 100644 device-types/rtc/driver-conformance.tex
> > > > >
>
> [...]
>
> > > > > +For \field{struct virtio_rtc_resp_head}, the device MUST NOT set the
> > > > > +\field{status} field if the \field{status} field does not fit into the
> > > > > +device write-only part.
> > > > > +
> > > > > +For \field{struct virtio_rtc_resp_head}, the device MUST set the
> > > > > +\field{status} field to VIRTIO_RTC_S_EIO if none of the previous
> > > > > +requirements in this document stipulated another \field{status}.
> > > > > +
> > > > > +If the device read-only part of a message M is bigger than the size of
> > > > > +the request specified for message M, the device MUST ignore the
> > > > > +additional space.
> > > > > +
> > > > > +If the device write-only part of a message M is bigger than the size of
> > > > > +the response specified for message M, the device MUST ignore the
> > > > > +additional space.
> > > > > +
> > > > > +The device MUST NOT interpret \emph{reserved} fields in the
> > > > > +device-readable part of the message.
> > > >
> > > > I wonder if last three requirements are needed.
> > > >
> > >
> > > I included them because in my understanding this is not specified in the
> > > generic part of the spec, and because I tried to be *somewhat*
> > > forward-compatible.
> >
> > I see.
> >
> > >
> > > Do you think they are unneeded because they are self-evident? Then I
> > > think they could be dropped indeed. However, the IOMMU device explicitly
> > > requires the device to reject a request if some reserved field is not
> > > zero, and to ignore some other reserved field.
> >
> > I think is OK to have a requirement that says that the device rejects a
> > request if reserved fields are not zero. For the other requirements, if
> > you drop them, they will be implementation-specific. What is the
> > drawback of doing this?
> >
>
> If the ignore-size-is-bigger requirements are dropped, it complicates
> the driver handling of any message which is lengthened through feature
> bits in the future: the driver would then need to size the
> request/response according to the negotiated features, instead of using
> sizeof.
>
> As for ignoring reserved fields in the device-readable part
> (effectively: in the request), there is arguably no good reason for the
> driver to put data in the request which the device will not process
> according to the negotiated feature bits.
>
> But does the device actually need to check that reserved fields are
> zero? I would say no. Depending on future additions, there could also be
> multiple reserved fields per request.
>
> So my suggestion would be to drop the last requirement and retain the
> other two.
Sounds good.
>
> (For the response, adding extra information not negotiated by feature
> bits should be unproblematic. I think I added the requirement on the
> request for symmetry.)
>
> > >
> > > > > +
> > > > > +The device MUST set \emph{reserved} fields in the device-writable part
> > > > > +of the message to zero.
> > > > > +
> > > > > +The device MUST set unnamed bits in \emph{flags} fields in the
> > > > > +device-writable part of the message to zero.
> > > > > +
> > > > > +After feature negotiation completion the device MUST NOT change the set
> > > > > +of clocks until device reset.
> > > > > +
> > > > > +The device SHOULD NOT change the set of clocks on a device reset after
> > > > > +the first device reset.
> > > > > +
> > > > > +The device MUST use non-negative integers, which are smaller than the
> > > > > +number of clocks, as clock identifiers.
> > > > > +
> > > > > +For a fixed request value V, the device SHOULD either, for every request
> > > > > +with value V, always execute successfully, or, for every request with
> > > > > +value V, always set a fixed \field{status} other than VIRTIO_RTC_S_OK.
> > > >
> > > > I am not familiar enough with rtc clocks, what does `request with value
> > > > V` mean ?
> > > >
> > >
> > > I tried to avoid ambiguity by emulating the language used in semi-formal
> > > proofs. "V" refers to any particular request, e.g. VIRTIO_REQ_READ with
> > > clock_id 1. For any such particular request the device SHOULD either
> > > always return success, or always return the same error code.
> > >
> > > Maybe I could rephrase like this:
> > >
> > > The device SHOULD always set the same \field{status} in response
> > > to identical requests.
> > >
> >
> > Oh, I see. Do you think we could drop this requirement? It is not clear
> > to me if it is required.
> >
>
> This requirement says that the device SHOULD respond to requests in an
> consistent way, i.e. it SHOULD not sporadically fail requests. If there
> are sporadic failures, clock synchronization daemons such as chrony may
> choose a suboptimal clock reading method, i.e. one without READ_CROSS.
>
> OTOH there is another requirement
>
> The device SHOULD continuously support reading of all clocks
> once DRIVER_OK has been set.
>
> I think the latter might suffice and the former could indeed be dropped.
I think so or something like that the device is considered live after
DRIVER_OK command.
>
> > > > > +
> > > > > +For any clock C and read requests \emph{A} and \emph{B} which read C,
> > > > > +\emph{A} being the message which the driver marks as available before
> > > > > +\emph{B}, the device MUST obtain the \field{clock_reading} value for the
> > > > > +message \emph{A} response before the \field{clock_reading} value for the
> > > > > +message \emph{B} response, or the device MUST obtain the same
> > > > > +\field{clock_reading} value for both \emph{A} and \emph{B}.
> > > >
> > > > Can't we just say that the device MUST processes the available ring as FIFO
> > > > queue?
> > > >
> > >
> > > The spec mandates FIFO processing for all the readings *of the same
> > > clock* by the above requirement (in conjunction with the below
> > > requirement, if you care about the used order).
> > >
> > > The ordering requirement only applies to readings of the same clock, so
> > > that clocks which are slow to read (such as GPS clocks read over UART),
> > > or future slow commands, do not necessarily stall other processing, such
> > > as fast clock readings.
> > >
> > > To guarantee that a clock never goes backwards (unless the clock is
> > > stepped backwards), the spec explicitly specifies the order of
> > > clock_reading values of successive clock readings. Just mandating FIFO
> > > processing would not prohibit a clock from going backwards.
> > >
> > > E.g. when hardware counters are not properly synchronized across a
> > > multi-core device, the clock can go backwards when read on different
> > > cores. The spec mandates that the device avoid this. By this, the
> > > driver does not have to do mitigation.
> >
> > The example helped a lot. You mean that the device must ensure that
> > requests that are read in order from the available ring, their output
> > values must respect that order. In a multicore system, I think
> > measurements should base always on the same physical clock for a given
> > clock to prevent getting the value from the clock in which the request
> > is processed. Does it make sense?
> >
>
> I would not say that the device always needs to use the same physical
> clock (counter). It suffices if the physical clocks are synchronized
> good enough so that the clock never goes backwards between successive
> requests.
Right.
>
> > >
> > > > > +
> > > > > +For any clock C, the device MUST mark all read requests reading C as
> > > > > +used in the total order in which the driver marked these messages as
> > > > > +available.
> > > > > +
> > > > > +For any clock C of type VIRTIO_RTC_CLOCK_MONOTONIC and read requests
> > > > > +\emph{A} and \emph{B} which read C, \emph{A} being the message which the
> > > > > +driver marks as available before \emph{B}, the device MUST set the
> > > > > +\field{clock_reading} value for the message \emph{B} response to a value
> > > > > +greater than or equal to the \field{clock_reading} value for the message
> > > > > +\emph{A} response.
> > > >
> > > > See comment above.
> > > >
> > > > > +
> > > > > +The device MUST support VIRTIO_RTC_REQ_READ for every clock.
> > > > > +
> > > > > +For VIRTIO_RTC_REQ_READ and for any clock type listed in this
> > > > > +specification, the device MUST use the nanosecond as unit for field
> > > > > +\field{clock_reading}.
> > > >
> > > > In other parts of the document the word `field` is written after the
> > > > name of the field. I think we could just put it after the name
> > > > everywhere.
> > > >
> > >
> > > OK.
> > >
> > > > > +
> > > > > +For read requests, the device MUST obtain the \field{clock_reading}
> > > > > +response value after the read request is marked as available.
> > > > > +
> > > > > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set
> > > > > +\field{counter_cycles} to a value which approximates the value which the
> > > > > +driver would have read from the hardware counter identified by
> > > > > +\field{hw_counter} at the time instant when the device read the
> > > > > +\field{clock_reading} value.
> > > >
> > > > s/read/reads
> > > >
> > >
> > > "read" seems correct to me. The last "read" in the above sentence is
> > > simple past tense, since the device sets the field after reading the
> > > clock_reading value.
> > >
> >
> > I wonder if it is not `have read` them, I may be wrong and simple past
> > tense is OK.
> >
>
> Can somebody help? I looked at some grammar website and I still think
> simple past is the appropriate tense, because "the device read the
> clock_reading value" describes something which finished in the past.
>
I think simple past is then alright.
> > > > > +
> > > > > +For VIRTIO_RTC_REQ_READ_CROSS, the device SHOULD assume that the driver
> > > > > +reads the hardware counter identified by \field{hw_counter} through the
> > > > > +CPU which the driver enumerates as the first.
> > > > > +
> > > > > +For VIRTIO_RTC_REQ_READ_CROSS, the device MUST set \field{status} to a
> > > > > +value other than VIRTIO_RTC_S_OK if the device cannot determine the
> > > > > +approximate value which the driver would have read from the hardware
> > > > > +counter identified by \field{hw_counter} at the time instant when the
> > > > > +device read the \field{clock_reading} value.
> > > >
> > > > s/read/reads
> > > >
> > >
> > > The last "read" above is simple past tense, too.
> > >
> > > > > +
> > > > > +For any clock C, and VIRTIO_RTC_REQ_READ_CROSS messages \emph{A} and
> > > > > +\emph{B} which read C and which specify the same hardware counter
> > > > > +\emph{H} through field \field{hw_counter}, \emph{A} being the message
> > > > > +which the driver marks as available before \emph{B}, the device MUST set
> > > > > +the \field{counter_cycles} value for \emph{B} to a value which \emph{H}
> > > > > +shows after the \field{counter_cycles} value of \emph{A}, or the device
> > > > > +MUST set the same \field{counter_cycles} value for \emph{A} and
> > > > > +\emph{B}.
> > > >
> > > > Can't we just say that the request queue follows a FIFO semantics?
> > > >
> > >
> > > As discussed above, this would not guarantee that the counter_cycles
> > > value does not decrease across reads (ignoring rollovers), e.g. if
> > > successive READ_CROSS messages are processed on different cores whose
> > > hardware counters are not properly synchronized.
> >
> > Got it. Is that not a wrong implementation? Because the device must base
> > on the same physical clock. It must not base on the clock in which the
> > request is processed. In other words, the device should ensure that
> > request to a given clock (virtual) are got always from the same physical
> > clock.
> >
> > Matias
> >
>
> I think it should be up to the implementation to use any suitable
> physical clock and translation mechanism to virtual clock (if required).
> It should be possible to implement the device as a program which can
> migrate across cores. The spec should only mandate that the responses to
> the driver are self-consistent (which relieves the driver of
> post-processing).
>
I see. It makes sense. Thanks.
Matias
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v7 3/4] virtio-rtc: Add alarm feature
2025-01-23 10:16 [PATCH v7 0/4] virtio-rtc: Add device specification Peter Hilber
2025-01-23 10:16 ` [PATCH v7 1/4] virtio-rtc: Add initial " Peter Hilber
2025-01-23 10:16 ` [PATCH v7 2/4] virtio-rtc: Add initial normative statements Peter Hilber
@ 2025-01-23 10:16 ` Peter Hilber
2025-02-11 11:51 ` Matias Ezequiel Vara Larsen
2025-01-23 10:16 ` [PATCH v7 4/4] virtio-rtc: Add normative statements for " Peter Hilber
3 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-01-23 10:16 UTC (permalink / raw)
To: virtio-comment
Cc: Cornelia Huck, Parav Pandit, Jason Wang, David Woodhouse,
Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri, Peter Hilber
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/
device-types/rtc/description.tex | 270 ++++++++++++++++++++++++++++++-
1 file changed, 268 insertions(+), 2 deletions(-)
diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
index 2aefc22cb649..47ad50cd95ca 100644
--- a/device-types/rtc/description.tex
+++ b/device-types/rtc/description.tex
@@ -4,6 +4,7 @@ \section{RTC Device}\label{sec:Device Types / RTC Device}
time. The device can provide different clocks, e.g.\ for the UTC or TAI
time standards, or for physical time elapsed since some past epoch. The
driver can read the clocks with simple or more accurate methods.
+Optionally, the driver can set an alarm.
\subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
@@ -13,13 +14,23 @@ \subsection{Virtqueues}\label{sec:Device Types / RTC Device / Virtqueues}
\begin{description}
\item[0] requestq
+\item[1] alarmq
\end{description}
The driver enqueues requests to the requestq.
+Through the alarmq, the device notifies the driver about alarm
+expirations. The alarmq exists only if VIRTIO_RTC_F_ALARM was
+negotiated.
+
\subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
-No device-specific feature bits are defined yet.
+\begin{description}
+\item[VIRTIO_RTC_F_ALARM (0)] Device supports alarm.
+\end{description}
+
+VIRTIO_RTC_F_ALARM determines whether the device supports setting an
+alarm for some of the clocks.
\subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
@@ -376,7 +387,8 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
struct virtio_rtc_resp_head head;
u8 type;
u8 leap_second_smearing;
- u8 reserved[6];
+ u8 flags;
+ u8 reserved[5];
};
\end{lstlisting}
@@ -387,6 +399,15 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
variant} through field \field{leap_second_smearing}. All other clocks
set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
+The \field{flags} field provides the following information:
+
+\begin{lstlisting}
+#define VIRTIO_RTC_FLAG_ALARM_CAP (1 << 0)
+\end{lstlisting}
+
+If VIRTIO_RTC_F_ALARM was negotiated, flag VIRTIO_RTC_FLAG_ALARM_CAP
+indicates that the clock supports an alarm.
+
\item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
cross-timestamping for a particular pair of clock and hardware counter.
@@ -693,3 +714,248 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
For VIRTIO_RTC_REQ_READ_CROSS and for any clock type listed in
this specification, the device MUST use the nanosecond as unit for
field \field{clock_reading}.
+
+\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
+alarm expiration, the device notifies the driver. On alarm expiration,
+the device may also wake up the driver, while the driver is in a sleep
+state, or while the driver is powered off. How this is done is beyond
+the scope of the specification. The driver can set one alarm time per
+clock, if the clock supports this.
+
+The device may retain alarm times across device resets.\footnote{Drivers
+ may reset the device on boot or on resume from sleep state. It
+ can make sense for the device to retain the alarm time then,
+ similar to other alarm clocks.}
+
+The alarm feature, and the associated alarmq for notifications from the
+device, are available if VIRTIO_RTC_F_ALARM was negotiated. In addition,
+if the driver previously set an alarm time, even if the device no longer
+both
+
+\begin{itemize}
+\item is live and
+\item has negotiated VIRTIO_RTC_F_ALARM,
+\end{itemize}
+
+the device may still execute implementation-specific actions on alarm
+expiration.
+
+An alarm expires
+
+\begin{itemize}
+\item 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, or
+
+\item when the driver sets an alarm time which is not in the future, or
+
+\item when the device is reset, if the alarm time is retained and not in
+ the future.\footnote{The device is always responsible for
+ detecting alarm expiration events. This avoids that the driver
+ needs to reason about when it shall poll for alarm expiration.}
+\end{itemize}
+
+When an alarm expires, the driver can disable it. Otherwise, the alarm
+expires each time when one of the above expiration events occurs, even
+if it occurred before.\footnote{This avoids that the driver may
+ miss an alarm when the clock steps backwards after alarm
+ expiration, but before the driver has resumed operation. This
+ also facilitates distinct drivers using the same device,
+ e.g.\ a driver in the bootloader, and a driver in the OS.}
+
+On alarm expiration, the device executes the alarm actions. The alarm
+actions are:
+
+\begin{itemize}
+\item The device notifies the driver through the alarmq. If the device
+ is not live, or no buffers are available in the alarmq, the
+ device will notify once the device is live and buffers are
+ available.
+
+\item Optionally, the device executes other, implementation-specific,
+ actions. The device may execute those immediately, regardless of
+ the device state.
+\end{itemize}
+
+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}
+
+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.
+
+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}
+
+The device supports all \field{driver_alarm_time} values which the
+driver can request through alarm control requests.
+
+Initially, \field{driver_alarm_time} is the epoch of the clock ($0$),
+and \field{alarm_enabled} is false.
+
+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.
+
+\paragraph{Alarm Control Requests}
+
+If VIRTIO_RTC_F_ALARM is negotiated,
+
+\begin{itemize}
+\item the driver can determine if a clock supports an alarm through the
+ VIRTIO_RTC_FLAG_ALARM_CAP flag in the VIRTIO_RTC_REQ_CLOCK_CAP
+ response,
+
+\item the driver can enqueue the alarm control requests into the
+ requestq: VIRTIO_RTC_REQ_READ_ALARM, VIRTIO_RTC_REQ_SET_ALARM,
+ and VIRTIO_RTC_REQ_SET_ALARM_ENABLED.
+\end{itemize}
+
+The unit of all \field{alarm_time} fields is 1 nanosecond.
+
+\begin{description}
+\item[VIRTIO_RTC_REQ_READ_ALARM] reads the current alarm.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_READ_ALARM 0x1003 /* message type */
+
+struct virtio_rtc_req_read_alarm {
+ struct virtio_rtc_req_head head;
+ le16 clock_id;
+ u8 reserved[6];
+};
+
+struct virtio_rtc_resp_read_alarm {
+ struct virtio_rtc_resp_head head;
+ le64 alarm_time;
+#define VIRTIO_RTC_FLAG_ALARM_ENABLED (1 << 0)
+ u8 flags;
+ u8 reserved[7];
+};
+\end{lstlisting}
+
+\field{clock_id} identifies the alarm through its associated clock.
+Field \field{alarm_time} returns \field{driver_alarm_time}. In field
+\field{flags}, flag VIRTIO_RTC_FLAG_ALARM_ENABLED indicates whether
+\field{alarm_enabled} is true.
+
+\item[VIRTIO_RTC_REQ_SET_ALARM] sets the alarm.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_SET_ALARM 0x1004 /* message type */
+
+struct virtio_rtc_req_set_alarm {
+ struct virtio_rtc_req_head head;
+ le64 alarm_time;
+ le16 clock_id;
+ /* flag: VIRTIO_RTC_FLAG_ALARM_ENABLED */
+ u8 flags;
+ u8 reserved[5];
+};
+
+struct virtio_rtc_resp_set_alarm {
+ struct virtio_rtc_resp_head head;
+ /* no response params */
+};
+\end{lstlisting}
+
+\field{clock_id} identifies the alarm through its associated clock.
+Field \field{alarm_time} sets \field{driver_alarm_time}. If flag
+VIRTIO_RTC_FLAG_ALARM_ENABLED is set in field \field{flags}, the device
+sets \field{alarm_enabled} to true; otherwise, the device sets
+\field{alarm_enabled} to false.
+
+\item[VIRTIO_RTC_REQ_SET_ALARM_ENABLED] sets \field{alarm_enabled}.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_REQ_SET_ALARM_ENABLED 0x1005 /* message type */
+
+struct virtio_rtc_req_set_alarm_enabled {
+ struct virtio_rtc_req_head head;
+ le16 clock_id;
+ /* flag: VIRTIO_RTC_FLAG_ALARM_ENABLED */
+ u8 flags;
+ u8 reserved[5];
+};
+
+struct virtio_rtc_resp_set_alarm_enabled {
+ struct virtio_rtc_resp_head head;
+ /* no response params */
+};
+\end{lstlisting}
+
+\field{clock_id} identifies the alarm through its associated clock. If
+flag VIRTIO_RTC_FLAG_ALARM_ENABLED is set in field \field{flags}, the
+device sets \field{alarm_enabled} to true; otherwise, the device sets
+\field{alarm_enabled} to false.
+
+When processing this request, the device retains
+\field{driver_alarm_time}.
+\end{description}
+
+\paragraph{Alarm Notifications}
+
+If the alarmq is present, the driver should make buffers available in
+the alarmq, which the device uses for alarm notification messages. All
+alarmq fields are device-writable. The alarmq uses a common notification
+header.
+
+\begin{lstlisting}
+/* common notification header */
+struct virtio_rtc_notif_head {
+ le16 msg_type;
+ u8 reserved[6];
+};
+\end{lstlisting}
+
+The \field{msg_type} field identifies the message type.
+
+\begin{description}
+\item[VIRTIO_RTC_NOTIF_ALARM] notifies the driver about an alarm
+ expiration.
+
+\begin{lstlisting}
+#define VIRTIO_RTC_NOTIF_ALARM 0x2000 /* message type */
+
+struct virtio_rtc_notif_alarm {
+ struct virtio_rtc_notif_head head;
+ le16 clock_id;
+ u8 reserved[6];
+};
+\end{lstlisting}
+
+\end{description}
+
+\field{clock_id} identifies the expired alarm through its associated
+clock.
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v7 3/4] virtio-rtc: Add alarm feature
2025-01-23 10:16 ` [PATCH v7 3/4] virtio-rtc: Add alarm feature Peter Hilber
@ 2025-02-11 11:51 ` Matias Ezequiel Vara Larsen
2025-02-13 18:13 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-11 11:51 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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/
>
> device-types/rtc/description.tex | 270 ++++++++++++++++++++++++++++++-
> 1 file changed, 268 insertions(+), 2 deletions(-)
>
> diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
> index 2aefc22cb649..47ad50cd95ca 100644
> --- a/device-types/rtc/description.tex
> +++ b/device-types/rtc/description.tex
> @@ -4,6 +4,7 @@ \section{RTC Device}\label{sec:Device Types / RTC Device}
> time. The device can provide different clocks, e.g.\ for the UTC or TAI
> time standards, or for physical time elapsed since some past epoch. The
> driver can read the clocks with simple or more accurate methods.
> +Optionally, the driver can set an alarm.
>
> \subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
>
> @@ -13,13 +14,23 @@ \subsection{Virtqueues}\label{sec:Device Types / RTC Device / Virtqueues}
>
> \begin{description}
> \item[0] requestq
> +\item[1] alarmq
> \end{description}
>
> The driver enqueues requests to the requestq.
>
> +Through the alarmq, the device notifies the driver about alarm
> +expirations. The alarmq exists only if VIRTIO_RTC_F_ALARM was
> +negotiated.
> +
> \subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
>
> -No device-specific feature bits are defined yet.
> +\begin{description}
> +\item[VIRTIO_RTC_F_ALARM (0)] Device supports alarm.
> +\end{description}
> +
> +VIRTIO_RTC_F_ALARM determines whether the device supports setting an
> +alarm for some of the clocks.
>
> \subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
>
> @@ -376,7 +387,8 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
> struct virtio_rtc_resp_head head;
> u8 type;
> u8 leap_second_smearing;
> - u8 reserved[6];
> + u8 flags;
I wonder if you can just define flags in the first patch instead of
introduce it here. I think flags field may be used for other purpose in
the future but I do not have an strong opinion.
> + u8 reserved[5];
> };
> \end{lstlisting}
>
> @@ -387,6 +399,15 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
> variant} through field \field{leap_second_smearing}. All other clocks
> set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
>
> +The \field{flags} field provides the following information:
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_FLAG_ALARM_CAP (1 << 0)
> +\end{lstlisting}
> +
> +If VIRTIO_RTC_F_ALARM was negotiated, flag VIRTIO_RTC_FLAG_ALARM_CAP
> +indicates that the clock supports an alarm.
> +
> \item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
> cross-timestamping for a particular pair of clock and hardware counter.
>
> @@ -693,3 +714,248 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
> For VIRTIO_RTC_REQ_READ_CROSS and for any clock type listed in
> this specification, the device MUST use the nanosecond as unit for
> field \field{clock_reading}.
> +
> +\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
> +alarm expiration, the device notifies the driver. On alarm expiration,
> +the device may also wake up the driver, while the driver is in a sleep
> +state, or while the driver is powered off. How this is done is beyond
> +the scope of the specification. The driver can set one alarm time per
> +clock, if the clock supports this.
> +
> +The device may retain alarm times across device resets.\footnote{Drivers
> + may reset the device on boot or on resume from sleep state. It
> + can make sense for the device to retain the alarm time then,
> + similar to other alarm clocks.}
> +
> +The alarm feature, and the associated alarmq for notifications from the
> +device, are available if VIRTIO_RTC_F_ALARM was negotiated. In addition,
> +if the driver previously set an alarm time, even if the device no longer
> +both
> +
> +\begin{itemize}
> +\item is live and
> +\item has negotiated VIRTIO_RTC_F_ALARM,
> +\end{itemize}
> +
> +the device may still execute implementation-specific actions on alarm
> +expiration.
> +
> +An alarm expires
> +
> +\begin{itemize}
> +\item 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, or
> +
> +\item when the driver sets an alarm time which is not in the future, or
> +
> +\item when the device is reset, if the alarm time is retained and not in
> + the future.\footnote{The device is always responsible for
> + detecting alarm expiration events. This avoids that the driver
> + needs to reason about when it shall poll for alarm expiration.}
> +\end{itemize}
> +
> +When an alarm expires, the driver can disable it. Otherwise, the alarm
> +expires each time when one of the above expiration events occurs, even
> +if it occurred before.\footnote{This avoids that the driver may
When an alarm expires, the device keeps notifying the driver that the
alarm has expired? is that implementation-specific?
> + miss an alarm when the clock steps backwards after alarm
> + expiration, but before the driver has resumed operation. This
> + also facilitates distinct drivers using the same device,
> + e.g.\ a driver in the bootloader, and a driver in the OS.}
> +
> +On alarm expiration, the device executes the alarm actions. The alarm
> +actions are:
> +
> +\begin{itemize}
> +\item The device notifies the driver through the alarmq. If the device
Do you mean the driver?
> + is not live, or no buffers are available in the alarmq, the
> + device will notify once the device is live and buffers are
> + available.
> +
> +\item Optionally, the device executes other, implementation-specific,
> + actions. The device may execute those immediately, regardless of
> + the device state.
> +\end{itemize}
> +
> +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.
> +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?
> +
> +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?
> +
> +The device supports all \field{driver_alarm_time} values which the
> +driver can request through alarm control requests.
> +
> +Initially, \field{driver_alarm_time} is the epoch of the clock ($0$),
> +and \field{alarm_enabled} is false.
> +
> +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.
> +
> +\paragraph{Alarm Control Requests}
> +
> +If VIRTIO_RTC_F_ALARM is negotiated,
> +
> +\begin{itemize}
> +\item the driver can determine if a clock supports an alarm through the
> + VIRTIO_RTC_FLAG_ALARM_CAP flag in the VIRTIO_RTC_REQ_CLOCK_CAP
> + response,
> +
> +\item the driver can enqueue the alarm control requests into the
> + requestq: VIRTIO_RTC_REQ_READ_ALARM, VIRTIO_RTC_REQ_SET_ALARM,
> + and VIRTIO_RTC_REQ_SET_ALARM_ENABLED.
> +\end{itemize}
> +
> +The unit of all \field{alarm_time} fields is 1 nanosecond.
> +
> +\begin{description}
> +\item[VIRTIO_RTC_REQ_READ_ALARM] reads the current alarm.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_READ_ALARM 0x1003 /* message type */
> +
> +struct virtio_rtc_req_read_alarm {
> + struct virtio_rtc_req_head head;
> + le16 clock_id;
> + u8 reserved[6];
> +};
> +
> +struct virtio_rtc_resp_read_alarm {
> + struct virtio_rtc_resp_head head;
> + le64 alarm_time;
> +#define VIRTIO_RTC_FLAG_ALARM_ENABLED (1 << 0)
> + u8 flags;
> + u8 reserved[7];
> +};
> +\end{lstlisting}
> +
> +\field{clock_id} identifies the alarm through its associated clock.
> +Field \field{alarm_time} returns \field{driver_alarm_time}. In field
> +\field{flags}, flag VIRTIO_RTC_FLAG_ALARM_ENABLED indicates whether
> +\field{alarm_enabled} is true.
> +
> +\item[VIRTIO_RTC_REQ_SET_ALARM] sets the alarm.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_SET_ALARM 0x1004 /* message type */
> +
> +struct virtio_rtc_req_set_alarm {
> + struct virtio_rtc_req_head head;
> + le64 alarm_time;
> + le16 clock_id;
> + /* flag: VIRTIO_RTC_FLAG_ALARM_ENABLED */
> + u8 flags;
> + u8 reserved[5];
> +};
> +
> +struct virtio_rtc_resp_set_alarm {
> + struct virtio_rtc_resp_head head;
> + /* no response params */
> +};
> +\end{lstlisting}
> +
> +\field{clock_id} identifies the alarm through its associated clock.
> +Field \field{alarm_time} sets \field{driver_alarm_time}. If flag
> +VIRTIO_RTC_FLAG_ALARM_ENABLED is set in field \field{flags}, the device
> +sets \field{alarm_enabled} to true; otherwise, the device sets
> +\field{alarm_enabled} to false.
> +
> +\item[VIRTIO_RTC_REQ_SET_ALARM_ENABLED] sets \field{alarm_enabled}.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_REQ_SET_ALARM_ENABLED 0x1005 /* message type */
> +
> +struct virtio_rtc_req_set_alarm_enabled {
> + struct virtio_rtc_req_head head;
> + le16 clock_id;
> + /* flag: VIRTIO_RTC_FLAG_ALARM_ENABLED */
> + u8 flags;
> + u8 reserved[5];
> +};
> +
> +struct virtio_rtc_resp_set_alarm_enabled {
> + struct virtio_rtc_resp_head head;
> + /* no response params */
> +};
> +\end{lstlisting}
> +
> +\field{clock_id} identifies the alarm through its associated clock. If
> +flag VIRTIO_RTC_FLAG_ALARM_ENABLED is set in field \field{flags}, the
> +device sets \field{alarm_enabled} to true; otherwise, the device sets
> +\field{alarm_enabled} to false.
> +
> +When processing this request, the device retains
> +\field{driver_alarm_time}.
> +\end{description}
> +
> +\paragraph{Alarm Notifications}
> +
> +If the alarmq is present, the driver should make buffers available in
> +the alarmq, which the device uses for alarm notification messages. All
> +alarmq fields are device-writable. The alarmq uses a common notification
> +header.
> +
> +\begin{lstlisting}
> +/* common notification header */
> +struct virtio_rtc_notif_head {
> + le16 msg_type;
> + u8 reserved[6];
> +};
> +\end{lstlisting}
> +
> +The \field{msg_type} field identifies the message type.
> +
> +\begin{description}
> +\item[VIRTIO_RTC_NOTIF_ALARM] notifies the driver about an alarm
> + expiration.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_NOTIF_ALARM 0x2000 /* message type */
> +
> +struct virtio_rtc_notif_alarm {
> + struct virtio_rtc_notif_head head;
> + le16 clock_id;
> + u8 reserved[6];
> +};
> +\end{lstlisting}
> +
> +\end{description}
> +
> +\field{clock_id} identifies the expired alarm through its associated
> +clock.
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 3/4] virtio-rtc: Add alarm feature
2025-02-11 11:51 ` Matias Ezequiel Vara Larsen
@ 2025-02-13 18:13 ` Peter Hilber
2025-02-19 16:08 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-13 18:13 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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/
> >
> > device-types/rtc/description.tex | 270 ++++++++++++++++++++++++++++++-
> > 1 file changed, 268 insertions(+), 2 deletions(-)
> >
> > diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
> > index 2aefc22cb649..47ad50cd95ca 100644
> > --- a/device-types/rtc/description.tex
> > +++ b/device-types/rtc/description.tex
> > @@ -4,6 +4,7 @@ \section{RTC Device}\label{sec:Device Types / RTC Device}
> > time. The device can provide different clocks, e.g.\ for the UTC or TAI
> > time standards, or for physical time elapsed since some past epoch. The
> > driver can read the clocks with simple or more accurate methods.
> > +Optionally, the driver can set an alarm.
> >
> > \subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
> >
> > @@ -13,13 +14,23 @@ \subsection{Virtqueues}\label{sec:Device Types / RTC Device / Virtqueues}
> >
> > \begin{description}
> > \item[0] requestq
> > +\item[1] alarmq
> > \end{description}
> >
> > The driver enqueues requests to the requestq.
> >
> > +Through the alarmq, the device notifies the driver about alarm
> > +expirations. The alarmq exists only if VIRTIO_RTC_F_ALARM was
> > +negotiated.
> > +
> > \subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
> >
> > -No device-specific feature bits are defined yet.
> > +\begin{description}
> > +\item[VIRTIO_RTC_F_ALARM (0)] Device supports alarm.
> > +\end{description}
> > +
> > +VIRTIO_RTC_F_ALARM determines whether the device supports setting an
> > +alarm for some of the clocks.
> >
> > \subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
> >
> > @@ -376,7 +387,8 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
> > struct virtio_rtc_resp_head head;
> > u8 type;
> > u8 leap_second_smearing;
> > - u8 reserved[6];
> > + u8 flags;
>
> I wonder if you can just define flags in the first patch instead of
> introduce it here. I think flags field may be used for other purpose in
> the future but I do not have an strong opinion.
>
Extending the spec by adding new fields in place of reserved array
members should be unproblematic, so I added the field only when
introducing a meaningful flag. Actually, other messages could also get
flags fields in the future (which would only become meaningful if the
respective features are negotiated).
So I propose to not change, since otherwise "flags" fields could
arguably be added all over the place in the first patch.
> > + u8 reserved[5];
> > };
> > \end{lstlisting}
> >
> > @@ -387,6 +399,15 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
> > variant} through field \field{leap_second_smearing}. All other clocks
> > set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
> >
> > +The \field{flags} field provides the following information:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_RTC_FLAG_ALARM_CAP (1 << 0)
> > +\end{lstlisting}
> > +
> > +If VIRTIO_RTC_F_ALARM was negotiated, flag VIRTIO_RTC_FLAG_ALARM_CAP
> > +indicates that the clock supports an alarm.
> > +
> > \item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
> > cross-timestamping for a particular pair of clock and hardware counter.
> >
> > @@ -693,3 +714,248 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
> > For VIRTIO_RTC_REQ_READ_CROSS and for any clock type listed in
> > this specification, the device MUST use the nanosecond as unit for
> > field \field{clock_reading}.
> > +
> > +\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
> > +alarm expiration, the device notifies the driver. On alarm expiration,
> > +the device may also wake up the driver, while the driver is in a sleep
> > +state, or while the driver is powered off. How this is done is beyond
> > +the scope of the specification. The driver can set one alarm time per
> > +clock, if the clock supports this.
> > +
> > +The device may retain alarm times across device resets.\footnote{Drivers
> > + may reset the device on boot or on resume from sleep state. It
> > + can make sense for the device to retain the alarm time then,
> > + similar to other alarm clocks.}
> > +
> > +The alarm feature, and the associated alarmq for notifications from the
> > +device, are available if VIRTIO_RTC_F_ALARM was negotiated. In addition,
> > +if the driver previously set an alarm time, even if the device no longer
> > +both
> > +
> > +\begin{itemize}
> > +\item is live and
> > +\item has negotiated VIRTIO_RTC_F_ALARM,
> > +\end{itemize}
> > +
> > +the device may still execute implementation-specific actions on alarm
> > +expiration.
> > +
> > +An alarm expires
> > +
> > +\begin{itemize}
> > +\item 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, or
> > +
> > +\item when the driver sets an alarm time which is not in the future, or
> > +
> > +\item when the device is reset, if the alarm time is retained and not in
> > + the future.\footnote{The device is always responsible for
> > + detecting alarm expiration events. This avoids that the driver
> > + needs to reason about when it shall poll for alarm expiration.}
> > +\end{itemize}
> > +
> > +When an alarm expires, the driver can disable it. Otherwise, the alarm
> > +expires each time when one of the above expiration events occurs, even
> > +if it occurred before.\footnote{This avoids that the driver may
>
> When an alarm expires, the device keeps notifying the driver that the
> alarm has expired? is that implementation-specific?
>
Devices are not required to retain the alarm across a reset. So whether
an alarm is retained across a reset is unspecified.
Apart from this, there is nothing implementation-specific about when the
device notifies the driver through the alarmq (once after each expiry
event as per the above enumeration).
The expiry events avoid that the driver misses an alarm, or that the
driver needs to implement extra logic to recognize a missed alarm.
As for "keeps notifying", the notification only happens once after each
of the expiry events described above. Real-life drivers will likely
disable the alarm when they first acknowledge the notification,
preventing any further expiry events. The Linux kernel RTC subsystem
does this.
> > + miss an alarm when the clock steps backwards after alarm
> > + expiration, but before the driver has resumed operation. This
> > + also facilitates distinct drivers using the same device,
> > + e.g.\ a driver in the bootloader, and a driver in the OS.}
> > +
> > +On alarm expiration, the device executes the alarm actions. The alarm
> > +actions are:
> > +
> > +\begin{itemize}
> > +\item The device notifies the driver through the alarmq. If the device
>
> Do you mean the driver?
>
No. "The device is live" means that the DRIVER_OK status bit is set (and
the DEVICE_NEEDS_RESET bit is cleared). But the "live" terminology is
apparently not used much outside of "Driver Requirements: Device
Initialization".
Maybe I should write instead "If the device is not operating, or if no
buffers are available [...]".
> > + is not live, or no buffers are available in the alarmq, the
> > + device will notify once the device is live and buffers are
> > + available.
> > +
> > +\item Optionally, the device executes other, implementation-specific,
> > + actions. The device may execute those immediately, regardless of
> > + the device state.
> > +\end{itemize}
> > +
> > +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.
> > +
> > +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".
Thanks for the review,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 3/4] virtio-rtc: Add alarm feature
2025-02-13 18:13 ` Peter Hilber
@ 2025-02-19 16:08 ` Matias Ezequiel Vara Larsen
2025-02-20 16:51 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-19 16:08 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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/
> > >
> > > device-types/rtc/description.tex | 270 ++++++++++++++++++++++++++++++-
> > > 1 file changed, 268 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
> > > index 2aefc22cb649..47ad50cd95ca 100644
> > > --- a/device-types/rtc/description.tex
> > > +++ b/device-types/rtc/description.tex
> > > @@ -4,6 +4,7 @@ \section{RTC Device}\label{sec:Device Types / RTC Device}
> > > time. The device can provide different clocks, e.g.\ for the UTC or TAI
> > > time standards, or for physical time elapsed since some past epoch. The
> > > driver can read the clocks with simple or more accurate methods.
> > > +Optionally, the driver can set an alarm.
> > >
> > > \subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
> > >
> > > @@ -13,13 +14,23 @@ \subsection{Virtqueues}\label{sec:Device Types / RTC Device / Virtqueues}
> > >
> > > \begin{description}
> > > \item[0] requestq
> > > +\item[1] alarmq
> > > \end{description}
> > >
> > > The driver enqueues requests to the requestq.
> > >
> > > +Through the alarmq, the device notifies the driver about alarm
> > > +expirations. The alarmq exists only if VIRTIO_RTC_F_ALARM was
> > > +negotiated.
> > > +
> > > \subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature bits}
> > >
> > > -No device-specific feature bits are defined yet.
> > > +\begin{description}
> > > +\item[VIRTIO_RTC_F_ALARM (0)] Device supports alarm.
> > > +\end{description}
> > > +
> > > +VIRTIO_RTC_F_ALARM determines whether the device supports setting an
> > > +alarm for some of the clocks.
> > >
> > > \subsection{Device configuration layout}\label{sec:Device Types / RTC Device / Device configuration layout}
> > >
> > > @@ -376,7 +387,8 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
> > > struct virtio_rtc_resp_head head;
> > > u8 type;
> > > u8 leap_second_smearing;
> > > - u8 reserved[6];
> > > + u8 flags;
> >
> > I wonder if you can just define flags in the first patch instead of
> > introduce it here. I think flags field may be used for other purpose in
> > the future but I do not have an strong opinion.
> >
>
> Extending the spec by adding new fields in place of reserved array
> members should be unproblematic, so I added the field only when
> introducing a meaningful flag. Actually, other messages could also get
> flags fields in the future (which would only become meaningful if the
> respective features are negotiated).
>
> So I propose to not change, since otherwise "flags" fields could
> arguably be added all over the place in the first patch.
Sounds good.
>
> > > + u8 reserved[5];
> > > };
> > > \end{lstlisting}
> > >
> > > @@ -387,6 +399,15 @@ \subsubsection{Control Requests}\label{sec:Device Types / RTC Device / Device Op
> > > variant} through field \field{leap_second_smearing}. All other clocks
> > > set \field{leap_second_smearing} to VIRTIO_RTC_SMEAR_UNSPECIFIED.
> > >
> > > +The \field{flags} field provides the following information:
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_RTC_FLAG_ALARM_CAP (1 << 0)
> > > +\end{lstlisting}
> > > +
> > > +If VIRTIO_RTC_F_ALARM was negotiated, flag VIRTIO_RTC_FLAG_ALARM_CAP
> > > +indicates that the clock supports an alarm.
> > > +
> > > \item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device supports
> > > cross-timestamping for a particular pair of clock and hardware counter.
> > >
> > > @@ -693,3 +714,248 @@ \subsubsection{Read Requests}\label{sec:Device Types / RTC Device / Device Opera
> > > For VIRTIO_RTC_REQ_READ_CROSS and for any clock type listed in
> > > this specification, the device MUST use the nanosecond as unit for
> > > field \field{clock_reading}.
> > > +
> > > +\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
> > > +alarm expiration, the device notifies the driver. On alarm expiration,
> > > +the device may also wake up the driver, while the driver is in a sleep
> > > +state, or while the driver is powered off. How this is done is beyond
> > > +the scope of the specification. The driver can set one alarm time per
> > > +clock, if the clock supports this.
> > > +
> > > +The device may retain alarm times across device resets.\footnote{Drivers
> > > + may reset the device on boot or on resume from sleep state. It
> > > + can make sense for the device to retain the alarm time then,
> > > + similar to other alarm clocks.}
> > > +
> > > +The alarm feature, and the associated alarmq for notifications from the
> > > +device, are available if VIRTIO_RTC_F_ALARM was negotiated. In addition,
> > > +if the driver previously set an alarm time, even if the device no longer
> > > +both
> > > +
> > > +\begin{itemize}
> > > +\item is live and
> > > +\item has negotiated VIRTIO_RTC_F_ALARM,
> > > +\end{itemize}
> > > +
> > > +the device may still execute implementation-specific actions on alarm
> > > +expiration.
> > > +
> > > +An alarm expires
> > > +
> > > +\begin{itemize}
> > > +\item 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, or
> > > +
> > > +\item when the driver sets an alarm time which is not in the future, or
> > > +
> > > +\item when the device is reset, if the alarm time is retained and not in
> > > + the future.\footnote{The device is always responsible for
> > > + detecting alarm expiration events. This avoids that the driver
> > > + needs to reason about when it shall poll for alarm expiration.}
> > > +\end{itemize}
> > > +
> > > +When an alarm expires, the driver can disable it. Otherwise, the alarm
> > > +expires each time when one of the above expiration events occurs, even
> > > +if it occurred before.\footnote{This avoids that the driver may
> >
> > When an alarm expires, the device keeps notifying the driver that the
> > alarm has expired? is that implementation-specific?
> >
>
> Devices are not required to retain the alarm across a reset. So whether
> an alarm is retained across a reset is unspecified.
>
> Apart from this, there is nothing implementation-specific about when the
> device notifies the driver through the alarmq (once after each expiry
> event as per the above enumeration).
>
> The expiry events avoid that the driver misses an alarm, or that the
> driver needs to implement extra logic to recognize a missed alarm.
>
> As for "keeps notifying", the notification only happens once after each
> of the expiry events described above. Real-life drivers will likely
> disable the alarm when they first acknowledge the notification,
> preventing any further expiry events. The Linux kernel RTC subsystem
> does this.
>
I see, thanks for the explanation.
> > > + miss an alarm when the clock steps backwards after alarm
> > > + expiration, but before the driver has resumed operation. This
> > > + also facilitates distinct drivers using the same device,
> > > + e.g.\ a driver in the bootloader, and a driver in the OS.}
> > > +
> > > +On alarm expiration, the device executes the alarm actions. The alarm
> > > +actions are:
> > > +
> > > +\begin{itemize}
> > > +\item The device notifies the driver through the alarmq. If the device
> >
> > Do you mean the driver?
> >
>
> No. "The device is live" means that the DRIVER_OK status bit is set (and
> the DEVICE_NEEDS_RESET bit is cleared). But the "live" terminology is
> apparently not used much outside of "Driver Requirements: Device
> Initialization".
I see, thanks.
>
> Maybe I should write instead "If the device is not operating, or if no
> buffers are available [...]".
I think `live` is fine.
>
> > > + is not live, or no buffers are available in the alarmq, the
> > > + device will notify once the device is live and buffers are
> > > + available.
> > > +
> > > +\item Optionally, the device executes other, implementation-specific,
> > > + actions. The device may execute those immediately, regardless of
> > > + the device state.
> > > +\end{itemize}
> > > +
> > > +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 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.
> > > +
> > > +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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 3/4] virtio-rtc: Add alarm feature
2025-02-19 16:08 ` Matias Ezequiel Vara Larsen
@ 2025-02-20 16:51 ` Peter Hilber
2025-02-24 11:58 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-20 16:51 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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 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.)
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.
Thanks for the reply,
Peter
[2] https://lore.kernel.org/all/20231218064253.9734-1-peter.hilber@opensynergy.com/T/#m7215c69a093618086138ad3817b740b19b98bd3a
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 3/4] virtio-rtc: Add alarm feature
2025-02-20 16:51 ` Peter Hilber
@ 2025-02-24 11:58 ` Matias Ezequiel Vara Larsen
0 siblings, 0 replies; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-24 11:58 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-01-23 10:16 [PATCH v7 0/4] virtio-rtc: Add device specification Peter Hilber
` (2 preceding siblings ...)
2025-01-23 10:16 ` [PATCH v7 3/4] virtio-rtc: Add alarm feature Peter Hilber
@ 2025-01-23 10:16 ` Peter Hilber
2025-02-11 12:33 ` Matias Ezequiel Vara Larsen
3 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-01-23 10:16 UTC (permalink / raw)
To: virtio-comment
Cc: Cornelia Huck, Parav Pandit, Jason Wang, David Woodhouse,
Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri, Peter Hilber
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.
+
\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.
+
\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.
+
+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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-01-23 10:16 ` [PATCH v7 4/4] virtio-rtc: Add normative statements for " Peter Hilber
@ 2025-02-11 12:33 ` Matias Ezequiel Vara Larsen
2025-02-13 18:14 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-11 12:33 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-11 12:33 ` Matias Ezequiel Vara Larsen
@ 2025-02-13 18:14 ` Peter Hilber
2025-02-19 15:36 ` Matias Ezequiel Vara Larsen
2025-03-04 16:25 ` Peter Hilber
0 siblings, 2 replies; 27+ messages in thread
From: Peter Hilber @ 2025-02-13 18:14 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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).
> > \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?
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-13 18:14 ` Peter Hilber
@ 2025-02-19 15:36 ` Matias Ezequiel Vara Larsen
2025-02-20 17:06 ` Peter Hilber
2025-03-04 16:25 ` Peter Hilber
1 sibling, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-19 15:36 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-19 15:36 ` Matias Ezequiel Vara Larsen
@ 2025-02-20 17:06 ` Peter Hilber
2025-02-24 12:13 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-20 17:06 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Wed, Feb 19, 2025 at 04:36:14PM +0100, Matias Ezequiel Vara Larsen wrote:
> 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(+)
> > > >
[...]
> > > > +
> > > > +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
>
The intent of these requirements is to clarify that this is legitimate
and expected behavior of the device. I can drop them if this is
considered unnecessary as normative requirements. Or maybe one
requirement would be enough:
While the device is not serving an alarm expiration event, the
device MUST NOT execute implementation-specific alarm actions.
Thanks for the reply,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-20 17:06 ` Peter Hilber
@ 2025-02-24 12:13 ` Matias Ezequiel Vara Larsen
2025-02-25 11:18 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-24 12:13 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Feb 20, 2025 at 06:06:38PM +0100, Peter Hilber wrote:
> On Wed, Feb 19, 2025 at 04:36:14PM +0100, Matias Ezequiel Vara Larsen wrote:
> > 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(+)
> > > > >
>
> [...]
>
> > > > > +
> > > > > +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.
> > > > > +
Does not `serving` mean also that the device execute
implementation-specific alarm actions? Besides notifying the guest when
an alarm has expired, is not any other action or device behavior
implementation-specific and then not constrained by the spec?
> > > > > +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
> >
>
> The intent of these requirements is to clarify that this is legitimate
> and expected behavior of the device. I can drop them if this is
> considered unnecessary as normative requirements. Or maybe one
> requirement would be enough:
>
> While the device is not serving an alarm expiration event, the
> device MUST NOT execute implementation-specific alarm actions.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-24 12:13 ` Matias Ezequiel Vara Larsen
@ 2025-02-25 11:18 ` Peter Hilber
2025-02-25 15:04 ` Matias Ezequiel Vara Larsen
0 siblings, 1 reply; 27+ messages in thread
From: Peter Hilber @ 2025-02-25 11:18 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Mon, Feb 24, 2025 at 01:13:46PM +0100, Matias Ezequiel Vara Larsen wrote:
> On Thu, Feb 20, 2025 at 06:06:38PM +0100, Peter Hilber wrote:
> > On Wed, Feb 19, 2025 at 04:36:14PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > 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(+)
> > > > > >
> >
> > [...]
> >
> > > > > > +
> > > > > > +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.
> > > > > > +
>
> Does not `serving` mean also that the device execute
> implementation-specific alarm actions?
As for `serving an alarm expiration event`, I intended this to be a
speaking name for a state to which different requirements can refer.
Maybe this was bad naming.
> Besides notifying the guest when
> an alarm has expired, is not any other action or device behavior
> implementation-specific and then not constrained by the spec?
So this would imply that the below MUST NOT requirement should not be
adopted? I chose to refer to implementation-specific alarm actions so
that
1) it is clear to any spec reader that alarms may work even when an
alarmq notification would not work and
2) an implementation's documentation can usually refer to the spec as to
when the implementation-specific alarm actions may be executed.
I find the references helpful, but I do not have a strong opinion about
keeping them in the normative or non-normative parts.
Thanks for the comments,
Peter
> > > > > > +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
> > >
> >
> > The intent of these requirements is to clarify that this is legitimate
> > and expected behavior of the device. I can drop them if this is
> > considered unnecessary as normative requirements. Or maybe one
> > requirement would be enough:
> >
> > While the device is not serving an alarm expiration event, the
> > device MUST NOT execute implementation-specific alarm actions.
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-25 11:18 ` Peter Hilber
@ 2025-02-25 15:04 ` Matias Ezequiel Vara Larsen
2025-02-25 15:47 ` Peter Hilber
0 siblings, 1 reply; 27+ messages in thread
From: Matias Ezequiel Vara Larsen @ 2025-02-25 15:04 UTC (permalink / raw)
To: Peter Hilber
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Tue, Feb 25, 2025 at 12:18:51PM +0100, Peter Hilber wrote:
> On Mon, Feb 24, 2025 at 01:13:46PM +0100, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Feb 20, 2025 at 06:06:38PM +0100, Peter Hilber wrote:
> > > On Wed, Feb 19, 2025 at 04:36:14PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > > 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(+)
> > > > > > >
> > >
> > > [...]
> > >
> > > > > > > +
> > > > > > > +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.
> > > > > > > +
> >
> > Does not `serving` mean also that the device execute
> > implementation-specific alarm actions?
>
> As for `serving an alarm expiration event`, I intended this to be a
> speaking name for a state to which different requirements can refer.
> Maybe this was bad naming.
>
I just though that `serving an alarm expiration event` could include
executing implementation-specific alarm actions so that we do not need
to make a separation of the two concepts. I do not have a strong opinion
so we can keep it as it is.
> > Besides notifying the guest when
> > an alarm has expired, is not any other action or device behavior
> > implementation-specific and then not constrained by the spec?
>
> So this would imply that the below MUST NOT requirement should not be
> adopted? I chose to refer to implementation-specific alarm actions so
> that
>
> 1) it is clear to any spec reader that alarms may work even when an
> alarmq notification would not work and
>
> 2) an implementation's documentation can usually refer to the spec as to
> when the implementation-specific alarm actions may be executed.
>
> I find the references helpful, but I do not have a strong opinion about
> keeping them in the normative or non-normative parts.
>
I do not have a strong opinion either so we can keep it as it is.
Thanks, Matias.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-25 15:04 ` Matias Ezequiel Vara Larsen
@ 2025-02-25 15:47 ` Peter Hilber
0 siblings, 0 replies; 27+ messages in thread
From: Peter Hilber @ 2025-02-25 15:47 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Tue, Feb 25, 2025 at 04:04:44PM +0100, Matias Ezequiel Vara Larsen wrote:
> On Tue, Feb 25, 2025 at 12:18:51PM +0100, Peter Hilber wrote:
> > On Mon, Feb 24, 2025 at 01:13:46PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > On Thu, Feb 20, 2025 at 06:06:38PM +0100, Peter Hilber wrote:
> > > > On Wed, Feb 19, 2025 at 04:36:14PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > > > 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(+)
> > > > > > > >
> > > >
> > > > [...]
> > > >
> > > > > > > > +
> > > > > > > > +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.
> > > > > > > > +
> > >
> > > Does not `serving` mean also that the device execute
> > > implementation-specific alarm actions?
> >
> > As for `serving an alarm expiration event`, I intended this to be a
> > speaking name for a state to which different requirements can refer.
> > Maybe this was bad naming.
> >
>
> I just though that `serving an alarm expiration event` could include
> executing implementation-specific alarm actions so that we do not need
> to make a separation of the two concepts. I do not have a strong opinion
> so we can keep it as it is.
>
> > > Besides notifying the guest when
> > > an alarm has expired, is not any other action or device behavior
> > > implementation-specific and then not constrained by the spec?
> >
> > So this would imply that the below MUST NOT requirement should not be
> > adopted? I chose to refer to implementation-specific alarm actions so
> > that
> >
> > 1) it is clear to any spec reader that alarms may work even when an
> > alarmq notification would not work and
> >
> > 2) an implementation's documentation can usually refer to the spec as to
> > when the implementation-specific alarm actions may be executed.
> >
> > I find the references helpful, but I do not have a strong opinion about
> > keeping them in the normative or non-normative parts.
> >
>
> I do not have a strong opinion either so we can keep it as it is.
>
> Thanks, Matias.
>
If I kept track correctly, tentative resolutions for all review findings
so far in the 4 patches have been discussed. I will try to send a
corrected version this or next week.
Thanks for the reviews,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v7 4/4] virtio-rtc: Add normative statements for alarm feature
2025-02-13 18:14 ` Peter Hilber
2025-02-19 15:36 ` Matias Ezequiel Vara Larsen
@ 2025-03-04 16:25 ` Peter Hilber
1 sibling, 0 replies; 27+ messages in thread
From: Peter Hilber @ 2025-03-04 16:25 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: virtio-comment, Cornelia Huck, Parav Pandit, Jason Wang,
David Woodhouse, Ridoux, Julien, Trilok Soni, Srivatsa Vaddagiri
On Thu, Feb 13, 2025 at 07:14:35PM +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.
>
After looking at the context, I think `can support` is better here,
since in the paragraph above the normative requirement, `supports
setting an alarm` implies that the feature was actually negotiated.
So I now propose not to remove `can`.
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread