[RFC PATCH v2 1/1] virtio-can: Device specification - 2nd RFC draft.

[RFC PATCH v2 1/1] virtio-can: Device specification - 2nd RFC draft.

[Harald Mommer]

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.

This is the reworked (2nd) version of the specification after having
gotten some feedback on the virtio lists and the Linux CAN mailing
lists. (The 1st now outdated version of the specification draft has been
sent out on 1-Apr-2021 to the virtio lists.) There is now also a virtio
CAN Linux driver which in the meantime has become good enough to be
shown.

There were a lot of changes, only mentioning the most important ones
omitting editorial changes.

- Classic CAN is indeed non-mandatory, so a feature bit is needed.
  According to ISO all CAN controllers support classic CAN but it may be
  disabled by configuration. So classic CAN is indeed a feature which
  may not be available in some environments.

- Introduce a new feature flag VIRTIO_CAN_F_LATE_TX_ACK. While marking
  as used after the actual transmission has been done on the CAN bus
  this cannot be implemented reliably in all environments. SocketCAN is
  affected at least under heavy load for TX and RX.

- RTR frames were requested on the Linux mailing list. They cannot be
  supported when the underlying CAN driver backend is a 3rd party
  AUTOSAR driver as AUTOSAR CAN does not support RTR frames. A feature
  flag VIRTIO_CAN_F_RTR_FRAMES has been added to make support of RTR
  frames an optional feature.

- Add le32 flags. There is now a reserved field in both RX and TX
  messages which might serve to contain an AUTOSAR hardware object
  handle or similar in a future version of the specification without
  need to change the layout of the RX and TX message structures.

- Removal of priorities and config space. Priorities cannot be supported
  using SocketCAN, the information delivered in the config space cannot
  be determined using SocketCAN. Support of different priorities was
  anyway too much for a first version of a specification. If priorities
  are supported in a future version there will probably be only 2
  different priorities, normal and high. In a future version of the
  specification high priority messages may either be supported by using
  a flag bit in the TX message but most probably the better solution
  will be to add add a dedicated 2nd TX queue for high priority messages
  in a review comment. But as already said priorities are not for now.
---
 conformance.tex  |  27 +++++-
 content.tex      |   1 +
 introduction.tex |   2 +
 virtio-can.tex   | 229 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 255 insertions(+), 4 deletions(-)
 create mode 100644 virtio-can.tex

diff --git a/conformance.tex b/conformance.tex
index c3c1d3e..db09353 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}
@@ -335,6 +337,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / PMEM Device / Device Initialization}
 \end{itemize}
 
+\conformance{\subsection}{CAN Driver Conformance}\label{sec:Conformance / Driver Conformance / CAN Driver Conformance}
+
+A CAN driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / CAN Device / Device Operation / Initialization}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -621,6 +631,15 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / PMEM Device / Device Operation / Virtqueue return}
 \end{itemize}
 
+\conformance{\subsection}{CAN Device Conformance}\label{sec:Conformance / Device Conformance / CAN Device Conformance}
+
+A CAN device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / CAN Device / Feature bits}
+\item \ref{devicenormative:Device Types / CAN Device / Device Operation / CAN Message Transmission}
+\end{itemize}
+
 \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
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index e863709..0a82212 100644
--- a/content.tex
+++ b/content.tex
@@ -6851,6 +6851,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-scmi.tex}
 \input{virtio-gpio.tex}
 \input{virtio-pmem.tex}
+\input{virtio-can.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/introduction.tex b/introduction.tex
index 287c5fc..e76c8a9 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -98,6 +98,8 @@ \section{Normative References}\label{sec:Normative References}
 	\phantomsection\label{intro:SEC1}\textbf{[SEC1]} &
     Standards for Efficient Cryptography Group(SECG), ``SEC1: Elliptic Cureve Cryptography'', Version 1.0, September 2000.
 	\newline\url{https://www.secg.org/sec1-v2.pdf}\\
+	\phantomsection\label{intro:CAN}\textbf{[CAN]} &
+	ISO 11898-1:2015 Road vehicles -- Controller area network (CAN) -- Part 1: Data link layer and physical signalling\\
 
 \end{longtable}
 
diff --git a/virtio-can.tex b/virtio-can.tex
new file mode 100644
index 0000000..ef4de7b
--- /dev/null
+++ b/virtio-can.tex
@@ -0,0 +1,229 @@
+\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
+\item[3] Indicationq
+\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.
+
+The \field{Indicationq} is used to receive unsolicited indications of
+CAN controller state changes.
+
+\devicenormative{\subsubsection}{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.
+Without this feature flag 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.
+
+\end{description}
+
+\drivernormative{\subsubsection}{Device Initialization}{Device Types / CAN Device / Initialization}
+
+\begin{enumerate}
+
+\item Read the feature bits and negotiate with the device.
+
+\item The driver MUST populate the \field{Rxq} with empty
+device-writeable buffers of at least the struct virtio_can_rx size to be
+ready for the reception of CAN messages.
+
+\item The driver MUST populate the \field{Indicationq} with empty
+device-writeable buffers of at least the struct virtio_can_event_ind size
+so that the CAN device is able to provide status change indications to the
+virtio CAN driver.
+
+\end{enumerate}
+
+\subsection{Device Operation}\label{sec:Device Types / CAN Device / Device Operation}
+
+A device operation has an outcome which is described by one of the
+following values:
+
+\begin{lstlisting}
+#define VIRTIO_CAN_RESULT_OK     0u
+#define VIRTIO_CAN_RESULT_NOT_OK 1u
+\end{lstlisting}
+
+Other values are to be treated like VIRTIO_CAN_RESULT_NOT_OK.
+
+The type of a CAN message identifier is determined by \field{flags}. The
+3 most significant bits of
+\field{can_id} do not bear the information about the type of the CAN
+message identifier and are 0.
+
+\begin{lstlisting}
+#define VIRTIO_CAN_FLAGS_FD            0x4000U
+#define VIRTIO_CAN_FLAGS_EXTENDED      0x8000U
+#define VIRTIO_CAN_FLAGS_RTR           0x2000U
+\end{lstlisting}
+
+\subsubsection{Controller Mode}\label{sec:Device Types / CAN Device / Device Operation / Controller Mode}
+
+The general format of a request in the \field{Controlq} is
+
+\begin{lstlisting}
+struct virtio_can_control_out {
+#define VIRTIO_CAN_SET_CTRL_MODE_START  0x0201u
+#define VIRTIO_CAN_SET_CTRL_MODE_STOP   0x0202u
+        le16 msg_type; 
+};
+\end{lstlisting}
+
+To participate in bus communication the CAN controller is started by
+sending a VIRTIO_CAN_SET_CTRL_MODE_START control message, to stop
+participating in bus communication it is stopped by sending a
+VIRTIO_CAN_SET_CTRL_MODE_STOP control message. Both requests are
+confirmed by the result of the operation.
+
+\begin{lstlisting}
+struct virtio_can_control_in {
+        u8 result;
+};
+\end{lstlisting}
+
+If the transition succeeded the \field{result} is VIRTIO_CAN_RESULT_OK
+otherwise it is VIRTIO_CAN_RESULT_NOT_OK. The device marks the request
+used when the CAN controller has finalized the transition to the
+requested controller mode.
+
+On transition to the STOPPED state the device cancels all CAN messages
+already pending for transmission and marks them as used with
+\field{result} VIRTIO_CAN_RESULT_NOT_OK. In the STOPPED state the
+device marks messages received from the
+\field{Txq} as used with \field{result} VIRTIO_CAN_RESULT_NOT_OK without
+transmitting them to the CAN bus.
+
+Initially the CAN controller is in the STOPPED state.
+
+Control queue messages are processed in order.
+
+\devicenormative{\subsubsection}{CAN Message Transmission}{Device Types / CAN Device / Device Operation / CAN Message Transmission}
+
+Messages are transmitted by placing outgoing CAN messages in the
+\field{Txq} virtqueue.
+
+\begin{lstlisting}
+struct virtio_can_tx_out {
+#define VIRTIO_CAN_TX 0x0001u
+        le16 msg_type;
+        le16 reserved;
+        le32 flags;
+        le32 can_id;
+        u8 sdu[];
+};
+
+struct virtio_can_tx_in {
+        u8 result;
+};
+\end{lstlisting}
+
+The length of \field{sdu} is implicit in the length of the device
+read-only descriptors.
+
+If transmission of a CAN frame type is requested for which support has
+not been negotiated \field{result} shall be set to
+VIRTIO_CAN_RESULT_NOT_OK and the message shall not be scheduled for
+transmission.
+
+If \field{can_id} or field{sdu} length are out of range or the CAN
+controller is in an invalid state \field{result} shall be set to
+VIRTIO_CAN_RESULT_NOT_OK and the message shall not be scheduled for
+transmission.
+
+If the parameters are valid the message is scheduled for transmission.
+
+If feature VIRTIO_CAN_F_CAN_LATE_TX_ACK has been negotiated the
+transmission request MUST be marked as used with \field{result} set to
+VIRTIO_CAN_OK after the CAN controller acknowledged the successful
+transmission on the CAN bus. Without feature
+VIRTIO_CAN_F_CAN_LATE_TX_ACK negotiated the transmission request MAY
+already be marked as used with \field{result} set to VIRTIO_CAN_OK when
+the transmission request has been processed by the virtio CAN device and
+send down the protocol stack being scheduled for transmission.
+
+\subsubsection{CAN Message Reception}\label{sec:Device Types / CAN Device / Device Operation / CAN Message Reception}
+
+Messages can be received by providing empty incoming buffers to the
+virtqueue \field{Rxq}.
+
+\begin{lstlisting}
+struct virtio_can_rx {
+#define VIRTIO_CAN_RX 0x0101u
+        le16 msg_type;
+        le16 reserved;
+        le32 flags;
+        le32 can_id;
+        u8 sdu[];
+};
+\end{lstlisting}
+
+If the feature VIRTIO_CAN_F_CAN_FD has been negotiated the maximal
+possible \field{sdu} length is 64, if the feature has not been
+negotiated the maximal possible \field{sdu} length is 8.
+
+The actual length of the \field{sdu} is calculated from the length of the
+driver read-only descriptors.
+
+\subsubsection{BusOff Indication}\label{sec:Device Types / CAN Device / Device Operation / BusOff Indication}
+
+There are certain error conditions so that the physical CAN controller
+has to stop participating in CAN communication on the bus. If such an
+error condition occurs the device informs the driver about the
+unsolicited CAN controller state change by a CAN BusOff indication.
+
+\begin{lstlisting}
+struct virtio_can_event_ind {
+#define VIRTIO_CAN_BUSOFF_IND 0x0301u
+        le16 msg_type;
+};
+\end{lstlisting}
+
+After bus-off detection the CAN controller is in STOPPED state. The CAN
+module does not participate in bus communication any more so all CAN
+messages pending for transmission are put into the used queue with
+\field{result} VIRTIO_CAN_RESULT_NOT_OK.
-- 
2.17.1

Re: [virtio-comment] [RFC PATCH v2 1/1] virtio-can: Device specification - 2nd RFC draft.

[Cornelia Huck]

On Thu, Aug 25 2022, Harald Mommer <harald.mommer@opensynergy.com> wrote:

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

Note that I'm not familiar with CAN, so I'll comment mostly from a
virtio spec POV.

>
> This is the reworked (2nd) version of the specification after having
> gotten some feedback on the virtio lists and the Linux CAN mailing
> lists. (The 1st now outdated version of the specification draft has been
> sent out on 1-Apr-2021 to the virtio lists.) There is now also a virtio
> CAN Linux driver which in the meantime has become good enough to be
> shown.
>
> There were a lot of changes, only mentioning the most important ones
> omitting editorial changes.
>
> - Classic CAN is indeed non-mandatory, so a feature bit is needed.
>   According to ISO all CAN controllers support classic CAN but it may be
>   disabled by configuration. So classic CAN is indeed a feature which
>   may not be available in some environments.
>
> - Introduce a new feature flag VIRTIO_CAN_F_LATE_TX_ACK. While marking
>   as used after the actual transmission has been done on the CAN bus
>   this cannot be implemented reliably in all environments. SocketCAN is
>   affected at least under heavy load for TX and RX.
>
> - RTR frames were requested on the Linux mailing list. They cannot be
>   supported when the underlying CAN driver backend is a 3rd party
>   AUTOSAR driver as AUTOSAR CAN does not support RTR frames. A feature
>   flag VIRTIO_CAN_F_RTR_FRAMES has been added to make support of RTR
>   frames an optional feature.
>
> - Add le32 flags. There is now a reserved field in both RX and TX
>   messages which might serve to contain an AUTOSAR hardware object
>   handle or similar in a future version of the specification without
>   need to change the layout of the RX and TX message structures.
>
> - Removal of priorities and config space. Priorities cannot be supported
>   using SocketCAN, the information delivered in the config space cannot
>   be determined using SocketCAN. Support of different priorities was
>   anyway too much for a first version of a specification. If priorities
>   are supported in a future version there will probably be only 2
>   different priorities, normal and high. In a future version of the
>   specification high priority messages may either be supported by using
>   a flag bit in the TX message but most probably the better solution
>   will be to add add a dedicated 2nd TX queue for high priority messages
>   in a review comment. But as already said priorities are not for now.

Please keep the change log separate from the description.

[Most people add a S-o-b, although we don't enforce DCO.]

> ---
>  conformance.tex  |  27 +++++-
>  content.tex      |   1 +
>  introduction.tex |   2 +
>  virtio-can.tex   | 229 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 255 insertions(+), 4 deletions(-)
>  create mode 100644 virtio-can.tex

(...)

> diff --git a/virtio-can.tex b/virtio-can.tex
> new file mode 100644
> index 0000000..ef4de7b
> --- /dev/null
> +++ b/virtio-can.tex
> @@ -0,0 +1,229 @@
> +\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
> +\item[3] Indicationq
> +\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.
> +
> +The \field{Indicationq} is used to receive unsolicited indications of
> +CAN controller state changes.
> +
> +\devicenormative{\subsubsection}{Feature bits}{Device Types / CAN Device / Feature bits}

We usually don't put the whole feature bit specification into a
normative section, especially as it applies to both device and
driver. A device normative section is for statements like "The device
MUST offer <feature> if <condition> applies", and a driver normative
section for things like "The driver MUST accept <feature> if offered".

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

Are any combinations of those three feature bits valid (both to be
offered, and to be negotiated?) It looks like F_RTR_FRAMES would only
work with F_CAN_CLASSIC?

> +
> +\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.
> +Without this feature flag negotiated the device is allowed to mark

"If this feature bit has not been negotiated, ..."

> +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.
> +
> +\end{description}
> +
> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / CAN Device / Initialization}
> +
> +\begin{enumerate}
> +
> +\item Read the feature bits and negotiate with the device.

I think this step is redundant.

> +
> +\item The driver MUST populate the \field{Rxq} with empty
> +device-writeable buffers of at least the struct virtio_can_rx size to be
> +ready for the reception of CAN messages.

virtio_can_rx is only defined further below, and, IIUC, depends on the
frames that are supported. Can this be expressed as a kind of absolute
value (depending on the negotiated features)?

> +
> +\item The driver MUST populate the \field{Indicationq} with empty
> +device-writeable buffers of at least the struct virtio_can_event_ind size
> +so that the CAN device is able to provide status change indications to the
> +virtio CAN driver.

Is virtio_can_event_ind always supposed to be an le16 value? If yes,
it would probably be easier to specify that here.

> +
> +\end{enumerate}
> +
> +\subsection{Device Operation}\label{sec:Device Types / CAN Device / Device Operation}
> +
> +A device operation has an outcome which is described by one of the
> +following values:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CAN_RESULT_OK     0u
> +#define VIRTIO_CAN_RESULT_NOT_OK 1u
> +\end{lstlisting}
> +
> +Other values are to be treated like VIRTIO_CAN_RESULT_NOT_OK.
> +
> +The type of a CAN message identifier is determined by \field{flags}. The
> +3 most significant bits of
> +\field{can_id} do not bear the information about the type of the CAN
> +message identifier and are 0.
> +
> +\begin{lstlisting}
> +#define VIRTIO_CAN_FLAGS_FD            0x4000U
> +#define VIRTIO_CAN_FLAGS_EXTENDED      0x8000U
> +#define VIRTIO_CAN_FLAGS_RTR           0x2000U
> +\end{lstlisting}

You refer to the "flags" and "can_id" fields, but it is unclear in which
structure. Would this benefit from reordering?

> +
> +\subsubsection{Controller Mode}\label{sec:Device Types / CAN Device / Device Operation / Controller Mode}
> +
> +The general format of a request in the \field{Controlq} is
> +
> +\begin{lstlisting}
> +struct virtio_can_control_out {
> +#define VIRTIO_CAN_SET_CTRL_MODE_START  0x0201u
> +#define VIRTIO_CAN_SET_CTRL_MODE_STOP   0x0202u
> +        le16 msg_type; 
> +};
> +\end{lstlisting}
> +
> +To participate in bus communication the CAN controller is started by
> +sending a VIRTIO_CAN_SET_CTRL_MODE_START control message, to stop
> +participating in bus communication it is stopped by sending a
> +VIRTIO_CAN_SET_CTRL_MODE_STOP control message. Both requests are
> +confirmed by the result of the operation.
> +
> +\begin{lstlisting}
> +struct virtio_can_control_in {
> +        u8 result;
> +};
> +\end{lstlisting}
> +
> +If the transition succeeded the \field{result} is VIRTIO_CAN_RESULT_OK
> +otherwise it is VIRTIO_CAN_RESULT_NOT_OK. The device marks the request
> +used when the CAN controller has finalized the transition to the
> +requested controller mode.
> +
> +On transition to the STOPPED state the device cancels all CAN messages
> +already pending for transmission and marks them as used with
> +\field{result} VIRTIO_CAN_RESULT_NOT_OK. In the STOPPED state the
> +device marks messages received from the
> +\field{Txq} as used with \field{result} VIRTIO_CAN_RESULT_NOT_OK without
> +transmitting them to the CAN bus.
> +
> +Initially the CAN controller is in the STOPPED state.

Is that an internal state of the controller that can be changed from the
outside?

> +
> +Control queue messages are processed in order.
> +
> +\devicenormative{\subsubsection}{CAN Message Transmission}{Device Types / CAN Device / Device Operation / CAN Message Transmission}

I think this should be a normal subsection, as you describe not only
what the device needs to do, but also what the driver does. Maybe use
separate normative sections for MUST statements?

> +
> +Messages are transmitted by placing outgoing CAN messages in the
> +\field{Txq} virtqueue.

Who places the messages? The driver? Probably better to use active voice
here.

> +
> +\begin{lstlisting}
> +struct virtio_can_tx_out {
> +#define VIRTIO_CAN_TX 0x0001u
> +        le16 msg_type;
> +        le16 reserved;
> +        le32 flags;
> +        le32 can_id;
> +        u8 sdu[];
> +};
> +
> +struct virtio_can_tx_in {
> +        u8 result;
> +};
> +\end{lstlisting}
> +
> +The length of \field{sdu} is implicit in the length of the device
> +read-only descriptors.

Not sure what this means -- does that mean that any length of the
desriptors is ok, as long as it fits the whole structure?

> +
> +If transmission of a CAN frame type is requested for which support has
> +not been negotiated \field{result} shall be set to
> +VIRTIO_CAN_RESULT_NOT_OK and the message shall not be scheduled for
> +transmission.

This probably should rather be something like "The device MUST reject
any CAN frame type for which support has not been negotiated with
VIRTIO_CAN_RESULT_NOT_OK in \field{result} and MUST NOT schedule the
message for transmission."

> +
> +If \field{can_id} or field{sdu} length are out of range or the CAN
> +controller is in an invalid state \field{result} shall be set to
> +VIRTIO_CAN_RESULT_NOT_OK and the message shall not be scheduled for
> +transmission.

Same here.

> +
> +If the parameters are valid the message is scheduled for transmission.
> +
> +If feature VIRTIO_CAN_F_CAN_LATE_TX_ACK has been negotiated the
> +transmission request MUST be marked as used with \field{result} set to
> +VIRTIO_CAN_OK after the CAN controller acknowledged the successful
> +transmission on the CAN bus. Without feature
> +VIRTIO_CAN_F_CAN_LATE_TX_ACK negotiated the transmission request MAY
> +already be marked as used with \field{result} set to VIRTIO_CAN_OK when
> +the transmission request has been processed by the virtio CAN device and
> +send down the protocol stack being scheduled for transmission.
> +
> +\subsubsection{CAN Message Reception}\label{sec:Device Types / CAN Device / Device Operation / CAN Message Reception}
> +
> +Messages can be received by providing empty incoming buffers to the
> +virtqueue \field{Rxq}.
> +
> +\begin{lstlisting}
> +struct virtio_can_rx {
> +#define VIRTIO_CAN_RX 0x0101u
> +        le16 msg_type;
> +        le16 reserved;
> +        le32 flags;
> +        le32 can_id;
> +        u8 sdu[];
> +};
> +\end{lstlisting}
> +
> +If the feature VIRTIO_CAN_F_CAN_FD has been negotiated the maximal
> +possible \field{sdu} length is 64, if the feature has not been
> +negotiated the maximal possible \field{sdu} length is 8.
> +
> +The actual length of the \field{sdu} is calculated from the length of the
> +driver read-only descriptors.

So the provided descriptors must fit at least the size of the structure
plus whatever length the negotiated frame sizes support? Can the driver
negotiate F_CAN_FD and still provide descriptors that only allow an sdu
of length 8?

> +
> +\subsubsection{BusOff Indication}\label{sec:Device Types / CAN Device / Device Operation / BusOff Indication}
> +
> +There are certain error conditions so that the physical CAN controller
> +has to stop participating in CAN communication on the bus. If such an
> +error condition occurs the device informs the driver about the
> +unsolicited CAN controller state change by a CAN BusOff indication.
> +
> +\begin{lstlisting}
> +struct virtio_can_event_ind {
> +#define VIRTIO_CAN_BUSOFF_IND 0x0301u
> +        le16 msg_type;
> +};
> +\end{lstlisting}
> +
> +After bus-off detection the CAN controller is in STOPPED state. The CAN
> +module does not participate in bus communication any more so all CAN

s/module/device/ ?

> +messages pending for transmission are put into the used queue with
> +\field{result} VIRTIO_CAN_RESULT_NOT_OK.

Re: [virtio-dev] Re: [virtio-comment] [RFC PATCH v2 1/1] virtio-can: Device specification - 2nd RFC draft.

[Harald Mommer]

Hello,

there was a printout on my desk for some time to go through and address 
the comments but the E-Mail I did indeed not answer yet. There should go 
out in the next days a reworked specification and driver.

On 07.11.22 13:30, Cornelia Huck wrote:
> On Thu, Aug 25 2022, Harald Mommer <harald.mommer@opensynergy.com> wrote:
>
>> 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.
> Note that I'm not familiar with CAN, so I'll comment mostly from a
> virtio spec POV.
>
>> This is the reworked (2nd) version of the specification after having
>> gotten some feedback on the virtio lists and the Linux CAN mailing
>> lists. (The 1st now outdated version of the specification draft has been
>> sent out on 1-Apr-2021 to the virtio lists.) There is now also a virtio
>> CAN Linux driver which in the meantime has become good enough to be
>> shown.
>>
>> There were a lot of changes, only mentioning the most important ones
>> omitting editorial changes.
>>
>> - Classic CAN is indeed non-mandatory, so a feature bit is needed.
>>    According to ISO all CAN controllers support classic CAN but it may be
>>    disabled by configuration. So classic CAN is indeed a feature which
>>    may not be available in some environments.
>>
>> - Introduce a new feature flag VIRTIO_CAN_F_LATE_TX_ACK. While marking
>>    as used after the actual transmission has been done on the CAN bus
>>    this cannot be implemented reliably in all environments. SocketCAN is
>>    affected at least under heavy load for TX and RX.
>>
>> - RTR frames were requested on the Linux mailing list. They cannot be
>>    supported when the underlying CAN driver backend is a 3rd party
>>    AUTOSAR driver as AUTOSAR CAN does not support RTR frames. A feature
>>    flag VIRTIO_CAN_F_RTR_FRAMES has been added to make support of RTR
>>    frames an optional feature.
>>
>> - Add le32 flags. There is now a reserved field in both RX and TX
>>    messages which might serve to contain an AUTOSAR hardware object
>>    handle or similar in a future version of the specification without
>>    need to change the layout of the RX and TX message structures.
>>
>> - Removal of priorities and config space. Priorities cannot be supported
>>    using SocketCAN, the information delivered in the config space cannot
>>    be determined using SocketCAN. Support of different priorities was
>>    anyway too much for a first version of a specification. If priorities
>>    are supported in a future version there will probably be only 2
>>    different priorities, normal and high. In a future version of the
>>    specification high priority messages may either be supported by using
>>    a flag bit in the TX message but most probably the better solution
>>    will be to add add a dedicated 2nd TX queue for high priority messages
>>    in a review comment. But as already said priorities are not for now.
> Please keep the change log separate from the description.
>
> [Most people add a S-o-b, although we don't enforce DCO.]

I understand neither "S-o-b" nor "DCO". Neither Wikipedia nor Google are 
helpful, nothing fits.

>> ---
>>   conformance.tex  |  27 +++++-
>>   content.tex      |   1 +
>>   introduction.tex |   2 +
>>   virtio-can.tex   | 229 +++++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 255 insertions(+), 4 deletions(-)
>>   create mode 100644 virtio-can.tex
> (...)
>
>> diff --git a/virtio-can.tex b/virtio-can.tex
>> new file mode 100644
>> index 0000000..ef4de7b
>> --- /dev/null
>> +++ b/virtio-can.tex
>> @@ -0,0 +1,229 @@
>> +\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
>> +\item[3] Indicationq
>> +\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.
>> +
>> +The \field{Indicationq} is used to receive unsolicited indications of
>> +CAN controller state changes.
>> +
>> +\devicenormative{\subsubsection}{Feature bits}{Device Types / CAN Device / Feature bits}
> We usually don't put the whole feature bit specification into a
> normative section, especially as it applies to both device and
> driver. A device normative section is for statements like "The device
> MUST offer <feature> if <condition> applies", and a driver normative
> section for things like "The driver MUST accept <feature> if offered".
Reworked.
>> +
>> +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.
> Are any combinations of those three feature bits valid (both to be
> offered, and to be negotiated?) It looks like F_RTR_FRAMES would only
> work with F_CAN_CLASSIC?

Without VIRTIO_CAN_F_CAN_CLASSIC no VIRTIO_CAN_F_RTR_FRAMES. Added.

>> +
>> +\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.
>> +Without this feature flag negotiated the device is allowed to mark
> "If this feature bit has not been negotiated, ..."
Replaced now not only here but also in a later chapter.
>> +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.
>> +
>> +\end{description}
>> +
>> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / CAN Device / Initialization}
>> +
>> +\begin{enumerate}
>> +
>> +\item Read the feature bits and negotiate with the device.
> I think this step is redundant.
Removed.
>
>> +
>> +\item The driver MUST populate the \field{Rxq} with empty
>> +device-writeable buffers of at least the struct virtio_can_rx size to be
>> +ready for the reception of CAN messages.
> virtio_can_rx is only defined further below, and, IIUC, depends on the
> frames that are supported. Can this be expressed as a kind of absolute
> value (depending on the negotiated features)?

Made a forward reference to the chapter where the structure is defined.

I could add here a a constant but nobody else is doing this in the 
specification. 2 le16 + 2 le32 = 12 bytes + max payload length. (Will 
become 16 bytes instead of 12 bytes in the next version.) With CAN 
classic the payload may have a length of up to 8 bytes, with CAN FD the 
payload may have a length of up to 64 bytes. The max payload length of 
CAN classic and CAN FD are mentioned at different places already, for 
CAN people the max. payload length are fundamental constants they know 
anyway. All is given. 12+8 = X for CAN classic and 12+64 = Y for CAN FD.

>> +
>> +\item The driver MUST populate the \field{Indicationq} with empty
>> +device-writeable buffers of at least the struct virtio_can_event_ind size
>> +so that the CAN device is able to provide status change indications to the
>> +virtio CAN driver.
> Is virtio_can_event_ind always supposed to be an le16 value? If yes,
> it would probably be easier to specify that here.
The indication queue will be thrown out. An extra queue to indicate a 
rare event which could be represented in a single bit in the config 
space was considered overkill and the indication queue also caused 
headache with certain race conditions we thought about.
>> +
>> +\end{enumerate}
>> +
>> +\subsection{Device Operation}\label{sec:Device Types / CAN Device / Device Operation}
>> +
>> +A device operation has an outcome which is described by one of the
>> +following values:
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CAN_RESULT_OK     0u
>> +#define VIRTIO_CAN_RESULT_NOT_OK 1u
>> +\end{lstlisting}
>> +
>> +Other values are to be treated like VIRTIO_CAN_RESULT_NOT_OK.
>> +
>> +The type of a CAN message identifier is determined by \field{flags}. The
>> +3 most significant bits of
>> +\field{can_id} do not bear the information about the type of the CAN
>> +message identifier and are 0.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CAN_FLAGS_FD            0x4000U
>> +#define VIRTIO_CAN_FLAGS_EXTENDED      0x8000U
>> +#define VIRTIO_CAN_FLAGS_RTR           0x2000U
>> +\end{lstlisting}
> You refer to the "flags" and "can_id" fields, but it is unclear in which
> structure. Would this benefit from reordering?
Put the flags now close to struct virtio_can_tx_out and mentioned for 
struct virtio_can_tx_in that the flags are the same. Should be more 
clear now to what those definitions belong.
>> +
>> +\subsubsection{Controller Mode}\label{sec:Device Types / CAN Device / Device Operation / Controller Mode}
>> +
>> +The general format of a request in the \field{Controlq} is
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_control_out {
>> +#define VIRTIO_CAN_SET_CTRL_MODE_START  0x0201u
>> +#define VIRTIO_CAN_SET_CTRL_MODE_STOP   0x0202u
>> +        le16 msg_type;
>> +};
>> +\end{lstlisting}
>> +
>> +To participate in bus communication the CAN controller is started by
>> +sending a VIRTIO_CAN_SET_CTRL_MODE_START control message, to stop
>> +participating in bus communication it is stopped by sending a
>> +VIRTIO_CAN_SET_CTRL_MODE_STOP control message. Both requests are
>> +confirmed by the result of the operation.
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_control_in {
>> +        u8 result;
>> +};
>> +\end{lstlisting}
>> +
>> +If the transition succeeded the \field{result} is VIRTIO_CAN_RESULT_OK
>> +otherwise it is VIRTIO_CAN_RESULT_NOT_OK. The device marks the request
>> +used when the CAN controller has finalized the transition to the
>> +requested controller mode.
>> +
>> +On transition to the STOPPED state the device cancels all CAN messages
>> +already pending for transmission and marks them as used with
>> +\field{result} VIRTIO_CAN_RESULT_NOT_OK. In the STOPPED state the
>> +device marks messages received from the
>> +\field{Txq} as used with \field{result} VIRTIO_CAN_RESULT_NOT_OK without
>> +transmitting them to the CAN bus.
>> +
>> +Initially the CAN controller is in the STOPPED state.
> Is that an internal state of the controller that can be changed from the
> outside?
Can be changed from the outside, this is the control queue sending 
VIRTIO_CAN_SET_CTRL_MODE_START to start the controller or 
VIRTIO_CAN_SET_CTRL_MODE_STOP to stop the controller. And the controller 
transitions from start to stop on certain error conditions on the bus 
which are indicated by a bus off from the physical CAN controller hardware.
>> +
>> +Control queue messages are processed in order.
>> +
>> +\devicenormative{\subsubsection}{CAN Message Transmission}{Device Types / CAN Device / Device Operation / CAN Message Transmission}
> I think this should be a normal subsection, as you describe not only
> what the device needs to do, but also what the driver does. Maybe use
> separate normative sections for MUST statements?
What I have now in the reworked section is that I start with an active 
descriptive sentence "The driver transmits messages by placing outgoing 
CAN messages in the Txq virtqueue" and subsequently talk about the 
device. Could be ok now but I might be wrong.
>> +
>> +Messages are transmitted by placing outgoing CAN messages in the
>> +\field{Txq} virtqueue.
> Who places the messages? The driver? Probably better to use active voice
> here.
This is the descriptive sentence now in active voice explaining what the 
message is and where it comes from.
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_tx_out {
>> +#define VIRTIO_CAN_TX 0x0001u
>> +        le16 msg_type;
>> +        le16 reserved;
>> +        le32 flags;
>> +        le32 can_id;
>> +        u8 sdu[];
>> +};
>> +
>> +struct virtio_can_tx_in {
>> +        u8 result;
>> +};
>> +\end{lstlisting}
>> +
>> +The length of \field{sdu} is implicit in the length of the device
>> +read-only descriptors.
> Not sure what this means -- does that mean that any length of the
> desriptors is ok, as long as it fits the whole structure?

There is some information provided how much bytes the other side has 
written. From this the payload length can be deducted without having a 
length field in the structure itself. There were some more comments on 
this, we thought about it and now we have a le16 length field in the 
structure. More clear and probably more future proof.

However it was strange behavior / worth a warning / need to truncate 
when more than 8 bytes payload for CAN classic or more than 64 bytes 
payload for CAN FD were present.

>> +
>> +If transmission of a CAN frame type is requested for which support has
>> +not been negotiated \field{result} shall be set to
>> +VIRTIO_CAN_RESULT_NOT_OK and the message shall not be scheduled for
>> +transmission.
> This probably should rather be something like "The device MUST reject
> any CAN frame type for which support has not been negotiated with
> VIRTIO_CAN_RESULT_NOT_OK in \field{result} and MUST NOT schedule the
> message for transmission."
Reworded as proposed.
>> +
>> +If \field{can_id} or field{sdu} length are out of range or the CAN
>> +controller is in an invalid state \field{result} shall be set to
>> +VIRTIO_CAN_RESULT_NOT_OK and the message shall not be scheduled for
>> +transmission.
> Same here.
"shall" => "MUST". Done.
>> +
>> +If the parameters are valid the message is scheduled for transmission.
>> +
>> +If feature VIRTIO_CAN_F_CAN_LATE_TX_ACK has been negotiated the
>> +transmission request MUST be marked as used with \field{result} set to
>> +VIRTIO_CAN_OK after the CAN controller acknowledged the successful
>> +transmission on the CAN bus. Without feature
>> +VIRTIO_CAN_F_CAN_LATE_TX_ACK negotiated the transmission request MAY
>> +already be marked as used with \field{result} set to VIRTIO_CAN_OK when
>> +the transmission request has been processed by the virtio CAN device and
>> +send down the protocol stack being scheduled for transmission.
>> +
>> +\subsubsection{CAN Message Reception}\label{sec:Device Types / CAN Device / Device Operation / CAN Message Reception}
>> +
>> +Messages can be received by providing empty incoming buffers to the
>> +virtqueue \field{Rxq}.
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_rx {
>> +#define VIRTIO_CAN_RX 0x0101u
>> +        le16 msg_type;
>> +        le16 reserved;
>> +        le32 flags;
>> +        le32 can_id;
>> +        u8 sdu[];
>> +};
>> +\end{lstlisting}
>> +
>> +If the feature VIRTIO_CAN_F_CAN_FD has been negotiated the maximal
>> +possible \field{sdu} length is 64, if the feature has not been
>> +negotiated the maximal possible \field{sdu} length is 8.
>> +
>> +The actual length of the \field{sdu} is calculated from the length of the
>> +driver read-only descriptors.
> So the provided descriptors must fit at least the size of the structure
> plus whatever length the negotiated frame sizes support? Can the driver
> negotiate F_CAN_FD and still provide descriptors that only allow an sdu
> of length 8?
Negotiating CAN FD and not allowing for 64 bytes payload would be like 
having an Ethernet device not providing enough room to receive a 1518 
bytes frame with 1500 bytes payload. Means this was a bug.
>> +
>> +\subsubsection{BusOff Indication}\label{sec:Device Types / CAN Device / Device Operation / BusOff Indication}
>> +
>> +There are certain error conditions so that the physical CAN controller
>> +has to stop participating in CAN communication on the bus. If such an
>> +error condition occurs the device informs the driver about the
>> +unsolicited CAN controller state change by a CAN BusOff indication.
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_event_ind {
>> +#define VIRTIO_CAN_BUSOFF_IND 0x0301u
>> +        le16 msg_type;
>> +};
>> +\end{lstlisting}
>> +
>> +After bus-off detection the CAN controller is in STOPPED state. The CAN
>> +module does not participate in bus communication any more so all CAN
> s/module/device/ ?

Yes, device.

(And the rarely used message will be eliminated in the next version 
together with the queue in favor of a single bit in the config space. 
Already mentioned above.)

>> +messages pending for transmission are put into the used queue with
>> +\field{result} VIRTIO_CAN_RESULT_NOT_OK.

Re: [virtio-dev] Re: [virtio-comment] [RFC PATCH v2 1/1] virtio-can: Device specification - 2nd RFC draft.

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On 07.06.2023 15:45:33, Harald Mommer wrote:
> > > - Removal of priorities and config space. Priorities cannot be supported
> > >    using SocketCAN, the information delivered in the config space cannot
> > >    be determined using SocketCAN. Support of different priorities was
> > >    anyway too much for a first version of a specification. If priorities
> > >    are supported in a future version there will probably be only 2
> > >    different priorities, normal and high. In a future version of the
> > >    specification high priority messages may either be supported by using
> > >    a flag bit in the TX message but most probably the better solution
> > >    will be to add add a dedicated 2nd TX queue for high priority messages
> > >    in a review comment. But as already said priorities are not for now.
> > Please keep the change log separate from the description.
> > 
> > [Most people add a S-o-b, although we don't enforce DCO.]
> 
> I understand neither "S-o-b" nor "DCO". Neither Wikipedia nor Google are
> helpful, nothing fits.

S-o-b: Signed-off-by
DCO: Developer's Certificate of Origin

See https://elixir.bootlin.com/linux/v6.3/source/Documentation/process/submitting-patches.rst#L379

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]