Re: [Qemu-devel] [PATCH V4 0/7] CAN bus support for QEMU (SJA1000 PCI so far)

[Philippe Mathieu-Daudé]

Hi Pavel,

On 01/14/2018 05:14 PM, address@hidden wrote:
> From: Pavel Pisa <address@hidden>
> 
> Basic emulation of CAN bus controller and interconnection for QEMU.
> 
> Patches version 4:
> Resolve comments longer than 80 characters to suppress
> all warnings reported by scripts/checkpatch.pl.
> Follow all suggestions from Frederic Konrad review.
> Replace all printf and perror calls by QEMU equivalents.
> Include Deniz Eren signed-off confimation.
> 
> Patches version 3:
> Support to connect to host SocketCAN interface has been
> separated from the core bus implementation. Only simple
> statically initialize pointer to the connection function
> is used, no QOM concept for now.
> SJA1000 message filters redone and code unified where
> possible.
> Basic documentation added.
> QEMU_ALIGNED used in definition of CAN frame structure,
> structure and defines are separated from Linux/SocketCAN
> API defined ones to allow to keep QEMU message format
> independed from host system one. Check for correspondence
> to socketcan defines added.
> 
> Patches version 2:
> The bus emulation and the SJA1000 chip emulation introduced
> by individual patches as suggested by Frederic Konrad.
> Simple example board to test SJA1000 as single memory-mapped BAR
> has been omitted in a new series because emulation of real
> existing boards can provide same functions now.
> Conditionalized debug printfs changed to be exposed to compiler
> syntax check as suggested in review.
> 
> The work has been started by Jin Yang in the frame of GSoC 2013 slot
> contributed by RTEMS project which has been looking for environment
> to allow develop and test CAN drivers for multiple CPU architectures.
> 
> I have menthored the project and then done substantial code cleanup
> and update to QOM. Deniz Eren then used emulation for SJA1000 base card
> driver development for other operating system and contributed
> PCM-3680I and MIOe-3680 support.
> 
> Some page about the project
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus/wikis/home
> 
> FEE CTU GitLab repository with can-pci branch for 2.3, 2.4, 2.7, 2.8, 2.10
> and 2.11 QEMU version is available in the repository
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci
> 
> mirror at GitHub
> 
>   https://github.com/CTU-IIG/qemu
> 
> There are many areas for improvement and extension of the code still
> (for example freeze and migration is not implemented. CAN controllers
> use proper QOM model but bus/interconnection emulation uses simple broadcast
> connection which is required for CAN, but it is not based on QEMU bus model).
> I have tried to look into QEMU VLANs implementation but it
> does not map straightforward to CAN and I would need some help/opinion
> from more advanced developers to decide what is their right
> mapping to CAN.

I think your series is quite ready to get accepted, although I'm not
sure through with tree it will goes.

Most of my review comments are not blocking issues and might get
addressed later (like having an abstract sja1000).

The principal issue I'd like to discuss with Paolo/Marc-André is the
chardev backend, they might say it's OK to accept it in this current
state and refactor the CANBus backend later.

I'd still avoid using directly the socket() syscall and use the QEMU
socket API instead (also suggested by Daniel).

> 
> CAN-FD support would be interesting requires other developers/
> companies contributions or setup of some project to allow invite
> some students and colleagues from my university into project.
> 
> But I believe that (even in its actual state) provided solution
> is great help for embedded systems developers when they can connect
> SocketCAN from one or more embedded systems running in virtual
> environment together or with Linux host SocketCAN virtual
> or real bus interfaces.
> 
> We have even tested our generic CANopen device configured
> for CANopen 401 profile for generic I/O running in the virtual
> system which can control GPIO inputs/outputs through virtual
> industrial I/O card.
> 
> Generally QEMU can be interesting setup which allows
> to test complete industrial and automotive applications
> in virtual environment even before real hardware is availabe.

I have been thinking a bit about how to test some frame operations
(rather than the PCI devices) and the Linux vcan driver might be a good
option (Virtual Local CAN Interface). This is also useful to test this
series without having CAN hardware. How to use vcan might be worth his
own paragraph in docs/can.txt.

Do you think some of your tests can be added in the QEMU test suite
(qtests)?

Regards,

Phil.

> 
> Deniz Eren (2):
>   CAN bus PCM-3680I PCI (dual SJA1000 channel) emulation added.
>   CAN bus MIOe-3680 PCI (dual SJA1000 channel) emulation added.
> 
> Pavel Pisa (5):
>   CAN bus simple messages transport implementation for QEMU
>   CAN bus support to connect bust to Linux host SocketCAN interface.
>   CAN bus SJA1000 chip register level emulation for QEMU
>   CAN bus Kvaser PCI CAN-S (single SJA1000 channel) emulation added.
>   QEMU CAN bus emulation documentation
> 
>  default-configs/pci.mak   |    3 +
>  docs/can.txt              |   78 ++++
>  hw/Makefile.objs          |    1 +
>  hw/can/Makefile.objs      |   14 +
>  hw/can/can_core.c         |  136 ++++++
>  hw/can/can_host_stub.c    |   36 ++
>  hw/can/can_kvaser_pci.c   |  375 +++++++++++++++++
>  hw/can/can_mioe3680_pci.c |  336 +++++++++++++++
>  hw/can/can_pcm3680_pci.c  |  336 +++++++++++++++
>  hw/can/can_sja1000.c      | 1013 
> +++++++++++++++++++++++++++++++++++++++++++++
>  hw/can/can_sja1000.h      |  167 ++++++++
>  hw/can/can_socketcan.c    |  314 ++++++++++++++
>  include/can/can_emu.h     |  131 ++++++
>  13 files changed, 2940 insertions(+)
>  create mode 100644 docs/can.txt
>  create mode 100644 hw/can/Makefile.objs
>  create mode 100644 hw/can/can_core.c
>  create mode 100644 hw/can/can_host_stub.c
>  create mode 100644 hw/can/can_kvaser_pci.c
>  create mode 100644 hw/can/can_mioe3680_pci.c
>  create mode 100644 hw/can/can_pcm3680_pci.c
>  create mode 100644 hw/can/can_sja1000.c
>  create mode 100644 hw/can/can_sja1000.h
>  create mode 100644 hw/can/can_socketcan.c
>  create mode 100644 include/can/can_emu.h
>

signature.asc
Description: OpenPGP digital signature