RE: [virtio-comment] [RFC PATCH v2] virtio-rtc: Add device specification.

[Parav Pandit <parav@nvidia.com>]

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Peter Hilber
> Sent: Friday, June 30, 2023 5:30 AM
> Subject: [virtio-comment] [RFC PATCH v2] virtio-rtc: Add device specification.
>
Tiny nit: remove full stop from the subject, it usually does not have it.
 
> This is a partial draft for a Real-Time Clock (RTC) device. The virtio-rtc device
> provides information about current time through one or more clocks.
> 
> v2 is a minor update which should match the Linux kernel driver patch series to
> be posted soon. It is planned to add a timer/alarm feature as part of a later draft
> spec update.
> 
> The draft 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, but is up to
> discussion.
> 
> For this device, there is a prototypical Linux kernel driver for which upstreaming
> is planned, and a proprietary device implementation.
> 
> In the spec text, there is some redundancy between the informative and the
> normative text. The intent is that the informative text already gives all the
> essential information about the device.
> 
> In the spec, clock ids are 64 bit wide to have unique ids when supporting clock
It is unclear the namespace of uniqueness.
Do you mean unique id across multiple virtio devices?
If so, how is it proposed, we probably need UUID?

> hot-plugging in the future.
> 
> Any feedback is appreciated very much.
> 
This is a good feature.

A network device has a PHC which is very similar to what is being proposed here which is super set of this proposal.

A network device will support packet timestamping as phase-1, followed by PTP synchronized UTC/TAI format in phase-2 followed by PTP timestamping offload in phase-3.
Phase-1 is already being proposed using monotonic clock method at [1].

[1] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00287.html

We would like to have a single virtio network device to expose the optional RTC without creating a full blown new virtio device.

A quick grep in the industry indicates that at least 15+ nics in industry with dual PTP clock + nic functionality.
And we would like virtio network device also to have it.
At minimum such solution avoids creating an additional full blown virtio device.

As proposed here, RTC device as stand alone itself has its merit and use.

So lets build the RTC device interface that can be of dual use.
1. as part of stand alone RTC device
2. as part of virtio network device

Such work will enable to re-invent the wheel 2nd time in virtio net.

What I can think of is following definitions being common between two device types.
This will be able to leverage your rtc work done as part of this patchset.

1. _F feature bit that can be common between the rtc and net device
2. control vq command opcode and format be common
3. discovery of the readq id possibly using cvq and it must depend on _F_RESET bit.

> --- /dev/null
> +++ b/device-types/rtc/description.tex
> @@ -0,0 +1,582 @@
> +\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.
> +Drivers can read the clocks with simple or more accurate methods.
> +
The driver can read...

I am not sure if I see two different methods to read (simple and accurate one).
Is if the readq that is accurate one?

> +\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] readq
> +\item[1] controlq
> +\end{description}
> +
> +The driver initiates clock readings through requests to the
> +readq.
The driver initiates (or enqueues) clock reading requests to the readq.
Does above read better?

\footnote{The readq is a dedicated virtqueue to facilitate
> +reducing latency for clock readings, which may be more
> +performance-sensitive than other requests.} The driver uses the
> +controlq for other requests to the device.
> +
> +\subsection{Feature bits}\label{sec:Device Types / RTC Device / Feature
> +bits}
> +
> +\begin{description}
> +\item[VIRTIO_RTC_F_READ_CROSS (0)] Device supports cross-timestamping.
> +\end{description}
> +
> +VIRTIO_RTC_F_READ_CROSS determines whether the device supports clock
> +reading with cross-timestamping. Without this feature, the device does
> +not support cross-timestamping. Even when this feature was negotiated,
> +the device may still not support cross-timestamping for some clocks, or
> +for particular requests.
> +
Can you please explain why even after feature negotiation, device may still not support?
Is it because the cross timestamping is per clock feature?
If so, I guess no need of a feature bit as it is not serving any meaningful purpose.
Each clock information can advertise what is supports, right?

> +\devicenormative{\subsubsection}{Feature bits}{Device Types / RTC
> +Device / Feature bits}
> +
> +The device SHOULD offer VIRTIO_RTC_F_READ_CROSS if the device can
> +support cross-timestamping 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.
> +
> +\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 readq and the controlq, the driver sends a message with a
> +request, and receives the response in the device-writable part of the
> +message. readq and controlq messages use common request and response
> +headers.
> +
Below structure definition is _req and response.
So lets just call it request and request_type as its simple to explain using single thing.
Please drop the wording around message.

> +\begin{lstlisting}
> +/* common request header */
> +struct virtio_rtc_req_head {
> +	__le16 msg_type;
> +	__u8 reserved[2];
> +};
> +
> +/* common response header */
> +struct virtio_rtc_resp_head {
> +	__u8 status;
> +	__u8 reserved[3];
> +};
> +\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_UNSUPP 1
> +#define VIRTIO_RTC_S_NODEV 2
> +#define VIRTIO_RTC_S_INVAL 3
> +#define VIRTIO_RTC_S_DEVERR 4
> +\end{lstlisting}
> +
Recently for the AQ we picked Linux UAPI specific error codes such as EAGAIN, EINVAL etc.
You might want to have similar code reused for EINVAL, NOTSUPPORT, NODEV to be reused here.
No strong opinion though.

[..]
When the device and driver normative lines are long like this, it is best to put in separate patch, so that its easy to read (review) through the whole description and conformance as separate.

> +\item[VIRTIO_RTC_M_CFG] discovers the number of clocks.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_M_CFG 0x1000 /* message type */
> +
Like above, lets just call it RTC_REQ_CFG as below is a request structure.

> +struct virtio_rtc_req_cfg {
> +	struct virtio_rtc_req_head head;
> +	/* no request params */
> +	__u8 reserved[4];
> +};
> +
> +struct virtio_rtc_resp_cfg {
> +	struct virtio_rtc_resp_head head;
> +	__le16 num_clocks;
> +	__u8 reserved[10];
> +};
> +\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_M_CLOCK_CAP] discovers the capabilities of the clock
> +identified by the \field{clock_id} field.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_M_CLOCK_CAP 0x1001 /* message type */
> +
> +struct virtio_rtc_req_clock_cap {
> +	struct virtio_rtc_req_head head;
> +	__u8 reserved[4];
> +	__le64 clock_id;
> +};
Please drop "__" from the field data type definition.

> +
> +struct virtio_rtc_resp_clock_cap {
> +	struct virtio_rtc_resp_head head;
> +	__le16 type;
> +	__u8 reserved[10];
> +};
> +\end{lstlisting}
> +
> +The \field{type} field identifies the clock type. A device provides
> +zero or more clocks for a clock type. The following clock types are
> +defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_CLOCK_UTC 0
> +#define VIRTIO_RTC_CLOCK_TAI 1
> +#define VIRTIO_RTC_CLOCK_MONO 2
> +\end{lstlisting}
> +
> +VIRTIO_RTC_CLOCK_UTC uses the UTC (Coordinated Universal Time) time
> +standard. The device uses the time epoch of January 1, 1970, 00:00 UTC.
> +This is the same epoch as \emph{Unix time}.
> +
> +VIRTIO_RTC_CLOCK_TAI uses the TAI (International Atomic Time) time
> +standard. The device uses the time epoch of January 1, 1970, 00:00 TAI.
> +
> +VIRTIO_RTC_CLOCK_MONO uses monotonic physical time (SI seconds
> +subdivisions) since some unspecified epoch. The VIRTIO_RTC_CLOCK_MONO
> +epoch is before or during device reset.
> +
> +Additional clock types may be standardized in the future.
> +Implementation-specific clock type definitions are not recommended and
> +use a clock type id greater than or equal to 0xF000.
> +
> +\item[VIRTIO_RTC_M_CROSS_CAP] discovers whether the device supports
> +cross-timestamping for a particular pair of clock and hardware counter.
> +If the VIRTIO_RTC_F_READ_CROSS feature has been negotiated, the driver
> +can send the VIRTIO_RTC_M_CROSS_CAP message.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_M_CROSS_CAP 0x1002 /* message type */
> +
> +struct virtio_rtc_req_cross_cap {
> +	struct virtio_rtc_req_head head;
> +	__le16 hw_counter;
> +	__u8 reserved[2];
> +	__le64 clock_id;
> +};
> +
> +#define VIRTIO_RTC_FLAG_CROSS_CAP 0
> +
> +struct virtio_rtc_resp_cross_cap {
> +	struct virtio_rtc_resp_head head;
> +	__u8 flags;
> +	__u8 reserved[11];
> +};
> +\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 bit
> +\field{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 bit otherwise.
> +
> +\end{description}
> +
> +\devicenormative{\paragraph}{controlq Operation}{Device Types / RTC
> +Device / Device Operation / controlq Operation}
> +
> +For a fixed controlq message 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.
> +
> +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, the device MUST use the
> +time epoch of January 1, 1970, 00:00 UTC.
> +
> +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_MONO, the device MUST use SI
> +seconds subdivisions.
> +
> +For any clock of type VIRTIO_RTC_CLOCK_MONO, the device MUST use an
> +epoch at a time instant before or during device reset.
> +
> +For message VIRTIO_RTC_M_CLOCK_CAP, the device MUST set \field{type} to
> +one of the clock types defined in this specification, or to a value
> +greater than or equal to 0xF000.
> +
> +For a VIRTIO_RTC_M_CROSS_CAP message M, the device MUST set the
> +VIRTIO_RTC_FLAG_CROSS_CAP bit in the \field{flags} field if the device
> +would respond to a VIRTIO_RTC_M_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 bit otherwise.
> +
> +\drivernormative{\paragraph}{controlq Operation}{Device Types / RTC
> +Device / Device Operation / controlq Operation}
> +
> +The driver MUST put the request into the device-readable part of the
> +controlq message.
> +
> +The driver MUST allocate enough space for the response in the
> +device-writable part of the controlq message.
> +
> +The driver MUST negotiate VIRTIO_RTC_F_READ_CROSS before sending a
> +VIRTIO_RTC_M_CROSS_CAP message.
> +
> +For VIRTIO_RTC_M_CROSS_CAP, the driver MUST set \field{hw_counter} to
> +one of the hardware counter identifiers defined in this specification,
> +or to a value greater than or equal to 0xF000.
> +
> +\subsubsection{readq Operation}\label{sec:Device Types / RTC Device /
> +Device Operation / readq Operation}
> +
> +The driver requests clock readings through readq messages. 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 continuously support

The word "continuously" is likely not needed.
As long as readq is created, driver can read it.

> +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}
> +
> +E.g., a clock of type VIRTIO_RTC_CLOCK_UTC can insert UTC leap seconds.
> +
> +As long as a clock does not jump backwards, the driver clock readings
> +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 readq messages as available.
> +
> +\item The device marks readq messages 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_MONO, the device always returns
> +monotonically increasing clock readings.
> +
> +The unit of all \field{clock_reading} fields is 1 nanosecond for all
> +clock types which are part of the device specification so
> +far.\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_M_READ] reads the clock identified by the
> +\field{clock_id} field. The device supports this message for every
> +clock.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RTC_M_READ 0x0001 /* message type */
> +
> +struct virtio_rtc_req_read {
> +	struct virtio_rtc_req_head head;
> +	__u8 reserved[4];

I think the req_head structure to be 64-bit aligned, so that here individual requests does not have to align it.

> +	__le64 clock_id;
> +};
> +
> +struct virtio_rtc_resp_read {
> +	struct virtio_rtc_resp_head head;
> +	__u8 reserved[4];
> +	__le64 clock_reading;
> +};
> +\end{lstlisting}
> +
> +\field{clock_reading} is a device-side clock reading obtained after the
> +message was marked as available.
> +
If this clock device is a PCI device, on an empty system, a PCI device has to do at least couple of DMA.
1. read the descriptor (250 to 300nsec)
2. Read the request (250 to 300nsec)
3. Write the response (50 nsec+)
4. raise interrupt.

How is this time delta accounted in clock reading?
Is driver expected to record system time and subtract it on receiving response?
Or?

Ran out of time.
Will continue after I get above base line alignment.

Thanks.