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

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

On 17.04.23 13:49, Cornelia Huck wrote:
> On Mon, Mar 13 2023, Peter Hilber <peter.hilber@opensynergy.com> wrote:
>
>> 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.
>>
>> It is planned to add a timer/alarm feature as part of a 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 hot-plugging in the future.
>>
>> Any feedback is appreciated very much.
>>
>> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
>> ---
>>  conformance.tex                         |   2 +
>>  content.tex                             |   3 +-
>>  device-types/rtc/description.tex        | 523 ++++++++++++++++++++++++
>>  device-types/rtc/device-conformance.tex |  10 +
>>  device-types/rtc/driver-conformance.tex |   9 +
>>  5 files changed, 546 insertions(+), 1 deletion(-)
>>  create mode 100644 device-types/rtc/description.tex
>>  create mode 100644 device-types/rtc/device-conformance.tex
>>  create mode 100644 device-types/rtc/driver-conformance.tex
>
> (...)
>
>> +VIRTIO_RTC_S_INVAL indicates that
>> +
>> +\begin{itemize}
>> +\item the driver request values are not allowed by the specification or
>
> Do we need to specify that, as such a driver would be non-conforming
> anyway?

I would still consider this helpful for error handling and diagnostic
purposes.

>
>> +\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_DEVERR indicates that the device encountered an error while
>> +executing the request, which could not be attributed to invalid input
>> +from the driver.
>
> Is there any way for the driver to find out whether the request has
> partially been executed, and whether it should retry?

Maybe this could be changed to say that

    ... the device did not execute the request due to an error which was
    not caused by invalid input from the driver.

As for retrying: Should there be an additional status code? IMHO this
would increase complexity without a clear benefit. Could the device not
retry internally if immediate retrying makes sense at the particular
point in time?

>
>> +
>> +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.
>> +
>> +\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 VIRTIO_RTC_S_UNSUPP 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_UNSUPP for a request with a value
>> +of the \field{message_type} field which is neither described in this
>> +specification nor otherwise known to the implementation.
>
> Should any such implementation limitations be guarded via feature bits?
> I.e. can we drop _UNSUPP if we require features to be negotiated (and
> per-clock capabilites being clearly communicated)?
>

All new message types should be guarded by feature bits, of course. By
mistake, I added the `nor otherwise known to the implementation` suffix
to the requirement above; I will remove it.

What would dropping _UNSUPP mean? I think there should still be an error
status in this case, regardless of whether the driver was compliant or
not.

But I think I should change the spec draft so that there will be no
_UNSUPP statuses if both driver and device are compliant (and if the
driver does not choose to ignore the actual device capabilities). For
this, only one change should be required: Add a VIRTIO_RTC_M_CROSS_CAP
message, through which the driver can determine whether the device
supports cross-timestamping for a particular pair of clock and HW
counter.

(Also, the field name should read `msg_type`, not `message_type`.)

>> +
>> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
>> +\field{status} field to VIRTIO_RTC_S_UNSUPP 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_NODEV if the \field{clock_id} field
>> +value supplied with the message 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_INVAL if the request values are not
>> +allowed by the specification and none of the previous requirements in
>> +this document stipulated another \field{status}.
>
> I think we can drop this (see above).
>

Again, I would still consider a non-OK status helpful for error handling
and diagnostic purposes.

>> +
>> +For \field{struct virtio_rtc_resp_head}, the device MUST set the
>> +\field{status} field to VIRTIO_RTC_S_INVAL if the device read-only part
>> +of the message is too small.
>
> (...)
>
>> +\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;
>> +};
>> +
>> +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.
>
> Could this command return capabilities in addition to the type?
> E.g. indicate whether a clock supports cross-timestamping?

Yes. But cross-timestamping will typically only be requested by the
driver, and supported by the device, for specific HW counters. So I
think this would be better indicated with a new VIRTIO_RTC_M_CROSS_CAP
message (as discussed in the previous to last reply).

An additional `capabilities` field may be added with the next iteration
of the spec, which should add more features.

>
>> +
>> +\end{description}
>
> (...)
>
>> +\drivernormative{\paragraph}{readq Operation}{Device Types / RTC Device / Device Operation / readq Operation}
>> +
>> +The driver MUST put the request into the device-readable part of the
>> +readq message.
>> +
>> +The driver MUST allocate enough space for the response in the
>> +device-writable part of the readq message.
>> +
>> +If VIRTIO_RTC_F_READ_CROSS was not negotiated, the driver MUST NOT send
>> +the VIRTIO_RTC_M_READ_CROSS message.
>
> There seems to be some dislike for doubly-negated statements, what
> about:
>
> "The driver MUST negotiate VIRTIO_RTC_F_READ_CROSS before sending a
> VIRTIO_RTC_M_READ_CROSS message."
>

I will change the sentence as suggested. Thanks for the review!

> (...)
>
> Generally, looks reasonable to me, but I'm not that familiar with clocks
> and would appreciate if someone else took a look as well.
>