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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Apr 26 2023, Peter Hilber <peter.hilber@opensynergy.com> wrote:

> On 17.04.23 13:49, Cornelia Huck wrote:
>> On Mon, Mar 13 2023, Peter Hilber <peter.hilber@opensynergy.com> wrote:
>>> +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.

Ok, that looks clearer.

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

I guess that depends on the request; the device should probably not
report an error to the driver if it can recover/retry itself. If we
can't think of any request that actually require the driver to do the
retry itself, we can ignore that for now.

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

That sounds good to me.

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

Ok, I don't really object to that.

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

Sounds good!