Re: [PATCH v8 2/4] virtio-rtc: Add initial normative statements

On Wed, Apr 16, 2025 at 11:42:01AM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 06, 2025 at 10:51:10AM +0100, Peter Hilber wrote:
> > Add the normative statements for the initial device specification.
> > 
> > Signed-off-by: Peter Hilber <[email protected]>
> > ---
> > 
> > Notes:
> >     v8:
> >     
> >     - Drop requirement to ignore reserved fields in the device-readable part
> >       of the message (Matias Ezequiel Vara Larsen).
> >     
> >     - Drop requirement about returning same status in response to identical
> >       requests (Matias Ezequiel Vara Larsen).
> >     
> >     - Reword requirements about size mismatches for device
> >       read-only/write-only parts of the message (Matias Ezequiel Vara
> >       Larsen).
> >     
> >     - Change word order from "field X" to "the X field" (Matias Ezequiel
> >       Vara Larsen).
> >     
> >     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        | 272 +++++++++++++++++++++++-
> >  device-types/rtc/device-conformance.tex |   9 +
> >  device-types/rtc/driver-conformance.tex |   9 +
> >  4 files changed, 288 insertions(+), 4 deletions(-)
> >  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 5d8cf91a6991..f2a3907de511 100644
> > --- a/device-types/rtc/description.tex
> > +++ b/device-types/rtc/description.tex
> > @@ -73,7 +73,7 @@ \subsection{Device Operation}\label{sec:Device Types / RTC Device / Device Opera
> >  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}.
> > +in the \field{msg_type} or \field{hw_counter} fields.
> >  
> >  VIRTIO_RTC_S_ENODEV indicates that the \field{clock_id} field value
> >  supplied with the request does not identify a clock.
> > @@ -98,6 +98,106 @@ \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.
> 
> 
> This is problematic, it should depend on features negotiated
> not on features listed in the spec as we will not
> remember to change this and break drivers.
> Just add in the description explanation which flags are legal for each request,
> then refer to that.

OK.

> 
> 
> > +
> > +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.
> 
> same though a bit less of a problem here.

I will change this in a similar way as above.

> 
> 
> > +
> > +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 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 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 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.
> 
> really SHUOLD is more appropriate
> 
> and first in what sense? here is the text for features:
> 
> 
> 	If a device has successfully negotiated a set of features 
> 	at least once (by accepting the FEATURES_OK \field{device 
> 	status} bit during device initialization), then it SHOULD
> 	NOT fail re-negotiation of the same set of features after
> 	a device or system reset.  Failure to do so would interfere
> 	with resuming from suspend and error recovery.
> 
> do something similar and be specific pls.

OK.

Thanks for the comments,

Peter