Re: [RFC PATCH v3 1/4] virtio-rtc: Add initial device specification

[Peter Hilber <peter.hilber@opensynergy.com>]

On 20.01.24 11:07, Parav Pandit wrote:
> Hi Peter,
> 
>> From: Peter Hilber <peter.hilber@opensynergy.com>
>> Sent: Monday, December 18, 2023 12:13 PM
>>
>> This is a draft for a Real-Time Clock (RTC) device. The virtio-rtc device
>> provides information about current time through one or more clocks.
>>
>> 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.
>>
>> Open Questions
>> ==============
>>
>> Arm Generic Timer Clock Types
>> -----------------------------
>>
>> This spec version still distinguishes the Arm Generic Timer virtual and
>> physical counters:
>>
>> 	/* Arm Generic Timer CNTVCT_EL0, Counter-timer Virtual Count
>> register */
>> 	#define VIRTIO_RTC_COUNTER_ARM_VIRT 0
>> 	/* Arm Generic Timer CNTPCT_EL0, Counter-timer Physical Count
>> register */
>> 	#define VIRTIO_RTC_COUNTER_ARM_PHYS 1
>>
>> But there might be no benefit distinguishing them, as per a discussion on the
>> RFC Linux kernel driver [4].
>>
>> So unless somebody identifies some use in distinguishing Arm virtual and
>> physical counters, this distinction will be dropped in v4.
>>
>> virtio-rtc Functionality for Net Device
>> ---------------------------------------
>>
>> There is interest into sharing virtio-rtc functionality with the virtio-net device
>> [6]. It is proposed to encapsulate the requests resp.
>> responses in the pseudo-fields command-specific-data resp.
>> command-specific-result.
>>
>> This handles natural alignment of the payload in a way which is compatible
>> with
>>
>> - existing virtio-net controlq commands, and
>>
>> - with the standalone virtio-rtc messages.
>>
>> Additional Information
>> ======================
>>
>> The spec does not specify how a driver should interpret clock readings, esp.
>> also not how to perform clock synchronization.
>>
>> 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.
>>
>> Any feedback is appreciated very much.
>>
>> Changelog
>> =========
>>
>> 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/#mffb93800fea11d6dda9e151078abedd
>> 6ff1c0f1e
>> [3] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
>> b90aa4e50836@opensynergy.com/t/#m94efd0aa9b9c2b96a246b79ef8bfc3bf
>> 64ebe791
>> [4] https://lore.kernel.org/lkml/20230630171052.985577-1-
>> peter.hilber@opensynergy.com/T/#m65fa1d715933360498c4e33d7225e4220
>> 215a9d6
>> [5] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
>> b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469
>> 048c5d8b62
>> [6] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
>> b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469
>> 048c5d8b62
>>
>> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
>> ---
>>  content.tex                      |   3 +-
>>  device-types/rtc/description.tex | 328 +++++++++++++++++++++++++++++++
>>  2 files changed, 330 insertions(+), 1 deletion(-)  create mode 100644 device-
>> types/rtc/description.tex
>>
>> diff --git a/content.tex b/content.tex
>> index 98b9c319a666..f2b534ce97e6 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -683,7 +683,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
>> @@ -769,6 +769,7 @@ \chapter{Device Types}\label{sec:Device Types}
>> \input{device-types/scmi/description.tex}
>>  \input{device-types/gpio/description.tex}
>>  \input{device-types/pmem/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..abfa2206894e
>> --- /dev/null
>> +++ b/device-types/rtc/description.tex
>> @@ -0,0 +1,328 @@
>> +\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.
>> +
> Now that we bring the generic basic facilities to define device capabilities and resources in [1],
> Num_clocks can be structured a clock resources.
> 
> Please refer to max_limits in [1] as capabilities.
> [1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00095.html
> 
> We should bring a rtc capabilities.
> 

Hi Parav,

thank you for the suggestions! I think that it is a good idea to
standardize message layouts. But I also think this is late and overly
complex to become the default RTC interface. In particular, OpenSynergy has
no plans at the moment to support administration virtqueues (and the newly
proposed device capabilities and device resources) in the RTC prototype
implementation.

At the moment I see the following problems with using device capabilities
and resources:

- Device capabilities and resources were initially proposed last week. It
  remains to be seen how and when they will be accepted into the standard,
  and when the Linux kernel driver will support them. Especially the
  dynamic resource (de-)allocation seems a tricky topic to me.

  By contrast, RTC was originally proposed 10 months ago, and has had an
  open-source driver available since 7 months. In the past it was discussed
  that the net device could be supported through the net controlq.

- Administration virtqueues are only defined for Virtio Over PCI Bus. Our
  use cases also need Virtio Over MMIO, and maybe others in the future.

- The requests VIRTIO_RTC_REQ_CROSS_CAP, VIRTIO_RTC_REQ_READ_CROSS, and
  likely any requests added in the future do not fit into the current
  device resource pattern (as detailed below), so would need to get their
  own virtqueue; or the admin virtqueue would need to allow device-specific
  commands.

  Also, alarms would need to be addressed, perhaps as dedicated resources.

- For the RTC device, using the device resources adds considerable
  implementation overhead even for the simplest of implementations, with no
  direct gain.

  Both capabilities and resources can change dynamically. This would
  necessitate a number of requirements for RTC how to deal with this. Some
  extra generic requirements would likely also be required.

  E.g. the device resources draft does not address how resource removal
  would interact with resource utilization in other queues (e.g. alarmq).

  The only direct improvement I see is that the driver can dynamically
  indicate which clocks it wants to use, which is not present in RTC RFC
  spec v3. However, resource-saving features such as negotiating
  VIRTIO_RTC_REQ_READ_CROSS support (VIRTIO_RTC_F_READ_CROSS) were
  explicitly requested to be dropped from the spec in the past.

- The admin virtqueue spec says:

          It is the responsibility of the driver to ensure strict request
          ordering for commands, because they will be consumed with no order
          constraints.

  But the RTC requirements mandate certain constraints for the RTC (clock)
  read requests processing order (so that the driver will not see backwards
  clock jumps where there are none).

  Maybe there are more incompatibilities here; I didn't check this
  thoroughly.

So I propose to stay with the current interface as default.

It should be possible to introduce the device capabilities and resources as
an alternative interface, protected with a feature bit. This feature bit
would then also omit the device-specific virtqueues. The alternative
interface would need to address at least some of the above issues, such as
how to dynamically create and destroy clocks. The alternative interface can
perhaps also omit some features which are not required by its users. 

I added some extra comments below, but I think the above should be
discussed first.

Best regards,

Peter

[snip]
>> +\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;
>> +	le16 hw_counter;
>> +	u8 reserved[4];
>> +};
>> +
>> +#define VIRTIO_RTC_FLAG_CROSS_CAP (1 << 0)
>> +
>> +struct virtio_rtc_resp_cross_cap {
>> +	struct virtio_rtc_resp_head head;
>> +	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
>> +\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 flag otherwise.
>> +
> Hw_counter is also the resource attribute.
> 

But a clock may support multiple hw_counters; therefore the driver provides
the hw_counter in the request. So it would not fit into the
VIRTIO_ADMIN_CMD_RESOURCE_QUERY pattern out-of-the-box.

>> +\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.
>> +
> Reading the resource using generic query resource command of [2] will return the time value.
> So only time definition is needed in the resource.
> [2] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00096.html
> 

Treating current time as a resource attribute appears questionable. First,
it continuously changes, while other attributes don't. Second, it could
also be a slow operation when reading e.g. from a GNSS clock attached over
a slow interface.

Also, the query resource command does not fit read request
VIRTIO_RTC_REQ_READ_CROSS, which requires the hw_counter parameter, and
returns extra results.