Re: [virtio-comment] [RFC PATCH v3] virtio-can: Device specification.

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

On Mon, Jan 08, 2024 at 06:18:50PM +0100, Mikhail Golubev-Ciuchea wrote:
> Hi all!
> 
> I kindly request a vote.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/186
> 
> 
> Best wishes,
> Mikhail Golubev-Ciuchea
> 
> 
> 
> On 6/9/23 16:22, Mikhail Golubev-Ciuchea wrote:
> > From: Harald Mommer <harald.mommer@opensynergy.com>
> > 
> > virtio-can is a virtual CAN device. It provides a way to give access to
> > a CAN controller from a driver guest. The device is aimed to be used by
> > driver guests running a HLOS as well as by driver guests running a
> > typical RTOS as used in controller environments.
> > 
> > Signed-off-by: Harald Mommer <Harald.Mommer@opensynergy.com>
> > Signed-off-by: Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@opensynergy.com>
> > ---
> > 
> > RFC v3:
> > * Add length fields in CAN RX and TX messages.
> > * Replace bus off indication queue with a config space bit.
> > * Clarify handling of unknown flag bits set in CAN frame.
> > * Remove MISRA C suffixes in constants.
> > * Reserve 16 bits in RX/TX messages for CAN XL priority.
> > * Reserve 8 bits in RX/TX messages for CAN classic DLC.
> > * Rework according to general virtio spec POV.
> > * Implementation:
> >    driver: https://lore.kernel.org/all/20230607145613.133203-1-Mikhail.Golubev-Ciuchea@opensynergy.com/
> >    QEmu device: https://github.com/OpenSynergy/qemu/tree/virtio-can-spec-rfc-v3
> > 
> > RFC v2:
> > * Add CAN classic feature flag.
> > * Add feature flag VIRTIO_CAN_F_LATE_TX_ACK.
> > * Add feature flag VIRTIO_CAN_F_RTR_FRAMES.
> > * Reserve 32 bits in RX/TX messages.
> > * Remove priorities of messages.
> > 
> >   conformance.tex                         |  12 +-
> >   content.tex                             |   1 +
> >   device-types/can/description.tex        | 249 ++++++++++++++++++++++++
> >   device-types/can/device-conformance.tex |   8 +
> >   device-types/can/driver-conformance.tex |   7 +
> >   introduction.tex                        |   2 +
> >   6 files changed, 275 insertions(+), 4 deletions(-)
> >   create mode 100644 device-types/can/description.tex
> >   create mode 100644 device-types/can/device-conformance.tex
> >   create mode 100644 device-types/can/driver-conformance.tex
> > 
> > diff --git a/conformance.tex b/conformance.tex
> > index 01ccd69..a07ef02 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -32,8 +32,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >   \ref{sec:Conformance / Driver Conformance / Memory Driver Conformance},
> >   \ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance},
> >   \ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance},
> > -\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance} or
> > -\ref{sec:Conformance / Driver Conformance / PMEM Driver Conformance}.
> > +\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance},
> > +\ref{sec:Conformance / Driver Conformance / PMEM Driver Conformance} or
> > +\ref{sec:Conformance / Driver Conformance / CAN Driver Conformance}.
> >       \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
> >     \end{itemize}
> > @@ -59,8 +60,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >   \ref{sec:Conformance / Device Conformance / Memory Device Conformance},
> >   \ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance},
> >   \ref{sec:Conformance / Device Conformance / SCMI Device Conformance},
> > -\ref{sec:Conformance / Device Conformance / GPIO Device Conformance} or
> > -\ref{sec:Conformance / Device Conformance / PMEM Device Conformance}.
> > +\ref{sec:Conformance / Device Conformance / GPIO Device Conformance},
> > +\ref{sec:Conformance / Device Conformance / PMEM Device Conformance} or
> > +\ref{sec:Conformance / Device Conformance / CAN Device Conformance}.
> >       \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
> >     \end{itemize}
> > @@ -152,6 +154,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >   \input{device-types/scmi/driver-conformance.tex}
> >   \input{device-types/gpio/driver-conformance.tex}
> >   \input{device-types/pmem/driver-conformance.tex}
> > +\input{device-types/can/driver-conformance.tex}
> >   \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
> > @@ -238,6 +241,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >   \input{device-types/scmi/device-conformance.tex}
> >   \input{device-types/gpio/device-conformance.tex}
> >   \input{device-types/pmem/device-conformance.tex}
> > +\input{device-types/can/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/content.tex b/content.tex
> > index d2ab9eb..8806b57 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -765,6 +765,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/can/description.tex}
> >   \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > diff --git a/device-types/can/description.tex b/device-types/can/description.tex
> > new file mode 100644
> > index 0000000..2511d9c
> > --- /dev/null
> > +++ b/device-types/can/description.tex
> > @@ -0,0 +1,249 @@
> > +\section{CAN Device}\label{sec:Device Types / CAN Device}
> > +
> > +virtio-can is a virtio based CAN (Controller Area Network) controller.
> > +It is used to give a virtual machine access to a CAN bus. The CAN bus
> > +might either be a physical CAN bus or a virtual CAN bus between virtual
> > +machines or a combination of both.
> > +
> > +\subsection{Device ID}\label{sec:Device Types / CAN Device / Device ID}
> > +
> > +36
> > +
> > +\subsection{Virtqueues}\label{sec:Device Types / CAN Device / Virtqueues}
> > +
> > +\begin{description}
> > +\item[0] Txq
> > +\item[1] Rxq
> > +\item[2] Controlq
> > +\end{description}
> > +
> > +The \field{Txq} is used to send CAN packets to the CAN bus.
> > +
> > +The \field{Rxq} is used to receive CAN packets from the CAN bus.
> > +
> > +The \field{Controlq} is used to control the state of the CAN controller.
> > +
> > +\subsection{Feature bits}{Device Types / CAN Device / Feature bits}
> > +
> > +Actual CAN controllers support Extended CAN IDs with 29 bits (CAN~2.0B)
> > +as well as Standard CAN IDs with 11 bits (CAN~2.0A). The support of
> > +CAN~2.0B Extended CAN IDs is considered as mandatory for this
> > +specification.
> > +
> > +\begin{description}
> > +
> > +\item[VIRTIO_CAN_F_CAN_CLASSIC (0)]
> > +
> > +The device supports classic CAN frames with a maximum payload size of 8
> > +bytes.
> > +
> > +\item[VIRTIO_CAN_F_CAN_FD (1)]
> > +
> > +The device supports CAN FD frames with a maximum payload size of 64
> > +bytes.
> > +
> > +\item[VIRTIO_CAN_F_RTR_FRAMES (2)]
> > +
> > +The device supports RTR (remote transmission request) frames. RTR frames
> > +are only supported with classic CAN.
> > +
> > +\item[VIRTIO_CAN_F_LATE_TX_ACK (3)]
> > +
> > +The virtio CAN device marks transmission requests from the \field{Txq}
> > +as used after the CAN message has been transmitted on the CAN bus. If
> > +this feature bit has not been negotiated, the device is allowed to mark
> > +transmission requests already as used when the CAN message has been
> > +scheduled for transmission but might not yet have been transmitted on
> > +the CAN bus.
> > +

I think this description should be split it into two: 
1. The description of the feature.
2. How the behavior of the device changes whether the feature has been negotiated or not.

2) is already described in the device operation by using MAY and MUST. I
would rewrite 1) as "the device supports delayed used notification ..."
or something like that. What do you think?

Matias