All of lore.kernel.org
 help / color / mirror / Atom feed
* [virtio-comment] [PATCH 1/1] [RFC] virtio-can: Add the device specification.
@ 2021-04-01 15:20 Harald Mommer
  2021-04-26 10:05 ` Stefan Hajnoczi
  0 siblings, 1 reply; 4+ messages in thread
From: Harald Mommer @ 2021-04-01 15:20 UTC (permalink / raw)
  To: virtio-comment, virtio-dev; +Cc: 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.
---
 content.tex      |   1 +
 introduction.tex |   3 +
 virtio-can.tex   | 245 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 249 insertions(+)
 create mode 100644 virtio-can.tex

diff --git a/content.tex b/content.tex
index e536fd4..c1604db 100644
--- a/content.tex
+++ b/content.tex
@@ -6564,6 +6564,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-mem.tex}
 \input{virtio-i2c.tex}
 \input{virtio-scmi.tex}
+\input{virtio-can.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/introduction.tex b/introduction.tex
index 7204b24..84ea5c0 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -79,6 +79,9 @@ \section{Normative References}\label{sec:Normative References}
 	\phantomsection\label{intro:SCMI}\textbf{[SCMI]} &
 	Arm System Control and Management Interface, DEN0056,
 	\newline\url{https://developer.arm.com/docs/den0056/c}, version C and any future revisions\\
+	\phantomsection\label{intro:CAN_Driver}\textbf{[CAN Driver]} &
+	Specification of CAN Driver -- AUTOSAR CP R20-11,
+	\newline\url{https://www.autosar.org/fileadmin/user_upload/standards/classic/20-11/AUTOSAR_SWS_CANDriver.pdf}\\
 
 \end{longtable}
 
diff --git a/virtio-can.tex b/virtio-can.tex
new file mode 100644
index 0000000..c343759
--- /dev/null
+++ b/virtio-can.tex
@@ -0,0 +1,245 @@
+\section{CAN Device}\label{sec:Device Types / CAN Device}
+
+virtio-can is a virtio based CAN (Controller Area Network) device. It is
+used to give a virtual machine access to a CAN bus. The CAN bus may
+either be a physical CAN bus or a virtual CAN bus between virtual
+machines or a combination of both.
+
+This section relies on definitions made by the AUTOSAR
+\hyperref[intro:CAN_Driver]{CAN Driver} specification.
+
+\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.
+
+\subsection{Feature Bits}\label{sec:Device Types / CAN Device / Feature Bits}
+
+The virtio-can device always supports classic CAN frames with a maximum
+payload size of 8 bytes.
+
+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_FD (0)]
+
+In addition to classic CAN frames the device supports CAN FD frames with
+a maximum payload size of 64 bytes.
+
+\end{description}
+
+\subsection{Device configuration layout}\label{sec:Device Types / CAN Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for
+the driver.
+
+\begin{lstlisting}
+struct virtio_can_config {
+        le16 lo_prio_count; 
+        le16 hi_prio_count;
+};
+\end{lstlisting}
+
+To operate the Virtio CAN device it may be necessary to know some basic
+properties of the underlying physical CAN controller hardware and its
+configuration.
+
+Physical CAN controllers may support transmission by putting messages
+into FIFOs first and / or by using transmit buffers directly. The user
+of the Virtio CAN driver may need to know
+
+\begin{itemize}
+\item Number of TX FIFO places for non time critical CAN messages
+\item Number of TX buffers for high priority CAN messages
+\end{itemize}
+
+to schedule an optimal transmission of CAN messages. Non time critical
+messages may be sent via a FIFO where they may suffer "Inner Priority
+Inversion" (\hyperref[intro:CAN_Driver]{CAN Driver} chapter 2.1). High
+priority messages are preferably sent directly to a transmit buffer
+where they immediately participate in CAN bus arbitration.
+
+\subsection{Device Initialization}\label{sec:Device Types / CAN Device / Device Initialization}
+
+\begin{enumerate}
+
+\item Read the feature bits and negotiate with the device.
+
+\item Fill the virtqueue \field{Rxq} with empty buffers to be ready for
+the reception of CAN messages.
+
+\item Fill the virtqueue \field{Indicationq} with empty buffers so that
+the CAN device is able to provide status change indications to the
+virtio CAN driver.
+
+\item Read the CAN controller properties using the \field{Controlq}.
+
+\item Start the CAN controller using the \field{Controlq}.
+
+\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}
+
+The type of a CAN message identifier is identified by the most
+significant 2 bits of the internally used 32 bit value. This matches the
+definition for Can_IdType in
+\hyperref[intro:CAN_Driver]{CAN Driver} chapter 8.2.3.
+
+\begin{lstlisting}
+#define VIRTIO_CAN_ID_TYPE_STANDARD    0x00000000U
+#define VIRTIO_CAN_ID_TYPE_STANDARD_FD 0x40000000U
+#define VIRTIO_CAN_ID_TYPE_EXTENDED    0x80000000U
+#define VIRTIO_CAN_ID_TYPE_EXTENDED_FD 0xC0000000U
+\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 must be started
+by sending a VIRTIO_CAN_SET_CTRL_MODE_START control message,
+to stop participating in bus communication it must be 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_set_ctrl_mode_in {
+        u8 result;
+};
+\end{lstlisting}
+
+If the transition succeeded the result shall be VIRTIO_CAN_RESULT_OK
+otherwise it shall be VIRTIO_CAN_RESULT_NOT_OK. The request shall be put
+into the used queue when the CAN controller finalized the transition to
+the requested controller mode.
+
+A transition to STOPPED state cancels all CAN messages pending for
+transmission. A state transition to STOPPED state shall trigger to put
+all CAN messages pending for transmission into the used queue with
+result VIRTIO_CAN_RESULT_NOT_OK.
+
+Initially the CAN controller is in STOPPED state.
+
+\subsubsection{CAN Message Transmission}\label{sec:Device Types / CAN Device / Device Operation / CAN Message Transmission}
+
+Messages may be transmitted by placing outgoing CAN messages in the
+virtqueue \field{Txq}.
+
+\begin{lstlisting}
+struct virtio_can_tx_out {
+#define VIRTIO_CAN_TX 0x0001u
+        le16 msg_type;
+        le16 priority;
+        le32 can_id;
+        u8 sdu[];
+};
+
+struct virtio_can_tx_in {
+        u8 result;
+};
+\end{lstlisting}
+
+Priority is 0 for low priority and 1 for high priority CAN messages.
+
+The actual length of the SDU can be calculated from the length of the device
+read-only descriptor.
+
+To avoid internal priority inversion in the \field{Txq} the user of the
+driver may do a book keeping of in flight transmission requests and
+defer sending of TX messages until the chosen transmission resource
+becomes available.
+
+If priority, can_id or SDU length are out of range or the CAN controller
+is in an invalid state result shall be set to VIRTIO_CAN_RESULT_NOT_OK
+and the message shall not be scheduled for transmission. Sending a CAN
+message with a priority with 0 transmission places configured shall
+be considered as priority being out of range.
+
+If the parameters are valid the message is scheduled for transmission
+and result is set to VIRTIO_CAN_OK. The transmission request should be
+put into the used queue after the physical CAN controller acknowledged
+the transmission on the CAN bus (may have to be put under a feature flag
+as there may be non AUTOSAR CAN driver backends which don't provide a
+trigger to do this correctly).
+
+\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 can_id;
+        u8 sdu[];
+};
+\end{lstlisting}
+
+The structure element reserved may in the future be used to forward an
+AUTOSAR hoh, see (\hyperref[intro:CAN_Driver]{CAN Driver} chapter 7.6).
+The value should be set to 0xFFFF.
+
+If the feature \field{VIRTIO_CAN_F_CAN_FD} has been negotiated the
+maximal possible SDU length is 64, if the feature has not been
+negotiated the maximal possible SDU length is 8.
+
+The actual length of the SDU can be calculated from the length of the
+driver read-only descriptor.
+
+\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_busoff_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 must be put into the used queue with
+result VIRTIO_CAN_RESULT_NOT_OK.
-- 
2.17.1



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [virtio-comment] [PATCH 1/1] [RFC] virtio-can: Add the device specification.
  2021-04-01 15:20 [virtio-comment] [PATCH 1/1] [RFC] virtio-can: Add the device specification Harald Mommer
@ 2021-04-26 10:05 ` Stefan Hajnoczi
  2021-05-05 10:33   ` Harald Mommer
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Hajnoczi @ 2021-04-26 10:05 UTC (permalink / raw)
  To: Harald Mommer; +Cc: virtio-comment, virtio-dev

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

On Thu, Apr 01, 2021 at 05:20:13PM +0200, Harald Mommer 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.

Hi,
I'm not familiar with CAN but have tried to give general feedback since
no one else has replied yet.

The commit description isn't completely clear about what this new VIRTIO
device is. I guess it's a virtual CAN controller rather than a CAN
device. The "give access to a CAN controller" wording suggests that it's
for passthrough, but the CAN controller could probably be purely
software too. I'm sure this will become clearer from the diff below, but
wanted to mention it since others may also find it ambiguous.

> ---
>  content.tex      |   1 +
>  introduction.tex |   3 +
>  virtio-can.tex   | 245 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 249 insertions(+)
>  create mode 100644 virtio-can.tex
> 
> diff --git a/content.tex b/content.tex
> index e536fd4..c1604db 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -6564,6 +6564,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  \input{virtio-mem.tex}
>  \input{virtio-i2c.tex}
>  \input{virtio-scmi.tex}
> +\input{virtio-can.tex}
>  
>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  
> diff --git a/introduction.tex b/introduction.tex
> index 7204b24..84ea5c0 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -79,6 +79,9 @@ \section{Normative References}\label{sec:Normative References}
>  	\phantomsection\label{intro:SCMI}\textbf{[SCMI]} &
>  	Arm System Control and Management Interface, DEN0056,
>  	\newline\url{https://developer.arm.com/docs/den0056/c}, version C and any future revisions\\
> +	\phantomsection\label{intro:CAN_Driver}\textbf{[CAN Driver]} &
> +	Specification of CAN Driver -- AUTOSAR CP R20-11,
> +	\newline\url{https://www.autosar.org/fileadmin/user_upload/standards/classic/20-11/AUTOSAR_SWS_CANDriver.pdf}\\

"The commercial exploitation of the material contained in this work
requires a license to such intellectual property rights."?

CAN is an ISO standard. What is the relationship to AUTOSAR and why not
reference the ISO standard? The CAN Wikipedia page
(https://en.wikipedia.org/wiki/CAN_bus) only mentions ISO, not AUTOSAR.

Also, does the AUTOSAR reference mean that this is a specific flavor of
CAN that may not be compatible with other CAN variants?

>  
>  \end{longtable}
>  
> diff --git a/virtio-can.tex b/virtio-can.tex
> new file mode 100644
> index 0000000..c343759
> --- /dev/null
> +++ b/virtio-can.tex
> @@ -0,0 +1,245 @@
> +\section{CAN Device}\label{sec:Device Types / CAN Device}
> +
> +virtio-can is a virtio based CAN (Controller Area Network) device. It is

s/device/controller/ ?

> +used to give a virtual machine access to a CAN bus. The CAN bus may
> +either be a physical CAN bus or a virtual CAN bus between virtual
> +machines or a combination of both.
> +
> +This section relies on definitions made by the AUTOSAR
> +\hyperref[intro:CAN_Driver]{CAN Driver} specification.
> +
> +\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.
> +
> +\subsection{Feature Bits}\label{sec:Device Types / CAN Device / Feature Bits}
> +
> +The virtio-can device always supports classic CAN frames with a maximum
> +payload size of 8 bytes.
> +
> +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.

Please state this in a devicenormative section:

  \devicenormative{\subsection}{...}
  The device MUST support CAN~2.0B Extended CAN IDs.

The general spec text is informative and then specific statements about
what MUST, MUST NOT, SHOULD, etc are made in drivernormative and
devicenormative sections. You can find examples of this in the specs for
other devices.

> +
> +\begin{description}
> +
> +\item[VIRTIO_CAN_F_CAN_FD (0)]
> +
> +In addition to classic CAN frames the device supports CAN FD frames with
> +a maximum payload size of 64 bytes.

Is there a link for CAN FD? I found "CAN with Flexible Data-Rate" but
it's just a Wayback Machine link and not the official location:
https://web.archive.org/web/20151211125301/http://www.bosch-semiconductors.de/media/ubk_semiconductors/pdf_1/canliteratur/can_fd_spec.pdf

> +
> +\end{description}
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / CAN Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for
> +the driver.
> +
> +\begin{lstlisting}
> +struct virtio_can_config {
> +        le16 lo_prio_count; 
> +        le16 hi_prio_count;
> +};
> +\end{lstlisting}
> +
> +To operate the Virtio CAN device it may be necessary to know some basic
> +properties of the underlying physical CAN controller hardware and its
> +configuration.
> +
> +Physical CAN controllers may support transmission by putting messages

"may", "should", "can", "must", etc are not used in the informative
sections of the spec. Those kinds of statements go in the
drivernormative and devicenormative sections.

> +into FIFOs first and / or by using transmit buffers directly. The user
> +of the Virtio CAN driver may need to know

This can be rewritten without the concept of a "physical CAN
controller". Simply describe lo_prio_count/hi_prio_count as properties
of the virtio-can device itself and then the physical CAN controller
concept isn't needed. This is makes the spec more general since emulated
CAN controllers might have their own constraints even though there is no
physical CAN controller.

> +
> +\begin{itemize}
> +\item Number of TX FIFO places for non time critical CAN messages
> +\item Number of TX buffers for high priority CAN messages

Inconsistencies:
- FIFO places == buffers?
- non time critical == low priority?

Please use terminology consistently. Don't introduce multiple terms for
the same thing.

> +\end{itemize}
> +
> +to schedule an optimal transmission of CAN messages. Non time critical
> +messages may be sent via a FIFO where they may suffer "Inner Priority
> +Inversion" (\hyperref[intro:CAN_Driver]{CAN Driver} chapter 2.1). High
> +priority messages are preferably sent directly to a transmit buffer
> +where they immediately participate in CAN bus arbitration.

Please mention lo_prio_count and hi_prio_count so their meaning is
unambiguous and they can easily be searched by name in the document. The
\begin{itemize} above should explicitly mention lo_prio_count and
hi_prio_count.

> +
> +\subsection{Device Initialization}\label{sec:Device Types / CAN Device / Device Initialization}
> +
> +\begin{enumerate}
> +
> +\item Read the feature bits and negotiate with the device.
> +
> +\item Fill the virtqueue \field{Rxq} with empty buffers to be ready for
> +the reception of CAN messages.

How large do the buffers need to be? A drivernormative "MUST" statement
is probably needed for this.

> +
> +\item Fill the virtqueue \field{Indicationq} with empty buffers so that
> +the CAN device is able to provide status change indications to the
> +virtio CAN driver.

Do these buffers have a specific structure/size? I see struct
virtio_can_busoff_ind below but maybe a more general type is needed. If
the indication message types depend on the virtio-can device then it
could expose a configuration field so the driver knows how large
Indicationq buffers need to be.

> +
> +\item Read the CAN controller properties using the \field{Controlq}.
> +
> +\item Start the CAN controller using the \field{Controlq}.
> +
> +\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}
> +
> +The type of a CAN message identifier is identified by the most
> +significant 2 bits of the internally used 32 bit value. This matches the
> +definition for Can_IdType in
> +\hyperref[intro:CAN_Driver]{CAN Driver} chapter 8.2.3.
> +
> +\begin{lstlisting}
> +#define VIRTIO_CAN_ID_TYPE_STANDARD    0x00000000U
> +#define VIRTIO_CAN_ID_TYPE_STANDARD_FD 0x40000000U
> +#define VIRTIO_CAN_ID_TYPE_EXTENDED    0x80000000U
> +#define VIRTIO_CAN_ID_TYPE_EXTENDED_FD 0xC0000000U
> +\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 must be started
> +by sending a VIRTIO_CAN_SET_CTRL_MODE_START control message,
> +to stop participating in bus communication it must be 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_set_ctrl_mode_in {
> +        u8 result;
> +};
> +\end{lstlisting}
> +
> +If the transition succeeded the result shall be VIRTIO_CAN_RESULT_OK
> +otherwise it shall be VIRTIO_CAN_RESULT_NOT_OK. The request shall be put
> +into the used queue when the CAN controller finalized the transition to
> +the requested controller mode.

"shall" is not for drivernormative/devicenormative sections. Simply
using "is" instead of "shall be" works here.

"the used queue" is specific to Split Virtqueues. Packed Virtqueues are
organized differently. I suggest rewording this sentence:

  The device marks the request used when the CAN controller has
  finalized the transition to the requested controller mode.

("2.7.9 Multi-buffer requests" uses the "mark used" language. Another
option is "the device uses the request when ...".)

> +
> +A transition to STOPPED state cancels all CAN messages pending for
> +transmission. A state transition to STOPPED state shall trigger to put
> +all CAN messages pending for transmission into the used queue with
> +result VIRTIO_CAN_RESULT_NOT_OK.

The exact behavior is not clear to me. There are several cases:

1. A tx message that was being transmitted when the
   VIRTIO_CAN_SET_CTRL_MODE_STOP control message was submitted.
2. A tx message that was on the Txq but not yet transmitted when the
   VIRTIO_CAN_SET_CTRL_MODE_STOP control message was submitted.
3. A tx message that was added to the Txq after the
   VIRTIO_CAN_SET_CTRL_MODE_STOP control message was submitted.

I guess they behave as follows:

1. The message might finish transmitting successfully or the message
   might complete with VIRTIO_CAN_RESULT_NOT_OK.
2. These messages complete with VIRTIO_CAN_RESULT_NOT_OK.
3. These messages might complete with VIRTIO_CAN_RESULT_NOT_OK or they
   might transmit successfully.

Another thought: the text is clearer when it explicitly mentions the
entity (driver or device) and the virtqueue (Txq or Controlq) involved:

  When the device transitions to the STOPPED state all CAN messages on
  the Txq complete with VIRTIO_CAN_RESULT_NOT_OK.

That makes it clear that the device needs to do this, not the driver. It
also makes it clear that we're describing effects on the Txq, not the
Controlq where the VIRTIO_CAN_SET_CTRL_MODE_STOP was submitted.

> +
> +Initially the CAN controller is in STOPPED state.

s/in STOPPED state/in the STOPPED state/

> +
> +\subsubsection{CAN Message Transmission}\label{sec:Device Types / CAN Device / Device Operation / CAN Message Transmission}
> +
> +Messages may be transmitted by placing outgoing CAN messages in the

s/may be/are/

(To avoid using shall/may/should/etc language outside
drivernormative/devicenormative sections.)

> +virtqueue \field{Txq}.

Grammar tweak:
s/virtqueue \field{Txq}/\field{Txq} virtqueue/

> +
> +\begin{lstlisting}
> +struct virtio_can_tx_out {
> +#define VIRTIO_CAN_TX 0x0001u
> +        le16 msg_type;
> +        le16 priority;
> +        le32 can_id;
> +        u8 sdu[];
> +};
> +
> +struct virtio_can_tx_in {
> +        u8 result;
> +};
> +\end{lstlisting}
> +
> +Priority is 0 for low priority and 1 for high priority CAN messages.

Please use /field{priority} to make it clear which field is being
described.

> +
> +The actual length of the SDU can be calculated from the length of the device
> +read-only descriptor.

  The length of \field{sdu} is implicit in the length of the device
  read-only descriptors.

I used "descriptors" here because "2.6.4 Message Framing" says that
drivers can choose arbitrary descriptor lengths. The device cannot
assume struct virtio_can_tx_out will be in a single descriptor.

> +
> +To avoid internal priority inversion in the \field{Txq} the user of the
> +driver may do a book keeping of in flight transmission requests and
> +defer sending of TX messages until the chosen transmission resource
> +becomes available.
> +
> +If priority, can_id or SDU length are out of range or the CAN controller

\field is missing for priority and can_id.

> +is in an invalid state result shall be set to VIRTIO_CAN_RESULT_NOT_OK
> +and the message shall not be scheduled for transmission. Sending a CAN
> +message with a priority with 0 transmission places configured shall
> +be considered as priority being out of range.
> +
> +If the parameters are valid the message is scheduled for transmission
> +and result is set to VIRTIO_CAN_OK. The transmission request should be

\field{result}

> +put into the used queue after the physical CAN controller acknowledged
> +the transmission on the CAN bus (may have to be put under a feature flag
> +as there may be non AUTOSAR CAN driver backends which don't provide a
> +trigger to do this correctly).

There is a TODO item here.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [virtio-comment] [PATCH 1/1] [RFC] virtio-can: Add the device specification.
  2021-04-26 10:05 ` Stefan Hajnoczi
@ 2021-05-05 10:33   ` Harald Mommer
  2021-05-06  9:43     ` Stefan Hajnoczi
  0 siblings, 1 reply; 4+ messages in thread
From: Harald Mommer @ 2021-05-05 10:33 UTC (permalink / raw)
  To: Stefan Hajnoczi, Harald Mommer; +Cc: virtio-comment, virtio-dev

Hello,

a lot of helpful comments, thank you for this work. Took some time to 
answer and about some things I will still have to think.

Am 26.04.21 um 12:05 schrieb Stefan Hajnoczi:
> On Thu, Apr 01, 2021 at 05:20:13PM +0200, Harald Mommer 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.
> Hi,
> I'm not familiar with CAN but have tried to give general feedback since
> no one else has replied yet.
>
> The commit description isn't completely clear about what this new VIRTIO
> device is. I guess it's a virtual CAN controller rather than a CAN
> device. The "give access to a CAN controller" wording suggests that it's
> for passthrough, but the CAN controller could probably be purely
> software too. I'm sure this will become clearer from the diff below, but
> wanted to mention it since others may also find it ambiguous.
What I'm currently developing is a passthrough device. The device 
application will use SocketCAN to access an underlying Linux CAN driver 
and the Linux virtio CAN driver will be a network driver which can be 
accessed by SocketCAN. The underlying Linux driver on the device side 
may either drive a physical CAN controller or may be a pure software 
solution like vcan not accessing any real CAN hardware. My collegues 
from the AUTOSAR team will most probably put the their implementation 
some day on top of a physical AUTOSAR CAN driver providing also a 
AUTOSAR CAN driver interface on the virtio driver guest side. Other 
implementations not doing a passthrough device are possible however 
implementing this as a pass-through device is the easiest way of 
implementing this.
>> ---
>>   content.tex      |   1 +
>>   introduction.tex |   3 +
>>   virtio-can.tex   | 245 +++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 249 insertions(+)
>>   create mode 100644 virtio-can.tex
>>
>> diff --git a/content.tex b/content.tex
>> index e536fd4..c1604db 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -6564,6 +6564,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>   \input{virtio-mem.tex}
>>   \input{virtio-i2c.tex}
>>   \input{virtio-scmi.tex}
>> +\input{virtio-can.tex}
>>   
>>   \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>   
>> diff --git a/introduction.tex b/introduction.tex
>> index 7204b24..84ea5c0 100644
>> --- a/introduction.tex
>> +++ b/introduction.tex
>> @@ -79,6 +79,9 @@ \section{Normative References}\label{sec:Normative References}
>>   	\phantomsection\label{intro:SCMI}\textbf{[SCMI]} &
>>   	Arm System Control and Management Interface, DEN0056,
>>   	\newline\url{https://developer.arm.com/docs/den0056/c}, version C and any future revisions\\
>> +	\phantomsection\label{intro:CAN_Driver}\textbf{[CAN Driver]} &
>> +	Specification of CAN Driver -- AUTOSAR CP R20-11,
>> +	\newline\url{https://www.autosar.org/fileadmin/user_upload/standards/classic/20-11/AUTOSAR_SWS_CANDriver.pdf}\\
> "The commercial exploitation of the material contained in this work
> requires a license to such intellectual property rights."?
>
> CAN is an ISO standard. What is the relationship to AUTOSAR and why not
> reference the ISO standard? The CAN Wikipedia page
> (https://en.wikipedia.org/wiki/CAN_bus) only mentions ISO, not AUTOSAR.

The AUTOSAR CAN driver specification describes an API for an AUTOSAR CAN 
driver. One of the goals was that it should be possible to write a 
virtio CAN driver providing a usable subset of the AUTOSAR CAN driver 
API. There are collegues here working on embeeded controllers using 
AUTOSAR and virtio CAN should serve their purposes. On the other hand, 
virtio CAN must also serve the needs for the non-AUTOSAR audience. In 
the end the specification must fulfill the needs of the AUTOSAR audience 
and the non-AUTOSAR audience dancing on two weddings at the same time.

Not referenced the ISO standard because I did not really look into this 
to develop the specification. Referenced what I really looked in to 
specify something which MAY be used to develop a virtio CAN driver with 
an AUTOSAR CAN driver interface on top.

If the special licensing of the AUTOSAR specification is a problem 
(guess it is) I think the solution is to ensure that a non AUTOSAR 
implementation can be done without having to look into the AUTOSAR 
specification itself. If this is not sufficient I need to know to 
address the problem properly.

> Also, does the AUTOSAR reference mean that this is a specific flavor of
> CAN that may not be compatible with other CAN variants?

This reference to the AUTOSAR spec does not mean that virtio CAN is 
incompatible with ISO. It just means that I looked heavily into the 
AUTOSAR CAN specifiation.

But it may be that my brain is polluted with too much AUTOSAR the draft 
specification does not fulfill all needs of audience not interested in 
AUTOSAR but for example in Linux SocketCAN. During last week working at 
some driver/device prototype using SocketCAN I realized that there is no 
support for remote transmission request frames in the virtio CAN draft 
specification but in SocketCAN. AUTOSAR does not support this, the 
feature was removed totally from CAN FD. Could be that the missing 
support is a shortcoming. Could be that the feature was removed from CAN 
FD because nobody in the world is using it and it's obsolete.

So some detail(s) which may be considered mandatory - also some I'm not 
even aware of - may be missing. I hope to get some feedback about this 
on the list.

>>   \end{longtable}
>>   
>> diff --git a/virtio-can.tex b/virtio-can.tex
>> new file mode 100644
>> index 0000000..c343759
>> --- /dev/null
>> +++ b/virtio-can.tex
>> @@ -0,0 +1,245 @@
>> +\section{CAN Device}\label{sec:Device Types / CAN Device}
>> +
>> +virtio-can is a virtio based CAN (Controller Area Network) device. It is
> s/device/controller/ ?
yes
>> +used to give a virtual machine access to a CAN bus. The CAN bus may
>> +either be a physical CAN bus or a virtual CAN bus between virtual
>> +machines or a combination of both.
>> +
>> +This section relies on definitions made by the AUTOSAR
>> +\hyperref[intro:CAN_Driver]{CAN Driver} specification.
>> +
>> +\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.
>> +
>> +\subsection{Feature Bits}\label{sec:Device Types / CAN Device / Feature Bits}
>> +
>> +The virtio-can device always supports classic CAN frames with a maximum
>> +payload size of 8 bytes.
>> +
>> +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.
> Please state this in a devicenormative section:
>
>    \devicenormative{\subsection}{...}
>    The device MUST support CAN~2.0B Extended CAN IDs.
>
> The general spec text is informative and then specific statements about
> what MUST, MUST NOT, SHOULD, etc are made in drivernormative and
> devicenormative sections. You can find examples of this in the specs for
> other devices.
Means the document structure has to be reworked generally to have the 
requirements (MUST, SHOULD) in appropriate sections.
>> +
>> +\begin{description}
>> +
>> +\item[VIRTIO_CAN_F_CAN_FD (0)]
>> +
>> +In addition to classic CAN frames the device supports CAN FD frames with
>> +a maximum payload size of 64 bytes.
> Is there a link for CAN FD? I found "CAN with Flexible Data-Rate" but
> it's just a Wayback Machine link and not the official location:
> https://web.archive.org/web/20151211125301/http://www.bosch-semiconductors.de/media/ubk_semiconductors/pdf_1/canliteratur/can_fd_spec.pdf

Have this specification also in my folder, got it also exactly from 
there. No better link. While most things in this document are still 
valid and it's nice to have some (here probably irrelevant details like 
CRC calculation) in this Bosch spec are outdated now. CAN FD is now 
officially described in ISO 11898-1:2015, a link would go to the ISO 
shop or similar.

Adding the ISO to the reference list would eliminate some questions 
raised here, I should do this.

>> +
>> +\end{description}
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / CAN Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for
>> +the driver.
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_config {
>> +        le16 lo_prio_count;
>> +        le16 hi_prio_count;
>> +};
>> +\end{lstlisting}
>> +
>> +To operate the Virtio CAN device it may be necessary to know some basic
>> +properties of the underlying physical CAN controller hardware and its
>> +configuration.
>> +
>> +Physical CAN controllers may support transmission by putting messages
> "may", "should", "can", "must", etc are not used in the informative
> sections of the spec. Those kinds of statements go in the
> drivernormative and devicenormative sections.
Will have to restructure the document structure.
>> +into FIFOs first and / or by using transmit buffers directly. The user
>> +of the Virtio CAN driver may need to know
> This can be rewritten without the concept of a "physical CAN
> controller". Simply describe lo_prio_count/hi_prio_count as properties
> of the virtio-can device itself and then the physical CAN controller
> concept isn't needed. This is makes the spec more general since emulated
> CAN controllers might have their own constraints even though there is no
> physical CAN controller.

A classic CAN bus has a bandwith of typically 500 kbps. Which is not 
this much and is therefore to be considered as bottleneck. Virtual stuff 
come typically with bandwithes in the magnitude of the internal RAM 
(Gigabytes/s) or at least PCI (Gigabits/s). Prioritization makes no 
sense any more for those non-physical CAN devices on which the transport 
medium is magnitutes faster as a physical CAN bus. Have had this 
disussions internally any my opinion is that for non-physical devices 
prioritization makes no sense at all. So no, I prefer not to reformulate 
in this way.

>> +
>> +\begin{itemize}
>> +\item Number of TX FIFO places for non time critical CAN messages
>> +\item Number of TX buffers for high priority CAN messages
> Inconsistencies:
> - FIFO places == buffers?
> - non time critical == low priority?
>
> Please use terminology consistently. Don't introduce multiple terms for
> the same thing.
The different terminology is intentional as those are different things. 
There are wait queue places in the FIFO not participating in bus 
arbitration and transmission buffers participating in bus arbitration.
>> +\end{itemize}
>> +
>> +to schedule an optimal transmission of CAN messages. Non time critical
>> +messages may be sent via a FIFO where they may suffer "Inner Priority
>> +Inversion" (\hyperref[intro:CAN_Driver]{CAN Driver} chapter 2.1). High
>> +priority messages are preferably sent directly to a transmit buffer
>> +where they immediately participate in CAN bus arbitration.
> Please mention lo_prio_count and hi_prio_count so their meaning is
> unambiguous and they can easily be searched by name in the document. The
> \begin{itemize} above should explicitly mention lo_prio_count and
> hi_prio_count.
If I understand this correctly it's a formatting issue which has to be 
addressed.
>> +
>> +\subsection{Device Initialization}\label{sec:Device Types / CAN Device / Device Initialization}
>> +
>> +\begin{enumerate}
>> +
>> +\item Read the feature bits and negotiate with the device.
>> +
>> +\item Fill the virtqueue \field{Rxq} with empty buffers to be ready for
>> +the reception of CAN messages.
> How large do the buffers need to be? A drivernormative "MUST" statement
> is probably needed for this.
I consider this as an implementation decision. Doing an implementation l 
would provide enough buffers so that no CAN message received on the bus 
is lost. The actual choice depends on the maximum received message rate, 
time to service a received CAN message and in case of a polling driver 
also on the polling cycle.
>> +
>> +\item Fill the virtqueue \field{Indicationq} with empty buffers so that
>> +the CAN device is able to provide status change indications to the
>> +virtio CAN driver.
> Do these buffers have a specific structure/size? I see struct
> virtio_can_busoff_ind below but maybe a more general type is needed. If
> the indication message types depend on the virtio-can device then it
> could expose a configuration field so the driver knows how large
> Indicationq buffers need to be.

There is currently only one message type defined for this channel, this 
is VIRTIO_CAN_BUSOFF_IND. The message definition (struct 
virtio_can_busoff_ind) has a length of 2 bytes bearing only le16 
msg_type. So a buffer with this a size of 2 bytes has to be provided in 
order to bear the structure / message. A future version of the 
specification may of course define an additional message type on the 
channel with a bigger structure / message definition but this would then 
require a feature flag to allow the device to send this indication. (Do 
I have understood the problem? There seems to be none.)

>> +
>> +\item Read the CAN controller properties using the \field{Controlq}.
>> +
>> +\item Start the CAN controller using the \field{Controlq}.
>> +
>> +\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}
>> +
>> +The type of a CAN message identifier is identified by the most
>> +significant 2 bits of the internally used 32 bit value. This matches the
>> +definition for Can_IdType in
>> +\hyperref[intro:CAN_Driver]{CAN Driver} chapter 8.2.3.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CAN_ID_TYPE_STANDARD    0x00000000U
>> +#define VIRTIO_CAN_ID_TYPE_STANDARD_FD 0x40000000U
>> +#define VIRTIO_CAN_ID_TYPE_EXTENDED    0x80000000U
>> +#define VIRTIO_CAN_ID_TYPE_EXTENDED_FD 0xC0000000U
>> +\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 must be started
>> +by sending a VIRTIO_CAN_SET_CTRL_MODE_START control message,
>> +to stop participating in bus communication it must be 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_set_ctrl_mode_in {
>> +        u8 result;
>> +};
>> +\end{lstlisting}
>> +
>> +If the transition succeeded the result shall be VIRTIO_CAN_RESULT_OK
>> +otherwise it shall be VIRTIO_CAN_RESULT_NOT_OK. The request shall be put
>> +into the used queue when the CAN controller finalized the transition to
>> +the requested controller mode.
> "shall" is not for drivernormative/devicenormative sections. Simply
> using "is" instead of "shall be" works here.
>
> "the used queue" is specific to Split Virtqueues. Packed Virtqueues are
> organized differently. I suggest rewording this sentence:
>
>    The device marks the request used when the CAN controller has
>    finalized the transition to the requested controller mode.
>
> ("2.7.9 Multi-buffer requests" uses the "mark used" language. Another
> option is "the device uses the request when ...".)
Will have to re-formulate so that the sentence becomes valid for the 
newer packet virtqueues also.
>> +
>> +A transition to STOPPED state cancels all CAN messages pending for
>> +transmission. A state transition to STOPPED state shall trigger to put
>> +all CAN messages pending for transmission into the used queue with
>> +result VIRTIO_CAN_RESULT_NOT_OK.
> The exact behavior is not clear to me. There are several cases:
>
> 1. A tx message that was being transmitted when the
>     VIRTIO_CAN_SET_CTRL_MODE_STOP control message was submitted.
> 2. A tx message that was on the Txq but not yet transmitted when the
>     VIRTIO_CAN_SET_CTRL_MODE_STOP control message was submitted.
> 3. A tx message that was added to the Txq after the
>     VIRTIO_CAN_SET_CTRL_MODE_STOP control message was submitted.
>
> I guess they behave as follows:
>
> 1. The message might finish transmitting successfully or the message
>     might complete with VIRTIO_CAN_RESULT_NOT_OK.
> 2. These messages complete with VIRTIO_CAN_RESULT_NOT_OK.
> 3. These messages might complete with VIRTIO_CAN_RESULT_NOT_OK or they
>     might transmit successfully.
>
> Another thought: the text is clearer when it explicitly mentions the
> entity (driver or device) and the virtqueue (Txq or Controlq) involved:
>
>    When the device transitions to the STOPPED state all CAN messages on
>    the Txq complete with VIRTIO_CAN_RESULT_NOT_OK.
>
> That makes it clear that the device needs to do this, not the driver. It
> also makes it clear that we're describing effects on the Txq, not the
> Controlq where the VIRTIO_CAN_SET_CTRL_MODE_STOP was submitted.

I tend to use passive in specifying things which is considered as not 
optimal. Old mistake, got caught again.

1. 2. meets also my understanding. 3. also. But if the word "submitted" 
is replaced by "received" ruling out the race the result is expected to 
be VIRTIO_CAN_RESULT_NOT_OK.

>> +
>> +Initially the CAN controller is in STOPPED state.
> s/in STOPPED state/in the STOPPED state/
>
>> +
>> +\subsubsection{CAN Message Transmission}\label{sec:Device Types / CAN Device / Device Operation / CAN Message Transmission}
>> +
>> +Messages may be transmitted by placing outgoing CAN messages in the
> s/may be/are/
>
> (To avoid using shall/may/should/etc language outside
> drivernormative/devicenormative sections.)
>
>> +virtqueue \field{Txq}.
> Grammar tweak:
> s/virtqueue \field{Txq}/\field{Txq} virtqueue/
>
>> +
>> +\begin{lstlisting}
>> +struct virtio_can_tx_out {
>> +#define VIRTIO_CAN_TX 0x0001u
>> +        le16 msg_type;
>> +        le16 priority;
>> +        le32 can_id;
>> +        u8 sdu[];
>> +};
>> +
>> +struct virtio_can_tx_in {
>> +        u8 result;
>> +};
>> +\end{lstlisting}
>> +
>> +Priority is 0 for low priority and 1 for high priority CAN messages.
> Please use /field{priority} to make it clear which field is being
> described.
>
>> +
>> +The actual length of the SDU can be calculated from the length of the device
>> +read-only descriptor.
>    The length of \field{sdu} is implicit in the length of the device
>    read-only descriptors.
>
> I used "descriptors" here because "2.6.4 Message Framing" says that
> drivers can choose arbitrary descriptor lengths. The device cannot
> assume struct virtio_can_tx_out will be in a single descriptor.
Will replace the wrong sentence.
>> +
>> +To avoid internal priority inversion in the \field{Txq} the user of the
>> +driver may do a book keeping of in flight transmission requests and
>> +defer sending of TX messages until the chosen transmission resource
>> +becomes available.
>> +
>> +If priority, can_id or SDU length are out of range or the CAN controller
> \field is missing for priority and can_id.
>
>> +is in an invalid state result shall be set to VIRTIO_CAN_RESULT_NOT_OK
>> +and the message shall not be scheduled for transmission. Sending a CAN
>> +message with a priority with 0 transmission places configured shall
>> +be considered as priority being out of range.
>> +
>> +If the parameters are valid the message is scheduled for transmission
>> +and result is set to VIRTIO_CAN_OK. The transmission request should be
> \field{result}
>
>> +put into the used queue after the physical CAN controller acknowledged
>> +the transmission on the CAN bus (may have to be put under a feature flag
>> +as there may be non AUTOSAR CAN driver backends which don't provide a
>> +trigger to do this correctly).
> There is a TODO item here.

Having an AUTOSAR CAN driver backend there is a callback indication when 
the message was physically transmitted on the bus. No problem there. For 
SocketCAN it looks like there is a feature to receive the own sent 
message tagged as sent by the own CAN node. With this it should be 
possible to implement this timing requirement as intended. But I'm still 
investigating whether this feature is provided always and this may not 
be the case. It's a detail which is more complicated as I thought. So 
this TODO is something which cannot be closed today finally, still 
investigating this detail.

Will have to plan now internally when to update the draft spec and to 
re-send an updated version. I'm currently with both hands in some 
prototype implementation serving and using Linux SocketCAN. 
Prioritiziung this work may be benefitial for the next draft spec. as I 
may still learn about some details and pitfalls doing an actual 
implementation.



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [virtio-comment] [PATCH 1/1] [RFC] virtio-can: Add the device specification.
  2021-05-05 10:33   ` Harald Mommer
@ 2021-05-06  9:43     ` Stefan Hajnoczi
  0 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2021-05-06  9:43 UTC (permalink / raw)
  To: Harald Mommer; +Cc: Harald Mommer, virtio-comment, virtio-dev

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

On Wed, May 05, 2021 at 12:33:19PM +0200, Harald Mommer wrote:
> Am 26.04.21 um 12:05 schrieb Stefan Hajnoczi:
> > On Thu, Apr 01, 2021 at 05:20:13PM +0200, Harald Mommer wrote:
> > > diff --git a/introduction.tex b/introduction.tex
> > > index 7204b24..84ea5c0 100644
> > > --- a/introduction.tex
> > > +++ b/introduction.tex
> > > @@ -79,6 +79,9 @@ \section{Normative References}\label{sec:Normative References}
> > >   	\phantomsection\label{intro:SCMI}\textbf{[SCMI]} &
> > >   	Arm System Control and Management Interface, DEN0056,
> > >   	\newline\url{https://developer.arm.com/docs/den0056/c}, version C and any future revisions\\
> > > +	\phantomsection\label{intro:CAN_Driver}\textbf{[CAN Driver]} &
> > > +	Specification of CAN Driver -- AUTOSAR CP R20-11,
> > > +	\newline\url{https://www.autosar.org/fileadmin/user_upload/standards/classic/20-11/AUTOSAR_SWS_CANDriver.pdf}\\
> > "The commercial exploitation of the material contained in this work
> > requires a license to such intellectual property rights."?
> > 
> > CAN is an ISO standard. What is the relationship to AUTOSAR and why not
> > reference the ISO standard? The CAN Wikipedia page
> > (https://en.wikipedia.org/wiki/CAN_bus) only mentions ISO, not AUTOSAR.
> 
> The AUTOSAR CAN driver specification describes an API for an AUTOSAR CAN
> driver. One of the goals was that it should be possible to write a virtio
> CAN driver providing a usable subset of the AUTOSAR CAN driver API. There
> are collegues here working on embeeded controllers using AUTOSAR and virtio
> CAN should serve their purposes. On the other hand, virtio CAN must also
> serve the needs for the non-AUTOSAR audience. In the end the specification
> must fulfill the needs of the AUTOSAR audience and the non-AUTOSAR audience
> dancing on two weddings at the same time.
> 
> Not referenced the ISO standard because I did not really look into this to
> develop the specification. Referenced what I really looked in to specify
> something which MAY be used to develop a virtio CAN driver with an AUTOSAR
> CAN driver interface on top.
> 
> If the special licensing of the AUTOSAR specification is a problem (guess it
> is) I think the solution is to ensure that a non AUTOSAR implementation can
> be done without having to look into the AUTOSAR specification itself. If
> this is not sufficient I need to know to address the problem properly.

Yes, that sounds good. The VIRTIO spec can reference ISO standards while
still being flexible enough for AUTOSAR. Although I'm not familiar with
CAN, what you specified so far seems like core functionality that is
probably also covered in other standards under more permissive licensing
terms.

> 
> > Also, does the AUTOSAR reference mean that this is a specific flavor of
> > CAN that may not be compatible with other CAN variants?
> 
> This reference to the AUTOSAR spec does not mean that virtio CAN is
> incompatible with ISO. It just means that I looked heavily into the AUTOSAR
> CAN specifiation.
> 
> But it may be that my brain is polluted with too much AUTOSAR the draft
> specification does not fulfill all needs of audience not interested in
> AUTOSAR but for example in Linux SocketCAN. During last week working at some
> driver/device prototype using SocketCAN I realized that there is no support
> for remote transmission request frames in the virtio CAN draft specification
> but in SocketCAN. AUTOSAR does not support this, the feature was removed
> totally from CAN FD. Could be that the missing support is a shortcoming.
> Could be that the feature was removed from CAN FD because nobody in the
> world is using it and it's obsolete.
> 
> So some detail(s) which may be considered mandatory - also some I'm not even
> aware of - may be missing. I hope to get some feedback about this on the
> list.

Is there someone we can invite to review the spec from a CAN
perspective? Maybe someone you work with who is experienced with CAN.
Otherwise we could ask the Linux CAN maintainers for input (Wolfgang
Grandegger <wg@grandegger.com>, Marc Kleine-Budde <mkl@pengutronix.de>).

> > >   \end{longtable}
> > > diff --git a/virtio-can.tex b/virtio-can.tex
> > > new file mode 100644
> > > index 0000000..c343759
> > > --- /dev/null
> > > +++ b/virtio-can.tex
> > > @@ -0,0 +1,245 @@
> > > +\section{CAN Device}\label{sec:Device Types / CAN Device}
> > > +
> > > +virtio-can is a virtio based CAN (Controller Area Network) device. It is
> > s/device/controller/ ?
> yes
> > > +used to give a virtual machine access to a CAN bus. The CAN bus may
> > > +either be a physical CAN bus or a virtual CAN bus between virtual
> > > +machines or a combination of both.
> > > +
> > > +This section relies on definitions made by the AUTOSAR
> > > +\hyperref[intro:CAN_Driver]{CAN Driver} specification.
> > > +
> > > +\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.
> > > +
> > > +\subsection{Feature Bits}\label{sec:Device Types / CAN Device / Feature Bits}
> > > +
> > > +The virtio-can device always supports classic CAN frames with a maximum
> > > +payload size of 8 bytes.
> > > +
> > > +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.
> > Please state this in a devicenormative section:
> > 
> >    \devicenormative{\subsection}{...}
> >    The device MUST support CAN~2.0B Extended CAN IDs.
> > 
> > The general spec text is informative and then specific statements about
> > what MUST, MUST NOT, SHOULD, etc are made in drivernormative and
> > devicenormative sections. You can find examples of this in the specs for
> > other devices.
> Means the document structure has to be reworked generally to have the
> requirements (MUST, SHOULD) in appropriate sections.

Yes, please.

> > > +
> > > +\end{description}
> > > +
> > > +\subsection{Device configuration layout}\label{sec:Device Types / CAN Device / Device configuration layout}
> > > +
> > > +All fields of this configuration are always available and read-only for
> > > +the driver.
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_can_config {
> > > +        le16 lo_prio_count;
> > > +        le16 hi_prio_count;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +To operate the Virtio CAN device it may be necessary to know some basic
> > > +properties of the underlying physical CAN controller hardware and its
> > > +configuration.
> > > +
> > > +Physical CAN controllers may support transmission by putting messages
> > "may", "should", "can", "must", etc are not used in the informative
> > sections of the spec. Those kinds of statements go in the
> > drivernormative and devicenormative sections.
> Will have to restructure the document structure.
> > > +into FIFOs first and / or by using transmit buffers directly. The user
> > > +of the Virtio CAN driver may need to know
> > This can be rewritten without the concept of a "physical CAN
> > controller". Simply describe lo_prio_count/hi_prio_count as properties
> > of the virtio-can device itself and then the physical CAN controller
> > concept isn't needed. This is makes the spec more general since emulated
> > CAN controllers might have their own constraints even though there is no
> > physical CAN controller.
> 
> A classic CAN bus has a bandwith of typically 500 kbps. Which is not this
> much and is therefore to be considered as bottleneck. Virtual stuff come
> typically with bandwithes in the magnitude of the internal RAM (Gigabytes/s)
> or at least PCI (Gigabits/s). Prioritization makes no sense any more for
> those non-physical CAN devices on which the transport medium is magnitutes
> faster as a physical CAN bus. Have had this disussions internally any my
> opinion is that for non-physical devices prioritization makes no sense at
> all. So no, I prefer not to reformulate in this way.

If I understand correctly you are saying that
lo_prio_count/hi_prio_count will be determined by the physical CAN
controller since it is the bottleneck. Therefore it makes sense to
explicitly refer to the physical CAN controller here.

What lo_prio_count/hi_prio_count values should pure software
implementations of virtio-can use?

> > > +
> > > +\begin{itemize}
> > > +\item Number of TX FIFO places for non time critical CAN messages
> > > +\item Number of TX buffers for high priority CAN messages
> > Inconsistencies:
> > - FIFO places == buffers?
> > - non time critical == low priority?
> > 
> > Please use terminology consistently. Don't introduce multiple terms for
> > the same thing.
> The different terminology is intentional as those are different things.
> There are wait queue places in the FIFO not participating in bus arbitration
> and transmission buffers participating in bus arbitration.

This still isn't clear to me, probably because I don't know CAN. I'm not
sure what "TX FIFO" vs "TX buffers", "non time critical" vs "high
priority", or "wait queue places" are.

If this is obvious to someone who knows CAN then it's okay, but if you
can see a way to clarify things without explaining all of CAN from
scratch, then it would be nice to adjust this section.

> > > +\end{itemize}
> > > +
> > > +to schedule an optimal transmission of CAN messages. Non time critical
> > > +messages may be sent via a FIFO where they may suffer "Inner Priority
> > > +Inversion" (\hyperref[intro:CAN_Driver]{CAN Driver} chapter 2.1). High
> > > +priority messages are preferably sent directly to a transmit buffer
> > > +where they immediately participate in CAN bus arbitration.
> > Please mention lo_prio_count and hi_prio_count so their meaning is
> > unambiguous and they can easily be searched by name in the document. The
> > \begin{itemize} above should explicitly mention lo_prio_count and
> > hi_prio_count.
> If I understand this correctly it's a formatting issue which has to be
> addressed.

Yes, the text currently doesn't mention lo_prio_count/hi_prio_count
explicitly so the reader has to guess which parts of the text are
referring to which field. This could lead to confusion.

> > > +
> > > +\subsection{Device Initialization}\label{sec:Device Types / CAN Device / Device Initialization}
> > > +
> > > +\begin{enumerate}
> > > +
> > > +\item Read the feature bits and negotiate with the device.
> > > +
> > > +\item Fill the virtqueue \field{Rxq} with empty buffers to be ready for
> > > +the reception of CAN messages.
> > How large do the buffers need to be? A drivernormative "MUST" statement
> > is probably needed for this.
> I consider this as an implementation decision. Doing an implementation l
> would provide enough buffers so that no CAN message received on the bus is
> lost. The actual choice depends on the maximum received message rate, time
> to service a received CAN message and in case of a polling driver also on
> the polling cycle.

I meant how large does a single Rxq buffer need to be, in bytes, so that
the driver can successfully receive a CAN message? If the driver's Rxq
buffer size is too small the the contents of the buffer would not fit.
For example, classic Layer 2 Ethernet frames are 1522 bytes so a common
network driver rx buffer size is at least 1522 bytes.

> > > +
> > > +\item Fill the virtqueue \field{Indicationq} with empty buffers so that
> > > +the CAN device is able to provide status change indications to the
> > > +virtio CAN driver.
> > Do these buffers have a specific structure/size? I see struct
> > virtio_can_busoff_ind below but maybe a more general type is needed. If
> > the indication message types depend on the virtio-can device then it
> > could expose a configuration field so the driver knows how large
> > Indicationq buffers need to be.
> 
> There is currently only one message type defined for this channel, this is
> VIRTIO_CAN_BUSOFF_IND. The message definition (struct virtio_can_busoff_ind)
> has a length of 2 bytes bearing only le16 msg_type. So a buffer with this a
> size of 2 bytes has to be provided in order to bear the structure / message.
> A future version of the specification may of course define an additional
> message type on the channel with a bigger structure / message definition but
> this would then require a feature flag to allow the device to send this
> indication. (Do I have understood the problem? There seems to be none.)

Yes, this is fine. Please state in the specification that Indicationq
buffers must be at least as large as struct virtio_can_busoff_ind so
that driver authors don't have to infer this.

> Will have to plan now internally when to update the draft spec and to
> re-send an updated version. I'm currently with both hands in some prototype
> implementation serving and using Linux SocketCAN. Prioritiziung this work
> may be benefitial for the next draft spec. as I may still learn about some
> details and pitfalls doing an actual implementation.

Great. Most of my feedback was about specification writing and requests
for clarifications rather than issues with the virtio-can device design,
so once you have time to do the spec rewording I think there won't be
much more from my side. Overall this looks good.

Thanks,
Stefan

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-06  9:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 15:20 [virtio-comment] [PATCH 1/1] [RFC] virtio-can: Add the device specification Harald Mommer
2021-04-26 10:05 ` Stefan Hajnoczi
2021-05-05 10:33   ` Harald Mommer
2021-05-06  9:43     ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.