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

[Cornelia Huck <cohuck@redhat.com>]

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.